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

AllValidator should not check for type #26463

Closed
zerkms opened this issue Mar 8, 2018 · 7 comments
Closed

AllValidator should not check for type #26463

zerkms opened this issue Mar 8, 2018 · 7 comments

Comments

@zerkms
Copy link
Contributor

@zerkms zerkms commented Mar 8, 2018

Q A
Bug report? yes
Feature request? no
BC Break report? yes
RFC? no
Symfony version 4.0.6

At the moment the AllValidator makes the following extra check:

        if (!is_array($value) && !$value instanceof \Traversable) {
            throw new UnexpectedTypeException($value, 'array or Traversable');
        }

it causes a problem to validate whether a value is an array containing only integers.

Let's see the problem in action. Let's say we have an object that has a field $ids that must be a non-empty array of integers. The naive attempt would be to annotate it like this:

    /**
     * @var int[]
     * @Assert\NotBlank()
     * @Assert\Type("array")
     * @Assert\All({
     *     @Assert\Type("int"),
     *     @Assert\NotBlank(),
     * })
     */
    private $ids;

It works fine, as soon as you pass an array there. But if you pass non-array and non-iterable value, like 42 it fails miserably with the following exception:

Expected argument of type "array or Traversable", "integer" given

Why does the AllValidator throws at all?

        if (null === $value) {
            return;
        }

here it checks whether a value is set at all - so that you could validate it with NotBlank if necessary.

So why does it do the type check with exception, while that part of the job should have been done a Type("array") constraint instead?

What I suggest:

        if (!is_array($value) && !$value instanceof \Traversable) {
            return;
        }

instead. In that case if the value is not an array or traversable - the validation does not run. And if one wants to ensure it's an array - they must do a naive Type("array") check first.

@xabbuh

This comment has been minimized.

Copy link
Member

@xabbuh xabbuh commented Mar 9, 2018

Are you sure that the Type constraint does not add a violation?

@zerkms

This comment has been minimized.

Copy link
Contributor Author

@zerkms zerkms commented Mar 9, 2018

@xabbuh it matters in what order they invoked: if Type is checked first - it would be added (I haven't checked it but I see no reason why it wouldn't).

@xabbuh

This comment has been minimized.

Copy link
Member

@xabbuh xabbuh commented Mar 9, 2018

In that case I don't think that this is a bug. If you cannot ensure that the value you check using composite constraints is a collection, I suggest to add the Type constraint as you did, but additionally make use of validation groups and a group sequence which checks types first and only executes the other checks when the type requirements are fulfilled.

@zerkms

This comment has been minimized.

Copy link
Contributor Author

@zerkms zerkms commented Mar 9, 2018

@xabbuh added Type validation changes nothing - since the exception is still thrown. At the moment All validator does more job than it should have - it simply MUST NOT check for the type, it's not its responsibility.

At the moment it's impossible to validate collections with symfony validator.

I honestly cannot see what benefits that type check brings.

If you cannot ensure that the value you check using composite constraints is a collection

I surely cannot ensure (the request comes from the client and contains arbitrary data) - that's why I want to validate it, but I cannot due to a bug in the implementation.

@zerkms

This comment has been minimized.

Copy link
Contributor Author

@zerkms zerkms commented Mar 9, 2018

To clarify why it's a bug:

  1. If you pass an array - then validation works as expected
  2. If you pass not an array (eg a number) - then the validator throws an exception (while it should return a validation violation)

The All type does not check for emptiness - it's a NotBlank validator responsibility. The same way it must not check for the type - it's a Type validator responsibility.

@zerkms

This comment has been minimized.

Copy link
Contributor Author

@zerkms zerkms commented Mar 10, 2018

Right, I checked other constraints and it looks like almost every other constraint check for type. Which makes symfony/validator impossible to validate an arbitrary input. That's correct: validator must be able to validate anything, but it cannot. Correct me somebody if I wrong.

<?php

use Symfony\Component\Validator\Constraints\Email;
use Symfony\Component\Validator\Constraints\Type;
use Symfony\Component\Validator\Validation;

require __DIR__ . '/vendor/autoload.php';

$validator = Validation::createValidator();

$constraints = [
    new Type('string'),
    new Email(),
];

$violations = $validator->validate([], $constraints);

var_dump($violations);

This cannot validate an email. Expected: a violation. Actual: an exception.

What is the point to throw exceptions from a validator, while the validation result should be returned as a collection of violations instead?

@xabbuh

This comment has been minimized.

Copy link
Member

@xabbuh xabbuh commented Sep 24, 2018

Thank you all for the discussion. I am going to close here in favour of #12312.

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
4 participants
You can’t perform that action at this time.