8000 merged branch bschussek/issue3228 (PR #3317) · symfony/symfony@545c6f2 · GitHub
[go: up one dir, main page]
Skip to content

Commit 545c6f2

Browse files
fabpotfabpot
committed
merged branch bschussek/issue3228 (PR #3317)
Commits ------- 5208bbe [Validator] Fixed typo, updated CHANGELOG and UPGRADE dc059ab [Validator] Added default validate() implementation to ConstraintValidator for BC 6336d93 [Validator] Renamed ConstraintValidatorInterface::isValid() to validate() because of the lack of a return value 46f0393 [Validator] Removed return value from ConstraintValidatorInterface::isValid() Discussion ---------- [Validator] Renamed ConstraintValidatorInterface::isValid() to validate() and removed return value Bug fix: no Feature addition: no Backwards compatibility break: **YES** Symfony2 tests pass: yes Fixes the following tickets: - Todo: update the documentation ![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3228) Before I begin, this PR is up for discussion. I removed the return value of ConstraintValidator::isValid() because it wasn't used anymore within the framework. Removing it also means a simplification for userland implementations. Already now the validation component only depended on violation errors being present for deciding whether the validation was considered failed or not. Because the name `isValid` does not make a lot of sense without a return value, I changed it to `validate`. Note that this affects an interface (ConstraintValidatorInterface) previously marked with `@api` by us! What do you think about this change? --------------------------------------------------------------------------- by stof at 2012-02-09T17:51:38Z @bschussek IIRC, the Validator component was part of the components that are not considered as stable for 2.0 (there is 4 of them IIRC, including Config, Security and Form) so changing the interface should not be an issue here --------------------------------------------------------------------------- by lsmith77 at 2012-02-09T17:54:55Z No it was .. form wasn't: http://symfony.com/doc/2.0/book/stable_api.html --------------------------------------------------------------------------- by rdohms at 2012-02-10T13:23:32Z I fail to see the value of the BC in this case. Just because the framework does not use given functionality anymore is not reason to drop it, since all of this was clearly proposed as a "component" to be used in other projects. Other implementations of validator in other projects might actually depend on this. Is it possible to just add a new value and have both functionalities available? --------------------------------------------------------------------------- by stof at 2012-02-10T13:25:12Z @rdohms the point is that the return value is confusing. Someone may return ``false`` by thinking it will mark the field as invalid (which is implied by the name ``isValid``) whereas it is not the case at all --------------------------------------------------------------------------- by bschussek at 2012-02-10T13:30:13Z Exactly. UniqueEntityValidator for example always returned `true` and nobody ever noticed. --------------------------------------------------------------------------- by beberlei at 2012-02-10T13:53:03Z @bschussek but its not a bug, setting the execution context failure is enough. returning false would lead to a second error message being evicted. This is the weird problem of the API imho --------------------------------------------------------------------------- by bschussek at 2012-02-10T13:54:49Z @beberlei This has already been fixed. --------------------------------------------------------------------------- by stof at 2012-02-10T13:59:59Z @beberlei in the master branch, errors are not duplicated anymore as the return value is simply ignored. --------------------------------------------------------------------------- by Tobion at 2012-02-10T14:29:03Z I'm +1. If people are concerned about this strong BC break we could maybe add a fallback for the majority. Most people propably have extended the ConstraintValidator and not used the interface directly. So we can change the Interface and at the same time provide a default proxy method in ConstraintValidator for validate. I.e. public function validate($value, Constraint $constraint) { $this->isValid($value, $constraint); } Thus all people who have extended ConstraintValidator won't notice a BC break. --------------------------------------------------------------------------- by hades200082 at 2012-02-10T16:10:31Z Could you not have both validate and isValid as separate methods with distinct purposes? --------------------------------------------------------------------------- by stof at 2012-02-10T16:55:12Z @hades200082 which distinct purposes ? --------------------------------------------------------------------------- by hades200082 at 2012-02-10T17:02:57Z One should actually validate. The other should return whether it is valid or not as a bool. Even if isValid calls validate to determine this surely they are distinct purposes? doing it this way would not require a break to BC but the existing components in the framework could be switched to use validate. --------------------------------------------------------------------------- by bschussek at 2012-02-10T17:10:50Z @hades200082 Validators are stateless. They don't "remember" whether they validated successfully or not. --------------------------------------------------------------------------- by hades200082 at 2012-02-10T17:13:25Z Maybe they should? Would save revalidating every time --------------------------------------------------------------------------- by stof at 2012-02-10T17:16:10Z @hades200082 how could they be stateless ? you can use the same instance to validate several values. For instance, the UniqueEntityValidator is a service and so will be reused. --------------------------------------------------------------------------- by fabpot at 2012-02-11T23:40:09Z I would really like that we do not break BC in this case. --------------------------------------------------------------------------- by stof at 2012-02-11T23:59:02Z @fabpot there is also a BC break in the previous changes: the return value has no meaning at all now (it is not even considered by the GraphWalker. Most 2.0 validator will continue working because of the new implementation of setMessage but I can provide the 2 broken cases: ```php <?php /** * This validator always set the message, even when it is valid to keep things simple. * This works fine in 2.0.x (as the return value is what makes the decision) but will * add a violation in 2.1 (setMessage adds the violation to keep things working for * cases setting the message only for invalid values, like the core used to do). */ public function isValid($value, Constraint $constraint) { $this->setMessage($constraint->message); return true; } /** * This validator never set the message, failing with an empty message. * This works fine in 2.0.x (as the return value is what makes the decision) but will * not add the violation in 2.1. */ public function isValid($value, Constraint $constraint) { return false; } ``` The second one is clearly an edge case as it would absolutely not be user-friendly but the first one makes totally sense when using the 2.0.x API (with a boolean expression using the value of course) --------------------------------------------------------------------------- by fabpot at 2012-02-12T00:11:19Z I agree with you; I should probably have refused to merge the previous PR. And I think we need to reconsider this change. If not, why are we even bothering tagging stuff with the @api tag? --------------------------------------------------------------------------- by bschussek at 2012-02-12T10:15:55Z @stof I disagree with you. Setting an error message but not letting the validation fail is not how the API is supposed to work. Also the opposite was not meant to work, as it results in empty error messages. The third example is that a validator *had* to return true if it called `addViolation` directly. These cases show that the previous implementation was clearly buggy and needed to be fixed. This PR is only a consequence that cleans the API up. @fabpot IMHO validator was too young and not tried enough to be marked as stable. But as we can't change this anymore, I think the decision we have to make is * BC/reliance on `@api` marks vs. * API usability (also considering the coming LTR) --------------------------------------------------------------------------- by bschussek at 2012-02-12T10:18:12Z BTW @Tobion's suggestion could definitely make a transition easier. --------------------------------------------------------------------------- by fabpot at 2012-02-15T10:26:10Z @bschussek +1 for @Tobion's suggestion. --------------------------------------------------------------------------- by Brouznouf at 2012-02-15T16:06:12Z Could be nice to comment function as deprecated and/or trigger a E_USER_DEPRECATED error when using this method to prevent user calling this method. --------------------------------------------------------------------------- by stof at 2012-02-15T16:09:37Z trigger E_USER_DEPRECATED would be wrong as the kernel set the error reporting to ``-1`` and registers an error handler tuning all reported errors to exception in debug mode, so it would be a BC break. Commenting the function as deprecated in indeed possible --------------------------------------------------------------------------- by rdohms at 2012-02-29T11:15:01Z Went back to working on validators and it really makes me disagree with these changes a little more. Let me explain. In the isValid method, i like to work with return early checks, so straight up i check some stuff and return early either true/false. From the frameworks perspective true/false does not make a difference, but from a reader's perspective it adds a lot of value: if ($object->getId() === null) { return true; } versus if ($object->getId() === null) { return; } having the return true make it clear that in this case the object is valid for anyone who is reading my validator. i think this is a good practice to push forward. Anyway, my 2 cents on it. --------------------------------------------------------------------------- by stof at 2012-04-04T00:05:09Z @fabpot what do you think about this ? --------------------------------------------------------------------------- by bschussek at 2012-04-05T16:37:38Z @rdohms: Still, how do you want to deal with the fact that the return value is ignored anyway? --------------------------------------------------------------------------- by rdohms at 2012-04-06T06:51:07Z @bschussek Nobody has to know? I would keep it as it is, i have noticed that returning false without any error messages does not get me the expected results, so it seems there is no harm in keeping the parctice of true/false even if it is misleading. Other then that.. i would alter the code to self create a error message if false is returned, thus making true/false still work, but i'm guessing that's not what your vision says, even if i find it les readable and understandable. So yeah, just my opinioin. --------------------------------------------------------------------------- by bschussek at 2012-04-06T07:02:53Z @rdohms: Your opinion is appreciated. Self-creation of error messages is what we did before, unfortunately it's very hacky then to suppress the self-creation if you want to return false and add (potentially more than one) error messages yourself. --------------------------------------------------------------------------- by bschussek at 2012-04-17T14:58:07Z I added @Tobion's suggestion now. Can you please review again? Otherwise this is ready for merge. --------------------------------------------------------------------------- by Tobion at 2012-04-17T15:05:16Z Statement in changelog and upgrade is missing, or?
2 parents a721f1b + 5208bbe commit 545c6f2

File tree

69 files changed

+499
-567
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

69 files changed

+499
-567
lines changed

‎CHANGELOG-2.1.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -434,6 +434,9 @@ To get the diff between two versions, go to https://github.com/symfony/symfony/c
434434
* deprecated Constraint methods `setMessage`, `getMessageTemplate` and
435435
`getMessageParameters`
436436
* added support for dynamic group sequences with the GroupSequenceProvider pattern
437+
* [BC BREAK] ConstraintValidatorInterface method `isValid` has been renamed to
438+
`validate`, its return value was dropped. ConstraintValidator still contains
439+
`isValid` for BC
437440

438441
### Yaml
439442

‎UPGRADE-2.1.md

Lines changed: 103 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@
135135
* `MutableAclInterface::setParentAcl` now accepts `null`, review any
136136
implementations of this interface to reflect this change.
137137
138-
### Form and Validator
138+
### Form
139139
140140
* Child forms are no longer automatically validated. That means that you must
141141
explicitly set the `Valid` constraint in your model if you want to validate
@@ -283,8 +283,86 @@
283283
* `FormUtil::toArrayKey()` and `FormUtil::toArrayKeys()` have been removed.
284284
They were merged into ChoiceList and have no public equivalent anymore.
285285
286+
* The options passed to the `getParent()` method of form types no longer
287+
contain default options. They only contain the options passed by the user.
288+
289+
You should check if options exist before attempting to read their value.
290+
291+
Before:
292+
293+
```
294+
public function getParent(array $options)
295+
{
296+
return 'single_text' === $options['widget'] ? 'text' : 'choice';
297+
}
298+
```
299+
300+
After:
301+
302+
```
303+
public function getParent(array $options)
304+
{
305+
return isset($options['widget']) && 'single_text' === $options['widget'] ? 'text' : 'choice';
306+
}
307+
```
308+
309+
* The methods `getDefaultOptions()` and `getAllowedOptionValues()` of form
310+
types no longer receive an option array.
311+
312+
You can specify options that depend on other options using closures instead.
313+
314+
Before:
315+
316+
```
317+
public function getDefaultOptions(array $options)
318+
{
319+
$defaultOptions = array();
320+
321+
if ($options['multiple']) {
322+
$defaultOptions['empty_data'] = array();
323+
}
324+
325+
return $defaultOptions;
326+
}
327+
```
328+
329+
After:
330+
331+
```
332+
public function getDefaultOptions()
333+
{
334+
return array(
335+
'empty_data' => function (Options $options, $previousValue) {
336+
return $options['multiple'] ? array() : $previousValue;
337+
}
338+
);
339+
}
340+
```
341+
342+
The second argument `$previousValue` does not have to be specified if not
343+
needed.
344+
345+
* The `add()`, `remove()`, `setParent()`, `bind()` and `setData()` methods in
346+
the Form class now throw an exception if the form is already bound.
347+
348+
If you used these methods on bound forms, you should consider moving your
349+
logic to an event listener that observes one of the following events:
350+
`FormEvents::PRE_BIND`, `FormEvents::BIND_CLIENT_DATA` or
351+
`FormEvents::BIND_NORM_DATA`.
352+
353+
* The interface FormValidatorInterface was deprecated and will be removed
354+
in Symfony 2.3.
355+
356+
If you implemented custom validators using this interface, you can
357+
substitute them by event listeners listening to the FormEvents::POST_BIND
358+
(or any other of the BIND events). In case you used the CallbackValidator
359+
class, you should now pass the callback directly to `addEventListener`.
360+
361+
### Validator
362+
286363
* The methods `setMessage()`, `getMessageTemplate()` and
287-
`getMessageParameters()` in the Constraint class were deprecated.
364+
`getMessageParameters()` in the Constraint class were deprecated and will
365+
be removed in Symfony 2.3.
288366
289367
If you have implemented custom validators, you should use the
290368
`addViolation()` method on the `ExecutionContext` object instead.
@@ -350,80 +428,45 @@
350428
}
351429
```
352430
353-
* The options passed to the `getParent()` method of form types no longer
354-
contain default options. They only contain the options passed by the user.
431+
* The method `isValid` of `ConstraintValidatorInterface` was renamed to
432+
`validate` and its return value was dropped.
355433
356-
You should check if options exist before attempting to read their value.
434+
`ConstraintValidator` still contains the deprecated `isValid` method and
435+
forwards `validate` calls to `isValid` by default. This BC layer will be
436+
removed in Symfony 2.3. You are advised to rename your methods. You should
437+
also remove the return values, which have never been used by the framework.
357438
358439
Before:
359440
360441
```
361-
public function getParent(array $options)
442+
public function isValid($value, Constraint $constraint)
362443
{
363-
return 'single_text' === $options['widget'] ? 'text' : 'choice';
444+
// ...
445+
if (!$valid) {
446+
$this->context->addViolation($constraint->message, array(
447+
'{{ value }}' => $value,
448+
));
449+
450+
return false;
451+
}
364452
}
365453
```
366454
367455
After:
368456
369457
```
370-
public function getParent(array $options)
458+
public function validate($value, Constraint $constraint)
371459
{
372-
return isset($options['widget']) && 'single_text' === $options['widget'] ? 'text' : 'choice';
373-
}
374-
```
375-
376-
* The methods `getDefaultOptions()` and `getAllowedOptionValues()` of form
377-
types no longer receive an option array.
378-
379-
You can specify options that depend on other options using closures instead.
380-
381-
Before:
382-
383-
```
384-
public function getDefaultOptions(array $options)
385-
{
386-
$defaultOptions = array();
387-
388-
if ($options['multiple']) {
389-
$defaultOptions['empty_data'] = array();
460+
// ...
461+
if (!$valid) {
462+
$this->context->addViolation($constraint->message, array(
463+
'{{ value }}' => $value,
464+
));
465+
466+
return;
390467
}
391-
392-
return $defaultOptions;
393468
}
394469
```
395-
10000
396-
After:
397-
398-
```
399-
public function getDefaultOptions()
400-
{
401-
return array(
402-
'empty_data' => function (Options $options, $previousValue) {
403-
return $options['multiple'] ? array() : $previousValue;
404-
}
405-
);
406-
}
407-
```
408-
409-
The second argument `$previousValue` does not have to be specified if not
410-
needed.
411-
412-
* The `add()`, `remove()`, `setParent()`, `bind()` and `setData()` methods in
413-
the Form class now throw an exception if the form is already bound.
414-
415-
If you used these methods on bound forms, you should consider moving your
416-
logic to an event listener that observes one of the following events:
417-
`FormEvents::PRE_BIND`, `FormEvents::BIND_CLIENT_DATA` or
418-
`FormEvents::BIND_NORM_DATA`.
419-
420-
* The interface FormValidatorInterface was deprecated and will be removed
421-
in Symfony 2.3.
422-
423-
If you implemented custom validators using this interface, you can
424-
substitute them by event listeners listening to the FormEvents::POST_BIND
425-
(or any other of the BIND events). In case you used the CallbackValidator
426-
class, you should now pass the callback directly to `addEventListener`.
427470
428471
### Session
429472

‎src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.php

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,8 @@ public function __construct(ManagerRegistry $registry)
4040
/**
4141
* @param object $entity
4242
* @param Constraint $constraint
43-
*
44-
* @return bool
4543
*/
46-
public function isValid($entity, Constraint $constraint)
44+
public function validate($entity, Constraint $constraint)
4745
{
4846
if (!is_array($constraint->fields) && !is_string($constraint->fields)) {
4947
throw new UnexpectedTypeException($constraint->fields, 'array');
@@ -74,7 +72,7 @@ public function isValid($entity, Constraint $constraint)
7472
$criteria[$fieldName] = $class->reflFields[$fieldName]->getValue($entity);
7573

7674
if (null === $criteria[$fieldName]) {
77-
return true;
75+
return;
7876
}
7977

8078
if ($class->hasAssociation($fieldName)) {
@@ -113,11 +111,9 @@ public function isValid($entity, Constraint $constraint)
113111
* unique.
114112
*/
115113
if (0 === count($result) || (1 === count($result) && $entity === ($result instanceof \Iterator ? $result->current() : current($result)))) {
116-
return true;
114+
return;
117115
}
118116

119117
$this->context->addViolationAtSubPath($fields[0], $constraint->message, array(), $criteria[$fields[0]]);
120-
121-
return true; // all true, we added the violation already!
122118
}
123119
}

‎src/Symfony/Bundle/SecurityBundle/Validator/Constraint/UserPasswordValidator.php

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ public function __construct(SecurityContextInterface $securityContext, EncoderFa
2929
$this->encoderFactory = $encoderFactory;
3030
}
3131

32-
public function isValid($password, Constraint $constraint)
32+
public function validate($password, Constraint $constraint)
3333
{
3434
$user = $this->securityContext->getToken()->getUser();
3535

@@ -41,10 +41,6 @@ public function isValid($password, Constraint $constraint)
4141

4242
if (!$encoder->isPasswordValid($user->getPassword(), $password, $user->getSalt())) {
4343
$this->context->addViolation($constraint->message);
44-
45-
return false;
4644
}
47-
48-
return true;
4945
}
5046
}

‎src/Symfony/Component/Validator/ConstraintValidator.php

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,15 +11,15 @@
1111

1212
namespace Symfony\Component\Validator;
1313

14+
use Symfony\Component\Validator\Exception\ValidatorException;
15+
1416
/**
1517
* Base class for constraint validators
1618
*
1719
* @author Bernhard Schussek <bschussek@gmail.com>
1820
*
1921
* @api
2022
*/
21-
use Symfony\Component\Validator\Exception\ValidatorException;
22-
2323
abstract class ConstraintValidator implements ConstraintValidatorInterface
2424
{
2525
/**
@@ -54,7 +54,7 @@ public function initialize(ExecutionContext $context)
5454
/**
5555
* {@inheritDoc}
5656
*
57-
* @deprecated
57+
* @deprecated Deprecated since version 2.1, to be removed in 2.3.
5858
*/
5959
public function getMessageTemplate()
6060
{
@@ -64,7 +64,7 @@ public function getMessageTemplate()
6464
/**
6565
* {@inheritDoc}
6666
*
67-
* @deprecated
67+
* @deprecated Deprecated since version 2.1, to be removed in 2.3.
6868
*/
6969
public function getMessageParameters()
7070
{
@@ -74,7 +74,7 @@ public function getMessagePa F438 rameters()
7474
/**
7575
* Wrapper for $this->context->addViolation()
7676
*
77-
* @deprecated
77+
* @deprecated Deprecated since version 2.1, to be removed in 2.3.
7878
*/
7979
protected function setMessage($template, array $parameters = array())
8080
{
@@ -87,4 +87,25 @@ protected function setMessage($template, array $parameters = array())
8787

8888
$this->context->addViolation($template, $parameters);
8989
}
90+
91+
/**
92+
* Stub implementation delegating to the deprecated isValid method.
93+
*
94+
* This stub exists for BC and will be dropped in Symfony 2.3.
95+
*
96+
* @see ConstraintValidatorInterface::validate
97+
*/
98+
public function validate($value, Constraint $constraint)
99+
{
100+
return $this->isValid($value, $constraint);
101+
}
102+
103+
/**
104+
* BC variant of validate.
105+
*
106+
* @deprecated Deprecated since version 2.1, to be removed in 2.3.
107+
*/
108+
protected function isValid($value, Constraint $constraint)
109+
{
110+
}
90111
}

‎src/Symfony/Component/Validator/ConstraintValidatorInterface.php

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,7 @@ function initialize(ExecutionContext $context);
3131
* @param mixed $value The value that should be validated
3232
* @param Constraint $constraint The constrain for the validation
3333
*
34-
* @return Boolean Whether or not the value is valid
35-
*
3634
* @api
3735
*/
38-
function isValid($value, Constraint $constraint);
36+
function validate($value, Constraint $constraint);
3937
}

‎src/Symfony/Component/Validator/Constraints/AllValidator.php

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
use Symfony\Component\Validator\Exception\UnexpectedTypeException;
1717

1818
/**
19+
* @author Bernhard Schussek <bschussek@gmail.com>
20+
*
1921
* @api
2022
*/
2123
class AllValidator extends ConstraintValidator
@@ -26,14 +28,12 @@ class AllValidator extends ConstraintValidator
2628
* @param mixed $value The value that should be validated
2729
* @param Constraint $constraint The constraint for the validation
2830
*
29-
* @return Boolean Whether or not the value is valid
30-
*
3131
* @api
3232
*/
33-
public function isValid($value, Constraint $constraint)
33+
public function validate($value, Constraint $constraint)
3434
{
3535
if (null === $value) {
36-
return true;
36+
return;
3737
}
3838

3939
if (!is_array($value) && !$value instanceof \Traversable) {
@@ -53,7 +53,5 @@ public function isValid($value, Constraint $constraint)
5353
$walker->walkConstraint($constr, $element, $group, $propertyPath.'['.$key.']');
5454
}
5555
}
56-
57-
return true;
5856
}
5957
}

0 commit comments

Comments
 (0)
0