[Validator] The validator does not support inheritance properly for getter rules #2841

Closed
stof opened this Issue Dec 11, 2011 · 8 comments

Comments

Projects
None yet
7 participants
@stof
Member

stof commented Dec 11, 2011

The Validator component uses Reflection, even when doing the validation on getters. This means that a child class cannot overwrite a getter as the validator will still use the parent method.
this is totally confusing (I just spent quite 2 hours debugging my form validation before thinking about Reflection being used) and does not make sense IMO (when overwriting a method, we expect the child one to be called)
what do you think about it ?

@kriswallsmith

This comment has been minimized.

Show comment
Hide comment
@kriswallsmith

kriswallsmith Dec 11, 2011

Contributor

I agree that using reflection for methods is problematic. I have tried to define constraints on an interface before, which doesn't work at all. You get this error…

PHP Warning: Uncaught exception 'ReflectionException' with message 'Trying to invoke abstract method FooInterface::foo()'

Contributor

kriswallsmith commented Dec 11, 2011

I agree that using reflection for methods is problematic. I have tried to define constraints on an interface before, which doesn't work at all. You get this error…

PHP Warning: Uncaught exception 'ReflectionException' with message 'Trying to invoke abstract method FooInterface::foo()'

@schmittjoh

This comment has been minimized.

Show comment
Hide comment
@schmittjoh

schmittjoh Dec 11, 2011

Contributor

I think there are two cases which need to be distinguished:

  1. Using annotations:
    In this case, I expect validation to happen wherever I specify the annotation (property, or method). If a method with an annotation gets overridden, then the constraint should be ignored imo.
  2. Using other metadata sources:
    In this case, it's probably more intuitive to validate the outermost method.

As a side-note, the metadata library provides a very good toolset to handle such cases (including metadata on interfaces as supported by JMSSecurityExtraBundle).

Contributor

schmittjoh commented Dec 11, 2011

I think there are two cases which need to be distinguished:

  1. Using annotations:
    In this case, I expect validation to happen wherever I specify the annotation (property, or method). If a method with an annotation gets overridden, then the constraint should be ignored imo.
  2. Using other metadata sources:
    In this case, it's probably more intuitive to validate the outermost method.

As a side-note, the metadata library provides a very good toolset to handle such cases (including metadata on interfaces as supported by JMSSecurityExtraBundle).

@kriswallsmith

This comment has been minimized.

Show comment
Hide comment
@kriswallsmith

kriswallsmith Dec 11, 2011

Contributor

Why should annotations behave differently than other drivers?

Contributor

kriswallsmith commented Dec 11, 2011

Why should annotations behave differently than other drivers?

stof added a commit to FriendsOfSymfony/FOSUserBundle that referenced this issue Dec 11, 2011

Added a validation initializer for Propel proxies
If symfony/symfony#2841 is fixed, we will be able to use getter
constraints instead of having to update the parent properties
@schmittjoh

This comment has been minimized.

Show comment
Hide comment
@schmittjoh

schmittjoh Dec 11, 2011

Contributor

@kriswallsmith, I guess it would make sense to use the annotation behavior for the other drivers as well (that is what I'm doing in my bundles at least).

Contributor

schmittjoh commented Dec 11, 2011

@kriswallsmith, I guess it would make sense to use the annotation behavior for the other drivers as well (that is what I'm doing in my bundles at least).

@abretaud

This comment has been minimized.

Show comment
Hide comment
@abretaud

abretaud Dec 20, 2011

I don't know if it's really related, but if you want to overwrite a Choice constraint callback function, you end up with the same behavior i.e. the callback is called on the class where the annotation is written. And as you can't overwrite an annotation, there's no good solution.
I would really like to have better inheritance support for annotations in validator.

I don't know if it's really related, but if you want to overwrite a Choice constraint callback function, you end up with the same behavior i.e. the callback is called on the class where the annotation is written. And as you can't overwrite an annotation, there's no good solution.
I would really like to have better inheritance support for annotations in validator.

@Kasheen

This comment has been minimized.

Show comment
Hide comment
@Kasheen

Kasheen Jan 15, 2012

I've run into the same issue as @kriswallsmith was having with regards to trying to annotate interface getter methods.

Seems like it would be something useful to support since it means you don't need to copy your validation constraints to all classes that implement the contract of the interface.

It seems intuitive to me that validation constraints on a parent class are inherited onto a child class unless overridden (by which I mean different constraints are annotated) - perhaps for those who want to add to the parent constraints there could be something like @Assert\Parent followed by any further constraints that need to be checked (or if overriding a method removed constraints by default then at least an @Assert\Parent would let you put them back easily without copy-pasting).

Kasheen commented Jan 15, 2012

I've run into the same issue as @kriswallsmith was having with regards to trying to annotate interface getter methods.

Seems like it would be something useful to support since it means you don't need to copy your validation constraints to all classes that implement the contract of the interface.

It seems intuitive to me that validation constraints on a parent class are inherited onto a child class unless overridden (by which I mean different constraints are annotated) - perhaps for those who want to add to the parent constraints there could be something like @Assert\Parent followed by any further constraints that need to be checked (or if overriding a method removed constraints by default then at least an @Assert\Parent would let you put them back easily without copy-pasting).

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Apr 4, 2012

Member

@bschussek ping

Member

stof commented Apr 4, 2012

@bschussek ping

@wouterj

This comment has been minimized.

Show comment
Hide comment
@wouterj

wouterj Jul 24, 2014

Member

This is fixed by #7271 and can be closed

Member

wouterj commented Jul 24, 2014

This is fixed by #7271 and can be closed

@jakzal jakzal closed this Jul 24, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment