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

Add getMessageKey()/getMessageData() to ResetPasswordExceptionInterface #102

Closed
kbond opened this issue Apr 16, 2020 · 3 comments
Closed

Comments

@kbond
Copy link
Contributor

kbond commented Apr 16, 2020

Symfony's AuthenticationException has this to help with translation.

@weaverryan
Copy link
Contributor

Yo Kevin!

Hmm, I see yes. So the getReason() is already equivalent (I think) to getMessageKey(): both are basically hardcoded & safe "error strings". Unless you disagree, it means that we're just missing the getMessageData() method, which would give you access, for example, to the (for example) retry timeout in TooManyPasswordRequestsException. So, for that exception, it would look something like this:

public function getReasonData()
{
    return ['{{ retry_time }}' => $this->retryTime];
}

(btw, not sure on the naming there - should that exception give you the overall retry TTL? Or how much time is actually left until this one user can retry? And should it be in seconds, minutes? I don't think we can push a DateTime object into this, right?

Thanks!

@kbond
Copy link
Contributor Author

kbond commented Apr 17, 2020

So the getReason() is already equivalent (I think) to getMessageKey()

Ok, as long as it's understood that changing the getReason() text will be considered a BC break as it will break translation keys.

I don't think we can push a DateTime object into this, right?

I think the TooManyPasswordRequestsException should have a method that returns a DateTime object as to when it can be next retried and let the user determine how to use. We can discuss further in #101

it means that we're just missing the getMessageData()

I don't know if this is strictly necessary (and adding to the interface would be a BC break, correct?). Depending on how we handle the "retry after" for TooManyPasswordRequestsException it might not be feasible to put here...

kbond added a commit to kbond/reset-password-bundle that referenced this issue Apr 17, 2020
…loses SymfonyCasts#102)

- added `getAvailableAt()` to get a `DateTime` object for when a reset can be attempted again
- added `getRetryAfter()` to get the number of seconds before a reset can be attempted again
@weaverryan
Copy link
Contributor

Ok, as long as it's understood that changing the getReason() text will be considered a BC break as it will break translation keys.

Very good point. Yes, we will follow this 👍

I'm going to close for now, since we solved the specific problem with TooManyPasswordRequestsException.

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

No branches or pull requests

2 participants