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

Allow the mapped event subscriber to use an attribute reader by default #2713

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Oct 26, 2023

This is building off the conversation in #2683 as a way to try and decouple the hard doctrine/annotations dependency in Gedmo\Mapping\MappedEventSubscriber. Whenever any of the extension listener classes don't have a reader injected through the setAnnotationReader() method, this base class will call Gedmo\Mapping\MappedEventSubscriber::getDefaultAnnotationReader() and create a basic annotation reader. With the announced intent to deprecate the doctrine/annotations package, this is a step in helping to break the hard dependency here by introducing a new fallback behavior if doctrine/annotations isn't installed (right now it's a hard dependency to this package so these new fallbacks won't be hit at all yet, hence this being a WIP PR with time to discuss it).

For B/C, it will continue to default to creating an annotation reader first. It then falls back to creating an attribute reader if running PHP 8, then will hard fail if neither option is available.

A reader still needs to always be created for now as the $annotationReader argument for the Gedmo\Mapping\ExtensionMetadataFactory class constructor requires a reader, the logic in the subscriber's getDefaultAnnotationReader() method can be revisited when this hard requirement is addressed (as the factory only needs a reader if it creates a Gedmo\Mapping\Driver\AnnotationDriverInterface or Gedmo\Mapping\Driver\AttributeDriverInterface instance; XML or (deprecated) YAML drivers do not need this reader).

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (9d5c15d) 79.33% compared to head (8a1f28b) 79.24%.

Files Patch % Lines
src/Mapping/MappedEventSubscriber.php 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2713      +/-   ##
==========================================
- Coverage   79.33%   79.24%   -0.10%     
==========================================
  Files         162      162              
  Lines        8460     8471      +11     
==========================================
+ Hits         6712     6713       +1     
- Misses       1748     1758      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mbabker mbabker marked this pull request as ready for review November 3, 2023 22:38
@mbabker mbabker changed the title [WIP] Allow the mapped event subscriber to use an attribute reader by default Allow the mapped event subscriber to use an attribute reader by default Nov 28, 2023
@franmomu franmomu merged commit 2ba8f94 into doctrine-extensions:main Dec 5, 2023
16 of 18 checks passed
@franmomu
Copy link
Collaborator

franmomu commented Dec 5, 2023

thanks @mbabker!

@mbabker mbabker deleted the default-reader branch December 5, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants