-
-
Notifications
You must be signed in to change notification settings - Fork 835
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
feat: Allow additional login params, Introduce LogInValidator
#3670
Conversation
Co-authored-by: David Wheatley <hi@davwheat.dev>
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.
Hmm, an event doesn't really seem appropriate to me. I'd rather we create a login validator, empty by default, and extensions can extend the validator instead.
🤔 yeah... That would make more sense, actually. On it... |
Why not both? That would allow for processing of data from a user logging in, and the more traditional approach to validation. |
LogInValidator
I don't know, it's not apparent to me what the processing of data could be, and if whatever it is it couldn't be handled using the exsiting extension API (like we did here with the validator), so I prefer dealing with one use case at a time to avoid over-engineering it. |
Precursor to https://discuss.flarum.org/d/31904-enabling-cloudflare-turnstile-on-login
Changes proposed in this pull request:
params
into an extendable function so that additional params can be added inLogInModal
LogInController
runs validation of the suppliedparams
against a newLogInValidator
- empty ruleset by default.Reviewers should focus on:
Necessity
Confirmed
composer test
).Required changes: