Contributor tasks needed
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 usestatement 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

  1. \Doctrine\Common\Annotations\SimpleAnnotationReader
  2. \Doctrine\Tests\Common\Annotations\SimpleAnnotationReaderTest
  3. \Doctrine\Common\Annotations\DocParser
  4. \Doctrine\Tests\Common\Annotations\DocParserTest
  5. DocParserTest requires many fixture classes from Doctrine\Tests\Common\Annotations\Fixtures
  6. \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.

CommentFileSizeAuthor
#137 2631202-137.patch130.18 KBtedbow
#137 interdiff-131-137.txt5.16 KBtedbow
#131 2631202-131.patch127.79 KBlarowlan
#118 2631202-118.patch128.31 KBtedbow
#118 interdiff-113-118.txt3.68 KBtedbow
#113 2631202-111.patch127.49 KBtedbow
#113 interdiff-111-113.txt2.63 KBtedbow
#110 2631202-110.patch127.29 KBtedbow
#110 interdiff-107-110.txt17.92 KBtedbow
#107 2631202-107.patch127.25 KBtedbow
#107 interdiff-106-107.txt16.76 KBtedbow
#106 2631202-105.patch126.31 KBtedbow
#105 2631202-105.txt126.31 KBtedbow
#105 interdiff-97-105.txt80.85 KBtedbow
#97 2631202-97.patch47.53 KBlarowlan
#33 2631202-33.patch969 bytesdawehner
#35 2631202-35.patch17.71 KBdawehner
#44 2631202_44.patch2.67 KBMile23
#48 2631202-annotation-48.patch5.04 KBtim.plunkett
#58 2631202_58.patch47.45 KBMile23
#59 2631202_59.patch47.53 KBMile23
#59 interdiff.txt3.19 KBMile23
#67 2631202-67.patch1.28 KBEclipseGc
#69 2631202-69.patch4.96 KBEclipseGc
#73 Selection_406.png33.44 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cilefen created an issue. See original summary.

cilefen’s picture

Issue summary: View changes
dawehner’s picture

Yeah, sadly DocParser is even final, it will be hard to override anything without reimplementing basically everything.

neclimdul’s picture

Additionally, 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.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

cilefen’s picture

Version: 8.1.x-dev » 8.2.x-dev
Issue tags: +neworleans2016, +Needs issue summary update, +Triaged for D8 major current state

@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.

xjm’s picture

Thanks @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?

catch’s picture

Discussed 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.

xjm’s picture

Issue tags: +Triaged core major

Tagging. Thanks for helping confirm this major.

StephanieFuda’s picture

Thank 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.

xjm’s picture

(Updating credit, thanks @Stephanie_42!)

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

Title: Doctrine Annotations reader loads classes while parsing them » Remove dependency on Doctrine annotation reader by writing our own solution
Component: system.module » base system
Issue summary: View changes
Issue tags:

Doctrine (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.

catch’s picture

Issue summary: View changes
Issue tags:
Mile23’s picture

So 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?

dawehner’s picture

Is there any reason not to use https://github.com/php-annotations/php-annotations/blob/master/src/annot... or another external library?

I 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.

Mile23’s picture

Status: Active » Postponed

Classes like Component\AnnotatedClassDiscovery need more tests before this can go: #2435607: Tests for Annotation Component

dawehner’s picture

+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?

Mile23’s picture

That'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. :-)

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

Component: base system » plugin system
catch’s picture

Status: Postponed » Active

That issue is RTBC so unpostponing this.

dawehner’s picture

I 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.

Mile23’s picture

#2435607: Tests for Annotation Component is in, so we can push forward here.

joachim’s picture

Two 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...

Mile23’s picture

Title: Remove dependency on Doctrine annotation reader by writing our own solution » Replace SimpleAnnotationReader with a Drupal Annotation component class
Issue summary: View changes

This 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.

Mile23’s picture

fgm’s picture

I 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.

joachim’s picture

> 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?)

Mile23’s picture

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.

The 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.

fgm’s picture

Just 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.

dawehner’s picture

FileSize
969 bytes

I 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.

borisson_’s picture

I think the patch in #33 is missing a few things? It can't just that - can it?

dawehner’s picture

FileSize
17.71 KB

Nope, 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

Mile23’s picture

Issue summary: View changes

This is what I found. I can't work on it beyond this because of deadlines. Have fun. :-)

Mile23’s picture

Issue summary: View changes
dawehner’s picture

@Mile23
Well, doctrine is actually using class StaticReflectionClass extends ReflectionClass which should work around this particular problem.

