-
Notifications
You must be signed in to change notification settings - Fork 68
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
Update ResetPasswordToken.php #225
base: main
Are you sure you want to change the base?
Conversation
there is a problem when you have a timezone gmt +2 on server Just compare the interval always in GMT
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 approved your change, I opened an issue yesterday for that #241
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.
Good catch 👍
Could you merge soon this issue ? @bocharsky-bw |
If you confirm it solves the problem - I think I can. Could you re-build the tests to see they pass? |
Yes I can confirm that solves the problem. Thank you |
I noticed a failed Static Analysis build here, but it's outdated already so it has no context anymore, let's trigger a new build. For this, you can just push a new commit. We can cheat with an empty commit: $ git commit --allow-empty -m "An empty comment" And then push the changes :) It should trigger a new build to see if tests are passed now |
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.
Couple of thoughts - Requesting changes so i don't accidentally merge.
Also if you could update the title of the PR to better describe what this PR is doing, that would be awesome. We use the PR titles for changelog entries
$expiresAt = \DateTimeImmutable::createFromFormat('U', (string) $this->expiresAt->getTimestamp()); | ||
|
||
return $this->expiresAt->diff($createdAtTime); | ||
return $expiresAt->diff($createdAtTime); |
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'm not 100% sure this is the best way to fix this "bug". Two things come to mind when I'm looking at this.
-
What version of PHP are you using that you're getting an incorrect
DateInterval
instance? PHP 8.1 through 8.1.9 have several bugs related todiff
'ing DateTime instances that were fixed in PHP 8.1.10. -
If we have to create a new expires at instance to get a "correct" interval, why not just fix the existing
$expiresAt
instance that is set in the constructor? Meaning, if we are instantiating that instance incorrectly, we should fix it at the source. (e.g. MakerBundle)
I would think that for the users who are using this bundle on its own, this change would be a BC because when they call ResetPasswordToken::getExpiresAt()
the \DateTimeImuttable
instance returned would not be "compatible" w/ the \DateInterval
instance they are getting from this method.
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.
-
Hm, good question about the PHP version, it might be related to that known bug, so maybe it does not a problem on 8.1.10 anymore? If so, then all you need to do is to upgrade your PHP version.
-
I probably missed the fact that the
$expiresAt
is instantiated in the Maker bundle. If so - it makes sense to fix it there instead
there is a problem when you have a timezone gmt +2 on server
Just compare the interval always in GMT