Drupal needs comments in opcache [#2421451]

Problem/Motivation

Drupal makes use of annotations which appear in comments. As such, comments must be preserved by opcode caches. PHP's Opcache, for example, allows you to omit comments in the cached opcode, which will result in errors like this:

$ drush cr
exception 'Doctrine\Common\Annotations\AnnotationException' with message '[Semantical Error] The class "Drupal\Core\Entity\Annotation\ContentEntityType" is not annotated with @Annotation. Are you sure this class  [error]
can be used as annotation? If so, then you need to add @Annotation to the _class_ doc comment of "Drupal\Core\Entity\Annotation\ContentEntityType". If it is indeed no annotation, then you need to add
@IgnoreAnnotation("ContentEntityType") to the _class_ doc comment of class Drupal\aggregator\Entity\Feed.' in
/www/d8/core/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/AnnotationException.php:54
Stack trace:
#0 /www/d8/core/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php(727): Doctrine\Common\Annotations\AnnotationException::semanticalError('The class "Drup...')
#1 /www/d8/core/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php(641): Doctrine\Common\Annotations\DocParser->Annotation()
#2 /www/d8/core/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/DocParser.php(334): Doctrine\Common\Annotations\DocParser->Annotations()
#3 /www/d8/core/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/SimpleAnnotationReader.php(67): Doctrine\Common\Annotations\DocParser->parse('/**\n * Defines ...', 'class Drupal\\ag...')
#4 /www/d8/core/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/SimpleAnnotationReader.php(91):
Doctrine\Common\Annotations\SimpleAnnotationReader->getClassAnnotations(Object(Doctrine\Common\Reflection\StaticReflectionClass))
#5 /www/d8/core/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php(114):
Doctrine\Common\Annotations\SimpleAnnotationReader->getClassAnnotation(Object(Doctrine\Common\Reflection\StaticReflectionClass), 'Drupal\\Core\\Ent...')
#6 /www/d8/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php(90): Drupal\Component\Annotation\Plugin\Discovery\AnnotatedClassDiscovery->getDefinitions()
#7 /www/d8/core/lib/Drupal/Core/Entity/EntityManager.php(229): Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDefinitions()
#8 /www/d8/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php(151): Drupal\Core\Entity\EntityManager->findDefinitions()
#9 /www/d8/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryCachedTrait.php(27): Drupal\Core\Plugin\DefaultPluginManager->getDefinitions()
#10 /www/d8/core/lib/Drupal/Core/Entity/EntityManager.php(250): Drupal\Core\Plugin\DefaultPluginManager->getDefinition('field_storage_c...', false)
#11 /www/d8/core/lib/Drupal/Core/Entity/EntityManager.php(338): Drupal\Core\Entity\EntityManager->getDefinition('field_storage_c...')
#12 /www/d8/core/lib/Drupal/Core/Entity/EntityManager.php(274): Drupal\Core\Entity\EntityManager->getHandler('field_storage_c...', 'storage')
#13 /www/d8/core/includes/entity.inc(189): Drupal\Core\Entity\EntityManager->getStorage('field_storage_c...')
#14 /www/d8/core/modules/field/field.module(148): entity_load_multiple_by_properties('field_storage_c...', Array)
#15 /www/d8/core/lib/Drupal/Core/Extension/ModuleHandler.php(494): field_system_info_alter(Array, Object(Drupal\Core\Extension\Extension), 'module')
#16 /www/d8/core/modules/system/system.module(928): Drupal\Core\Extension\ModuleHandler->alter('system_info', Array, Object(Drupal\Core\Extension\Extension), 'module')
#17 /www/d8/core/modules/system/system.module(986): _system_rebuild_module_data()
#18 /www/d8/core/includes/common.inc(1754): system_rebuild_module_data()
#19 /www/d8/core/includes/utility.inc(50): drupal_flush_all_caches()
#20 /.composer/vendor/drush/drush/commands/core/cache.drush.inc(278): drupal_rebuild(Object(Symfony\Component\ClassLoader\ApcClassLoader), Object(Symfony\Component\HttpFoundation\Request))
#21 [internal function]: drush_cache_rebuild()
#22 /.composer/vendor/drush/drush/includes/command.inc(359): call_user_func_array('drush_cache_reb...', Array)
#23 /.composer/vendor/drush/drush/includes/command.inc(210): _drush_invoke_hooks(Array, Array)
#24 [internal function]: drush_command()
#25 /.composer/vendor/drush/drush/includes/command.inc(178): call_user_func_array('drush_command', Array)
#26 /.composer/vendor/drush/drush/lib/Drush/Boot/DrupalBoot.php(46): drush_dispatch(Array)
#27 /.composer/vendor/drush/drush/drush.php(74): Drush\Boot\DrupalBoot->bootstrap_and_dispatch()
#28 /.composer/vendor/drush/drush/drush.php(11): drush_main()
#29 {main}

The problem is insidious in HEAD, however, because it is not possible to use hook_requirements() to warn someone installing Drupal. Annotations are needed before hook_requirements() has a chance to be called.

Steps to reproduce

Add opcache.save_comments=0 into your php 5.5 php.ini
Run drush cr

Proposed resolution

Update https://www.drupal.org/requirements/php (done)
Add requirements errors. This may have to be low-level because of the hook_requirements() issue mentioned above.

Remaining tasks

Discuss whether this limitation is okay.

User interface changes

API changes

Comment	File	Size	Author
#14	drupal_needs_comments-2421451-14.patch	601 bytes	cilefen
#6	drupal_needs_comments-2421451-6.patch	641 bytes	cilefen
#3	drupal_needs_comments-2421451-3.patch	1.12 KB	cilefen

Comments

Comment #1

dawehner

German

commented 2 November 2015 at 14:45

Component:

base system

» system.module

IMHO we have to add a hook_requirements check here, moving to system module for that

Comment #2

cilefen commented 2 November 2015 at 15:00

Assigned:

Unassigned

» cilefen

Comment #3

cilefen commented 2 November 2015 at 15:09

Status:

Active

» Needs review

Status	File	Size
new	drupal_needs_comments-2421451-3.patch	1.12 KB

Unfortunately, this doesn't work because annotations are already needed before this hook is invoked.

Comment #4

cilefen commented 2 November 2015 at 15:10

Assigned:

cilefen

» Unassigned

Comment #5

dawehner

German

commented 2 November 2015 at 15:28

Unfortunately, this doesn't work because annotations are already needed before this hook is invoked.

OH wow really? Do you mind explaining what subsystem (I bet users => entity) needs it that early?

Comment #6

cilefen commented 2 November 2015 at 15:40

Status	File	Size
new	drupal_needs_comments-2421451-6.patch	641 bytes

This one works.

Comment #7

cilefen commented 2 November 2015 at 15:48

#6 plus an explaining comment could help for such requirements that are so fundamental that we cannot invoke hook_requirements().

Comment #8

cilefen commented 5 November 2015 at 14:48

Issue summary:

View changes

The summary says:

Discuss whether this limitation is okay

I can see no way around this since annotations are a fundamental part of Drupal now, unless there is some way to parse them into something else.

Comment #9

dawehner

German

commented 6 November 2015 at 10:17

@cilefen
Do you understand why we need the annotations that early?

Comment #10

dawehner

German

commented 6 November 2015 at 10:18

And well, the question is why is this actually broken. Don't we scan those files using file_get_contents() and static code parsing, not loading those files in the first place?

Comment #11

chx commented 6 November 2015 at 10:32

Yes, we do, relevant call chain

              $class = $namespace . '\\' . $sub_path . $fileinfo->getBasename('.php');
              $parser = new StaticReflectionParser($class, $finder, TRUE);

              /** @var $annotation \Drupal\Component\Annotation\AnnotationInterface */
              if ($annotation = $reader->getClassAnnotation($parser->getReflectionClass(),

and then StaticReflectionParser::parse has the $contents = file_get_contents($fileName); indeed. So: no idea.

Also look at #3 in the stack trace, that's a comment there.

Comment #12

catch

he/him

English

commented 26 November 2015 at 12:07

Priority:

Normal

» Major

#2623576: Installation annotation exception when opcache.save_comments is set to 0. was duplicate.

Bumping to major.

Comment #13

dawehner

German

commented 26 November 2015 at 13:30

Status:

Needs review

» Needs work

The tag is a bit confusing, this is not just about performance, its about taking care whether something works ... even I still don't get why we actually need it, given that our annotation parser is as chx described earlier. Having protection for those installations would be nice.

+++ b/core/install.php
@@ -26,6 +26,12 @@
+$opcache_enabled = (function_exists('opcache_get_status') && opcache_get_status()['opcache_enabled']);
+if ($opcache_enabled && !ini_get('opcache.save_comments')) {

IMHO there is no point in the local variable but I don't care ...

Comment #14

cilefen commented 26 November 2015 at 16:56

Status:

Needs work

» Needs review

Status	File	Size
new	drupal_needs_comments-2421451-14.patch	601 bytes

Note that this install.php approach does not trigger in drush installs.

Comment #15

cilefen commented 26 November 2015 at 16:57

(magic double-post)

Comment #16

chx commented 26 November 2015 at 20:12

Unless there's a burning desire to show this warning to users in 8.0.1 I would much rather have the new Drupal-PHP crack team (neclimdul and Fabian) poke at this. I hate things we do not understand -- what if there are other bugs due to the non-understood root?

Comment #17

catch

he/him

English

commented 26 November 2015 at 20:55

I'd also rather fix it than add the warning.

We can add a note to drupal.org/requirements and/or the 8.0.0 release notes until it's fixed too.

Comment #18

cilefen commented 26 November 2015 at 21:02

It is already noted on the requirements.

Comment #19

fabianx commented 27 November 2015 at 09:28

Status:

Needs review

» Needs work

The bug is that DocParser.php (Doctrine/Common/Annotations/DocParser.php) does use Reflection just to check that there are annotations on the annotation class:

private function Annotation()

calls

  if ( ! $this->classExists($name)) { (which loads the annotation class)

        if ( ! isset(self::$annotationMetadata[$name])) {
            $this->collectAnnotationMetadata($name);
        }

        // verify that the class is really meant to be an annotation and not just any ordinary class
        if (self::$annotationMetadata[$name]['is_annotation'] === false) {

collectAnnotationMetadata in itself does do:

        $class      = new \ReflectionClass($name);
        $docComment = $class->getDocComment();

which completely circumvents the parsing before ...

Which means:

TL;DR: We load all annotation classes into opcache while parsing them - the exact thing we wanted to avoid with using doctrine. While we do not load _all_ classes, this is still more than we want.

Comment #20

neclimdul

He/Him

commented 28 November 2015 at 03:32

Gugh... For some other reasons we need to rewrite SimpleAnnotationReader where this is used so I guess that can come along for the ride. Don't have the issue handy but I'll try to dig it up.

Comment #21

chx commented 28 November 2015 at 06:23

Well, we need the annotation class anyways because that's where the contents of the annotation is going. Nonetheless, we have wanted our own reader for some time now...

But I believe there's nothing better for now than just warning users. Shame.

Comment #22

dawehner

German

commented 28 November 2015 at 08:32

Yeah at least for now we should add the warning and then maybe figure out whether we can workaround that somehow.

Comment #23

chx commented 29 November 2015 at 06:23

Status:

Needs work

» Reviewed & tested by the community

Yay, PHP renaissance!

Comment #24

alexpott

he/they

English

🇪🇺🌍

commented 7 December 2015 at 15:20

Status:

Reviewed & tested by the community

» Needs review

SimpleAnnotationReader seems to be on the way out :( https://github.com/doctrine/annotations/pull/56#issuecomment-96204292

The problem that this patch will also have is what will happen when this flag is switched on an already installed site - just reporting in install.php does not seem sufficient.

Comment #25

dawehner

German

commented 7 December 2015 at 15:24

Yeah sometimes I don't get how people define collaboration.

Comment #26

cilefen commented 7 December 2015 at 15:29

What will happen is the site will crash. ;-)

Certainly this issue as-is is not really fixing the problem. The issue is not critical because we documented the requirement for the time being. So, what is the plan?

Comment #27

catch

he/him

English

commented 7 December 2015 at 15:54

Status:

Needs review

» Reviewed & tested by the community

The problem that this patch will also have is what will happen when this flag is switched on an already installed site - just reporting in install.php does not seem sufficient.

The install will already fail with a fatal error, this just gives more information.

We'll need a follow-up to avoid using reflection for annotations altogether, which was the idea of the original Doctrine patch, but I think adding the warning is a lot better than nothing, and given we don't have an immediate fix for upstream, nothing seems likely for the next patch release or so.

Comment #28

alexpott

he/they

English

🇪🇺🌍

commented 8 December 2015 at 13:57

Status:

Reviewed & tested by the community

» Fixed

This is a good stop-gap fix. Fixing the annotation reader to not need this is going to take a lot longer and at least people will know what is wrong during installation and https://www.drupal.org/requirements/php has been updated. Committed 42cad40 and pushed to 8.0.x and 8.1.x. Thanks!

Comment #29

8 December 2015 at 13:57

alexpott committed 71a15fe on 8.1.x

Issue #2421451 by cilefen, dawehner: Drupal needs comments in opcache

Comment #30

8 December 2015 at 13:57

alexpott committed 42cad40 on

Issue #2421451 by cilefen, dawehner: Drupal needs comments in opcache

(...

Comment #31

effulgentsia commented 8 December 2015 at 17:40

Issue tags:

+Needs followup

This is a good stop-gap fix.

Do we have an issue open for the proper fix?

Comment #32

cilefen commented 8 December 2015 at 18:05

Issue tags:	-Needs followup
Related issues:		+#2631202: Doctrine no longer supports SimpleAnnotationReader, incorporate a solution into core

#2631202: Doctrine no longer supports SimpleAnnotationReader, incorporate a solution into core

Comment #33

22 December 2015 at 18:04

Status:

Fixed

» Closed (fixed)

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

Drupal needs comments in opcache

Problem/Motivation

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Comments

Related issues

Referenced by