8000 [Validator] UuidValidator must accept a Uuid constraint. by hhamon · Pull Request #19267 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Validator] UuidValidator must accept a Uuid constraint. #19267

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 4, 2016
Merged

[Validator] UuidValidator must accept a Uuid constraint. #19267

merged 1 commit into from
Jul 4, 2016

Conversation

hhamon
Copy link
Contributor
@hhamon hhamon commented Jul 1, 2016
Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets ~
License MIT
Doc PR ~

@hhamon hhamon force-pushed the improve-uuid-validator branch from 32ffd03 to 23fc892 Compare July 1, 2016 19:12
@hhamon hhamon force-pushed the improve-uuid-validator branch from 23fc892 to ff8bb4b Compare July 1, 2016 19:13
@hhamon
Copy link
Contributor Author
hhamon commented Jul 1, 2016

At first sight it may look like a BC break but the UuidValidator class already has several methods arguments that are typehinted on the Uuid constraint concrete class in their signatures.

@ro0NL
Copy link
Contributor
ro0NL commented Jul 2, 2016 •
edited
Loading

Im not sure.. some validators do it, others dont. There are also not much validators that typehint against constraints (ie the job is always done directy in validate()).

Currently we can give an any constraint providing the same options (which is nice i guess... but only works if we depend on constraint options, not specific constraints) for most validators. This should be consistent imo.

Undefined option calls failing due invalid constraints is a different story :) It could be solved with something like Constraint::get($constraint, 'message', 'Default message');..

@hhamon
Copy link
Contributor Author
hhamon commented Jul 2, 2016

If you look at the validateLoose and validateStrict methods, they already typehint against the Uuid constraint concrete type. It was already forced by design.

@ro0NL
Copy link
Contributor
ro0NL commented Jul 2, 2016 •
edited
Loading

From a class scope pov the type check is good (ie. better than violating typehints), from a package scope pov i would just stick with private function validateLoose($value, Constraint $constraint). However currently its in between.. so lets see :)

@fabpot
Copy link
Member
fabpot commented Jul 2, 2016

Do we have any other class with such a check? If not, I propose to close this PR as it does not really add any value.

@ro0NL
Copy link
Contributor
ro0NL commented Jul 3, 2016 •
edited
Loading

This one added a lot: df56c23

edit: edit: Seems like Uuid is the last one without such a check :') lets do it.
8000

@hhamon
Copy link
Contributor Author
hhamon commented Jul 3, 2016

@fabpot every other concrete validator classes does that! For instance LengthValidator::validate method checks the received constraint is an instance of the Length constraint.

@fabpot
Copy link
Member
fabpot commented Jul 4, 2016

Thank you @hhamon.

@fabpot fabpot merged commit ff8bb4b into symfony:2.7 Jul 4, 2016
fabpot added a commit that referenced this pull request Jul 4, 2016
@fabpot
bug #19267 [Validator] UuidValidator must accept a Uuid constraint. (…
0259bed 
…hhamon)

This PR was merged into the 2.7 branch.

Discussion
----------

[Validator] UuidValidator must accept a Uuid constraint.

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ~
| License       | MIT
| Doc PR        | ~

Commits
-------

ff8bb4b [Validator] UuidValidator must accept a Uuid constraint.
This was referenced Jul 30, 2016
Merged
Merged
Merged
Merged
@hhamon hhamon deleted the improve-uuid-validator branch February 22, 2017 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug Status: Needs Review Validator
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hhamon @ro0NL @fabpot @carsonbot
0