[Validator] Added ConstraintValidator::buildViolation() helper for BC with the 2.4 API #12016
Conversation
|
this will help for the core, but not for other bundles as they will still need to support Symfony 2.3 LTS |
| '{{ value }}' => $this->formatValue($value), | ||
| )); | ||
| if ($constraint->checkHost) { | ||
| if (!$this->checkHost($host)) { |
There was a problem hiding this comment.
you can use && instead of nesting conditions
|
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) |
|
@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 to
31f0764
Compare
6f94e5d to
682b734
Compare
682b734 to
6b0c24a
Compare
|
This is ready to merge from my side. The fabbot failures are due to fabbot bugs. ping @symfony/deciders |
|
The remaining failing tests are due to #12034. |
|
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 #12021 Commits ------- 6b0c24a [Validator] Added ConstraintValidator::buildViolation() helper for BC with 2.4 API
| @@ -21,7 +21,7 @@ | |||
| */ | |||
| class DateTimeValidator extends DateValidator | |||
| { | |||
| const PATTERN = '/^(\d{4})-(\d{2})-(\d{2}) (0[0-9]|1[0-9]|2[0-3]):([0-5][0-9]):([0-5][0-9])$/'; | |||
There was a problem hiding this comment.
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.
Erm... that's exactly what's written there. Typo?
There was a problem hiding this comment.
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.
Did you see the checkTime() and checkDate() assertions below?
There was a problem hiding this comment.
The test input 2010-01-01 00:60:00 is testing an invalid time value, for example.
There was a problem hiding this comment.
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 | TODO This PR depends on #12015 and #12016 being merged first. However, a few changes in 52cb7df 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