serializer icon indicating copy to clipboard operation
serializer copied to clipboard

fix AttributeReader:getPropertyAnnotation AttributeReader:getMethodAnnotation

Open andriinavrotskii opened this issue 2 years ago • 5 comments

| Bug fix? | yes

andriinavrotskii avatar Mar 30 '22 15:03 andriinavrotskii

Hello @andriinavrotskii !

Thank you for your PR! Could you tell a bit more about the issue that is solves? Is it possible to add some unit tests that was not working before this changes?

Best, Marcin.

scyzoryck avatar Apr 03 '22 20:04 scyzoryck

Hello @andriinavrotskii !

Thank you for your PR! Could you tell a bit more about the issue that is solves? Is it possible to add some unit tests that was not working before this changes?

Best, Marcin.

Hi, Marcin.

Problem:

Doctrine\Common\Annotations\PsrCachedReader::getClassAnnotation(): Argument #1 ($class) must be of type ReflectionClass, ReflectionProperty given, called in /var/www/vendor/jms/serializer/src/Metadata/Driver/AttributeDriver/AttributeReader.php on line 68

Method Doctrine\Common\Annotations\Reader::getClassAnnotation(ReflectionClass $class, $annotationName) has attribute "$class" typehinted as ReflectionClass

Method JMS\Serializer\Metadata\Driver\AttributeDriver\AttributeReader::getPropertyAnnotation(ReflectionProperty $property, $annotationName) got "$property" typehinted as ReflectionProperty

Same for getMethodAnnotation - we try to send instance of ReflectionMethod to method getClassAnnotation which expect instance of ReflectionClass

andriinavrotskii avatar Apr 04 '22 07:04 andriinavrotskii

is there a way to provide a test case for this?

goetas avatar May 11 '22 07:05 goetas

Looking closer at the AttributeReader class it seems the getMethodAnnotation and getPropertyAnnotation are not using their counterparts from the delegated reader.

I think the solution from this commit is not correct because when you call getMethodAnnotation or getPropertyAnnotation you expect to get the annotation instance from the method or from the property respectively, not from the class annotations.

IMO the fix is to call to the delegated reader with the same method names.

public function getMethodAnnotation(ReflectionMethod $method, $annotationName): ?object
{
    $attributes = $method->getAttributes($annotationName);

    return $this->reader->getMethodAnnotation($method, $annotationName) ?? $this->buildAnnotation($attributes);
}

public function getPropertyAnnotation(ReflectionProperty $property, $annotationName): ?object
{
    $attributes = $property->getAttributes($annotationName);

    return $this->reader->getPropertyAnnotation($property, $annotationName) ?? $this->buildAnnotation($attributes);
}

ixarlie avatar Jan 10 '23 20:01 ixarlie

@mbabker you have worked on this area...would you mind having a look at this?

goetas avatar Feb 03 '23 05:02 goetas