Fix errors assigning bool to DateTime properties in Keys
#326
Conversation
|
I think I have fixed the issues found by the various checks and tests. More information on what I said up top: >>> date_create_from_format('Y-m-d\TH:i:s.uP', '2001-02-03T04:05:06.123456Z')
=> DateTime @978310861 {#11847
date: 2001-02-03 04:05:06.123456 +00:00,
}
>>> date_create_from_format('Y-m-d\TH:i:s.vuP', '2001-02-03T04:05:06.123456Z')
=> DateTime @978310861 {#11849
date: 2001-02-03 04:05:06.456 +00:00,
}Note how Also: I found another variant of timestamp that can again not be parsed by even this updated version: >>> app(MeiliSearch\Client::class)->getRawKeys()['results'][1]['createdAt']
=> "2022-05-20T08:51:30.4825815Z"
>>> date_create_from_format('Y-m-d\TH:i:s.uP', '2022-05-20T08:51:30.4825815Z')
=> falseIf it is not necessary to only accept just these two precise timestamp formats, it would probably be advisable to simply use the >>> new DateTime('2001-02-03T04:05:06Z')
=> DateTime @981173106 {#11848
date: 2001-02-03 04:05:06.0 UTC (+00:00),
}
>>> new DateTime('2001-02-03T04:05:06.123Z')
=> DateTime @981173106 {#11852
date: 2001-02-03 04:05:06.123 UTC (+00:00),
}
>>> new DateTime('2001-02-03T04:05:06.123456Z')
=> DateTime @981173106 {#11855
date: 2001-02-03 04:05:06.123456 UTC (+00:00),
}
>>> new DateTime('2001-02-03T04:05:06.123456789Z')
=> DateTime @981173106 {#11850
date: 2001-02-03 04:05:06.123456 UTC (+00:00),
} |
|
@brunoocasali protected function createDate($attribute): ?DateTime
{
if (!\is_string($attribute)) {
return null;
}
if (false === strpos($attribute, '.')) {
$date = date_create_from_format(DateTime::ATOM, $attribute);
} else {
$attribute = preg_replace('/(\.\d{6})\d+/', '$1', $attribute, 1);
$date = date_create_from_format('Y-m-d\TH:i:s.uP', $attribute);
}
return false === $date ? null : $date;
}
protected function createDate($attribute): ?DateTime
{
if (!\is_string($attribute)) {
return null;
}
try {
return new DateTime($attribute);
} catch (\Exception $e) {
return null;
}
}Edit for posterity: The |
|
I'll give a look in your PR today, thanks for being so involved :) |
|
@Nextra I have a hard time understanding the issue, I was not able to reproduce it, can you create a test case to cover this bug? About your variants, I think the second one is better to read, but I'm not sure if it will solve the problem of multiple formats simultaneously, will it? |
Any information I provided in #321 and in this thread comes from a real-world test. When I present output from a Tinker REPL session12, that actually talked to a proper QA Meilisearch installation, and shows it outputting timestamps that this SDK fails to parse. I know it is hard to test because Meilisearch doesn't output consistent timestamp formats, and instead three "variants":
The reason your tests don't fail outright is that your After working around this issue in my personal setup, I noticed that
I showed it accepting the three variants I mentioned, and correctly discarding microsecond digits that PHP cannot handle6. I wanted to provide both of them to provide you will all the information you might need for a decision. Otherwise I have no preference. Footnotes
|
|
Hi @Nextra I was able to create a failing test for your issue: <?php
declare(strict_types=1);
namespace Tests\Endpoints;
use MeiliSearch\Http\Client;
use MeiliSearch\MeiliSearch;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;
use Psr\Http\Client\ClientInterface;
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\StreamInterface;
use Tests\Http\ClientTest;
final class DateHandlingTest extends ClientTest
{
/**
* @group failing
* Tests the api edit form
*/
public function testHandleDateFormat(): void
{
$httpClient = $this->createHttpClientMock(200, '
{
"results": [
{
"description": "my key",
"key": "z6ySBsnp002e8bc6a31b794a95d623333be1fe4fd2d7eacdeaf7baf2c439866723e659ee",
"actions": ["*"],
"indexes": ["*"],
"expiresAt": "2023-06-14T10:34:03.629690014Z",
"createdAt": "2022-06-14T10:34:03.627606639Z",
"updatedAt": "2022-06-14T10:34:03.627606639Z"
}
]
}
');
$newClient = new \MeiliSearch\Client('https://localhost:7700', null, $httpClient);
$response = $newClient->getKeys();
$this->assertCount(2, $response);
$this->assertNull($response[0]->getExpiresAt());
}
/**
* @return ClientInterface|MockObject
*/
private function createHttpClientMock(int $status = 200, string $content = '{', string $contentType = 'application/json')
{
$stream = $this->createMock(StreamInterface::class);
$stream->expects(self::once())
->method('getContents')
->willReturn($content);
$response = $this->createMock(ResponseInterface::class);
$response->expects(self::any())
->method('getStatusCode')
->willReturn($status);
$response->expects(self::any())
->method('getHeader')
->with('content-type')
->willReturn([$contentType]);
$response->expects(self::once())
->method('getBody')
->willReturn($stream);
$httpClient = $this->createMock(ClientInterface::class);
$httpClient->expects(self::once())
->method('sendRequest')
->with(self::isInstanceOf(RequestInterface::class))
->willReturn($response);
return $httpClient;
}
}It was a pure CTRL+C/V with the
|
|
I don't quite have the time to setup a test environment today, but both of my |
|
I thought I should mention this: The two variants should not cause different behavior if ISO8601 timestamps are used as input ( >>> new DateTime('2001-02-03 04:05:06')
=> DateTime @981173106 {
date: 2001-02-03 04:05:06.0 UTC (+00:00),
}
>>> new DateTime('Sat Feb 03, 2001 04:05:06')
=> DateTime @981173106 {
date: 2001-02-03 04:05:06.0 UTC (+00:00),
}
>>> new DateTime('@981173106')
=> DateTime @981173106 {
date: 2001-02-03 04:05:06.0 +00:00,
} |
There was a problem hiding this comment.
Thanks a lot for your hard work in fixing this issue @Nextra
Meilisearch is better with contributors like you! ❤️
|
bors merge |
What does this PR do?
Fixes #321
Comments on the format choices:
MeiliSearch outputs various slight variations of timestamp formats depending on the precise moment represented by it (see #321). This SDK can neither assume that
expiresAtnever contains milli- or microseconds, nor thatcreatedAtorupdatedAtalways contains microseconds. I don't think PHP provides a way to handle this in a singledate_create_from_formatcall, so this change makes it always try both.In my opinion- Not quite true, see #326 (comment).vuas a format term for microseconds is unnecessarily restrictive, forcing the milli-/microsecond part to always contain four or more digits, while using justucovers all possibilities.Similar goes for forcing the timezone to appear as
Z. While MeiliSearch seems to adhere to this, I don't think it is necessary to disallow generic time zone notations.If an invalid timestamp is provided, so either a non-string is passed or the string has an invalid format, it defaults to returning
null. This reliably prevents the assignment error, but might not be desired behavior as it eats formatting errors.Please do not hesitate to make style or implementation changes. I would consider this a high priority issue, so please do not wait for me to get back to you. The SDK is virtually unusable in practice if you need to interact with API keys.
PR checklist
Please check if your PR fulfills the following requirements: