@chx seems to have added this, but why don't we use it?

CommentFileSizeAuthor
#8 1901390_8.patch940 byteschx
drupal8.annotation-only.0.patch1.8 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

EclipseGc’s picture

+++ b/core/vendor/doctrine/common/lib/Doctrine/Common/Reflection/StaticReflectionParser.phpundefined
@@ -119,7 +119,7 @@ protected function parse()
         $this->parsed = true;
         $contents = file_get_contents($fileName);
         if ($this->classAnnotationOptimize) {
-            if (preg_match("/(\A.*)^\s+(abstract|final)?\s+class\s+$className\s+{/sm", $contents, $matches)) {
+            if (preg_match('/(\A.*)^\s+(abstract|final)?\s+class\s+' . preg_quote($this->className, '/') . '\s+{/sm', $contents, $matches)) {
                 $contents = $matches[1];
             }
         }

This has to be done upstream in Doctrine. Do we need this for the change in AnnotatedClassDiscovery?

chx’s picture

wat? I ack the bug that $className needs to be $this->className (needs to be fixed upstream) but since when does a php identifier need a preg_quote??

chx’s picture

This has been fixed upstream about one day before sun filed this issue.

sun’s picture

The primary and essential change in question is only this anyway:

+++ b/core/lib/Drupal/Component/Plugin/Discovery/AnnotatedClassDiscovery.php
@@ -86,7 +86,7 @@ public function getDefinitions() {
-              $parser = new StaticReflectionParser($class, $finder);
+              $parser = new StaticReflectionParser($class, $finder, TRUE);

Was there any particular reason for not using the option?

chx’s picture

None but we need the commit i linked before we can use it cos *blush* it's broken in the current version core has.

jibran’s picture

drupal8.annotation-only.0.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal8.annotation-only.0.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
940 bytes
chx’s picture

Title: Enable StaticReflectionParser performance optimization to speed up Annotation parsing? » Enable StaticReflectionParser performance optimization to speed up Annotation parsing
Status: Needs review » Reviewed & tested by the community

I have only rerolled #0 really w/o the Doctrine change -- and I know this works :)

neclimdul’s picture

Seems reasonable. To be clear because I was confused, we already pulled in the other changes so we just need to turn on the optimization. I can't see a problem with that its pretty straightforward and if you're following our standards there won't be a problem.

RTBC+1

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 47ff5ff and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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