-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Cache] Fix incorrect timestamps generated by FilesystemAdapter #19435
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
Conversation
nicolas-grekas
commented
Jul 26, 2016
Q | A |
---|---|
Branch? | 3.1 |
Bug fix? | yes |
New feature? | no |
BC breaks? | no |
Deprecations? | no |
Tests pass? | yes |
Fixed tickets | #19431 |
License | MIT |
Doc PR | - |
@@ -126,7 +126,7 @@ protected function doDelete(array $ids) | ||
protected function doSave(array $values, $lifetime) | ||
{ | ||
$ok = true; | ||
$expiresAt = $lifetime ? time() + $lifetime : PHP_INT_MAX; | ||
$expiresAt = time() + ($lifetime ?: 31557600); // 31557600s = 1 year |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about defining a constant instead of adding the comment? Something like self::YEAR
would be self explanatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to keep the number. To be completely self-explanatory, the constant should be named self::ONE_YEAR_IN_SECONDS
or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a constant creates a public interface change. Even if we can tag it as internal, it looks overkill and the comment looks good enough to me. Also, whenever we want to change this default duration, we won't have to create a new const.
Status: reviewed 👍 |
👍 |
…apter (nicolas-grekas) This PR was merged into the 3.1 branch. Discussion ---------- [Cache] Fix incorrect timestamps generated by FilesystemAdapter | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19431 | License | MIT | Doc PR | - Commits ------- bd2e795 [Cache] Fix incorrect timestamps generated by FilesystemAdapter
This seems to be a issue in linux not in this component. In my opinion, this should be reverted, as PHP_INT_MAX is effectively cache forever whereas the new code defaults to one year.
If I had to make any guesses, I'll bet this happens when the year's representation can no longer fit into a 32 bit integer. EDIT: according to the article on the 2038 problem. I was close. It's 2^32 + 1900.
|