tim.plunkett’s picture

I 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?

Mile23’s picture

This is from AnnotatedClassDiscovery:

  protected function getAnnotationReader() {
    if (!isset($this->annotationReader)) {
      $this->annotationReader = new SimpleAnnotationReader();
[..]

  public function getDefinitions() {
    $definitions = [];

    $reader = $this->getAnnotationReader();
[..]
              // The filename is already known, so there is no need to find the
              // file. However, StaticReflectionParser needs a finder, so use a
              // mock version.
              $finder = MockFileFinder::create($fileinfo->getPathName());
              $parser = new StaticReflectionParser($class, $finder, TRUE);

              /** @var $annotation \Drupal\Component\Annotation\AnnotationInterface */
              if ($annotation = $reader->getClassAnnotation($parser->getReflectionClass(), $this->pluginDefinitionAnnotationName)) {
                $this->prepareAnnotationDefinition($annotation, $class);

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() because SimpleAnnotationReader::getClassAnnotation() requires a \ReflectionClass. StaticReflectionParser is not doing the annotation parsing.

StaticReflectionParser::getReflectionClass() returns a StaticReflectionClass, 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.

I don't see where \Doctrine\Common\Annotations\SimpleAnnotationReader loads the class.

    public function getClassAnnotation(\ReflectionClass $class, $annotationName)
    {
        foreach ($this->getClassAnnotations($class) as $annot) {

[..]
    public function getClassAnnotations(\ReflectionClass $class)
    {
        return $this->parser->parse($class->getDocComment(), 'class '.$class->getName());
    }

This part: $class->getDocComment().

EclipseGc’s picture

@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:


return $this->staticReflectionParser->getDocComment();

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

Mile23’s picture

StaticReflectionParser *only* gets the class name, using tokenization.

It does not parse for the docblock. It's the wrong place to look.

From #40:

I don't see where \Doctrine\Common\Annotations\SimpleAnnotationReader loads the class.

    public function getClassAnnotation(\ReflectionClass $class, $annotationName)
    {
        foreach ($this->getClassAnnotations($class) as $annot) {

[..]
    public function getClassAnnotations(\ReflectionClass $class)
    {
        return $this->parser->parse($class->getDocComment(), 'class '.$class->getName());
    }

This part: $class->getDocComment().

EclipseGc’s picture

Yes 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

Mile23’s picture

Status: Active » Needs review
FileSize
2.67 KB

OK, 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.

dawehner’s picture

Classes are loaded during reflection.

Can you prove that? I thought the entire point of this static reflection class was to avoid that.

neclimdul’s picture

Pretty 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.

neclimdul’s picture

Sorry, 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. :)

tim.plunkett’s picture

But 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.

Mile23’s picture

Pretty decent test to show we're not loading the class. Though we already had one though but maybe that's just in doctrine

I didn't see any before, and just looked again.

Can we adapt the test from #44 for NamespaceAnnotationReader?

tim.plunkett’s picture

NamespaceAnnotationReader 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.

alexpott’s picture

donquixote’s picture

It 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).

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Mile23’s picture

Title: Replace SimpleAnnotationReader with a Drupal Annotation component class » Remove dependency on Doctrine annotation reader by writing our own solution

Moving 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 or AnnotationReader, it's that DocParser 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.

EclipseGc’s picture

I 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

tim.plunkett’s picture

Let'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

Mile23’s picture

https://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 marked final, we can't subclass it for our own purposes. That leaves us with having our own DocParser 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 as DocLexer.

And if we're cloning DocParser, we can then clone SimpleAnnotationReader 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 want DocLexer 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.

Mile23’s picture

Changing 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.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

catch’s picture

Issue summary: View changes
catch’s picture

Category: Bug report » Task
Priority: Major » Critical
Issue tags: +Drupal 9

Moving 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.

catch’s picture

Title: Remove dependency on Doctrine annotation reader by writing our own solution » Doctrine no longer supports SimpleAnnotationReader, incorporate a solution into core
Issue tags: -Performance, -neworleans2016, -Triaged for D8 major current state, -Triaged core major +Needs issue summary update

The 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.

Mile23’s picture

Patch still applies, re-running test.

Status: Needs review » Needs work

The last submitted patch, 59: 2631202_59.patch, failed testing. View results

EclipseGc’s picture

Status: Needs work » Needs review
FileSize
1.28 KB

Ok, 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

Status: Needs review » Needs work

The last submitted patch, 67: 2631202-67.patch, failed testing. View results

EclipseGc’s picture

Status: Needs work » Needs review
FileSize
4.96 KB

Ok, 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

Status: Needs review » Needs work

The last submitted patch, 69: 2631202-69.patch, failed testing. View results

borisson_’s picture

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.

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?

alexpott’s picture

We'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.

Berdir’s picture

FileSize
33.44 KB

> 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).

EclipseGc’s picture

Yeah, 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:

  1. Alignment with PHP reducing yet one more Drupalism
  2. Namespaces will actively work for annotations of the same name. (just like php classes)
  3. Better PHPStorm integration (No need for Drupal specific annotation plugins)
  4. Don't have to create our own replacement for SimpleAnnotationReader
  5. I THINK it'll remove the @see, @etc matching that's happening today, not sure though we should test it.

Cons:

  1. Drupal's used to not having annotations in the use statements and some people dislike that change.

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

alexpott’s picture

Cons 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.

EclipseGc’s picture

Fair 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

tim.plunkett’s picture

@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...

effulgentsia’s picture

Are 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?

markhalliwell’s picture

FTR, 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.

tim.plunkett’s picture

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.

That'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

EclipseGc’s picture

Yes, everything Tim just said +1000000.

andypost’s picture

Are 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?

@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)

effulgentsia’s picture

@effulgentsia Is this question from BC POV?

Sort 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 the use 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 subclass AnnotationReader and override getClassAnnotations(), 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?

tim.plunkett’s picture

I 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 :)

Mile23’s picture

For example, can we subclass AnnotationReader and override getClassAnnotations(), within which we could call $this->parser->addNamespace() instead of $this->parser->setImports()?

See 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.

I mean, at that point we might as well copy SAR wholesale and be done.

Yah, looks like a solution. :-)

tim.plunkett’s picture

So we need more than SAR. But with the above patch, what do we still need from doctrine/annotations?

effulgentsia’s picture

See analysis in #55 to #59. That's why I wrote it down.

Sorry, 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.

donquixote’s picture

Hello,
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 or ObjectResolverInterface, 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.)

larowlan’s picture

Can 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

andypost’s picture

@larowlan for thst purpose we have https://www.drupal.org/project/drupalmoduleupgrader

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

yonailo’s picture

Hello,

My point of view is like #80

That'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

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.

alexpott’s picture

Personally I'm not a huge fan of a use statement for an annotation in a comment. It feels very meh.

for 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:

EclipseGc’s picture

@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

catch’s picture

larowlan’s picture

Status: Needs work » Needs review
FileSize
47.53 KB

Given 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

catch’s picture

Agreed with #97, we can always have a follow-up for requiring the use statements against 9.x (deprecating for 10.x).

alexpott’s picture

I updated doctrine/common and doctrine/annotations after apply this patch.

> Drupal\Core\Composer\Composer::ensureComposerVersion
Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 3 installs, 2 updates, 0 removals
  - Updating doctrine/annotations (v1.2.7 => v1.7.0): Loading from cache
> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
  - Installing doctrine/reflection (v1.0.0): Loading from cache
> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
  - Installing doctrine/event-manager (v1.0.0): Loading from cache
> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
  - Installing doctrine/persistence (1.1.1): Loading from cache
> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
  - Updating doctrine/common (v2.6.2 => v2.11.0): Loading

The tests supplied by the patch ran fine, Drupal standard installed and navigating through the Views UI worked.

tim.plunkett’s picture

@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.

catch’s picture

Issue summary: View changes

Updated the issue summary.

alexpott’s picture

I'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?

alexpott’s picture

Here'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.

larowlan’s picture

Plus 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?

tedbow’s picture

FileSize
80.85 KB
126.31 KB

Ok bringing in the DocParserTest

  1. @alexpott was there a reason you linked to the particular version. We copied DocParser from the v1.2.7 tag it seems so I think should get the test from there also
  2. DocParserTest relies on tons of Fixture files to pass. I copied all of these.
  3. DocParserTest references these fixtures by full namespace through out the file so it needed tons of changes to pass 😞
  4. The fixture files also reference other fixture classes by namespace so they also have tons of changes
  5. Also copied DCOM58Test and DCOM58Entity.php which it needs
+++ b/core/lib/Drupal/Component/Annotation/Doctrine/DocParser.php
@@ -0,0 +1,1121 @@
+class DocParser

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.

tedbow’s picture

And now I will stick with convention and upload this as a patch file 😜

tedbow’s picture

Forgot to add @group and adding
// @codingStandardsIgnoreFile
to all the fixture files

Status: Needs review » Needs work

The last submitted patch, 107: 2631202-107.patch, failed testing. View results

alexpott’s picture

Personally 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.

tedbow’s picture

Status: Needs work » Needs review
FileSize
17.92 KB
127.29 KB
  1. re #104. I think we should use codingStandardsIgnore which I added to all the files. Hopefully we won't have to touch these files except for backports
  2. re #109 Making the classes final
  3. +++ b/core/tests/Drupal/Tests/Component/Annotation/Doctrine/DocParserTest.php
    @@ -0,0 +1,1374 @@
    +class DocParserTest extends \PHPUnit_Framework_TestCase
    

    This should extend TestCase

  4. +++ b/core/tests/Drupal/Tests/Component/Annotation/Doctrine/Fixtures/Annotation/Secure.php
    @@ -0,0 +1,18 @@
    +// @codingStandardsIgnoreFile
    +namespace Drupal\Tests\Component\Annotation\Doctrine\Fixtures\Annotation;
    

    Need to add an extra line in here, same in all files in this folder.

catch’s picture

Fine 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?

Status: Needs review » Needs work

The last submitted patch, 110: 2631202-110.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
127.49 KB
  1. Adding @internal
  2. +++ b/core/tests/Drupal/Tests/Component/Annotation/Doctrine/DocParserTest.php
    @@ -0,0 +1,1375 @@
    +        try {
    +            $parser = $this->createTestParser();
    +            $parser->parse($docblock);
    +        } catch (\Exception $e) {
    +            $this->fail($e->getMessage());
    +        }
    

    This 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

  3. +++ b/core/tests/Drupal/Tests/Component/Annotation/Doctrine/Ticket/DCOM58Test.php
    @@ -0,0 +1,119 @@
    +namespace Doctrine\Tests\Common\Annotations\Ticket;
    

    Namespace needs to be updated. caused tested failure

  4. Also in
    core/lib/Drupal/Component/Annotation/README.txt
    we have

    The Drupal Annotation Component

    Thanks for using this Drupal component.

    This seems to be a standard message for all our components. But in this case do we actually want people using this component?

mondrake’s picture

Issue tags: +PHP 7.4

I 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

exception: [Other] Line 0 of sites/default/files/simpletest/phpunit-21.xml:
PHPunit Test failed to complete; Error: PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\aggregator\Functional\ImportOpmlTest
E                                                                   1 / 1 (100%)

Time: 1.46 seconds, Memory: 4.00MB

There was 1 error:

1) Drupal\Tests\aggregator\Functional\ImportOpmlTest::testOpmlImport
Trying to access array offset on value of type null

/var/www/html/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php:967
/var/www/html/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php:665
/var/www/html/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php:641
/var/www/html/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php:334
/var/www/html/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php:496
/var/www/html/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php:718
/var/www/html/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php:641
/var/www/html/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php:334
/var/www/html/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/SimpleAnnotationReader.php:67
/var/www/html/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/SimpleAnnotationReader.php:91
/var/www/html/core/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php:145
/var/www/html/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php:86
/var/www/html/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php:284
/var/www/html/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php:175
/var/www/html/core/lib/Drupal/Core/Render/ElementInfoManager.php:110
/var/www/html/core/lib/Drupal/Core/Render/ElementInfoManager.php:77
/var/www/html/core/lib/Drupal/Core/Form/FormBuilder.php:812
/var/www/html/core/lib/Drupal/Core/Form/FormBuilder.php:486
/var/www/html/core/includes/install.core.inc:971
/var/www/html/core/includes/install.core.inc:625
/var/www/html/core/includes/install.core.inc:578
/var/www/html/core/includes/install.core.inc:117
/var/www/html/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:296
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:565
/var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:398
/var/www/html/core/modules/aggregator/tests/src/Functional/AggregatorTestBase.php:36
/var/www/html/core/modules/aggregator/tests/src/Functional/ImportOpmlTest.php:25

ERRORS!
Tests: 1, Assertions: 0, Errors: 1.
mondrake’s picture

The corresponding fail of the original Doctrine code in this patch is at

+++ b/core/lib/Drupal/Component/Annotation/Doctrine/DocParser.php
@@ -0,0 +1,1123 @@
+        while ($this->lexer->lookahead['position'] === ($this->lexer->token['position'] + strlen($this->lexer->token['value']))

$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

+        $position = isset($this->lexer->lookahead) ? $this->lexer->lookahead['position'] : null;
+        while ($position === ($this->lexer->token['position'] + strlen($this->lexer->token['value']))

and repeating

+        $position = isset($this->lexer->lookahead) ? $this->lexer->lookahead['position'] : null;

as last statement of the while loop.

tedbow’s picture

@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.

catch’s picture

Issue summary: View changes

fwiw agreed with #116, fine with this going in then becoming 7.4 compatible with the rest of core.

tedbow’s picture

Updating 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

tedbow’s picture

alexpott’s picture

Issue tags: +Needs change record

@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

tedbow’s picture

Glad it looks like we are getting this in before 8.8.0!

Here is the change record https://www.drupal.org/node/3086773

+++ b/core/lib/Drupal/Component/Annotation/composer.json
@@ -7,7 +7,7 @@
-    "doctrine/annotations": "1.2.*",
+    "doctrine/annotations": "^1.2",

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.0

I 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.

catch’s picture

@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.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Agreed 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

xjm’s picture

Issue tags: +9.0.0 release notes

I 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.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Also 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.

xjm’s picture

Issue tags: +Needs release note

(For D9, not D8.)

xjm’s picture

Oh, 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 for doctrine/annotations, what about doctrine/common? Or is that used for other things?)

xjm’s picture

If 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).

