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

[bug] increase time before expired requests are garbage collected to 1 week #103

Merged
merged 1 commit into from
Apr 17, 2020

Conversation

kbond
Copy link
Contributor

@kbond kbond commented Apr 17, 2020

Fix for #100.

@jrushlow
Copy link
Collaborator

I'm in favor of the concept of this change, but it will break manual garbage collection runs. I think that using reset-password:remove-expired should remove requests that are expired at the time the command is run. This allows for a more customized garbage collection implementation.

But if we merge this PR in now, it forces the command to leave expired requests in persistence.

@weaverryan
Copy link
Contributor

Hmm, good point. Do you have some ideas how to do this @jrushlow? We could add an optional \DateTimeInterface $since argument to removeExpiredResetPasswordRequests(), but then we would still need to call that with different arguments from ResetPasswordCleaner:: handleGarbageCollection() based on the context.

Whatever we do, let's try to get it done as quickly as we can. This could contain a BC break... and I don't want to release a v2 for an internal interface change. So we might be able to "get away with it" if we have a fast release - I can take the blame :)

@kbond
Copy link
Contributor Author

kbond commented Apr 17, 2020

I would want the garbage collection command to follow the same logic as the auto (or at least be configurable). If running the cron nightly, I wouldn't want to prevent people who requested the day before to not get the expired token message if they click the link the day after the cron job.

@weaverryan
Copy link
Contributor

Fair point :).

So, I'm guessing that we probably need to make this "1 week ago" part configurable. Which means:

A) Add an argument to removeExpiredResetPasswordRequests() for the Date
B) Pass the correct DateTime in from ResetPasswordCleaner:: handleGarbageCollection()
C) Add a Configuration option to default it to -1 week

Thoughts?

@weaverryan
Copy link
Contributor

Going to merge and keep it simple for now. Thanks @kbond!

@weaverryan weaverryan merged commit a2cc78c into SymfonyCasts:master Apr 17, 2020
@kbond kbond deleted the bug/remove-expired-time branch April 17, 2020 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants