Task | Novice task? | Contributor instructions | Complete? |
---|
Follow-up to #2421451: Drupal needs comments in opcache
Problem/Motivation
SimpleAnnotationReader
has been deprecated by Doctrine. It allows us to use annotations without a use
statement for the annotation class.
https://github.com/doctrine/annotations/pull/199
Proposed resolution
Fork the code that Doctrine has deprecated into core.
Code the needs to copied
- \Doctrine\Common\Annotations\SimpleAnnotationReader
- \Doctrine\Tests\Common\Annotations\SimpleAnnotationReaderTest
- \Doctrine\Common\Annotations\DocParser
- \Doctrine\Tests\Common\Annotations\DocParserTest
- DocParserTest requires many fixture classes from Doctrine\Tests\Common\Annotations\Fixtures
- \Doctrine\Tests\Common\Annotations\Ticket\DCOM58Test
Remaining tasks
Review
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
SimpleAnnotationReader, which has been dropped from the master branch of Doctrine Annotations, has been forked into Drupal core to maintain the same functionality. Contributed modules should not be relying on this library directly.
Comment | File | Size | Author |
---|---|---|---|
#137 | 2631202-137.patch | 130.18 KB | tedbow |
#137 | interdiff-131-137.txt | 5.16 KB | tedbow |
#131 | 2631202-131.patch | 127.79 KB | larowlan |
#118 | 2631202-118.patch | 128.31 KB | tedbow |
#118 | interdiff-113-118.txt | 3.68 KB | tedbow |
Comments
Comment #2
cilefen CreditAttribution: cilefen commentedComment #3
dawehnerYeah, sadly DocParser is even final, it will be hard to override anything without reimplementing basically everything.
Comment #4
neclimdulAdditionally, while trying to resolve a performance issue, Doctrine basically told us SimpleAnnotationReader should never be used and is deprecated. So we're already in need of replacing our use of SimpleAnnotationReader and writing our own.
Comment #6
cilefen CreditAttribution: cilefen commented@stephanie_42 and I are triaging this Major issue at New Orleans.
According to the upstream issue mentioned in #4, it seems AnnotationReader is the way to go. But #4, and other commentators on the parent issue also mention writing our own. I don't understand annotation parsing enough right now to comment on rolling our own. I'm tagging "Needs issue summary update" because we need a plan.
Comment #7
xjmThanks @cilefen for helping triage this! Updating issue credit for the triage work. @stephanie_42, if you are following, can you also comment to ensure you get credit for the triage work?
Comment #8
catchDiscussed this with @alexpott @effulgentsia @xjm @webchick @cottser and we agreed this is definitely major.
APC used to get fragmented and then spit php warnings when it couldn't find a big enough spot for a file.
opcode definitely empties itself when it gets full, not sure how it handles fragmentation.
Both of these are 'very bad' and it's nearly impossible for someone running into them to trace the problem back to this issue, unless they already know about it. You'd have to read through the files list on opcode.php and spot that there are annotated classes from enabled modules but which your site never uses to have any idea at all.
The issue on cache rebuilds about OOM is also very hard to debug - there aren't good memory tools (including xhprof) that can tell you what exactly is responsible for using memory. However loading PHP classes is memory that can never be reclaimed by gc, so while it might not be the worst thing we do, it's increases the baseline of memory usage in a way that can't be tweaked except by fixing this.
I think we need a new issue to discuss how to deal with the problem of Doctrine deprecating SimpleAnnotationParser/slating it for removal as well as the final which blocks this. We added that as a pre-requisite of using annotations in the first place, so may need to either find another library or adopt that code as our own (even if via a new library or fork somehow).
Not opening that issue yet, but when I do, will postpone this one on it.
Comment #9
xjmTagging. Thanks for helping confirm this major.
Comment #10
StephanieFuda CreditAttribution: StephanieFuda as a volunteer commentedThank you @cilefen and @xjm for your guidance and emails.
Per the upstream issue in #4 and cilefen's comment in #6 - Doctrine says that "AnnotationReader" is the correct successor to "SimpleAnnotationReader" and is seems that the comments in the parent issue (#4) are moving in that direction.
Comment #11
xjm(Updating credit, thanks @Stephanie_42!)
Comment #14
catchDoctrine (along with Zend framework) now requires PHP 5.6. Since for at least the next 6 months we'll be supporting PHP 5.5, their support schedule is out of sync with ours.
Additionally this issue shows that:
- there's a major bug with Doctrine simple annotation reader (i.e. it does the exact thing that chx's contribution to it was designed to prevent)
- Doctrine has no intention of fixing that bug, and in fact is planning to deprecate it altogether.
With the combination of these, it's time to discuss seriously replacing it altogether with our own component. Doing a small title and issue summary update but it could do with fleshing out of course.
Comment #15
catchComment #16
Mile23So the difference between this issue and the solution in #2557433-17: Add internal, event, and property to the list of ignored annotations in the plugin annotation system is that this issue specifies that we shouldn't use reflection, or otherwise class-load the files we're parsing.
That means we'll accomplish the goals of #2557433: Add internal, event, and property to the list of ignored annotations in the plugin annotation system which is to avoid trying to autoload classes for annotations like @see.
Is there any reason not to use https://github.com/php-annotations/php-annotations/blob/master/src/annot... or another external library?
Comment #17
dawehnerI think there is no reason not to use a library. The particular example you have chosen: https://php-annotations.readthedocs.io/en/latest/UsingAnnotations.html#a... seems though to have a different syntax. I guess we need to be 100% compliant with what we have right now.
Comment #18
Mile23Classes like Component\AnnotatedClassDiscovery need more tests before this can go: #2435607: Tests for Annotation Component
Comment #19
dawehner+1 for improving the test coverage.
Replacing that would actually let us probably keep
AnnotatedClassDiscovery
basically exactly the same. Instead it would "just" replace the annotation parser, right?Comment #20
Mile23That's what it looks like. Basically, we only use reflection so we can feed doctrine's SimpleAnnotationReader, which then only feeds the strings to doctrine's parser/lexer. So we should replace SimpleAnnotationReader with our own semi-clone that parses tokens.
I started to write the token parser, but realized I couldn't even let the testbot figure out if I did the right thing. :-)
Comment #22
tim.plunkettComment #23
catchThat issue is RTBC so unpostponing this.
Comment #24
dawehnerI had a look at other annotation systems for PHP out there, and well the problem is that the doctrine syntax is unique in the sense that other libraries have a different syntax. The glory of not having it a language feature :)
I still hope we can rewrite a part of doctrine but reuse other part of doctrine, so we don't reinvent the entire wheel.
Comment #25
Mile23#2435607: Tests for Annotation Component is in, so we can push forward here.
Comment #26
joachim CreditAttribution: joachim as a volunteer commentedTwo nice things we could have if we had our own annotations system:
1. we could allow comments inside annotations. With the huge annotations we get for things like entity types, being able to document a particular setting is sometimes useful.
2. We could think about how to make annotations extensible -- for example, at the moment the Content Entity type annotation, which is in the core Entity component, has a property from Field UI module for the base route...
Comment #27
Mile23This issue has the narrow scope of replacing
SimpleAnnotationReader
with our own so that we can solve the specific problems of memory usage and op cache dilution due to it using reflection on every class in a namespace.If we want to have a larger remove-all-of-Doctrine type issue someone should file that, and any other requested features should have their own issues as well.
Comment #28
Mile23Comment #29
fgmI know this may sound heretical, but isn't this really a chance to reconsider the choice made to use annotations. They could be replaced by other less brittle mechanisms like manifest files, or even info hooks (although the chance for abuse is a bit high on these, considering what we saw in D7.
But more simply, it seems most annotations, especially those for plugins, could be replaced by static methods on the classes, complete with result type hinting to ensure resilience and avoid issues like incorrectly types annotations like @ocramius discussed on https://github.com/doctrine/annotations/pull/56 . The simplest way to handle this being probably just to add a new DiscoveryInterface implementation based on this mechanism, that would compose the AnnotatedClassDiscovery instance as a fallback for deprecated discovery support, and use that one instead of AnnotatedClassDiscovery in DefaultPluginManager, EntityTypeManager, LayoutPluginManager, and MigrateSourcePluginManager, which appear to be the only 4 non-test classes directly instantiating this discovery.
Thus we could deprecate the existing annotation, keeping the obsolete deprecated Doctrine support for SimpleAnnotationReader for compatibility, and dropping it altogether for D9, thus solving our dependency issue.
Comment #30
joachim CreditAttribution: joachim as a volunteer commented> I know this may sound heretical, but isn't this really a chance to reconsider the choice made to use annotations.
I don't think that's a bad idea. I for one find the notion of executable code inside comments to be slightly crazy.
> But more simply, it seems most annotations, especially those for plugins, could be replaced by static methods on the classes
That would still load the classes, and that's one of the things this issue wants to prevent.
Also, for the same of backwards compatibility, we'd need to keep on supporting annotations... so we still need this issue for the sake of the ton of contrib plugins that won't necessarily convert to a new system for plugin metadata.
(Is there an issue for replacing the annotation system?)
Comment #31
Mile23The specific issue here is that we currently use annotations for discovery and that process eats up memory and op cache. Using SimpleAnnotationReader is what causes these problems.
If anyone wants to re-architect this system, please file an issue about it.
Comment #32
fgmJust wondering: if the class loading occurred in an subprocess, wouldn't it solve the memory issue ? As in, if class_exists($klass, false) use it, else run a subprocess with the autoloader to load it and return the result to parent. Anyway, creating a follow-up to discuss this at #2919424: Deprecate annotations, one of the four plugin discovery approaches.
Comment #33
dawehnerI wrote our own implementation of
SimpleAnnotationReader
, but then I thought, where do we actually do the loading of the class using reflection.Does someone know where that happens and can point it out in the issue summary? There is one place inside
DocParser::collectAnnotationMetadata
, but that actually just loads the annotation classes, not the actual plugin classes themselves.Comment #34
borisson_I think the patch in #33 is missing a few things? It can't just that - can it?
Comment #35
dawehnerNope, but I think we should do more, maybe decouple from the entire library or at least fix the class loading bit described vaguely in the issue summary
Comment #36
Mile23This is what I found. I can't work on it beyond this because of deadlines. Have fun. :-)
Comment #37
Mile23Comment #38
dawehner@Mile23
Well, doctrine is actually using
class StaticReflectionClass extends ReflectionClass
which should work around this particular problem.Comment #39
tim.plunkettI don't see where \Doctrine\Common\Annotations\SimpleAnnotationReader loads the class. Furthermore, I don't see where the interface \Doctrine\Common\Annotations\Reader would be any different?
We're using \Doctrine\Common\Reflection\StaticReflectionParser. Regardless of what Reader we have, that's what does the parsing and returns the ReflectionClass. Except as #38 says, it doesn't actually use reflection. It uses \Doctrine\Common\Reflection\StaticReflectionClass which is token parsing, after a
file_get_contents()
call.My understanding was that this loaded the file, but did not load the class into memory?
Comment #40
Mile23This is from
AnnotatedClassDiscovery
:We only use
StaticReflectionParser
because we have a path and we need its class name.$parser
is misnamed. It should be$class_name_parser
.Then we use
StaticReflectionParser::getReflectionClass()
becauseSimpleAnnotationReader::getClassAnnotation()
requires a\ReflectionClass
.StaticReflectionParser
is not doing the annotation parsing.StaticReflectionParser::getReflectionClass()
returns aStaticReflectionClass
, which extends\ReflectionClass
.Classes are loaded during reflection.
Therefore, we need to replace most of that code above with a method that takes the file name and parses its tokens enough to get the plugin annotation, or fails in a useful way so we can move on to the next file.
This part:
$class->getDocComment()
.Comment #41
EclipseGc CreditAttribution: EclipseGc at Acquia commented@Mile23
So following the code further:
AnnotatedPluginDiscovery::getDefinitions() line 142 instantiates new StaticReflectionParser
AnnotatedPluginDiscovery::getDefinitions() line 145 calls StaticReflectionParser::getReflectionClass() and passes it to SimpleAnnotationReader::getClassAnnotation()
SimpleAnnotationReader::getClassAnnotation() expects a \ReflectionClass
This draws into question what StaticReflectionParser::getReflectionClass() just did, so let's look:
StaticReflectionParser::getReflectionClass() instantiates a new StaticReflectionClass object and passes the StaticReflectionParser to it as it's only parameter.
StaticReflectionClass::__construct() simply saves the parser that was passed to it.
StaticReflectionClass::getDocComment() literally does:
So we can go back to StaticReflectionParser::getDocComment() which importantly calls $this->parse()
StaticReflectionParser::parse does a file_get_contents() against our file, and ultimately also tokenizes it via a new TokenParser object, which token_get_all()s the contents of the file, and then a bunch of token manipulation happens.
I never see a literal reflected class in this process. What am I missing?
Eclipse
Comment #42
Mile23StaticReflectionParser
*only* gets the class name, using tokenization.It does not parse for the docblock. It's the wrong place to look.
From #40:
Comment #43
EclipseGc CreditAttribution: EclipseGc at Acquia commentedYes but the "\ReflectionClass" passed to this method IS the StaticReflectionClass, whose getDocComment() method proxies to StaticReflectionParser::getDocComment() which calls StaticReflectionParser::parse() which tokenizes everything. StaticReflectionParser::getDocComment() Then simply returns values in StaticReflectionParser::docComment property.
Eclipse
Comment #44
Mile23OK, so yah. :-)
Just goes to show: Start with the test.
This issue was originally about eating up memory with tons of autoloaded classes. Anyone have a better demonstration of that problem?
We still need to replace SimpleAnnotationReader, but maybe not for those reasons.
Comment #45
dawehnerCan you prove that? I thought the entire point of this static reflection class was to avoid that.
Comment #46
neclimdulPretty decent test to show we're not loading the class. Though we already had one though but maybe that's just in doctrine. Here was the discussion that lead to writing SimpleAnnotationReader #2151829: Doctrine annotation parsing takes an unacceptable amount of time/memory on install where doctrine and php's out of the box reflection was parsing/loading the class into memory and breaking sites.
Comment #47
neclimdulSorry, didn't go far enough back in my search. Correct links should be.
#1683644: Use Annotations for plugin discovery
and
https://github.com/doctrine/common/pull/152
Regardless, the problem with reflection is that it has to act on PHP's internal class structure which must exists in memory and can't be disposed of. The implementation we're using from doctrine uses Tokenizer to parse a file's PHP structure into a data object we can then inspect and dispose of without leaving the class in memory. That will be a hard requirement because of problems like the issue I accidentally linked. :)
Comment #48
tim.plunkettBut AFAIK they aren't getting rid of StaticReflectionParser. Which is really the important part.
They're only getting rid of SimpleAnnotationReader, which is all of 100 lines of code, 99% of which delegates to the parser.
Comment #49
Mile23I didn't see any before, and just looked again.
Can we adapt the test from #44 for NamespaceAnnotationReader?
Comment #50
tim.plunkettNamespaceAnnotationReader doesn't have anything to do with loading the class though.
That's 100% a feature of \Doctrine\Common\Reflection\StaticReflectionParser
Which we happen to use in \Drupal\Component\Annotation\Plugin\Discovery\AnnotatedClassDiscovery.
But just because ACD also uses this new NamespaceAnnotationReader, is unrelated.
Comment #51
alexpottI like the direction - means that at some point we can fix https://github.com/doctrine/annotations/pull/56 / #2557433: Add internal, event, and property to the list of ignored annotations in the plugin annotation system
Comment #52
donquixote CreditAttribution: donquixote as a volunteer commentedIt seems the issue summary needs to be updated.
The supposed problem stated in the issue summary was that plugin class files are supposedly included during discovery.
Several comments in this thread explain that this is not the case.
This is also what I remember from when we introduced StaticReflectionClass for exactly this purpose.
It extends native ReflectionClass, but it does not behave like native ReflectionClass. The main difference being that it does not include the file.
So, the originally described problem does not exist.
The NamespaceAnnotationReader in #48 is a copy of Doctrine's SimpleAnnotationReader.
From reading around, I conclude the (only) remaining reason for this patch is that Doctrine's SimpleAnnotationReader is or will be deprecated, so we will maintain our own version of it.
Is this correct?
If so, we should update the issue summary, or open a new issue.
Issue summary should have a link to a place that shows that SimpleAnnotationReader will be deprecated (if this is indeed the case).
Comment #54
Mile23Something to consider: https://github.com/doctrine/annotations/pull/211
Comment #55
Mile23Moving back to @catch's rescope from #14 because the conversation from this pull request looks like
DocParser
is not going to allow ignore lists when namespaces are provided: https://github.com/doctrine/annotations/pull/211(That's our use case.)
So the problem isn't
SimpleAnnotationReader
orAnnotationReader
, it's thatDocParser
disallows our use case. And even if the PR is successful, the change likely won't occur in versions of doctrine/annotations that support PHP 5.5.So we probably need a general replacement strategy.
Comment #56
EclipseGc CreditAttribution: EclipseGc at Acquia commentedI certainly don't interpret all this data together as being "We need a general replacement". They seem favorable to your PR in spirit at least, and are mostly negotiating over the details of how not to forcibly break everyone else. That said, I can't imagine that we need to seriously consider a replacement based on PHP version. we're so close to no longer being stuck on 5.5 that I'm inclined to just "hold on" a little longer. The problem is of course that 5.6 isn't good enough. doctrine/annotations is not alone in this regard, and we can't fault devs for not wanting to support version of PHP at or past EOL... I'm very against rolling back to an NIH approach BECAUSE we don't upgrade our dependencies fast enough.
Eclipse
Comment #57
tim.plunkettLet's get it done upstream. And then if we want to discuss temporarily fork the repo to backport that fix. And if that gets done before Drupal 8.7, great. And if it doesn't, it won't matter because 8.7 will bump to PHP 7: #2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7
Comment #58
Mile23https://github.com/doctrine/annotations/pull/211 is now closed as 'invalid,' due to BC breaking for them.
This means we can't backport a fix, as in #57.
Since
DocParser
is markedfinal
, we can't subclass it for our own purposes. That leaves us with having our ownDocParser
clone which will short-circuit ignorable annotations such as@param
despite having namespaces. We want that so that we can continue to leverage other parts of doctrine/annotation such asDocLexer
.And if we're cloning
DocParser
, we can then cloneSimpleAnnotationReader
to our ends.Here's a PoC patch that includes the test from #2557433-40: Add internal, event, and property to the list of ignored annotations in the plugin annotation system with some edits that are the kind of thing you do when you're not sure whether you need to or not (such as
DocParser::__construct()
stuff).Whether this is a good idea: It beats re-writing the whole thing. Even if we re-wrote
DocParser
and the annotation reader, we'd still wantDocLexer
to get our tokens for us. Also, this locks us into doctrine/annotations <2.0.0 due to a namespace change, but v.2.0 will require PHP 7.2+ so we're OK for now. In fact, that's something that NW: add doctrine/annotations <2.0.0 to drupal/core-annotation's composer.json.Comment #59
Mile23Changing drupal/core-annotation composer.json based on test results from #2876669-55: Fix dependency version requirement declarations in components and the fact that doctrine/annotations changes their namespace structure in 2.0.x-dev. (it's not released yet.)
The rest is cleanup.
Comment #61
catchComment #62
catchComment #63
catchMoving this to a critical task - Doctrine has actually removed
SimpleAnnotationReader
in their 2.0 branch: https://github.com/doctrine/annotations/pull/199.While there's no release yet, as soon as there is one, we won't be able to update to it without resolving this issue. Given their 2.0 release is being prepared, it's likely there'll be a release and the eventually dropping of 1.6 support before the end of Drupal 9's support cycle. This means removing our dependency on the deprecated classes prior to 9.0.x's release.
Comment #64
catchThe last time I looked at this, I couldn't find any evidence that we're actually relying on reflection, it looked like the ReflectionParser we use is also using the tokenizer.
However we definitely need a replacement for SimpleAnnotationReader before Doctrine drops support for 1.x, which really means for 9.0.x.
Comment #65
Mile23Patch still applies, re-running test.
Comment #67
EclipseGc CreditAttribution: EclipseGc at Acquia commentedOk, so Tim and I were discussing this. At this point it seems like our best solution is to move off of SimpleAnnotationReader and standardize with the rest of the Doctrine-using community on adding 'use' statements to our classes for the annotations we use. To that end, Tim made the comment that if we could identify plugins that do not have the appropriate use statements and throw a trigger_error() we could move off of SimpleAnnotationReader as we upgrade to D9. This makes a lot of sense to me, and I believe would not fundamentally alter how we currently tokenize, but would also (long term) remove the @see, @etc annotations from the list of stuff we try to solve for in this process.
I haven't tested far enough to prove all of that, but I wanted to produce the leanest version of this patch to see if we could all agree on this as a way forward. If so, we will have to add the appropriate 'use' statements to all plugin classes in core to prevent triggering the error on ourselves. That will be a big patch, but its implications should be minimal beyond the raw size of the diff.
Eclipse
Comment #69
EclipseGc CreditAttribution: EclipseGc at Acquia commentedOk, I took this a little further. There's a long standing issue in plugin annotations that has driven me nuts since I came to understand how completely ignorant I was when I wrote that code. It prevents a plugin definition from ever having a state that would document all annotations within a plugin definition, which we'd want to know if we're going to do this. Also, yes I'm using a little reflection to extract all the plugin definition related annotations, but it's behind a cached action and we'd remove all this code in 9.0.0 so I think those things mitigate what I'm doing here. Also added the @ in front of trigger_error() heh... my bad.
Anyway, this should find all Annotation classes and ensure they're in the use statements and if not, it'll trigger an error. We would convert all core plugin classes using annotations to have actual corresponding use statements for their annotations, which is what the rest of the PHP world does and is easier from a discoverability perspective and doesn't require special Drupal specific plugins in PHPStorm to work any longer which are all net wins IMO. Let's see if testbot hates this or not.
Eclipse
Comment #71
borisson_This is great. Aligning ourselves with more of the PHP world that comes with little to no downsides is a good thing!
The current fails (all 2400 of them) are because of the deprecation notice. When we get consensus on this, should we change core in this issue as well?
Comment #72
alexpottWe'll need to update \Drupal\Sniffs\Classes\UnusedUseStatementSniff to be able to detect a used use statement in this case. Plus it would be good to check that PHPStorm is now fixed for this too.
Personally I'm not a huge fan of a use statement for an annotation in a comment. It feels very meh.
Comment #73
Berdir> Plus it would be good to check that PHPStorm is now fixed for this too.
More than just fixed. It actually actively complains about the missing use now.
> Personally I'm not a huge fan of a use statement for an annotation in a comment. It feels very meh.
Kinda feel the same, but that might just be a question of being used to it or not. But we do have to keep in mind that we *explicitly* switched to this annotation reader for the main purpose of not having to add the use statements. So it might need a better argument than than just being able to do it like everyone else :) (by providing pro/con for this change, e.g. vs implementing our own annotation reader).
Comment #74
EclipseGc CreditAttribution: EclipseGc at Acquia commentedYeah, for me, for a long time now, I've felt very compelled to align to the rest of PHP. I know that's perhaps not a good enough reason all on its own, but I rewrote the plugin system from scratch outside of a Drupal a couple years ago now just as an exercise in "What would this look like if I did it again from scratch today" and was much more pleased with the general results than what exists in Drupal. Of course, any dev can look at code they wrote a month ago and say something similar, but still the point is that I got used to the idea of annotation classes in use statements, and then I actively started to like it because it meant annotations benefited from all the same goodness namespaced classes always had i.e. I could now have 2 different "Block" annotations in different namespaces and not have it end up mattering (for example). Even though I've never needed that, I could see how someone, somewhere has probably needed that. So as a quick pro/con attempt (other people, please add to it):
Pros:
Cons:
I'm trying to be fair here, but I couldn't think of other cons. If other people have any, let's expand the list. If we decide to go this direction we can update the IS with a canonical list.
Eclipse
Comment #75
alexpottCons is missing:
All of contrib / custom / core plugins have to change to be supported in Drupal 8 / 9 - more busy work for no obvious gain to the developer making the change.
Comment #76
EclipseGc CreditAttribution: EclipseGc at Acquia commentedFair enough. Though I'd argue that the burden on each developer will be rather low. Even in core, the volume of plugins associated to any plugin manager is a relative handful. Large numbers come in the way of derivatives which won't be affected by this change beyond their 1 plugin definition.
By way of a bit of justification, and I'm relying on really really ancient brain cells here, but as I recall I supported the move to SimpleAnnotationReader because PHPStorm didn't show the Annotation class as having been used in the class in the first place. This meant we ended up with gray squigglies in the use statements, and we all universally hated that. I don't know if we all missed that there was a plugin for that, or if the plugin didn't exist, or didn't fix that at the time, but whatever the case, that was the big _REASON_ as I recall. Or at the very least, it was my reason for supporting the change. We've got a bit of technical debt around that decision and there are other issues filed that are related to our use of SimpleAnnotationReader at this point. Changing off of it completely has potential to fix those issues and get us on a more maintained code path. In the abstract, that all sounds good, I guess the question is just whether or not we all agree it sounds good enough to pursue.
I'll see if I have a few minutes to look into the UnusedUseStatementSniff issue you mentioned and see if I can figure out what would be required for that.
Eclipse
Comment #77
tim.plunkett@dawehner, @damiankloip, and I had already done all of the annotations for Views (the first code to use the new plugin system), adding all the
use
statements really was a huge drag. It wasn't until almost a year later though that we looked into alternate approaches.Which resulted in #2090353: Don't require to put the use statement into plugin classes
So for the same reason it pissed us off enough to "fix" it back then, so too will everyone be pissed off to have to add them.
But I understand where @EclipseGc is coming from. Plus, SAR is dead upstream anyway...
Comment #78
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAre we sure there's nothing simple that we can override from
AnnotationReader
,StaticReflectionParser
, or something else in the chain, in order to insert the use statement at parse time?Comment #79
markhalliwellFTR, coder actively complains about these "unused classes" and is why I removed the annotation use statements from Bootstrap's code: #2825131: Run project through coder and fix notices and warnings.
Originally, I fought to keep these because the PHPStorm PHP Annotation plugin used them. However, the plugin was fixed to work without them so I relented and removed the use statements.
Regardless of the semantics, actually requiring these just so the annotation reader could parse them would technically be a breaking change and would require all contrib modules to explicitly add these to the codebase.
For this reason alone, I say requiring use statements should be moved to 9.x; nevermind the relentless ambiguity this topic has received over the years.
If this issue needs a resolution, I say that we implement a deprecated BC friendly version for ourselves and then change over in 9.x.
Comment #80
tim.plunkettThat's exactly what is being proposed. That's what @EclipseGc's patch does. Because in order to deprecate, we need to add the @trigger_error AND fix all of core's usages in D8, and only in 9.0.0 can we drop the BC layer.
If we wait for "9.x" then we won't be able to drop it until 10.0.0
Comment #81
EclipseGc CreditAttribution: EclipseGc at Acquia commentedYes, everything Tim just said +1000000.
Comment #82
andypost@effulgentsia Is this question from BC POV?
IMO it looks very nice way to refactor plugins (at least to prepare to 9.0)
So the only blockers are
- BC workaround
- sniffers
- possibly multiple annotation classes that may cause collision (I imagine a case when plugin with BC shim provides "use" as well)
Comment #83
effulgentsia CreditAttribution: effulgentsia at Acquia commentedSort of. What I mean though is maintaining BC into 9.x as well (i.e., not deprecating the lack of a
use
). #75 and #77 say that it will be a significant annoyance for all contrib plugins to have to add theuse
statements in in order to be compatible with 9.x. My question in #78 is asking why we think we need to force that annoyance on people. For example, can we subclassAnnotationReader
and overridegetClassAnnotations()
, within which we could call$this->parser->addNamespace()
instead of$this->parser->setImports()
? Or if not that exactly, then some other relatively painless class extension?Comment #84
tim.plunkettI mean, at that point we might as well copy SAR wholesale and be done. It's only 128 lines.
If that's not possible due to other breaking changes in Doctrine, then something along what #84 suggests sounds reasonable.
EDIT
I said #84 but that's this one. But #85 is better anyway :)
Comment #85
Mile23See analysis in #55 to #59. That's why I wrote it down.
The patch in #59 doesn't require us to re-write plugins with use statements, doesn't use reflection (the original reason this issue was filed), and, uh, still applies. :-)
That solves the problem of Doctrine dependency moving forward. Then we can discuss what behaviors to deprecate in #2919424: Deprecate annotations, one of the four plugin discovery approaches or another follow-up if someone wants to open it.
Yah, looks like a solution. :-)
Comment #86
tim.plunkettSo we need more than SAR. But with the above patch, what do we still need from doctrine/annotations?
Comment #87
effulgentsia CreditAttribution: effulgentsia at Acquia commentedSorry, I missed that. I haven't been following this issue all that closely. #75 just peaked my interest.
If the issue with #55 is that we can't use
DocParser::addNamespace()
, then can we still do #83, but with->setImports()
? Where we e.g., scan the directories of the namespaces we want to add in order to generate a list of all possible imports? Just trying to float ideas that would require less custom or forked code to maintain, but still achieve the goal of not burdening contrib maintainers with avoidable tedium.Comment #88
donquixote CreditAttribution: donquixote as a volunteer commentedHello,
just to chime in, having followed this issue for a while:
I maintain a distinct annotation parser package,
https://github.com/donquixote/annotation-parser
https://packagist.org/packages/donquixote/annotation-parser
The purpose was specifically to break the assumption that annotation names have to be interpreted as PHP class names.
Instead, it separates the parsing step from the interpretation step.
There are behavior components like
ClassNameResolverInterface
orObjectResolverInterface
, that allow to interpret annotation names the way we want to. It can be wired up to behave like Doctrine (with use statements) or like Drupal (without use statements) or something completely different.The
AnnotationReaderInterface::reflectorGetAnnotations()
takes a\Reflector
as parameter, which can be a native reflector or an "artificial" parser-based reflector.I remember that I crafted something in this direction, but then noticed that for my own purposes the native reflector worked well enough.
It has been a while since I worked on this. Perhaps it will work out of the box. Or perhaps not.
For now it is silently doing its job in cfrplugindiscovery.
What I could do, when I have more time, would be to start a proof-of-concept implementation in a separate issue.
(EDIT: Perhaps I have mentioned this in the past somewhere in the Drupal core issue queue. But I could not find it.)
Comment #89
larowlanCan we ship not only a sniff for detecting, but a sniff that attempts to auto-fix by adding the use statements.
We could make use of it to fix this patch but also, it would be super-useful for contrib module authors
Comment #90
andypost@larowlan for thst purpose we have https://www.drupal.org/project/drupalmoduleupgrader
Comment #92
yonailo CreditAttribution: yonailo as a volunteer commentedHello,
My point of view is like #80
I support the case for deprecation in 9.0.0
Are developers becoming so lazy as to not being able to add a couple of 'use' statements for their classes ?
If I 'use' an object, I need to declare the namespace of its class. That rule should apply everywhere, I don't really see the point of doing something "different" just because it seems to be easier. At the end it becomes awkward to understand, not easier.
I don't like Don't require to put the use statement into plugin classes, although a lot of people seem to be happy with it.
Comment #93
alexpottfor what's it's worth I feel a little different about this now. On one hand the shortcut we have now reduces verbosity. On the other hand our current solution is magical and reduces the ability for a developer to discover what is going on.
I think in these cases we should prefer solutions that make things super obvious to developers over solutions that are more ambiguous. Therefore I'm in favour of deprecating our use of the simple annotation reader and going full doctrine annotation reader.
In order to do that we need to do a couple of things:
Comment #94
EclipseGc CreditAttribution: EclipseGc at Acquia commented@alexpott,
I'm so very happy to hear you say that. Moving towards use statements when we would normally need them instead of magical solutions is a big++ in my opinion. It sounds like we're starting to find something closer to a consensus. Would it be worth pursuing the approach in #69 further?
Eclipse
Comment #95
catchComment #96
Gábor HojtsyRe-parent.
Comment #97
larowlanGiven where we are in the cycle, I don't think #93 is feasible anymore without major disruption.
So I think that means the patch at #59 is now our best bet
Here's a re-roll
Comment #98
catchAgreed with #97, we can always have a follow-up for requiring the use statements against 9.x (deprecating for 10.x).
Comment #99
alexpottI updated doctrine/common and doctrine/annotations after apply this patch.
The tests supplied by the patch ran fine, Drupal standard installed and navigating through the Views UI worked.
Comment #100
tim.plunkett@tedbow and I were just discussing the pros and cons of trying to push for this in 8.x, and I agree with #97.
Still needs an issue summary update, but I think the patch looks good.
Comment #101
catchUpdated the issue summary.
Comment #102
alexpottI'm wondering whether we need to bring in more of Doctrine's test coverage - ie. https://github.com/doctrine/annotations/blob/fa4c4e861e809d6a1103bd620cc... and test coverage of DocParser now that that is in our code base?
Comment #103
alexpottHere's the DocParserTest - https://github.com/doctrine/annotations/blob/fa4c4e861e809d6a1103bd620cc...
And the other test I think we should consider bringing is https://github.com/doctrine/annotations/blob/fa4c4e861e809d6a1103bd620cc... as that combines DocParser and SimpleAnnotatioReader.
Comment #104
larowlanPlus one for adding the tests.
Question: the files as they stand don't meet our coding standards, but because they retain much of the original Doctrine code - should we codingStandardsIgnore those files to make sure future diffs/backports are easier? Or should we make them meet our standards?
Comment #105
tedbowOk bringing in the DocParserTest
Shouldn't we keep our copied version also
final
?We don't actually want anybody extended this do we? It seems like that would be asking for trouble unless I missing something.
Comment #106
tedbowAnd now I will stick with convention and upload this as a patch file 😜
Comment #107
tedbowForgot to add @group and adding
// @codingStandardsIgnoreFile
to all the fixture files
Comment #109
alexpottPersonally I'm in favour of making our copies of these classes final. We know that at some point we want to deprecate and remove this from core. We should be doing everything we can to minimise additional API that we need to maintain as a result of copying this.
Comment #110
tedbowThis should extend
TestCase
Need to add an extra line in here, same in all files in this folder.
Comment #111
catchFine with adding final here these are very specific classes that no-one should be interacting with in any way whatsoever, but can we also explicitly mark them @internal?
Comment #113
tedbowThis test makes no asserts so it fails our tests. Updating to assert the result of
parse()
This test method and another 1 failed for the same reason
Namespace needs to be updated. caused tested failure
core/lib/Drupal/Component/Annotation/README.txt
we have
This seems to be a standard message for all our components. But in this case do we actually want people using this component?
Comment #114
mondrakeI think this is also needed to support PHP 7.4 in Drupal 8 given that HEAD (net of other deprecations to be addressed) has lots of failures on PHP 7.4.x-dev & MySQL 5.7 like for example
Comment #115
mondrakeThe corresponding fail of the original Doctrine code in this patch is at
$this->lexer->lookahead
can apparently be null at this point so trying to access it as an array fails in PHP 7.4.One workaround could be
and repeating
as last statement of the while loop.
Comment #116
tedbow@mondrake thanks for mentioning this. I did know about this requirement until recently. Relating #3085735: Support PHP 7.4 in Drupal 8
Since there are many other incompatibilities with 7.4 in vendor and core it seems like it would be a good idea to get this patch in and then do a follow up to make it 7.4 compatible. This way we can also commit the copies with as minimal changes as possible.
Comment #117
catchfwiw agreed with #116, fine with this going in then becoming 7.4 compatible with the rest of core.
Comment #118
tedbowUpdating the comments in the classes that mention they were copied from doctrine so they are the same across all classes.
I did not update all of the files in core/tests/Drupal/Tests/Component/Annotation/Doctrine/Fixtures with the comment but mentioned
\Drupal\Tests\Component\Annotation\Doctrine\DocParserTest
which is the test that requires these classes they were also copied.I will work on updating the IS now and make the follow for 9.x to require use statements
Comment #119
tedbowcreated #3086628: Adopt \Doctrine\Common\Annotations\AnnotationReader and deprecate SimpleAnnotationReader forked from Doctrine
Comment #120
alexpott@tedbow this looks great - that's for all the issue management work too. Great to have that in place.
I think we need a change record in the unlikely event that some contrib or custom code is using the SimpleAnnotationReader directly. Oh not unlikely at all... http://grep.xnddx.ru/search?text=SimpleAnnotationReader&filename= - one thought about how to deal with that is that we could alias our class to the doctrine namespace so everything would use our SimpleAnnoationReader... hmmm. Well at the very least we need that change record. I guess we can deal with the BC implications when we update to a version of Doctrine without the SimpleAnnoationReader
Comment #121
tedbowGlad it looks like we are getting this in before 8.8.0!
Here is the change record https://www.drupal.org/node/3086773
It makes sense that we want to make sure our version of doctrine doesn't go to >=1.3 but just wanted to point out that
\Doctrine\Common\Annotations\SimpleAnnotationReader
will still be available before 2.0I imagine though our fork is more likely to break if we go higher than >=1.3 but I haven't tested this.
Just wanted to point this out.
Comment #122
catch@alexpott #120 given our forked annotation reader would allow those modules to update now, and that we'll probably update to the version that drops this in a major core release, I don't think we need to do any more than we're already doing here for bc.
Comment #124
tim.plunkettAgreed with #122.
The CR looks good, the patch is good, issue summary and credits are up-to-date.
composer version constraints aren't my forte, but #121 makes sense to me
Comment #125
xjmI can't think of a way to raise a deprecation warning for this. Can anyone else? (Edit: because Doctrine itself doesn't mark it
@deprecated
, at least in the version we're using.)If there isn't one, we might have our first issue for the 9.0.0 release notes here.
Comment #126
xjmAlso patch apparently failed to apply; unsure why the issue didn't get marked NW? IIRC that may be a bug related to the bulk update but not sure.
Comment #127
xjm(For D9, not D8.)
Comment #128
xjmOh, one additional question. Are we forking all the code we need form
doctrine/annotations
(such that we could remove that dependency entirely), or does this fork depend on other code in that dependency? (And if we're removing the need fordoctrine/annotations
, what aboutdoctrine/common
? Or is that used for other things?)Comment #129
xjmIf the answer to #128 includes any "yes, we don't need that dependency anymore", I think we should still list this in the 8.8.0 release notes as well, because I'd like to have a section of all the whole dependencies and modules that have been deprecated for Drupal 9, since the upgrade path might require site owners and custom module developers to do something (besides just following the pattern for deprecated PHP code).
Comment #130
neclimdul@xjm Skimming the patch, the use statements at the top of the annotation class seem to imply we depend on doctrine's annotations still.
I guess a weird question this late but do we need to embed Doctrines license in these files or something? Seems like we'd need to include some sort of copyright/license since that's a clause in the MIT license they use.
Comment #131
larowlanComment #132
larowlanStraight re-roll, back to RTBC - we still need an answer to the license question from @neclimdul in #130
Comment #133
johnwebdev CreditAttribution: johnwebdev commentedYeah removing the license in the docblock seems questionable.
Also, the docblock states for the affected files another license LGPL rather than MIT.This definitely needs to be clarified.* That was from Doctrine Common, not Annotations.
Comment #134
catchWe still need doctrine/annotations. What we're forking is just a class or so that avoids the need to use annotation classes. The follow-up at #3086628: Adopt \Doctrine\Common\Annotations\AnnotationReader and deprecate SimpleAnnotationReader forked from Doctrine would remove our fork again and just rely entirely on doctrine/annotations (but will also be very disruptive because every plugin etc. will need to use annotation classes).
The licence question is a good one, I'd think we need to leave that in place and not remove it.
Comment #135
neclimdul@catch I'm not sure I understand your comment. There isn't a doctrine license in the patch currently. That's why I suggested embedding it in the docblock since its a limited number of files. Alternatively maybe we drop a license.txt in the namespace and test directories? INAL just guessing what we can do based on the MIT "shall be included" wording and us wanting to be clear on what is covered under this MIT and what is under Drupal's GPL.
Comment #136
catch@neclimdul oh good point, just mean I think we should have a mention of the license in the patch one way or another.
Comment #137
tedbow@neclimdul thanks for catching the license issue.
core/tests/Drupal/Tests/Component/Annotation/Doctrine
if mentions they are copies and then includes the original copy right.
because the original reads in the middle
The permission notice is above and the warranty is below
From my reading we don't have to include the warranty portion(and shouldn't because are relicensing?)
DocParserIgnoredClassesTest
out of this folder because this isn't a copy@file
Comment #138
kreynen CreditAttribution: kreynen at University of Colorado Boulder commentedTL;DR - The approach in #137 is solid, but likely overkill. AFAIK, there is no best practice for copying code parts of a project with one license into a project that use another. You are essentially forking the original project without maintaining an actual fork. The advice we are likely to get when asking how best to this will likely be not to do this.
Standard IANAL disclaimer. I chair the Drupal Association's Licensing Working Group. Like many working groups, this one tends to only get active when the DA requests our input. Our opinions about DA policy should not be confused with legal advice.
I'm going to use JQuery to describe what the Drupal project as done in the past. There may be more recent examples of handling MIT code differently, but this is example I'm most familiar with. Initially the Drupal project asked the JQuery project to dual license as MIT and GPL-2.0+, but after much discussion JQuery changed their license back to just MIT. Hopefully everyone agrees that a project distributed as GPL-2.0 can include MIT code regardless of whether the code is PHP or javascript and we can can just address the question of how to best do that.
There will be less agreement about how the licenses of third party code committed directly into another project with a different licnese. As far as I know, it is up to each project to establish this and I've seen it done many ways in many project and many ways within Drupal.
The goal should always be not to further restrict the rights any developer included with their code when they originally shared it. All of the suggestions I've looked at hear are already attempting to do that.
As @tedbow pointed out, there are 2 key parts of the MIT license in (emphasis mine) that we want to adhere to...
For the copyright notice, it is not saying that this cannot be altered in any way. Only that it must be included. In some situations, we've copied to copyright claims to https://git.drupalcode.org/project/drupal/blob/8.8.x/core/COPYRIGHT.txt instead of being left in their original file/format. In other places, we leave the copyright claims unaltered.
The original remains in effect, but we've been inconsistent about how we've handled this. You could only declare the copyright in https://git.drupalcode.org/project/drupal/blob/8.8.x/core/COPYRIGHT.txt and not in the docblock, but I personaly think the docblock is cleaner and easier for another developer to figure out the proviance of the code. There is no legal requirement to do both.
The other key part of the MIT license is the license text itself. Within Drupal, we often leave the license where the original project left it. https://git.drupalcode.org/project/drupal/blob/8.8.x/core/assets/vendor/... is a good example of this since this project use multiple license and we end up with copies of the license text in the project. We also have copies of the MIT license test https://git.drupalcode.org/project/drupal/blob/8.8.x/core/misc/icons/lic... and https://git.drupalcode.org/project/drupal/blob/8.8.x/core/themes/stable/....
I think this is why @tedbow is adding a copyright.md file. Not wrong, but this creates a lot of noise.
We currently don't have in Drupal is a license appendix like CKEditor with the text of the original licenses. The practice of including license text in the distribution was established back we open source was uncommon and distributions were often on floppy disk. Including the license text is still ideal, but it is increasingly common for projects to only link to the license via a URL.
We could consolidate the license text of the MIT into a single place as long as it is still clear which code is using which license. This can be done in the code itself like or in the project's manifest. We're not always doing this as well as we could be. For example https://git.drupalcode.org/project/drupal/blob/8.8.x/core/assets/vendor/... indicates that the project is licensed as MIT, but we probably should also be including https://github.com/jashkenas/backbone/blob/master/LICENSE in https://git.drupalcode.org/project/drupal/tree/8.8.x/core/assets/vendor/....
As more projects rely on package management to pull in dependency trees with the dependencies license intact, making sure the licenses are properly declared is getting easier. You end up with License.txt and License.md files all over your codebase, but it's relatively easy to track each project back to it's source.
This approach doesn't work when you only want part of a project. This also gets into a gray area if this code is further modified within the context of Drupal without contributing them back to the upstream project under the original license.
As far as I know, the Drupal project doesn't have a policy or recommendation for how to handle this.
My personal recommendation would be to keep it simple and extend the docblock already in the patch to include a link instead of the entire license rather than litter the codebase with more license files. Something like...
If there is a desire to do something more like CKEditor so we don't have so much licensing noise, let's tackle that in another issue.
Comment #139
tim.plunkettThanks @kreynen for that comment, very helpful.
With a broader follow-up already open (#3087886: [policy, maybe patch] core/COPYRIGHT.txt is inconsistent in which libraries or copyrights are mentioned), I think that the additions made in the last interdiff are sufficient for the scope of this issue. Back to RTBC!
Comment #143
catchAgreed the license looks good for now and we can address this more in a follow-up. We also have the follow-up to unfork (i.e. remove) this code in the 10.0.x branch.
Committed 40a7bb0 and pushed to 9.0.x, then cherry-picked to 8.9.x and 8.8.x. Thanks!
Added a release note.
Comment #144
tedbowThanks @catch and everyone!
Also looking forward to removing this in Drupal 10!🍴
Comment #145
catchComment #146
alexpottComment #147
MixologicThis seems to have introduced an odd bug with running phpunit on the command line, perhaps only with group?
Comment #148
alexpottHere's a fix for #147 - #3088518: Can not run tests using phpunit with the --group option
Comment #149
jibranPublished the change record.