From 82304ed90fe2c5dc874da0d5641bf865ace5f537 Mon Sep 17 00:00:00 2001 From: Nathaniel Catchpole Date: Tue, 27 Sep 2016 14:35:22 +0100 Subject: [PATCH] Issue #2776235 by mikeryan, alexpott: Cached autoloader misses cause failures when missed class becomes available --- .../Component/ClassFinder/ClassFinder.php | 32 ++ .../Drupal/Component/ClassFinder/LICENSE.txt | 339 ++++++++++++++++++ .../Drupal/Component/ClassFinder/README.txt | 12 + .../Drupal/Component/ClassFinder/TESTING.txt | 18 + .../Component/ClassFinder/composer.json | 16 + core/lib/Drupal/Core/DrupalKernel.php | 30 +- core/modules/migrate/migrate.services.yml | 2 +- ...otatedClassDiscoveryAutomatedProviders.php | 16 +- .../src/Plugin/MigrateSourcePluginManager.php | 7 +- .../src/Tests/Module/ClassLoaderTest.php | 14 + ...module_install_class_loader_test1.info.yml | 6 + ...le_install_class_loader_test1.services.yml | 5 + .../src/EventSubscriber.php | 29 ++ ...module_install_class_loader_test2.info.yml | 8 + ...le_install_class_loader_test2.services.yml | 5 + .../src/EventSubscriber.php | 19 + .../Component/ClassFinder/ClassFinderTest.php | 39 ++ 17 files changed, 578 insertions(+), 19 deletions(-) create mode 100644 core/lib/Drupal/Component/ClassFinder/ClassFinder.php create mode 100644 core/lib/Drupal/Component/ClassFinder/LICENSE.txt create mode 100644 core/lib/Drupal/Component/ClassFinder/README.txt create mode 100644 core/lib/Drupal/Component/ClassFinder/TESTING.txt create mode 100644 core/lib/Drupal/Component/ClassFinder/composer.json create mode 100644 core/modules/system/tests/modules/module_install_class_loader_test1/module_install_class_loader_test1.info.yml create mode 100644 core/modules/system/tests/modules/module_install_class_loader_test1/module_install_class_loader_test1.services.yml create mode 100644 core/modules/system/tests/modules/module_install_class_loader_test1/src/EventSubscriber.php create mode 100644 core/modules/system/tests/modules/module_install_class_loader_test2/module_install_class_loader_test2.info.yml create mode 100644 core/modules/system/tests/modules/module_install_class_loader_test2/module_install_class_loader_test2.services.yml create mode 100644 core/modules/system/tests/modules/module_install_class_loader_test2/src/EventSubscriber.php create mode 100644 core/tests/Drupal/Tests/Component/ClassFinder/ClassFinderTest.php diff --git a/core/lib/Drupal/Component/ClassFinder/ClassFinder.php b/core/lib/Drupal/Component/ClassFinder/ClassFinder.php new file mode 100644 index 00000000000..346f7f66cd8 --- /dev/null +++ b/core/lib/Drupal/Component/ClassFinder/ClassFinder.php @@ -0,0 +1,32 @@ + + Copyright (C) + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 2 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License along + with this program; if not, write to the Free Software Foundation, Inc., + 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +Also add information on how to contact you by electronic and paper mail. + +If the program is interactive, make it output a short notice like this +when it starts in an interactive mode: + + Gnomovision version 69, Copyright (C) year name of author + Gnomovision comes with ABSOLUTELY NO WARRANTY; for details type `show w'. + This is free software, and you are welcome to redistribute it + under certain conditions; type `show c' for details. + +The hypothetical commands `show w' and `show c' should show the appropriate +parts of the General Public License. Of course, the commands you use may +be called something other than `show w' and `show c'; they could even be +mouse-clicks or menu items--whatever suits your program. + +You should also get your employer (if you work as a programmer) or your +school, if any, to sign a "copyright disclaimer" for the program, if +necessary. Here is a sample; alter the names: + + Yoyodyne, Inc., hereby disclaims all copyright interest in the program + `Gnomovision' (which makes passes at compilers) written by James Hacker. + + , 1 April 1989 + Ty Coon, President of Vice + +This General Public License does not permit incorporating your program into +proprietary programs. If your program is a subroutine library, you may +consider it more useful to permit linking proprietary applications with the +library. If this is what you want to do, use the GNU Lesser General +Public License instead of this License. diff --git a/core/lib/Drupal/Component/ClassFinder/README.txt b/core/lib/Drupal/Component/ClassFinder/README.txt new file mode 100644 index 00000000000..425e97b5a51 --- /dev/null +++ b/core/lib/Drupal/Component/ClassFinder/README.txt @@ -0,0 +1,12 @@ +The Drupal ClassFinder Component + +Thanks for using this Drupal component. + +You can participate in its development on Drupal.org, through our issue system: +https://www.drupal.org/project/issues/drupal + +You can get the full Drupal repo here: +https://www.drupal.org/project/drupal/git-instructions + +You can browse the full Drupal repo here: +http://cgit.drupalcode.org/drupal diff --git a/core/lib/Drupal/Component/ClassFinder/TESTING.txt b/core/lib/Drupal/Component/ClassFinder/TESTING.txt new file mode 100644 index 00000000000..c7e97fc955d --- /dev/null +++ b/core/lib/Drupal/Component/ClassFinder/TESTING.txt @@ -0,0 +1,18 @@ +HOW-TO: Test this Drupal component + +In order to test this component, you'll need to get the entire Drupal repo and +run the tests there. + +You'll find the tests under core/tests/Drupal/Tests/Component. + +You can get the full Drupal repo here: +https://www.drupal.org/project/drupal/git-instructions + +You can find more information about running PHPUnit tests with Drupal here: +https://www.drupal.org/node/2116263 + +Each component in the Drupal\Component namespace has its own annotated test +group. You can use this group to run only the tests for this component. Like +this: + +$ ./vendor/bin/phpunit -c core --group ClassFinder diff --git a/core/lib/Drupal/Component/ClassFinder/composer.json b/core/lib/Drupal/Component/ClassFinder/composer.json new file mode 100644 index 00000000000..97d36687755 --- /dev/null +++ b/core/lib/Drupal/Component/ClassFinder/composer.json @@ -0,0 +1,16 @@ +{ + "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", + "doctrine/common": "2.5.*" + }, + "autoload": { + "psr-4": { + "Drupal\\Component\\ClassFinder\\": "" + } + } +} diff --git a/core/lib/Drupal/Core/DrupalKernel.php b/core/lib/Drupal/Core/DrupalKernel.php index ab2f645c87d..ba2c0fbd237 100644 --- a/core/lib/Drupal/Core/DrupalKernel.php +++ b/core/lib/Drupal/Core/DrupalKernel.php @@ -2,6 +2,7 @@ namespace Drupal\Core; +use Composer\Autoload\ClassLoader; use Drupal\Component\Assertion\Handle; use Drupal\Component\FileCache\FileCacheFactory; use Drupal\Component\Utility\Unicode; @@ -761,6 +762,10 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface { * needed. */ public function updateModules(array $module_list, array $module_filenames = array()) { + $pre_existing_module_namespaces = []; + if ($this->booted && is_array($this->moduleList)) { + $pre_existing_module_namespaces = $this->getModuleNamespacesPsr4($this->getModuleFileNames()); + } $this->moduleList = $module_list; foreach ($module_filenames as $name => $extension) { $this->moduleData[$name] = $extension; @@ -773,6 +778,20 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface { // container in order to refresh the serviceProvider list and container. $this->containerNeedsRebuild = TRUE; if ($this->booted) { + // We need to register any new namespaces to a new class loader because + // the current class loader might have stored a negative result for a + // class that is now available. + // @see \Composer\Autoload\ClassLoader::findFile() + $new_namespaces = array_diff_key( + $this->getModuleNamespacesPsr4($this->getModuleFileNames()), + $pre_existing_module_namespaces + ); + if (!empty($new_namespaces)) { + $additional_class_loader = new ClassLoader(); + $this->classLoaderAddMultiplePsr4($new_namespaces, $additional_class_loader); + $additional_class_loader->register(); + } + $this->initializeContainer(); } } @@ -1387,8 +1406,15 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface { * Array where each key is a namespace like 'Drupal\system', and each value * is either a PSR-4 base directory, or an array of PSR-4 base directories * associated with this namespace. + * @param object $class_loader + * The class loader. Normally \Composer\Autoload\ClassLoader, as included by + * the front controller, but may also be decorated; e.g., + * \Symfony\Component\ClassLoader\ApcClassLoader. */ - protected function classLoaderAddMultiplePsr4(array $namespaces = array()) { + protected function classLoaderAddMultiplePsr4(array $namespaces = array(), $class_loader = NULL) { + if ($class_loader === NULL) { + $class_loader = $this->classLoader; + } foreach ($namespaces as $prefix => $paths) { if (is_array($paths)) { foreach ($paths as $key => $value) { @@ -1398,7 +1424,7 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface { elseif (is_string($paths)) { $paths = $this->root . '/' . $paths; } - $this->classLoader->addPsr4($prefix . '\\', $paths); + $class_loader->addPsr4($prefix . '\\', $paths); } } diff --git a/core/modules/migrate/migrate.services.yml b/core/modules/migrate/migrate.services.yml index c9a64570199..efbaedd221b 100644 --- a/core/modules/migrate/migrate.services.yml +++ b/core/modules/migrate/migrate.services.yml @@ -7,7 +7,7 @@ services: arguments: [migrate] plugin.manager.migrate.source: class: Drupal\migrate\Plugin\MigrateSourcePluginManager - arguments: [source, '@container.namespaces', '@cache.discovery', '@module_handler', '@class_loader'] + arguments: [source, '@container.namespaces', '@cache.discovery', '@module_handler'] plugin.manager.migrate.process: class: Drupal\migrate\Plugin\MigratePluginManager arguments: [process, '@container.namespaces', '@cache.discovery', '@module_handler', 'Drupal\migrate\Annotation\MigrateProcessPlugin'] diff --git a/core/modules/migrate/src/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php b/core/modules/migrate/src/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php index aba60b19e38..c102bb890ef 100644 --- a/core/modules/migrate/src/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php +++ b/core/modules/migrate/src/Plugin/Discovery/AnnotatedClassDiscoveryAutomatedProviders.php @@ -6,6 +6,7 @@ use Doctrine\Common\Annotations\AnnotationRegistry; use Doctrine\Common\Reflection\StaticReflectionParser as BaseStaticReflectionParser; use Drupal\Component\Annotation\AnnotationInterface; use Drupal\Component\Annotation\Reflection\MockFileFinder; +use Drupal\Component\ClassFinder\ClassFinder; use Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery; use Drupal\migrate\Annotation\MultipleProviderAnnotationInterface; @@ -20,9 +21,9 @@ use Drupal\migrate\Annotation\MultipleProviderAnnotationInterface; class AnnotatedClassDiscoveryAutomatedProviders extends AnnotatedClassDiscovery { /** - * Any class loader with a findFile() method. + * A utility object that can use active autoloaders to find files for classes. * - * @var \Composer\Autoload\ClassLoader + * @var \Doctrine\Common\Reflection\ClassFinderInterface */ protected $finder; @@ -41,17 +42,10 @@ class AnnotatedClassDiscoveryAutomatedProviders extends AnnotatedClassDiscovery * Defaults to 'Drupal\Component\Annotation\Plugin'. * @param string[] $annotation_namespaces * Additional namespaces to scan for annotation definitions. - * @param $class_loader - * The class loader already knows where to find the classes so it is reused - * as the class finder. */ - public function __construct($subdir, \Traversable $root_namespaces, $plugin_definition_annotation_name = 'Drupal\Component\Annotation\Plugin', array $annotation_namespaces = [], $class_loader) { + public function __construct($subdir, \Traversable $root_namespaces, $plugin_definition_annotation_name = 'Drupal\Component\Annotation\Plugin', array $annotation_namespaces = []) { parent::__construct($subdir, $root_namespaces, $plugin_definition_annotation_name, $annotation_namespaces); - - if (!method_exists($class_loader, 'findFile')) { - throw new \LogicException(sprintf('Class loader (%s) must implement findFile() method', get_class($class_loader))); - } - $this->finder = $class_loader; + $this->finder = new ClassFinder(); } diff --git a/core/modules/migrate/src/Plugin/MigrateSourcePluginManager.php b/core/modules/migrate/src/Plugin/MigrateSourcePluginManager.php index 34ac0c54b53..1aaf4f180e3 100644 --- a/core/modules/migrate/src/Plugin/MigrateSourcePluginManager.php +++ b/core/modules/migrate/src/Plugin/MigrateSourcePluginManager.php @@ -40,12 +40,9 @@ class MigrateSourcePluginManager extends MigratePluginManager { * Cache backend instance to use. * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler * The module handler to invoke the alter hook with. - * @param object $class_loader - * The class loader. */ - public function __construct($type, \Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler, $class_loader) { + public function __construct($type, \Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler) { parent::__construct($type, $namespaces, $cache_backend, $module_handler, 'Drupal\migrate\Annotation\MigrateSource'); - $this->classLoader = $class_loader; } /** @@ -53,7 +50,7 @@ class MigrateSourcePluginManager extends MigratePluginManager { */ protected function getDiscovery() { if (!$this->discovery) { - $discovery = new AnnotatedClassDiscoveryAutomatedProviders($this->subdir, $this->namespaces, $this->pluginDefinitionAnnotationName, $this->additionalAnnotationNamespaces, $this->classLoader); + $discovery = new AnnotatedClassDiscoveryAutomatedProviders($this->subdir, $this->namespaces, $this->pluginDefinitionAnnotationName, $this->additionalAnnotationNamespaces); $this->discovery = new ContainerDerivativeDiscoveryDecorator($discovery); } return $this->discovery; diff --git a/core/modules/system/src/Tests/Module/ClassLoaderTest.php b/core/modules/system/src/Tests/Module/ClassLoaderTest.php index 1e7a0a54f76..bc2dfd5fc72 100644 --- a/core/modules/system/src/Tests/Module/ClassLoaderTest.php +++ b/core/modules/system/src/Tests/Module/ClassLoaderTest.php @@ -70,4 +70,18 @@ class ClassLoaderTest extends WebTestBase { } } + /** + * Ensures the negative caches in the class loader don't result in crashes. + */ + public function testMultipleModules() { + $this->drupalLogin($this->rootUser); + $edit = [ + "modules[Testing][module_install_class_loader_test1][enable]" => TRUE, + "modules[Testing][module_install_class_loader_test2][enable]" => TRUE, + ]; + $this->drupalPostForm('admin/modules', $edit, t('Install')); + $this->rebuildContainer(); + $this->assertTrue(\Drupal::moduleHandler()->moduleExists('module_install_class_loader_test2'), 'The module_install_class_loader_test2 module has been installed.'); + } + } diff --git a/core/modules/system/tests/modules/module_install_class_loader_test1/module_install_class_loader_test1.info.yml b/core/modules/system/tests/modules/module_install_class_loader_test1/module_install_class_loader_test1.info.yml new file mode 100644 index 00000000000..83c54d91e4b --- /dev/null +++ b/core/modules/system/tests/modules/module_install_class_loader_test1/module_install_class_loader_test1.info.yml @@ -0,0 +1,6 @@ +name: 'Module install class loader test1' +description: 'Support module for tests that the class loader behaves as expected during module install.' +type: module +package: Testing +core: 8.x +version: VERSION diff --git a/core/modules/system/tests/modules/module_install_class_loader_test1/module_install_class_loader_test1.services.yml b/core/modules/system/tests/modules/module_install_class_loader_test1/module_install_class_loader_test1.services.yml new file mode 100644 index 00000000000..71b5d585ba7 --- /dev/null +++ b/core/modules/system/tests/modules/module_install_class_loader_test1/module_install_class_loader_test1.services.yml @@ -0,0 +1,5 @@ +services: + module_install_class_loader_test1.event_subscriber: + class: Drupal\module_install_class_loader_test1\EventSubscriber + tags: + - { name: event_subscriber } diff --git a/core/modules/system/tests/modules/module_install_class_loader_test1/src/EventSubscriber.php b/core/modules/system/tests/modules/module_install_class_loader_test1/src/EventSubscriber.php new file mode 100644 index 00000000000..ce0ef4981ae --- /dev/null +++ b/core/modules/system/tests/modules/module_install_class_loader_test1/src/EventSubscriber.php @@ -0,0 +1,29 @@ +assertStringEndsWith('core/tests/Drupal/Tests/UnitTestCase.php', $finder->findFile(UnitTestCase::class)); + $class = 'Not\\A\\Class'; + $this->assertNull($finder->findFile($class)); + + // Register an autoloader that can find this class. + $loader = new ClassLoader(); + $loader->addClassMap([$class => __FILE__]); + $loader->register(); + $this->assertEquals(__FILE__, $finder->findFile($class)); + // This shouldn't prevent us from finding the original file. + $this->assertStringEndsWith('core/tests/Drupal/Tests/UnitTestCase.php', $finder->findFile(UnitTestCase::class)); + + // Clean up the additional autoloader after the test. + $loader->unregister(); + } + +}