Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid timestamp value passed to DateTimeResult::fromTimestamp on PHP 8.1 #2327

Closed
3 tasks done
driesvints opened this issue Oct 4, 2021 · 8 comments · Fixed by #2337
Closed
3 tasks done

Invalid timestamp value passed to DateTimeResult::fromTimestamp on PHP 8.1 #2327

driesvints opened this issue Oct 4, 2021 · 8 comments · Fixed by #2337
Labels
bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.

Comments

@driesvints
Copy link
Contributor

driesvints commented Oct 4, 2021

Describe the bug
When I run the Laravel framework test suite on the upcoming PHP 8.1 release (now in release candidate), related tests to DynamoDB fail from an invalid timestamp issue. These tests are running fine on PHP 7.3 til PHP 8.0. This failure is specifically related to the DateTimeResult class but can origin further up the stacktrace.

https://github.com/laravel/framework/runs/3599702797#step:8:165

Stacktrace

Illuminate\Tests\Integration\Cache\DynamoDbStoreTest::testItemsCanBeAtomicallyAdded
166
Aws\Api\Parser\Exception\ParserException: Invalid timestamp value passed to DateTimeResult::fromTimestamp
167

168
/home/runner/work/framework/framework/vendor/aws/aws-sdk-php/src/Api/DateTimeResult.php:78
169
/home/runner/work/framework/framework/vendor/aws/aws-sdk-php/src/Api/Parser/JsonParser.php:51
170
/home/runner/work/framework/framework/vendor/aws/aws-sdk-php/src/Api/Parser/JsonParser.php:27
171
/home/runner/work/framework/framework/vendor/aws/aws-sdk-php/src/Api/Parser/JsonParser.php:27
172
/home/runner/work/framework/framework/vendor/aws/aws-sdk-php/src/Api/Parser/JsonRpcParser.php:49
173
/home/runner/work/framework/framework/vendor/aws/aws-sdk-php/src/Api/Parser/JsonRpcParser.php:36
174
/home/runner/work/framework/framework/vendor/aws/aws-sdk-php/src/Api/Parser/Crc32ValidatingParser.php:44
175
/home/runner/work/framework/framework/vendor/aws/aws-sdk-php/src/WrappedHttpHandler.php:125
176
/home/runner/work/framework/framework/vendor/aws/aws-sdk-php/src/WrappedHttpHandler.php:92
177
/home/runner/work/framework/framework/vendor/guzzlehttp/promises/src/Promise.php:204
178
/home/runner/work/framework/framework/vendor/guzzlehttp/promises/src/Promise.php:153
179
/home/runner/work/framework/framework/vendor/guzzlehttp/promises/src/TaskQueue.php:48
180
/home/runner/work/framework/framework/vendor/guzzlehttp/guzzle/src/Handler/CurlMultiHandler.php:158
181
/home/runner/work/framework/framework/vendor/guzzlehttp/guzzle/src/Handler/CurlMultiHandler.php:183
182
/home/runner/work/framework/framework/vendor/guzzlehttp/promises/src/Promise.php:248
183
/home/runner/work/framework/framework/vendor/guzzlehttp/promises/src/Promise.php:224
184
/home/runner/work/framework/framework/vendor/guzzlehttp/promises/src/Promise.php:269
185
/home/runner/work/framework/framework/vendor/guzzlehttp/promises/src/Promise.php:226
186
/home/runner/work/framework/framework/vendor/guzzlehttp/promises/src/Promise.php:269
187
/home/runner/work/framework/framework/vendor/guzzlehttp/promises/src/Promise.php:226
188
/home/runner/work/framework/framework/vendor/guzzlehttp/promises/src/Promise.php:62
189
/home/runner/work/framework/framework/vendor/aws/aws-sdk-php/src/AwsClientTrait.php:58
190
/home/runner/work/framework/framework/vendor/aws/aws-sdk-php/src/AwsClientTrait.php:86
191
/home/runner/work/framework/framework/tests/Integration/Cache/DynamoDbStoreTest.php:96
192
/home/runner/work/framework/framework/vendor/orchestra/testbench-core/src/Concerns/CreatesApplication.php:345
193
/home/runner/work/framework/framework/vendor/orchestra/testbench-core/src/Concerns/CreatesApplication.php:223
194
/home/runner/work/framework/framework/vendor/orchestra/testbench-core/src/TestCase.php:75
195
/home/runner/work/framework/framework/vendor/orchestra/testbench-core/src/Concerns/Testing.php:81
196
/home/runner/work/framework/framework/vendor/orchestra/testbench-core/src/TestCase.php:43
197
/home/runner/work/framework/framework/tests/Integration/Cache/DynamoDbStoreTest.php:23

