Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Date validator problem vant validate big unix timestamps #6359

Closed
wants to merge 3 commits into from

Conversation

svycka
Copy link
Contributor

@svycka svycka commented Jun 7, 2014

fixes #6352

@svycka
Copy link
Contributor Author

svycka commented Jun 7, 2014

not sure about tests maybe someone can help?

@samsonasik
Copy link
Contributor

ping @DASPRiD

@@ -70,8 +70,10 @@ public function datesDataProvider()
// int
array(0, null, true),
array(1340677235, null, true),
// Commenting out, as value appears to vary based on OS
// array(999999999999, null, true),
// 32bit version of php will convert this to double
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this affect the outcome?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes in 32-bit 999,999,999,999 was converted to 2,147,483,647 when casting to double. you can see the examples in this issue: #6352

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if gmp can help here... is the value already broken when leaving the data-provider?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read this entirely wrong; this test uncomments a test set. We're covered, and verified this passes.

@weierophinney
Copy link
Member

My notes:

  • The test was commented out because the contributor is using a 32-bit architecture, and that test failed due to int -> float conversion.
  • The referenced issue essentially asks if the framework should act the same on 32- vs 64-bit architectures.
  • Our own continuous integration uses 64-bit architectures.

My take is: no, the framework should not handle this detail. The reason is that the value passed as a timestamp to DateTime must be an integer for reliable results.

This means we cannot pass a string (e.g., for use with bcmath) or GMP instance and expect reliable results. At the same time, we need to test sufficiently large numbers in order to verify everything works as expected.

My take would be to test using PHP_INT_MAX, which will vary based on architecture; we can then change expectations based on architecture (using PHP_INT_SIZE, which will be 4 on 32-bit and 8 on 64-bit).

@weierophinney weierophinney added this to the 2.4.0 milestone Feb 19, 2015
weierophinney added a commit that referenced this pull request Mar 18, 2015
Date validator problem vant validate big unix timestamps
weierophinney added a commit that referenced this pull request Mar 18, 2015
@weierophinney
Copy link
Member

Merged to develop for release with 2.4.

weierophinney added a commit to zendframework/zend-validator that referenced this pull request May 15, 2015
Date validator problem vant validate big unix timestamps
weierophinney added a commit to zendframework/zend-validator that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Date validator problem vant validate big unix timestamps
6 participants