[Validator] Removed return value from ConstraintValidatorInterface::isValid()
[Validator] Renamed ConstraintValidatorInterface::isValid() to validate() because of the lack of a return value
[Validator] Added default validate() implementation to ConstraintValidator for BC
[Validator] Fixed typo, updated CHANGELOG and UPGRADE
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?