Skip to content

[Validator] Encourage type safety #9979

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.

Sign up for GitHub

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

Jump to bottom
Closed
kiler129 opened this issue Jun 29, 2018 · 2 comments
Closed

[Validator] Encourage type safety #9979

kiler129 opened this issue Jun 29, 2018 · 2 comments
Labels
actionable Clear and specific issues ready for anyone to take them. DX hasPR A Pull Request has already been submitted for this issue. Validator
Milestone
2.8

Comments

@kiler129
Copy link
Contributor

Example provided at https://symfony.com/doc/current/validation/custom_constraint.html#creating-the-validator-itself shows example validation as:

    public function validate($value, Constraint $constraint)
    {
        if (!preg_match('/^[a-zA-Z0-9]+$/', $value, $matches)) {
            $this->context->buildViolation($constraint->message)
                ->setParameter('{{ string }}', $value)
                ->addViolation();
        }
    }

This is a perfectly fine example, but sets a trap creating assumption that $value is always a string. I think at minimum we should include:

        if (!is_string($value)) {
            throw new UnexpectedTypeException($value, 'string');
        }

if not even something like DateTimeValidator has:

        if (!is_scalar($value) && !(is_object($value) && method_exists($value, '__toString'))) {
            throw new UnexpectedTypeException($value, 'string');
        }

        $value = (string) $value;

WDYT?
@xabbuh
Copy link
Member

xabbuh commented Jun 30, 2018

👍 from my side. I (almost) always do this in custom constraint validators. Usually, this should never be triggered, but in case you misconfigure some validation it may greatly help debugging because you end up with much more meaningful error messages.

@xabbuh xabbuh added the DX label Jun 30, 2018
@HeahDude HeahDude added this to the 2.8 milestone Jul 1, 2018
@HeahDude HeahDude added the actionable Clear and specific issues ready for anyone to take them. label Jul 1, 2018
@javiereguiluz javiereguiluz added the hasPR A Pull Request has already been submitted for this issue. label Jul 2, 2018
@javiereguiluz javiereguiluz mentioned this issue Jul 2, 2018
Closed
javiereguiluz added a commit that referenced this issue Jul 3, 2018
@javiereguiluz
minor #9993 Made the code of a validation example more robust (javier…
676fc4e 
…eguiluz)

This PR was squashed before being merged into the 2.8 branch (closes #9993).

Discussion
----------

Made the code of a validation example more robust

Fixes #9979.

Commits
-------

53e2af1 Made the code of a validation example more robust
@javiereguiluz
Copy link
Member

@kiler129 thanks for reporting this issue. It was fixed in #9993.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
actionable Clear and specific issues ready for anyone to take them. DX hasPR A Pull Request has already been submitted for this issue. Validator
Projects
None yet
Milestone
2.8
Development

No branches or pull requests
4 participants
@javiereguiluz @kiler129 @xabbuh @HeahDude