ComponentPluginManager should detect if an extension provides two different components that resolve to the same plugin ID

Created on 19 August 2024, 8 months ago

Problem/Motivation

  1. I write a module.
  2. I add components/image.component.yml
  3. Months later, I add many components, and I add component/simple/image.component.yml
  4. On some environments, this crashes, on others it does not.

Steps to reproduce

See above.

Proposed resolution

Detect the duplicate plugin ID within an extension, and throw a helpful exception.

Remaining tasks

  • Explicit test coverage. (I suggest following the pattern that \Drupal\Tests\ckeditor5\Kernel\CKEditor5PluginManagerTest::testInvalidPluginDefinitions() established.)
  • Fix.

User interface changes

None.

Introduced terminology

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

๐Ÿ“Œ Task
Status

Active

Version

11.0 ๐Ÿ”ฅ

Component
single-directory componentsย  โ†’

Last updated about 8 hours ago

Created by

๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @wim leers
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand quietone
  • First commit to issue fork.
  • ๐Ÿ‡ฎ๐Ÿ‡ฉIndonesia el7cosmos ๐Ÿ‡ฎ๐Ÿ‡ฉ GMT+7

    I tried adding duplicate components in test, but canโ€™t reproduce the crash.
    Q: do we have any plugin manager/discovery that throws exception when there are duplicate plugins?

  • ๐Ÿ‡จ๐Ÿ‡ทCosta Rica alemadlei
  • ๐Ÿ‡จ๐Ÿ‡ทCosta Rica alemadlei

    For mentored contribution at DrupalCon Atlanta, the SDC group is taking a look at this issue.

    So far one of the team members has verified that this is not crashing on the latest Drupal 11 stable version. We did find a couple of things that are odd or unexpected, but once all team members validate, we will create separate issue tickets.

    Since this was reported in August of 2024 we are going to test with Drupal 10.3 and verify it crashed there and was addressed by a later release.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mcolebank

    We created duplicate components with the same name ('image') in a custom module and in a custom theme on Drupal 11.

    In a Test Module we created and tested the components Image/image.component.yml and Example/image/image.component.yml which had the same yml files. When the component was included in the theme, Image/image.component.yml rendered and the Example/image component was ignored.

    In a Test Theme we created and tested the components Image/image.component.yml and Example/image/image.component.yml which had the same yml files. When the component was included in the theme, Example/image/image.component.yml rendered and the Image/image component was ignored.

    The duplicate components with the same name did not crash Drupal. We also tested in Drupal 10.3 with the same results. It is unexpected behavior that a different version of the duplicated component is rended if the duplicate components are in a theme or in a module.

    Recommend that we close this issue and create a new issue where SDC shows a warning message when two or more components have the same name.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mcolebank

    We started to implement a warning and logger if a duplicate component was found. Following Drupal patterns we wanted to use dependency injection using ContainerInjectionInterface instead of using Drupal services but DirectoryWithMetadataDiscovery.php is calling YamlDirectoryDiscovery with only 2 arguments. This threw an error because we were increasing the arguments.

    In the `web/core/lib/Drupal/Component/Discovery/YamlDirectoryDiscovery.php` we were trying to add this code:

         if ($files) {
          // Keep track of duplicates.
          $duplicates = [];
          foreach ($files as $file => $provider) {
            // If a file is empty or its contents are commented out, return an empty
            // array instead of NULL for type consistency.
            try {
              $data = Yaml::decode(file_get_contents($file)) ?: [];
            }
            catch (InvalidDataTypeException $e) {
              throw new DiscoveryException("The $file contains invalid YAML", 0, $e);
            }
            $data[static::FILE_KEY] = $file;
            $componentid = $this->getIdentifier($file, $data);
    
            // Check for duplicates, warn user and log if found.
            if (isset($duplicates[$componentid])) {
               \Drupal::messenger()->addWarning('Duplicate component found: ' . $componentid);
               \Drupal::logger('YamlDirectoryDiscovery')->warning('Duplicate component found: ' . $componentid);
            }
            $duplicates[$componentid] = $componentid;
    
            $all[$provider][$componentid] = $data;
            $file_cache->set($file, $data);
          }
        }
        return $all;
      }
    
  • Pipeline finished with Failed
    8 days ago
    Total: 146s
    #459070
  • ๐Ÿ‡จ๐Ÿ‡ทCosta Rica estebanvalerio.h

    Worked with @mcolebank debugging and updating code including dependency injection to avoid calls like `\Drupal::messenger()`, thanks @ultimike , @markie and @rfay for their guidance
    Also thanks to https://gregrusson.com/ and @tylerhastain for debugging this issue.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mradcliffe USA

    I am marking this as Needs work because we still need to add explicit test coverage based on the issue summary and we probably want to make the messenger string a translatable markup string with a placeholder.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mradcliffe USA
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mcolebank
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mcolebank

    We have added translation. Needs review and testing.

  • Pipeline finished with Failed
    8 days ago
    Total: 138s
    #459130
Production build 0.71.5 2024