Verified Commit e8aec68a authored by Lee Rowlands's avatar Lee Rowlands
Browse files

Issue #3458177 by mondrake, catch, godotislate, longwave, xjm: Changing...

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

(cherry picked from commit 7a331d50)
parent d0da23b5
Loading
Loading
Loading
Loading
Loading
+24 −10
Original line number Diff line number Diff line
@@ -80,19 +80,32 @@ public function getDefinitions() {
              $sub_path = $iterator->getSubIterator()->getSubPath();
              $sub_path = $sub_path ? str_replace(DIRECTORY_SEPARATOR, '\\', $sub_path) . '\\' : '';
              $class = $namespace . '\\' . $sub_path . $fileinfo->getBasename('.php');

              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 the file is not parsed again.
                  // Store a NULL object, so that 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 @@ public function getDefinitions() {
   *   'content' is the plugin definition.
   *
   * @throws \ReflectionException
   * @throws \Error
   */
  protected function parseClass(string $class, \SplFileInfo $fileinfo): array {
    // @todo Consider performance improvements over using reflection.
+2 −1
Original line number Diff line number Diff line
@@ -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.
+20 −2
Original line number Diff line number Diff line
@@ -4,6 +4,7 @@

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 @@ protected function setUp(): void {

    // 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 @@ public function testGetDefinitions(): void {
    $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 @@ public function testGetDefinitions(): void {
      ],
    ], $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, [
+6 −1
Original line number Diff line number Diff line
@@ -4,6 +4,7 @@

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 @@ protected function setUp(): void {

    // 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);
  }

  /**
+16 −0
Original line number Diff line number Diff line
<?php

declare(strict_types=1);

namespace com\example\PluginNamespace;

use Drupal\a_module_that_does_not_exist\Plugin\Custom;

/**
 * Provides a custom test plugin that extends from a missing dependency.
 */
#[CustomPlugin(
  id: "discovery_test_2",
  title: "Discovery test plugin 2"
)]
class AttributeDiscoveryTest2 extends Custom {}