Cached autoloader misses cause failures when missed class becomes available

Why does the call to Drupal\migrate_plus\Event\MigrateEvents not produce an error?

This suggests that the event is firing before both modules are actually installed - does mymodule depend on both the modules? And is it a separate module or just part of the install profile. Install profile dependencies are not real dependencies...

Try putting migrate_plus in the install profile before your module. In fact put all the dependencies of your module in there.

Adding information from the search api / facets issues that @mpp linked.

In #2775437: Fix the tests we had to fix some tests because views changed unpackArgumentValue, we also had a truckload of failures related to a class not found error.

fail: [PHP Fatal error] Line 17 of modules/search_api/src/Plugin/views/filter/SearchApiTerm.php:
 Class 'Drupal\taxonomy\Plugin\views\filter\TaxonomyIndexTid' not found

We fixed that by adding drupal:taxonomy and drupal:node as dependencies in our tests, but we didn't have to do this before, I can try figuring out which commit introduced this if that's helpful.

Because taxonomy was added as a dependency, facets will need to do the same thing and paragraphs also did that http://cgit.drupalcode.org/paragraphs/commit/?h=8.x-1.x&id=d31495d0ea308.... This is not a reasonable approach imho, an issue was added for search api to resolve this in a nice way: #2777483: Unmet dependencies.

We also see to have another issue with memory for search api that popped up recently, but that doesn't seem to be related: #2784849: Tests fail w/ out of memory error.

This problem has derailed #2485385: Move highwater field support to the source plugin, and do not expose its internals on MigrationInterface.

Moving out of migration system, since it's reported elsewhere.

Is #2796953: [regression] Plugins extending from classes of uninstalled modules lead to fatal error during discovery related? Is this specific to plugins?

I think this is a duplicate, yeah.

The MigrateEvents error is unrelated to that regression (or to plugins in general, for that matter).

Per phenaproxima in https://www.drupal.org/node/2485385#comment-11628073, the MigrateEvents error seems to be D7 only - are the other instances reported above also under D7?

s/D7/PHP7/, of course.

I've seen instances before in which the module installer couldn't resolve the dependency tree properly, so it installed modules in the wrong order.
Why this only appears on PHP7 I cannot say, but for debugging purposes I would checkout in core/lib/Drupal/Core/Extension/ModuleInstaller.php:191 in which order the module installation appears.

Here's an attempt to reduce the migrate highwater patch which triggered the problem to a bare minimum.

And here's a crazy idea...

Does it matter where the event subscriber class lives?

I've seen instances before in which the module installer couldn't resolve the dependency tree properly, so it installed modules in the wrong order.

The service definition, the event subscriber, and the referenced class are all in the migrate module, so it doesn't seem to be a module dependency issue.

Here's what's really odd - the event subscriber class is obviously loaded, since its getSubscribedEvents() is being called - yet the MigrateEvents class, in the same module, is not loaded.

I have a knee-jerk suspicion, based on nothing, that it has something to do with the MigrateEvents class being final. I can't prove this, of course...

I have a knee-jerk suspicion, based on nothing, that it has something to do with the MigrateEvents class being final. I can't prove this, of course...

Easy enough to test this. Seems unlikely, though, since all such event classes are final...

So, looking around at other event subscribers in core to try to figure out why only ours breaks - the namespace, the services definition, where the referenced event class lives - the closest thing is in locale. Comparing the relevant bits...

locale.services.yml:

  locale.locale_translation_cache_tag:
    class: Drupal\locale\EventSubscriber\LocaleTranslationCacheTag
    arguments: ['@cache_tags.invalidator']
    tags:
      - { name: event_subscriber }

migrate.services.yml:

  migrate.plugin_event_subscriber:
    class: Drupal\migrate\EventSubscriber\PluginEventSubscriber
    tags:
      - { name: event_subscriber }

LocaleTranslationCacheTag.php:

namespace Drupal\locale\EventSubscriber;

use Drupal\Core\Cache\CacheTagsInvalidatorInterface;
use Drupal\locale\LocaleEvents;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

class LocaleTranslationCacheTag implements EventSubscriberInterface {

  public static function getSubscribedEvents() {
    $events[LocaleEvents::SAVE_TRANSLATION][] = ['saveTranslation'];
    return $events;
  }

}

PluginEventSubscriber.php:

namespace Drupal\migrate\EventSubscriber;

use Drupal\migrate\Event\MigrateEvents;
use Drupal\migrate\Event\MigrateImportEvent;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

class PluginEventSubscriber implements EventSubscriberInterface {

  public static function getSubscribedEvents() {
    $events[MigrateEvents::PRE_IMPORT][] = ['preImport'];

    return $events;
  }

}

