Skip to content

[Validator] wrong constraint attached to ConstraintViolation #20368

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
pinkeen opened this issue Oct 30, 2016 · 11 comments
Closed

[Validator] wrong constraint attached to ConstraintViolation #20368

pinkeen opened this issue Oct 30, 2016 · 11 comments
Labels
Bug Status: Needs Review Validator

Comments

@pinkeen
Copy link

pinkeen commented Oct 30, 2016 •
edited
Loading

Version: symfony/validator 3.1.6, PHP 7.0.7

Background: I've been using the validator component for APIs for a long time (without forms though). One of the problems was that apart from the descriptive violation messages I wanted to return a fixed error name that can be handled by the application on the other end. I've achieved that by extending the base constraints and overriding the error messages (which means no descriptive message). I could live with that, waited for better times.

I noticed that the ConstraintViolation includes the ::getCode method now! Perfect for my little APIs. The code seems to be an UUID and each of the constraints includes a mapping to nice error names. UUIDs are usable but they are not the best UX for the end-programmer :).

The meat: The easiest way to get the error name would be to use Constraint::getErrorName method. This is where the problem is - in some cases the ConstraintViolation has the wrong Constraint attached. From the first look - it's the last tested constraint, not the one that failed.

I am aware that I am assuming the the ConstraintViolationInterface is actually a ConstraintViolation but IMO it's fair to assume when I'm using Symfony's validator component. Also the ConstraintViolation::getConstraint and Constraint::getErrorName methods are public so I am (I hope) not relying on some internal implementation details.

I am attaching a testcase for further illustration:
https://gist.github.com/pinkeen/7a3b6688efc3c493fc16807ac89d3a3f
@xabbuh
Copy link
Member

xabbuh commented Nov 12, 2016

I am not sure if I fully understood your problem. In your test case, what did happen and what did you actually expect to happen instead?

@pinkeen
Copy link
Author

pinkeen commented Nov 12, 2016 •
edited
Loading

In case of a nested (composite) constraints, specifically the Symfony\Component\Validator\Constraints\Collection constraint. When it fails the ConstraintViolation::getCode returns the proper error code which is Collection:MISSING_FIELD_ERROR. The expectation is that ConstraintViolation::getConstraint will return an instance of Constraints\Collection but instead I get an instance of one of the nested constraints.

I have updated the gist with comments.

@xabbuh
Copy link
Member

xabbuh commented Nov 12, 2016

Your issue actually is that you assign an empty array to the email field instead of the constraint that should be used to validate the field which is not supported. You need to provide the constraint that should be applied.

@pinkeen
Copy link
Author

pinkeen commented Nov 12, 2016

I did that in the testcase to better illustrate the problem. The constraint returned is wrong no matter what I do. See the 2nd test.

@xabbuh
Copy link
Member

xabbuh commented Nov 12, 2016

I missed your second test case. This is indeed an issue.

@xabbuh
Copy link
Member

xabbuh commented Nov 12, 2016

For the records, this is related to #14786.

@xabbuh
Copy link
Member

xabbuh commented Nov 12, 2016

Oh and actually this then is a duplicate of #14639. So I am closing here, but I will also look into how to solve #14639. Thank you for reporting this bug and for providing the reproducable example @pinkeen!

@xabbuh xabbuh closed this as completed Nov 12, 2016
@xabbuh xabbuh mentioned this issue Nov 28, 2016
Merged
@xabbuh
Copy link
Member

xabbuh commented Nov 28, 2016

@pinkeen Can you check if #20664 solves your issue?

fabpot added a commit that referenced this issue Nov 29, 2016
@fabpot
bug #20664 [Validator] ensure the proper context for nested validatio…
ec937cb 
…ns (xabbuh)

This PR was merged into the 2.7 branch.

Discussion
----------

[Validator] ensure the proper context for nested validations

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #14639, #20368
| License       | MIT
| Doc PR        | n/a

Commits
-------

56c8ff8 ensure the proper context for nested validations
@pinkeen
Copy link
Author

pinkeen commented Nov 29, 2016

Sorry for the late response. Yes, it solves the issue (2nd test passes). The first test still fails although as you noticed I use empty key (without constraints) which is unsupported.

@xabbuh
Copy link
Member

xabbuh commented Nov 29, 2016

Thank you very much for the confirmation @pinkeen!

@pinkeen
Copy link
Author

pinkeen commented Nov 29, 2016

I realized I have pulled wrong version of the validator component. Retested the proper version. Both tests pass, even with the key without constraints. Tried few other variations - changing order, changing the validators - all green! Thank you for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Needs Review Validator
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@javiereguiluz @xabbuh @pinkeen @carsonbot