Skip to content

Validate user data before save #443

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

Closed
wants to merge 2 commits into from
Closed

Validate user data before save #443

wants to merge 2 commits into from

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Jan 17, 2017

  1. I've added a validateUserData method to check for existent user email (this improves a better messages exception) and validate plainPassword and email in non-interactive mode (i.e. $ app:add-user username password email).
  2. Renamed the option --is-admin to just --admin, Imho is-admin feels more like a question rather than an instruction. (now $ app:add-user username password email --admin)

Any suggestion is welcome!

@javiereguiluz
Copy link
Member

@yceruto thanks for these improvements. I like them. The only question I have is about the --is-admin to --admin reword.

It's true that "is admin" can be understood as a question: "john-doe is admin?" but also as a non-question sentence: "john-doe is admin." In any case, --is-admin is not perfect. However, using just --admin is not perfectly clearly either (it can be understood in several ways in addition to the intended one).

So, let's wait for other opinions before merging it. Thanks!

@yceruto
Copy link
Member Author

yceruto commented Jan 20, 2017

As I see it in others commands with similar option, this flag is directly related to the command name rather than its related options, i.e. app:add-user --admin, it's like "I want: to add an admin user", or app:add-user john --admin (add an admin user named john).

This approach in FOSUserBundle commands for example:

Symfony commands also follow this approach for your flag options, but I'm fine with --is-admin approach too :)

@javiereguiluz
Copy link
Member

That's a nice argument. Reusing the existing practices is always a good idea.

@javiereguiluz
Copy link
Member

Merged! Thanks for improving this command!

@yceruto yceruto deleted the add-user-cmd branch January 22, 2017 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants