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 handleRegistration() method to register page #12020

Merged
merged 4 commits into from Mar 25, 2024

Conversation

dododedodonl
Copy link
Contributor

@dododedodonl dododedodonl commented Mar 24, 2024

Description

When customizing the user creation process, the whole register method has to be overwritten. With this, only the creation itself can be customized.

When making this edit, I was not sure what return type to choose. I ended up with choosing Authenticatable, the one required for ->login(). Should the $user parameter of sendEmailVerificationNotification() not also be changed to Authenticatable instead of Model?

Visual changes

None

Functional changes

  • Code style has been fixed by running the composer cs command.
  • Changes have been tested to not break existing functionality.
  • Documentation is up-to-date.

@zepfietje zepfietje changed the title Add method to register page Add handleRegistration() method to register page Mar 25, 2024
@zepfietje
Copy link
Member

When making this edit, I was not sure what return type to choose. I ended up with choosing Authenticatable, the one required for ->login(). Should the $user parameter of sendEmailVerificationNotification() not also be changed to Authenticatable instead of Model?

Changing the param type of existing methods would be a breaking change, right?

Maybe @danharrin knows the reasoning for using Model instead of Authenticatable?

@zepfietje zepfietje added this to the v3 milestone Mar 25, 2024
@zepfietje zepfietje added the enhancement New feature or request label Mar 25, 2024
@danharrin
Copy link
Member

danharrin commented Mar 25, 2024

Changing to Model for consistency. I don't think there's much reason to have one over the other, but there might be a situation where someone is implementing a custom login/registration flow that does not use Laravel's built in auth logic, so Authenticatable might not be applied. Model just feels safer considering we are interacting with Eloquent database features, and more open to extension.

@danharrin danharrin merged commit 3e16791 into filamentphp:3.x Mar 25, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants