Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Validator] Decouple type validation from constraint validators #12312

Closed
webmozart opened this issue Oct 24, 2014 · 6 comments
Closed

[Validator] Decouple type validation from constraint validators #12312

webmozart opened this issue Oct 24, 2014 · 6 comments

Comments

@webmozart
Copy link
Contributor

@webmozart webmozart commented Oct 24, 2014

Several issues have been raised in the past because of the validator's inconcistent type validation. Some constraints through UnexpectedTypeExceptions, others add constraint violations, others ignore unexpected types. This should be fixed.

An indepth preliminary discussion of this topic can be found here:
#10221 (comment)

The way I envision this to happen is by changing the Type constraint to a meta-constraint (like Valid or Traverse), which is handled by the validation engine directly. Currently, this constraint specifies:

  1. what type an element should have (or any)
  2. what violation message should be added if the type does not match
/**
 * @Type("int", message="This field expects an integer.")
 * @GreaterThan(0)
 */
private $price;

While the $type option of the Type constraint is mandatory at the moment, it will become optional. Instead, each constraint will have a new method returning the accepted types:

class GreaterThan extends Constraint
{
    // ...

    public function getAcceptedTypes()
    {
        return array(self::INT, self::FLOAT);
    }
}

Now the validation engine will take care that a given value matches the types expected by each constraint. If not, a constraint violation will be added. The Type constraint can be used to customize the violation message:

/**
 * @Type(message="The price should be a number.")
 * @GreaterThan(0)
 */
private $price;

We need to change GenericMetadata::addConstraint() to validate the compatibility of the defined constraints:

  • When adding a normal constraint, the expected type should be set to the intersection of all "accepted types" (dependent on the validation group). If the expected type is empty, the constraints conflict and an exception should be thrown.
  • When adding a Type constraint, the expected type is set manually. Again, check that there are no conflicts with the other constraints on that property.

The constraint validation process can then be adapted in the following way:

  1. Get the expected type for the current node
  2. Get the actual type (gettype()) of the node's value
  3. Check that the actual type matches one of the expected types. Otherwise, raise a constraint violation.
  4. Validate all "normal" constraints. Only validate a constraint if the node's value is not null or if null is in the expected types of that constraint.

Consequences:

  • While the Type constraint currently accepts any type for which either is_<type>() or ctype_<type>() is defined or which is a class name, that would be restricted to the results of gettype() and get_class(). A BC layer needs to be provided in some way.

Related issues:

@webmozart

This comment has been minimized.

Copy link
Contributor Author

@webmozart webmozart commented Oct 24, 2014

The Type constraint should also have a $strict option. When set to false, types should be accepted if they can be cast to the desired type (e.g. digit string -> integer).

@natorojr

This comment has been minimized.

Copy link

@natorojr natorojr commented Oct 24, 2014

Awesome! Glad to see some headway being made over this. I know you've put considerable thought into fixing these related issues over the last year. Let me know if I can be of any assistance.

@xabbuh

This comment has been minimized.

Copy link
Member

@xabbuh xabbuh commented Oct 25, 2014

When adding a normal constraint, the expected type should be set to the intersection of all "accepted types" (dependent on the validation group). If the expected type is empty, the constraints conflict and an exception should be thrown.

Won't this become hard to handle in practice some times? For example, when you have a form and you apply different validation groups depending on the clicked button, for one button everything will work as expected, but will break for the other one. Can we guarantee in some way that such an error is detected not only at runtime but when building the form?

@webdevilopers

This comment has been minimized.

Copy link

@webdevilopers webdevilopers commented Jan 20, 2015

👍

@k3nn7

This comment has been minimized.

Copy link

@k3nn7 k3nn7 commented Apr 9, 2017

Hey, it looks like this topic is dead for some time. Is it because there is some other solution or workaround for this problem?

I'm currently struggling with one of the issues described here and in related topics (UnexpectedTypeException is thrown e.g. from Length constraint even if I use it together with Type("string") constraint).

Because of that I need to either do some preliminary validation/data transformation (but I will hide some errors or won't be able to easily group all of them together) to make sure that types are right or wrap validators with some custom Composite constraint that will ignore UnexpectedTypeException (but it has security problems described in previous comments).

@lordjancso

This comment has been minimized.

Copy link

@lordjancso lordjancso commented May 23, 2017

I would live to have some workaround or a fix. Still no news?
I already asked it in #17758.

nicolas-grekas added a commit that referenced this issue Oct 24, 2018
…dation (xabbuh)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Validator] catch any UnexpectedValueException on validation

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

Commits
-------

fa35860 catch any UnexpectedValueException on validation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.