Skip to content

Conversation

@gogume
Copy link

@gogume gogume commented Feb 23, 2015

I had the need to implement the AuthenticatesAndRegistersUsers trait with just the authentication part so I thougth that these two should be splited. Authentication doesn't really need registration, but registration should imply the need of authentication.
I've also added the need of implementing the two methods (validator and create) if someone use the AuthenticatesAndRegistersUsers trait, since these are used but not defined as needed.

@JosephSilber
Copy link
Contributor

This is not a breaking change, so should probably go to 5.0

@gogume
Copy link
Author

gogume commented Feb 23, 2015

I've made this pull request on the master mostly because of those two abstract methods. I have an older project starter and my AuthController didn't have the validator & create methods and that broke things. My IDE could not tell me that I need to implement thoes two methods, I had to dig into the code to see which changes broke my code and thoes changes are not in 5.0 branch.

@MaartenStaa
Copy link

Nice, though I know Graham will give you a list of coding style errors to fix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't do that.

Copy link
Author

Choose a reason for hiding this comment

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

Can you elaborate?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No extra new line.

Copy link
Author

Choose a reason for hiding this comment

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

OK, got it. If you find something else, let me know.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's multiple occurrences in this pull request.

@taylorotwell
Copy link
Member

No plans to implement this right now.

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.

6 participants