neclimdul’s picture

@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.

larowlan’s picture

Status: Needs work » Needs review
FileSize
127.79 KB
larowlan’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

Straight re-roll, back to RTBC - we still need an answer to the license question from @neclimdul in #130

johnwebdev’s picture

Status: Reviewed & tested by the community » Needs review

Yeah 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.

catch’s picture

Oh, 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?

We 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.

neclimdul’s picture

@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.

catch’s picture

@neclimdul oh good point, just mean I think we should have a mention of the license in the patch one way or another.

tedbow’s picture

@neclimdul thanks for catching the license issue.

  1. I am adding 1 copyright.md file to 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 above copyright notice and this permission notice shall be included in all
    copies or substantial portions of the Software.

    The permission notice is above and the warranty is below

    THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR.......

    From my reading we don't have to include the warranty portion(and shouldn't because are relicensing?)

  2. I moved DocParserIgnoredClassesTest out of this folder because this isn't a copy
  3. For the 2 files in core/lib/Drupal/Component/Annotation/Doctrine I include the copyright message in the @file
  4. I didn't include them in core/COPYRIGHT.txt because I think we just including the message as "Original copyright" Since we have already changed them I don't think the original is in effect. But not sure
kreynen’s picture

TL;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...

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

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.

Since we have already changed them I don't think the original is in effect.

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...

/**
 * A parser for docblock annotations.
 *
 * This class is a near-copy of Doctrine\Common\Annotations\DocParser, which is
 * part of the Doctrine project: <http://www.doctrine-project.org>. It was
 * copied from version 1.2.7 under the MIT license. For licensing, see
 * https://github.com/doctrine/annotations/blob/v1.2.7/LICENSE
 *
 * This Drupal version allows for ignoring annotations when namespaces are
 * present.
 *
 * @internal
 */

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.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @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!

  • catch committed 40a7bb0 on 9.0.x
    Issue #2631202 by tedbow, Mile23, EclipseGc, dawehner, larowlan, tim....

  • catch committed 713618a on 8.9.x
    Issue #2631202 by tedbow, Mile23, EclipseGc, dawehner, larowlan, tim....

  • catch committed d1fca82 on 8.8.x
    Issue #2631202 by tedbow, Mile23, EclipseGc, dawehner, larowlan, tim....
catch’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Agreed 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.

tedbow’s picture

Thanks @catch and everyone!

Also looking forward to removing this in Drupal 10!🍴

catch’s picture

Version: 8.9.x-dev » 8.8.x-dev
Issue tags: -Needs release note +8.0.0 release notes
alexpott’s picture

Issue tags: -8.0.0 release notes +8.8.0 release notes
Mixologic’s picture

This seems to have introduced an odd bug with running phpunit on the command line, perhaps only with group?

➜  core git:(8.9.x) ✗ ../vendor/bin/phpunit --group Metapackage

PHP Fatal error:  Cannot declare class Entity, because the name is already in use in /Users/Ryan/Documents/Work/Current/DrupalAssociation/drupal_core/drupal_8/core/tests/Drupal/Tests/Component/Annotation/Doctrine/Ticket/DCOM58Entity.php on line 13

Fatal error: Cannot declare class Entity, because the name is already in use in /Users/Ryan/Documents/Work/Current/DrupalAssociation/drupal_core/drupal_8/core/tests/Drupal/Tests/Component/Annotation/Doctrine/Ticket/DCOM58Entity.php on line 13
alexpott’s picture

jibran’s picture

Published the change record.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.