LocaleEvents.php:

namespace Drupal\locale;

final class LocaleEvents {

  const SAVE_TRANSLATION = 'locale.save_translation';

}

MigrateEvents.php:

namespace Drupal\migrate\Event;

final class MigrateEvents {

  const PRE_IMPORT = 'migrate.pre_import';

}

Unless I'm missing something, the only difference I can see is the name space of the events class (top-level in local, Event in migrate), so my next stab will be to move MigrateEvents up one directory...

Moving that class...

Duh - ignore that last patch, moved the file without changing the namespace...

This is my last try today.

Friday afternoon, sigh... Forgot to restore final in those last two patches, but I'll let that go for now...

Well, of course all the use statements need to be updated...

I give up... Not sure where to go from here.

Well, here's one difference between locale and migrate...

Ah well - given the failure is reported in InstallUninstallTest right after the experimental modules confirmation form, I was thinking being experimental was a trigger (and that this might relate to #2771363: BigPipe and Migrate module won't install ) - but, seems not.

alexpott has diagnosed the issue in https://www.drupal.org/node/2485385#comment-11638119:

So the problem is that on PHP7 content_translation is install before migrate and the autoloader caches the missing class in \Drupal\content_translation\ContentTranslationUpdatesManager::getSubscribedEvents(). The only way to fix it in this issue is to do a module exists check instead. Other possibilities are to change multiple module install to be batched and do one by one (slow) or the way would be to wrap the classloader in something that would make it possible to remove cached misses from the classmap.

Alex has added a simple immediate workaround to that issue - let's keep this one open to potentially follow up with one of the other possibilities.

wow

Other possibilities are to change multiple module install to be batched and do one by one (slow)

We already have #1387438: Timeout on enabling modules: make it a batch operation open for that.

A third option is an upstream patch to not cache negative class_exists().

Here's a failing test case. That will fail regardless of PHP version because of module dependencies.

Here's a fix that adds a new class loader as required.

Okay and now we need to ensure that if you are mad enough to use want to discover migration source plugins during install this works too. This is tested in \Drupal\Tests\migrate\Kernel\Plugin\MigrationPluginListTest() but I'm also working on providing some tests for the new core component.

Here's test for the new ClassFinder component.

Here's the related composer issue https://github.com/composer/composer/issues/5619

Bumping to critical on the basis that:

- this blocks a migrate critical
- there's no mitigation

Nice - run-tests.sh adds all the PHPunit tests to the autoloader... therefore we need to take a different approach on the test.

The overall fix is really nice. This removes the need to have a class loader being around just for migrate.

1. +++ b/core/lib/Drupal/Component/ClassFinder/ClassFinder.php
   @@ -0,0 +1,32 @@
   +        // \Composer\Autoload\ClassLoader::findFile() returns FALSE whilst
   +        // \Doctrine\Common\Reflection\ClassFinderInterface::findFile()
   

   Nice usage of some British english

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -768,6 +773,18 @@ public function updateModules(array $module_list, array $module_filenames = arra
   +      // the current class loader might have negative caches.
   

   Are you sure this line really makes it easy for people to understand what is going on?

3. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1383,7 +1400,10 @@ protected function getModuleNamespacesPsr4($module_file_names) {
   -  protected function classLoaderAddMultiplePsr4(array $namespaces = array()) {
   +  protected function classLoaderAddMultiplePsr4(array $namespaces = array(), $classloader = NULL) {
   

   Let's update the docs

+++ b/core/modules/system/src/Tests/Module/ClassLoaderTest.php
@@ -70,4 +70,18 @@ function testClassLoadingDisabledModules() {
+  public function testMutlipleModules() {

Tpyo.

// the current class loader might have negative caches.
Are you sure this line really makes it easy for people to understand what is going on?

Negative cache : cache :: electron : positron

Clearly.

Watch out, that cache clear might release a lot of energy.

Note I have a very simple workaround to the original bug report in #2485385-165: Move highwater field support to the source plugin, and do not expose its internals on MigrationInterface. This is a real bug, but also partly we've shot ourselves in the foot the way we use Symfony events vs. the hook system.

I don't think we should be checking if modules are installed using class_exists() checks. But I think the real issue is that the module handler needs to be available when we are building the event list but event registration occurs too early for that. However, even saying that we still need to provide multiple class loaders because after installing a module the class should be available. Here's an updated patch to address the reviews in #47 and #48.

Watch out, that cache clear might release a lot of energy.

But I think the real issue is that the module handler needs to be available when we are building the event list but event registration occurs too early for that.

Well, we have the list of enabled modules though, given that its part of the container building process.

Watch out, that cache clear might release a lot of energy.

Hehe, I think we found some secret energy source.

I'm probably not overly qualified to review this patch so I'm not going to RTBC...but these are the things I noticed...

1. +++ b/core/lib/Drupal/Component/ClassFinder/composer.json
   @@ -0,0 +1,15 @@
   +{
   +  "name": "drupal/core-class-finder",
   +  "description": "This class provides a class finding utility.",
   +  "keywords": ["drupal"],
   +  "homepage": "https://www.drupal.org/project/drupal",
   +  "license": "GPL-2.0+",
   +  "require": {
   +    "php": ">=5.5.9"
   +  },
   +  "autoload": {
   +    "psr-4": {
   +      "Drupal\\Component\\ClassFinder\\": ""
   +    }
   +  }
   +}
   

   Why doesn't this depend on Doctrine? ClassFinder explicitly implements a Doctrine interface, so it seems that this should perhaps have Doctrine as a dependency.

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1382,8 +1401,15 @@ protected function getModuleNamespacesPsr4($module_file_names) {
   +    if ($class_loader === NULL) {
   +      $class_loader = $this->classLoader;
   +    }
   

   Would prefer if this were slightly more defensive -- i.e., if (empty($class_loader)).

3. +++ b/core/modules/migrate/src/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php
   @@ -41,17 +42,10 @@ class AnnotatedClassDiscoveryAutomatedProviders extends AnnotatedClassDiscovery
   +    $this->finder = new ClassFinder();
   

   This violates dependency injection...but I suppose it's not a big deal right now.

@phenaproxima thanks for the review
1. Fixed - nice spot
2. I'm not sure this is more defensive - unfortunately there is no way to assert that we have a class loader - there is no generic interface.
3. This is not an injected dependency - the class finder is not a service - it just is - like calling \Drupal\Component\Utility\Html::cleanCssIdentifier()

I've submitted a patch merging this with the highwater patch (minus the MigrateEvents-avoiding workaround) at https://www.drupal.org/node/2485385#comment-11660651 just to verify that this patch will fix the original problem.

I'm not confident enough in my understanding of class loading to give a full RTBC - but, I can offer a "T" for the highwater patch passing with this (apart from 3 random Sqlite failures, two of which passed on a second run, third one is rerunning now).

I believe its a clean situation for the problem. Especially the detail to not change the existing classloader but add an additional one.

Yep looked at this a couple of times and no complaints from me either.

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

Follow-up: #2824868: MigrateSourcePluginManager::$classLoader property is unused

Sorry I have to reopen this issue tested on latest Drupal 8.3-dev and 8.2.2 - PHP 7

Installation profile:

name: DV
description: 'Auto-generated bundle from package DV'
type: profile
core: 8.x
dependencies:
  - migrate
themes:
  - bartik
  - seven
package: DV
distribution:
  name: DV

Error:

ResponseText: Additional uncaught exception thrown while handling exception.OriginalError: Class 'Drupal\migrate\Event\MigrateEvents' not found in Drupal\migrate\Plugin\PluginEventSubscriber::getSubscribedEvents() (line 86 of /var/www/dv/docroot/core/modules/migrate/src/Plugin/PluginEventSubscriber.php). Drupal\migrate\Plugin\PluginEventSubscriber::getSubscribedEvents() (Line: 37)
Drupal\Core\DependencyInjection\Compiler\RegisterEventSubscribersPass->process(Object) (Line: 104)
Symfony\Component\DependencyInjection\Compiler\Compiler->compile(Object) (Line: 590)
Symfony\Component\DependencyInjection\ContainerBuilder->compile() (Line: 1274)
Drupal\Core\DrupalKernel->compileContainer() (Line: 873)
Drupal\Core\DrupalKernel->initializeContainer() (Line: 18)
Drupal\Core\Installer\InstallerKernel->initializeContainer() (Line: 465)
Drupal\Core\DrupalKernel->boot() (Line: 412)
install_begin_request(Object, Array) (Line: 112)
install_drupal(Object) (Line: 44)
AdditionalSymfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "theme.manager". in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 816 of /var/www/dv/vendor/symfony/dependency-injection/ContainerBuilder.php). Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition('theme.manager') (Line: 456)
Symfony\Component\DependencyInjection\ContainerBuilder->get('theme.manager') (Line: 665)
Drupal::theme() (Line: 22)
_drupal_maintenance_theme() (Line: 709)
drupal_maintenance_theme() (Line: 965)
install_display_output(Array, Array, Array) (Line: 264)
_drupal_log_error(Array, 1) (Line: 569)
_drupal_exception_handler(Object)