Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Validator] Added ConstraintValidator::buildViolation() helper for BC with the 2.4 API#12016
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
stof commentedSep 24, 2014
this will help for the core, but not for other bundles as they will still need to support Symfony 2.3 LTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you can use&& instead of nesting conditions
stof commentedSep 24, 2014
Given the few places where you changed the constraint validators to split adding violations, I suspect this is done to simplify the addition of violation codes. So 👍 once you fix the tests (there is a fatal error currently) |
webmozart commentedSep 24, 2014
@stof In 2.3 we didn't have ConstraintViolationBuilders yet, so there's no point in adding it there. Yes, I split the code where I'm going to add error codes in 2.6. Doing the structural changes in 2.5 already, we facilitate merges from 2.5 to 2.6 when we change 2.5 in the future. |
c78a9b0 to31f0764Compare6f94e5d to682b734Compare682b734 to6b0c24aComparewebmozart commentedSep 25, 2014
This is ready to merge from my side. The fabbot failures are due to fabbot bugs. ping @symfony/deciders |
webmozart commentedSep 25, 2014
The remaining failing tests are due to#12034. |
fabpot commentedSep 25, 2014
Thank you@webmozart. |
…lper for BC with the 2.4 API (webmozart)This PR was merged into the 2.5 branch.Discussion----------[Validator] Added ConstraintValidator::buildViolation() helper for BC with the 2.4 API| Q | A| ------------- | ---| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -This PR adds a `buildViolation()` helper method to the base `ConstraintValidator` to remove the API checks (2.4 vs. 2.5) from the constraint validator implementations. Once the 2.4 API is removed, this helper method will be removed as well.**Todos**- [x] Backport changes from#12021Commits-------6b0c24a [Validator] Added ConstraintValidator::buildViolation() helper for BC with 2.4 API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry@webmozart but this is not the same
I would prefer const PATTERN = '/^(\d{4})-(\d{2})-(\d{2}) (\d{2}):(\d{2}):(\d{2})$/';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Erm... that's exactly what's written there. Typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
sorry@webmozart, copy & paste error.
I'm wondering why you changed the pattern.
I would prefer
<?php const PATTERN = '/^(0[0-9]|1[0-9]|2[0-3]):([0-5][0-9]):([0-5][0-9])$/';Your pattern can be used with invalid timestamps and in
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Tests/Constraints/DateTimeValidatorTest.php I see no tests covering wrong time values.
What is the reason for changing the pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Did you see thecheckTime() andcheckDate() assertions below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The test input2010-01-01 00:60:00 is testing an invalid time value, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok, you are right. Thank you for the feedback@webmozart
…multiple error causes (webmozart)This PR was merged into the 2.6-dev branch.Discussion----------[Validator] Added error codes to all constraints with multiple error causes| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#7276| License | MIT| Doc PR | TODOThis PR depends on#12015 and#12016 being merged first. However, a few changes in52cb7df first must be backported to#12016.This PR introduces error codes for all constraints with multiple error paths. This lets you determine what exactly the reason was that a constraint failed:```php$validator = Validation::createValidator();$violations = $validator->validate('0-4X19-92619812', new Isbn());foreach ($violations as $violation) { var_dump($violation->getCode()); // => int(3) var_dump(Isbn::getErrorName($violation->getCode())); // => string(24) "ERROR_INVALID_CHARACTERS" var_dump($violation->getConstraint()->getErrorName($violation->getCode())); // => string(24) "ERROR_INVALID_CHARACTERS"}```The `getErrorName()` method is especially helpful for REST APIs, where you can return both an error code and a description of that error now.**Todos**- [x] Backport a few structural changes to#12016- [x] Update constraints outside of the Validator component- [x] Rebase on master (after merging#12015 and#12016)Commits-------3b50bf2 [Validator] Added error codes to all constraints with multiple error causes
This PR adds a
buildViolation()helper method to the baseConstraintValidatorto remove the API checks (2.4 vs. 2.5) from the constraint validator implementations. Once the 2.4 API is removed, this helper method will be removed as well.Todos