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

Remove the @internal annotation from the attribute reader #2743

Merged

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Dec 19, 2023

Gedmo\Mapping\Driver\AttributeReader isn't really internal. If you're running an application that doesn't actually use annotations (but the annotations library still gets installed because of dependencies like this one where there's still a hard coupling), the only way to actually configure this library to not use annotations is to use this @internal annotated class. Which is also typehinted in a fairly decent number of public APIs. So, let's just remove the annotation.

Copy link

codecov bot commented Dec 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1ec37fb) 79.26% compared to head (8bf69fb) 79.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2743      +/-   ##
==========================================
+ Coverage   79.26%   79.33%   +0.06%     
==========================================
  Files         162      162              
  Lines        8513     8506       -7     
==========================================
  Hits         6748     6748              
+ Misses       1765     1758       -7     

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

@franmomu
Copy link
Collaborator

Agree about making it non-internal, but I would like to take a look at some of the methods. IIRC the ones in plural are not really used, I think I can take a look at those this week.

@franmomu franmomu merged commit 97779cb into doctrine-extensions:main Feb 12, 2024
18 checks passed
@franmomu
Copy link
Collaborator

thanks @mbabker! It wasn't easy to remove, so when the times comes we can deprecate those methods.

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.

2 participants