Skip to content

Validator service #567

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 10 commits into from
Closed

Validator service #567

wants to merge 10 commits into from

Conversation

elkuku
Copy link
Contributor

@elkuku elkuku commented May 27, 2017

This will move the validator methods in the console commands to a service.

Currently there is a bug caused by the fact that the delete-user command uses a different method to validate the username than the add-user command.

So you can actually add a user but you can't remove the user ...

Later in 3.3 this can be nicely injected in the constructor done 😉


class Validator
{
public function username($username)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the name could be: usernameValidation?

Copy link

Choose a reason for hiding this comment

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

validateUsername

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finding the "right" name has always been the hardest part in software design 😉

Copy link
Contributor

@voronkovich voronkovich May 28, 2017

Choose a reason for hiding this comment

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

@elkuku, You're right, but a method name in most cases should be beginning with a verb. :)

@elkuku
Copy link
Contributor Author

elkuku commented May 30, 2017

This has been updated and corresponding unit tests have been added.

javiereguiluz added a commit that referenced this pull request Jul 27, 2017
This PR was squashed before being merged into the master branch (closes #615).

Discussion
----------

Added a validator service for commands

I can't merge #567 because of the conflicts, so this is an attempt to merge it.

Commits
-------

5b02ca2 Added a validator service for commands
@javiereguiluz
Copy link
Member

Closing in favor of #615. @elkuku I'm sorry but I couldn't fix the conflicts of your PR while merging, so I needed to create a different PR. Thanks!

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.

5 participants