Version of AWS SDK for PHP?
v3.193.3

Version of PHP (php -v)?
PHP 8.1.0RC2-dev

To Reproduce (observed behavior)
The related test that fails is https://github.com/laravel/framework/blob/8.x/tests/Integration/Cache/DynamoDbStoreTest.php#L49-L55. There's three more but they're all triggering the same issue.

The tests basically translates to the following:

$key = Str::random(6); // Random six character string.

$dynamodb = new DynamoDbClient(/* config */);

// The call below is made 2x...
$this->dynamo->putItem([
    'TableName' => 'cache',
    'Item' => [
        'key' => [
            'S' => $key,
        ],
        'value' => [
            'S' => 'Taylor',
        ],
       'expires_at' => [
            'N' => (string) '1633371128',
        ],
    ],
    'ConditionExpression' => 'attribute_not_exists(#key) OR #expires_at < :now',
    'ExpressionAttributeNames' => [
        '#key' => 'key',
        '#expires_at' => 'expires_at',
    ],
    'ExpressionAttributeValues' => [
        ':now' => [
            'N' => (string) Carbon::now()->getTimestamp(),
        ],
    ],
]);

Expected behavior
I'd expect the behavior to be exactly the same in PHP 8.1

@driesvints driesvints added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 4, 2021
@driesvints
Copy link
Contributor Author

Upon second view, it could be that these tests are failing because the first one is failing due to #2307. #2316 might fix them all together.

@driesvints
Copy link
Contributor Author

driesvints commented Oct 9, 2021

Unfortunately #2316 didn't fix the timestamp issue. I'm at a loss at what's causing this and would appreciate any help with figuring this out...

https://github.com/laravel/framework/pull/39034/checks?check_run_id=3846321821#step:8:127

@JackEllis
Copy link

This is a complete stab in the dark but is this related: #2053 (comment)

@27pchrisl
Copy link
Contributor

Hi @driesvints

Looks the SDK is internally passing a float unix timestamp into gmdate from the DynamoDB JSON response, which throws an exception Implicit conversion from float to int loses precision in 8.1. Probably it should be return new self(gmdate('c', (int)$unixTimestamp)); ?

return new self(gmdate('c', $unixTimestamp));

@stephenkhoo
Copy link

stephenkhoo commented Oct 9, 2021

Hi @driesvints

Looks the SDK is internally passing a float unix timestamp into gmdate from the DynamoDB JSON response, which throws an exception Implicit conversion from float to int loses precision in 8.1. Probably it should be return new self(gmdate('c', (int)$unixTimestamp)); ?

return new self(gmdate('c', $unixTimestamp));

Same finding:

I'm testing on PHP 8.1.0RC4-dev
and after diving through the code, the problem seems to only related to the gmdate.

I even tried comparing using Laravel 8.x with PHP8.0 and Laravel php8.1-fixes with PHP 8.1.0RC4-dev
The $timestamp value loaded are the same into the gmdate('c' $unixTimestamp)

Screenshot 2021-10-09 at 10 09 35 PM

Screenshot 2021-10-09 at 10 10 12 PM

By comparing PHP8.0 running gmdate, it has no problem running with float timestamp

But PHP 8.1.0RC4-dev gmdate refuse to accept float due to the precision loss.

One way is as suggested by @27pchrisl, otherwise might need to look for other date function that didn't loss that precision if that function exists?

@clemblanco
Copy link

This is the signature of gmdate

gmdate(string $format, ?int $timestamp = null): string

It should be an integer anyway.
I vote for @27pchrisl PR with the cast approach.

@driesvints
Copy link
Contributor Author

@JackEllis @stephenkhoo @27pchrisl @clemblanco wow thanks a lot for your help here! And for figuring that out! I agree, the PR from @27pchrisl looks good. Let's get that in! 💪

@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
5 participants