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

Decouple AnnotationDriver from doctrine/annotations #217

Closed

Conversation

derrabus
Copy link
Member

AnnotationDriver is a helpful utility class for all drivers that read metadata from annotated class files. It's main advantage is the ability to search paths for suitable class files.

Unfortunately, it is currently coupled to the doctrine/annotations library. The ORM uses this class for its AttributeDriver which feels like a hack currently.

I'd like to decouple the class by removing the $reader property and making the isTransient() method abstract in 3.0. This PR prepares the necessary deprecation layer.

@derrabus derrabus force-pushed the deprecate/annotation-dependency branch 3 times, most recently from e34d9b9 to 7b4b467 Compare December 19, 2021 22:51
@derrabus derrabus added this to the 2.3.0 milestone Dec 19, 2021
ignoreErrors:
# TODO: Remove in 3.0
-
message: "#^Call to function is_object\\(\\) with array\\<string\\>\\|string\\|null will always evaluate to false\\.$#"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be avoided by adding treatPhpDocTypesAsCertain: false in the phpstan parameters, which makes phpstan accept cases where a type check is done in the code that validates a type defined only in phpdoc (because phpdoc types are not enforced) while type checks done on a value that passed through a native type are still reported.

@stof
Copy link
Member

stof commented Dec 20, 2021

Maybe the right solution would be to extract most of the logic to a new parent class that does not use annotations in its name instead of deprecating passing an annotation reader to the annotation driver. Then, the ORM could extend this new class directly instead of extending AnnotationDriver for their AttributeDriver.

@derrabus derrabus force-pushed the deprecate/annotation-dependency branch from 7b4b467 to 2210242 Compare December 20, 2021 12:44
@derrabus
Copy link
Member Author

Maybe the right solution would be to extract most of the logic to a new parent class that does not use annotations in its name

How would you name it? AbstractPhpClassFilesDriver?

@stof
Copy link
Member

stof commented Dec 20, 2021

@derrabus I don't know. What is the feature actually implemented by that abstract class ?
But I'm quite sure that AnnotationDriver looks like the wrong name if the class does not implement support for annotations anymore.

@derrabus
Copy link
Member Author

@derrabus I don't know. What is the feature actually implemented by that abstract class ?

If we remove the code that interfaces with doctrine/annotations, we recive an abstract driver that is able to discover class names from filesystem paths.

But I'm quite sure that AnnotationDriver looks like the wrong name if the class does not implement support for annotations anymore.

Yes, possibly.

@greg0ire
Copy link
Member

greg0ire commented Jan 8, 2022

 How would you name it? AbstractPhpClassFilesDriver?

If that driver is meant to handle metadata located next to the properties, class names etc. it defines metadata on, how about AbstractColocatedMetadataDriver? I'd avoid having Class in the name because of #224

@derrabus derrabus modified the milestones: 2.3.0, 2.4.0 Jan 9, 2022
@derrabus derrabus changed the base branch from 2.3.x to 2.4.x January 9, 2022 21:28
@derrabus
Copy link
Member Author

derrabus commented Mar 3, 2022

Closing in favor of #246

@derrabus derrabus closed this Mar 3, 2022
@derrabus derrabus deleted the deprecate/annotation-dependency branch March 3, 2022 22:24
@stof stof removed this from the 2.4.0 milestone Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants