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

Update ResetPasswordToken.php #225

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LorenzoNet15
Copy link

there is a problem when you have a timezone gmt +2 on server
Just compare the interval always in GMT

there is a problem when you have a timezone gmt +2 on server
Just compare the interval always in GMT
Copy link

@gaetshun gaetshun left a 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

Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

Good catch 👍

@gaetshun
Copy link

Could you merge soon this issue ? @bocharsky-bw

@bocharsky-bw
Copy link
Member

If you confirm it solves the problem - I think I can.

Could you re-build the tests to see they pass?

@gaetshun
Copy link

Yes I can confirm that solves the problem.
Sorry but I am novice on github and I don't know what you mean when you tell rebuild test !! let me know how can I do that?

Thank you

@bocharsky-bw
Copy link
Member

bocharsky-bw commented Sep 16, 2022

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

@weaverryan weaverryan added the Status: Reviewed Has been reviewed by a maintainer label Sep 19, 2022
Copy link
Collaborator

@jrushlow jrushlow left a 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

Comment on lines +142 to +144
$expiresAt = \DateTimeImmutable::createFromFormat('U', (string) $this->expiresAt->getTimestamp());

return $this->expiresAt->diff($createdAtTime);
return $expiresAt->diff($createdAtTime);
Copy link
Collaborator

@jrushlow jrushlow Sep 27, 2022

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.

  1. 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 to diff'ing DateTime instances that were fixed in PHP 8.1.10.

  2. 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.

Copy link
Member

Choose a reason for hiding this comment

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

  1. 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.

  2. I probably missed the fact that the $expiresAt is instantiated in the Maker bundle. If so - it makes sense to fix it there instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants