doctrine-extensions / DoctrineExtensions

Doctrine2 behavioral extensions, Translatable, Sluggable, Tree-NestedSet, Timestampable, Loggable, Sortable
MIT License
4.01k stars 1.26k forks source link

Deprecate annotations support #2772

Closed mbabker closed 1 week ago

mbabker commented 4 months ago

Now that the hard dependency is broken, the next major task to coincide with the doctrine/annotations library being deprecated would be deprecating annotations support in this package. WIP for now because this is going to be quite a big deprecation and architectural task since the attribute drivers are all subclasses of their annotation counterparts so some thought will be needed into how to address that (as folks in the Symfony ecosystem will eventually come here with reports about it, even with the drivers all being flagged internal).

Other things to think about along the way include:

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 80.71279% with 92 lines in your changes missing coverage. Please review.

Project coverage is 78.85%. Comparing base (0632ab1) to head (6e633fa). Report is 50 commits behind head on main.

Files Patch % Lines
src/Tree/Mapping/Driver/Attribute.php 76.00% 24 Missing :warning:
src/Sluggable/Mapping/Driver/Attribute.php 78.12% 14 Missing :warning:
src/Loggable/Mapping/Driver/Attribute.php 79.06% 9 Missing :warning:
src/Mapping/ExtensionMetadataFactory.php 63.15% 7 Missing :warning:
src/Translatable/Mapping/Driver/Attribute.php 83.72% 7 Missing :warning:
src/Blameable/Mapping/Driver/Attribute.php 80.00% 6 Missing :warning:
src/IpTraceable/Mapping/Driver/Attribute.php 77.77% 6 Missing :warning:
src/Timestampable/Mapping/Driver/Attribute.php 81.48% 5 Missing :warning:
src/Sortable/Mapping/Driver/Attribute.php 83.33% 4 Missing :warning:
src/Mapping/Driver/AbstractAnnotationDriver.php 72.72% 3 Missing :warning:
... and 4 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2772 +/- ## ========================================== + Coverage 78.75% 78.85% +0.10% ========================================== Files 163 167 +4 Lines 8593 8640 +47 ========================================== + Hits 6767 6813 +46 - Misses 1826 1827 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

franmomu commented 4 months ago

I don't know if we can invert the inheritance and make the Annotation classes inherit from the Attribute ones

mbabker commented 4 months ago

I don't know if we can invert the inheritance and make the Annotation classes inherit from the Attribute ones

We'd have to flip it for the AnnotationDriverInterface and AttributeDriverInterface as well. There are checks like this one targeting only the attribute drivers so those bits and bobs would need to be inverted as well to not break any other behaviors.

Conceivably it's possible, and the mapping drivers within this package are a lot more of an internal implementation than in the object managers themselves given the way ExtensionMetadataFactory::getDriver() works, so even though there are some potential B/C implications they aren't as impactful to downstream users as the changes that were made upstream.

mbabker commented 4 months ago

Reversing the inheritance chain actually worked out a lot more smoothly than I expected it to. The only possible hangup from an end-user perspective is if someone implemented a custom extension (as in fully added their own listener extending from MappedEventSubscriber and adding their own annotation/attribute mapping drivers), they would also have to flip their inheritance chains. I would suggest the likelihood of that is pretty low, though.

franmomu commented 3 weeks ago

Reversing the inheritance chain actually worked out a lot more smoothly than I expected it to.

nice 🎉

The only possible hangup from an end-user perspective is if someone implemented a custom extension (as in fully added their own listener extending from MappedEventSubscriber and adding their own annotation/attribute mapping drivers), they would also have to flip their inheritance chains. I would suggest the likelihood of that is pretty low, though.

I'm fine with this, I don't think a lot of people would have both annotations and attributes drivers in their custom extensions.