Problem/Motivation
https://github.com/doctrine/reflection/pull/53 was merged today, officially abandoning doctrine/reflection.
https://packagist.org/packages/doctrine/reflection now has a note:
This package is abandoned and no longer maintained. The author suggests using the roave/better-reflection package instead.
Proposed resolution
Copy the parts of doctrine/reflection
that we use into our Annotation component, alongside other Doctrine classes.
Remaining tasks
User interface changes
None
API changes
The addition of Drupal\Component\Annotation\Doctrine
, which is copied from doctrine/reflection
.
Data model changes
None
Release notes snippet
The package Doctrine/reflection
is abandoned and the parts Drupal core relies on have been copied to Drupal\Component\Annotation\Doctrine
. The package will be removed from Drupal 10. Contrib and custom code should update any code that uses the package and remove their dependency too.
Comment | File | Size | Author |
---|
Issue fork drupal-3180351
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
andypostComment #3
alexpottThe problem with roave/better-reflection for us is that it does not extend from PHP's builtin \ReflectionClass so it's not a drop in replacement. This also goes for PHPDocumentor's reflection library. This means that we will need extend / rebuild Simple annotation to work with them. The reason we use doctrine/reflection is that it let's us get doc comments without have to load code. This results in a massive memory saving when doing test discovery and also to a lesser extent for plugin discovery.
Comment #4
alexpottAnd
\Doctrine\Common\Reflection\StaticReflectionParser::$classAnnotationOptimize
offers us a massive benefit because we only ever want the class annotations - so this rips out most of the code before passing it into\Doctrine\Common\Annotations\TokenParser::__construct()
and thentoken_get_all()
.Whatever we replace doctrine/reflection with will need performance testing to prove we've not regressed - especially wrt to memory usage.
Comment #5
Ghost of Drupal PastMay I humbly suggest just moving the entire vendor/doctrine/reflection/lib/Doctrine/Common/Reflection into core (*except one class, see below) ? I can attest it was written with Drupal in mind, it's not a lot of code, the only external dependency is the TokenParser from Annotations which I understand Drupal would still use? RuntimePublicReflectionProperty relies on Proxy as well but that class can be ommitted, it is simply not used -- I presume Doctrine might be using it elsewhere but Reflection itself does not and neither does Drupal.
Comment #6
longwaveThis patch implements the idea in #5, copying the parts of Doctrine\Common\Reflection that we use into Drupal\Component\Reflection and Drupal\Component\ClassFinder - you're right, it's not much code at all. I've also removed the dependency on doctrine/reflection just for testing purposes, but we probably can't fully remove that until Drupal 10.
Comment #7
longwaveComment #9
andypostLooks great, not so much of API surface
it could use type hints
Comment #10
longwaveIf we really only ever need the class annotations I wonder if we should bake-in the optimisation and remove the property and method handling?
Comment #12
andypostComment #14
longwaveConverted #7 to a merge request, added composer.json/license/readme/testing notes to the new component.
Comment #15
catchThis is critical in that we're going to need to resolve it before it goes completely unsupported, but more urgent due to #3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves).
Comment #16
longwaveShould we add a new component for this, or should we just merge this into Drupal\Component\Annotation\Doctrine, seeing as that's where we actually use it?
Comment #17
alexpottI don't think we want a new component.
Comment #18
alexpottThat is very very tempting. All of this might disappear anyway once PHP 8 is our minimum and we can move to the builtin attributes - see https://stitcher.io/blog/attributes-in-php-8
Comment #19
longwaveWe use the parser in three places, for some reason Migrate calls it with the optimisation disabled - though I wonder if that's deliberate or an oversight.
Comment #20
alexpott@longwave pretty sure it is deliberate - it is using more that annotations.
Comment #21
longwaveAh yeah migrate needs the parent class name as well, which the optimization strips out - so we need to keep that option in, which is fine as we are copying the class anyway.
However it looks like we can drop the method and property reflections and just throw an exception if someone tries to use those - we don't use them in core anyway, and I can't find any references to getReflectionMethod or getReflectionProperty in contrib either.
Comment #22
longwaveAs a followup we could extend the optimization regex to something like
which should include the parent class name and satisfy Migrate's requirements.
Comment #23
longwaveI guess we can drop ReflectionProviderInterface as well.
Comment #24
daffie CreditAttribution: daffie commentedFor the 2 outstanding threads.
Comment #25
longwaveComment #26
longwaveNW to fix merge conflicts and include the original license in all files that we copy from Doctrine.
Comment #27
Ghost of Drupal PastThe licensing situation is interesting because , we need to look this up carefully, it's been long ago, but I believe at least most but potentially all the code is written by yours truly who is happy to relicense it :)
Comment #28
daffie CreditAttribution: daffie commentedComment #29
daffie CreditAttribution: daffie commentedComment #30
longwaveRebased the branch and added the Doctrine copyright to the PHP 7/8 compatibility classes. I don't believe that ClassFinderInterface needs it; to me we are adding a new interface here as an API for our existing ClassFinder class.
@chx it is correct that you wrote the original StaticReflectionParser but others (including @dawehner and @Sam152 from Drupal land, but more names I do not recognise) have touched the code since: https://github.com/doctrine/reflection/commits/1.2.x/lib/Doctrine/Common... - so I am wary of simply removing the Doctrine copyright notice, better just to leave it in for now I think.
Comment #31
longwaveRemoving "Needs performance review" as we are just moving code around, there are no changes to any algorithms here.
Comment #32
longwaveComment #33
alexpottWe need to do something to provide a BC layer - the Doctrine reflection stuff is used in contrib - see http://grep.xnddx.ru/search?text=Doctrine%5CCommon%5CReflection&filename=
Or we could leave this in composer.json and encourage the contrib projects to change.
Comment #34
longwaveI put the dependency back, we can't guarantee to provide a full BC layer as we haven't copied all of the code, so let's just put it back and downstream users can make a choice to switch to our copy or keep using doctrine/reflection until it stops working on PHP 8.1.
Comment #35
catchLeaving it in composer.json unused sounds sensible to me, once we open 10.x and remove it there it'll be an extra prompt for contrib to stop using it, if 8.1 hasn't broken it first. Can always revisit later if there's a pressing reason to.
Comment #36
daffie CreditAttribution: daffie commentedIs there any way to let users know that the Doctrine reflection stuff is deprecated other then a CR?
Comment #37
catchThe only way would be to class_alias some classes and issue deprecation notices I think.
Comment #38
Ghost of Drupal PastThanks for the link. That's 12 people. We can always ask, if we want.
Comment #39
daffie CreditAttribution: daffie commentedThe code from doctrine/reflection has been copied to drupal/components.
The code from doctrine/reflection is no longer been used in core.
All copied code has the original license in the file.
For me it is RTBC.
If somebody can add a deprecation message to the external code with a class_alias call, then please do so.
Comment #40
alexpottI think we need a change record here that details any likely changes something that is using doctrine reflections in the same way core is. We also need a release note for this.
Comment #41
daffie CreditAttribution: daffie commentedUpdated the Is and created a CR.
Back to RTBC.
Comment #42
alexpottComment #43
alexpottCommitted a0a22f8 and pushed to 9.3.x. Thanks!
Comment #45
alexpottComment #47
mikemadison CreditAttribution: mikemadison at Acquia commentedFYI as of Drupal core 9.3.8 I am still seeing that drupal/core requires doctrine/reflection (^1.1). Do we need to reopen this?
Comment #48
KlemenDEV CreditAttribution: KlemenDEV as a volunteer and at Pylo commentedI think so, this only got closed due to inactivity
Comment #49
xjmThis was not closed due to inactivity; it was committed to HEAD well before 9.3.0 and closed due to being fixed. :)
The reason 9.3.8 (and all future Drupal 9 releases) still have a dependency on
doctrine/reflection
is that removing a dependency introduces a BC break. Therefore, the issue only deprecated core's dependency on the package alongside providing a replacement for it. As the release note explains, the legacy dependency is removed in Drupal 10.0.x instead.Thanks!