Deprecate the annotation reader being allowed to be any object

[mbabker]

The annotation reader references in this package look to predate the Doctrine\Common\Annotations\Reader interface, and right now any place accepting a reader essentially allows any arbitrary object that mostly implements that interface (only the class and property readers are required, the method readers aren't). This proposes tightening up the API by documenting that a Doctrine\Common\Annotations\Reader should be provided and deprecating support for any arbitrary object with 4.0 changing to require the interface.

[franmomu]

The only problem I see here is that AttributeReader must implement Reader interface as well, it's not ideal since it would depend on doctrine/annotations but I guess we'll be supporting annotations and attributes for a while.

[mbabker]

The only problem I see here is that AttributeReader must implement Reader interface as well, it's not ideal since it would depend on doctrine/annotations but I guess we'll be supporting annotations and attributes for a while.

I didn't look at the attributes implementation, but it almost feels like there's either a leaky abstraction or an incomplete abstraction layer if the attribute reader has to have the same interface as the annotation reader and inherently be coupled to a doc block annotation parsing package.

[franmomu]

I didn't look at the attributes implementation, but it almost feels like there's either a leaky abstraction or an incomplete abstraction layer if the attribute reader has to have the same interface as the annotation reader and inherently be coupled to a doc block annotation parsing package.

yep that was done on purpose to be able to reuse the Annotation driver. I think I used the same approach as in doctrine/orm where the $reader is duck-typed.

We could maybe introduce our Reader interface (with adapters and removing the getMethodAnnotation/s methods) or allow AttributeReader as well.

[mbabker]

They purposely noted the reader as being duck typed (and I didn’t make it back to where it was originally introduced to work out why it’s explicitly noted as duck typed). So the alternative here to a hard deprecation is to just duck type everything the same way they did it.

[stof]

No need to duck type. We can make the new type a union type (using 2 type checks before the deprecation)

[mbabker]

We can make the new type a union type (using 2 type checks before the deprecation)

Yeah, that could work. I'll play with that at some point this week.

[github-actions[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[codecov-commenter]

Codecov Report

Base: 80.27% // Head: 79.17% // Decreases project coverage by -1.10% :warning:

Coverage data is based on head (02382ce) compared to base (e448975). Patch coverage: 10.00% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2258      +/-   ##
============================================
- Coverage     80.27%   79.17%   -1.11%     
- Complexity     3166     3172       +6     
============================================
  Files           159      160       +1     
  Lines          7728     8285     +557     
============================================
+ Hits           6204     6560     +356     
- Misses         1524     1725     +201     

Impacted Files	Coverage Δ
src/Mapping/Driver/AbstractAnnotationDriver.php	63.63% <8.33%> (-31.61%)	:arrow_down:
src/Mapping/ExtensionMetadataFactory.php	82.71% <11.11%> (-8.72%)	:arrow_down:
src/Mapping/MappedEventSubscriber.php	82.97% <11.11%> (-7.39%)	:arrow_down:
src/Mapping/Annotation/Loggable.php	42.85% <0.00%> (-32.15%)	:arrow_down:
src/Mapping/Annotation/TreeRoot.php	42.85% <0.00%> (-32.15%)	:arrow_down:
src/Mapping/Annotation/TreeClosure.php	42.85% <0.00%> (-32.15%)	:arrow_down:
src/Mapping/Annotation/Translatable.php	42.85% <0.00%> (-32.15%)	:arrow_down:
src/Mapping/Annotation/TranslationEntity.php	42.85% <0.00%> (-32.15%)	:arrow_down:
src/Mapping/Annotation/SlugHandler.php	50.00% <0.00%> (-30.00%)	:arrow_down:
src/Mapping/Annotation/SlugHandlerOption.php	50.00% <0.00%> (-30.00%)	:arrow_down:
... and 66 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[mbabker]

The latest PHPStan fail is unrelated to this PR (something in https://github.com/doctrine/mongodb-odm/releases/tag/2.4.3 changed a SA assertation I guess?).

[phansys]

Thanks @mbabker!