From 7a331d5095aaacb40cd6e58679f642cb0afe3504 Mon Sep 17 00:00:00 2001 From: Lee Rowlands Date: Fri, 2 Aug 2024 09:08:11 +1000 Subject: [PATCH] Issue #3458177 by mondrake, catch, godotislate, longwave, xjm: Changing plugins from annotations to attributes in contrib leads to error if plugin extends from a missing dependency --- .../Discovery/AttributeClassDiscovery.php | 34 +++++++++++++------ core/phpstan.neon.dist | 3 +- .../AttributeClassDiscoveryCachedTest.php | 22 ++++++++++-- .../Attribute/AttributeClassDiscoveryTest.php | 7 +++- .../AttributeDiscoveryTest2.php | 16 +++++++++ 5 files changed, 68 insertions(+), 14 deletions(-) create mode 100644 core/tests/Drupal/Tests/Component/Plugin/Attribute/Fixtures/Plugins/PluginNamespace/AttributeDiscoveryTest2.php diff --git a/core/lib/Drupal/Component/Plugin/Discovery/AttributeClassDiscovery.php b/core/lib/Drupal/Component/Plugin/Discovery/AttributeClassDiscovery.php index 3477b746cc8..14c25f2f3f3 100644 --- a/core/lib/Drupal/Component/Plugin/Discovery/AttributeClassDiscovery.php +++ b/core/lib/Drupal/Component/Plugin/Discovery/AttributeClassDiscovery.php @@ -80,17 +80,30 @@ class AttributeClassDiscovery implements DiscoveryInterface { $sub_path = $iterator->getSubIterator()->getSubPath(); $sub_path = $sub_path ? str_replace(DIRECTORY_SEPARATOR, '\\', $sub_path) . '\\' : ''; $class = $namespace . '\\' . $sub_path . $fileinfo->getBasename('.php'); - - ['id' => $id, 'content' => $content] = $this->parseClass($class, $fileinfo); - - if ($id) { - $definitions[$id] = $content; - // Explicitly serialize this to create a new object instance. - $this->fileCache->set($fileinfo->getPathName(), ['id' => $id, 'content' => serialize($content)]); + try { + ['id' => $id, 'content' => $content] = $this->parseClass($class, $fileinfo); + if ($id) { + $definitions[$id] = $content; + // Explicitly serialize this to create a new object instance. + $this->fileCache->set($fileinfo->getPathName(), ['id' => $id, 'content' => serialize($content)]); + } + else { + // Store a NULL object, so that the file is not parsed again. + $this->fileCache->set($fileinfo->getPathName(), [NULL]); + } } - else { - // Store a NULL object, so the file is not parsed again. - $this->fileCache->set($fileinfo->getPathName(), [NULL]); + // Plugins may rely on Attribute classes defined by modules that + // are not installed. In such a case, a 'class not found' error + // may be thrown from reflection. However, this is an unavoidable + // situation with optional dependencies and plugins. Therefore, + // silently skip over this class and avoid writing to the cache, + // so that it is scanned each time. This ensures that the plugin + // definition will be found if the module it requires is + // enabled. + catch (\Error $e) { + if (!preg_match('/(Class|Interface) .* not found$/', $e->getMessage())) { + throw $e; + } } } } @@ -118,6 +131,7 @@ class AttributeClassDiscovery implements DiscoveryInterface { * 'content' is the plugin definition. * * @throws \ReflectionException + * @throws \Error */ protected function parseClass(string $class, \SplFileInfo $fileinfo): array { // @todo Consider performance improvements over using reflection. diff --git a/core/phpstan.neon.dist b/core/phpstan.neon.dist index f796fff520e..f7e1f0e2932 100644 --- a/core/phpstan.neon.dist +++ b/core/phpstan.neon.dist @@ -37,10 +37,11 @@ parameters: - scripts/generate-d?-content.sh # Skip data files. - lib/Drupal/Component/Transliteration/data/*.php - # Below extends on purpose a non existing class for testing. + # The following classes deliberately extend non-existent classes for testing. - modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/fruit/ExtendingNonInstalledClass.php - modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/custom_annotation/UsingNonInstalledTraitClass.php - modules/system/tests/modules/plugin_test/src/Plugin/plugin_test/custom_annotation/ExtendingNonInstalledClass.php + - tests/Drupal/Tests/Component/Plugin/Attribute/Fixtures/Plugins/PluginNamespace/AttributeDiscoveryTest2.php ignoreErrors: # new static() is a best practice in Drupal, so we cannot fix that. diff --git a/core/tests/Drupal/Tests/Component/Plugin/Attribute/AttributeClassDiscoveryCachedTest.php b/core/tests/Drupal/Tests/Component/Plugin/Attribute/AttributeClassDiscoveryCachedTest.php index 566058d7c88..2b2f0c25abd 100644 --- a/core/tests/Drupal/Tests/Component/Plugin/Attribute/AttributeClassDiscoveryCachedTest.php +++ b/core/tests/Drupal/Tests/Component/Plugin/Attribute/AttributeClassDiscoveryCachedTest.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Drupal\Tests\Component\Plugin\Attribute; +use Composer\Autoload\ClassLoader; use Drupal\Component\Plugin\Discovery\AttributeClassDiscovery; use Drupal\Component\FileCache\FileCacheFactory; use PHPUnit\Framework\TestCase; @@ -20,6 +21,7 @@ class AttributeClassDiscoveryCachedTest extends TestCase { */ protected function setUp(): void { parent::setUp(); + // Ensure FileCacheFactory::DISABLE_CACHE is *not* set, since we're testing // integration with the file cache. FileCacheFactory::setConfiguration([]); @@ -28,7 +30,10 @@ class AttributeClassDiscoveryCachedTest extends TestCase { // Normally the attribute classes would be autoloaded. include_once __DIR__ . '/Fixtures/CustomPlugin.php'; - include_once __DIR__ . '/Fixtures/Plugins/PluginNamespace/AttributeDiscoveryTest1.php'; + + $additionalClassLoader = new ClassLoader(); + $additionalClassLoader->addPsr4("com\\example\\PluginNamespace\\", __DIR__ . "/Fixtures/Plugins/PluginNamespace"); + $additionalClassLoader->register(TRUE); } /** @@ -41,6 +46,8 @@ class AttributeClassDiscoveryCachedTest extends TestCase { $discovery_path = __DIR__ . '/Fixtures/Plugins'; // File path that should be discovered within that directory. $file_path = $discovery_path . '/PluginNamespace/AttributeDiscoveryTest1.php'; + // Define a file path within the directory that should not be discovered. + $non_discoverable_file_path = $discovery_path . '/PluginNamespace/AttributeDiscoveryTest2.php'; $discovery = new AttributeClassDiscovery(['com\example' => [$discovery_path]]); $this->assertEquals([ @@ -50,11 +57,22 @@ class AttributeClassDiscoveryCachedTest extends TestCase { ], ], $discovery->getDefinitions()); - // Gain access to the file cache so we can change it. + // Gain access to the file cache. $ref_file_cache = new \ReflectionProperty($discovery, 'fileCache'); $ref_file_cache->setAccessible(TRUE); /** @var \Drupal\Component\FileCache\FileCacheInterface $file_cache */ $file_cache = $ref_file_cache->getValue($discovery); + + // The valid plugin definition should be cached. + $this->assertEquals([ + 'id' => 'discovery_test_1', + 'class' => 'com\example\PluginNamespace\AttributeDiscoveryTest1', + ], unserialize($file_cache->get($file_path)['content'])); + + // The plugin that extends a missing class should not be cached. + $this->assertNull($file_cache->get($non_discoverable_file_path)); + + // Change the file cache entry. // The file cache is keyed by the file path, and we'll add some known // content to test against. $file_cache->set($file_path, [ diff --git a/core/tests/Drupal/Tests/Component/Plugin/Attribute/AttributeClassDiscoveryTest.php b/core/tests/Drupal/Tests/Component/Plugin/Attribute/AttributeClassDiscoveryTest.php index f6c6370e7e1..727405f4b90 100644 --- a/core/tests/Drupal/Tests/Component/Plugin/Attribute/AttributeClassDiscoveryTest.php +++ b/core/tests/Drupal/Tests/Component/Plugin/Attribute/AttributeClassDiscoveryTest.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Drupal\Tests\Component\Plugin\Attribute; +use Composer\Autoload\ClassLoader; use Drupal\Component\Plugin\Discovery\AttributeClassDiscovery; use Drupal\Component\FileCache\FileCacheFactory; use PHPUnit\Framework\TestCase; @@ -22,6 +23,7 @@ class AttributeClassDiscoveryTest extends TestCase { */ protected function setUp(): void { parent::setUp(); + // Ensure the file cache is disabled. FileCacheFactory::setConfiguration([FileCacheFactory::DISABLE_CACHE => TRUE]); // Ensure that FileCacheFactory has a prefix. @@ -29,7 +31,10 @@ class AttributeClassDiscoveryTest extends TestCase { // Normally the attribute classes would be autoloaded. include_once __DIR__ . '/Fixtures/CustomPlugin.php'; - include_once __DIR__ . '/Fixtures/Plugins/PluginNamespace/AttributeDiscoveryTest1.php'; + + $additionalClassLoader = new ClassLoader(); + $additionalClassLoader->addPsr4("com\\example\\PluginNamespace\\", __DIR__ . "/Fixtures/Plugins/PluginNamespace"); + $additionalClassLoader->register(TRUE); } /** diff --git a/core/tests/Drupal/Tests/Component/Plugin/Attribute/Fixtures/Plugins/PluginNamespace/AttributeDiscoveryTest2.php b/core/tests/Drupal/Tests/Component/Plugin/Attribute/Fixtures/Plugins/PluginNamespace/AttributeDiscoveryTest2.php new file mode 100644 index 00000000000..d094b3b9793 --- /dev/null +++ b/core/tests/Drupal/Tests/Component/Plugin/Attribute/Fixtures/Plugins/PluginNamespace/AttributeDiscoveryTest2.php @@ -0,0 +1,16 @@ +