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

AbstractComparisonValidator gives TypeError with type hinted getter #30133

Closed
nick-zh opened this issue Feb 11, 2019 · 4 comments
Closed

AbstractComparisonValidator gives TypeError with type hinted getter #30133

nick-zh opened this issue Feb 11, 2019 · 4 comments

Comments

@nick-zh
Copy link
Contributor

nick-zh commented Feb 11, 2019

Symfony version(s) affected: symfony/valdiator 4.2.3

Description
I am using the constraint Symfony\Component\Validator\Constraints\GreaterThanOrEqual
My getters have typehinted return values. So if i validate a string (expected to be int) i get a type error.
This is happening because the Symfony\Component\Validator\Constraints\AbstractComparisonValidator uses the PropertyAccessor (which calls the getter) unlike e.g. the RecursiveContextualValidator
which uses

$propertyValue = $propertyMetadata->getPropertyValue($object);

I haven't found a good solution besides changing my code or implementing an own property accessor.
Maybe i have overlooked something, but i think this should still work without custom additions.

EDIT: The constructcor of the AbstractComparisonValidator now uses the concrete class in the constructor, i think this should at least be:

public function __construct(PropertyAccessorInterface $propertyAccessor = null)
{
    $this->propertyAccessor = $propertyAccessor;
}
fabpot added a commit that referenced this issue Feb 12, 2019
…(nick-zh)

This PR was submitted for the master branch but it was squashed and merged into the 3.4 branch instead (closes #30136).

Discussion
----------

use PropertyAccessorInterface instead of PropertyAccessor

[Validator] [Constraints] use PropertyAccessorInterface instead of PropertyAccessor

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

Commits
-------

3e3ca9d use PropertyAccessorInterface instead of PropertyAccessor
@xabbuh
Copy link
Member

xabbuh commented Feb 16, 2019

I could imagine that we could find a solution to get the value without calling the getter. But I am not convinced that this really is a good fix. In your case this will then mean that the following comparison is done with a value that may not be comparable at all. My guess is that you are better off by trying to perform the comparison only if the value to compare with is actually of the expected type (but I may be missing some important details of your use case). What do you think?

@nick-zh
Copy link
Contributor Author

nick-zh commented Feb 18, 2019

@xabbuh thanks for getting back to me on this. My actual use case is this:
I use the symfony validator, i do have two fields that i want to compare with the GreaterThanOrEqual constraint. I also have constraints to ensure the type of the fields (int), but since all constraints are evaluated, i never get the Type validation error, but rather the TypeError from GreaterThanOrEqual.
I do agree though that by fixing this, it might end up in values that aren't comparable, which of course is also not favorable.
I can't make a constraint dependent of another being successful, right?
I personally can live with how it is right now, since with #30136 i can now use my own PropertyAccessor.

@xabbuh
Copy link
Member

xabbuh commented Feb 18, 2019

Thank you for explaining me your use case. If I understand you correctly, this is a perfect example for using group sequences. So you would first check all the needed types and only if these checks succeed, the comparison validator will be executed.

@nick-zh
Copy link
Contributor Author

nick-zh commented Feb 18, 2019

@xabbuh thank you very much, i didn't know about that. I am closing this, i think the adjustment and group sequences covers this quite nicely.

@nick-zh nick-zh closed this as completed Feb 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants