Contrib.social

Calling chdir() causes plugin discovery to fail

Open on Drupal.org →
Created on 18 April 2024, 12 months ago
Updated 31 May 2024, 10 months ago

Problem/Motivation

Discovered when investigating an occasional production issue when SimpleSAMLphp was in use; SimpleSAMLphp bootstraps Drupal separately and then calls chdir() to swap back to its own root directory. In a cold/stale plugin cache situation, the cache will then be rebuilt as empty, resulting in errors that should not be able to happen, such as:

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "entity" plugin does not exist. Valid plugin IDs for Drupal\Core\TypedData\TypedDataManager are: in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 53 of /app/web/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php)

"entity" is a core plugin so discovery should always succeed! But if chdir() is called and then plugin discovery is triggered, discovery fails to find any plugins at all.

This is because AnnotatedClassDiscovery::getPluginNamespaces() uses relative paths, even though the comment mentions DRUPAL_ROOT:

        foreach ((array) $dirs as $dir) {
          // Append the directory suffix to the PSR-4 base directory, to obtain
          // the directory where plugins are found. For example,
          // DRUPAL_ROOT . '/core/modules/views/src' may become
          // DRUPAL_ROOT . '/core/modules/views/src/Plugin/Block'.
          $plugin_namespaces[$namespace][] = $dir . $this->directorySuffix;
        }

$dir is relative in core, although many tests pass absolute paths in here.

Steps to reproduce

Bootstrap Drupal, call chdir(), rebuild a plugin cache.

Proposed resolution

Prepend DRUPAL_ROOT to all discovered directories.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Plugin  →

Last updated 6 days ago

Created by

🇬🇧United Kingdom longwave UK

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @longwave
  • Merge request !7583Resolve #3441833 "Calling chdir causes" → (Open) created by longwave
  • Status changed to Needs review 12 months ago
  • Comment 12 months ago →
    🇬🇧United Kingdom longwave UK
  • Comment 12 months ago →
    🇬🇧United Kingdom longwave UK
  • Comment 12 months ago →
    🇬🇧United Kingdom longwave UK

    We could assume that all paths are relative and always prepend DRUPAL_ROOT here, but I guess there might be code that is doing something different here and passing in already-absolute paths, so better to check each case.

  • Pipeline finished with Failed
    12 months ago
    Total: 957s
    #150210
  • Comment 12 months ago →
    🇬🇧United Kingdom alexpott 🇪🇺🌍

    What about attribute discovery?

  • Pipeline finished with Canceled
    12 months ago
    Total: 297s
    #150300
  • Comment 12 months ago →
    🇬🇧United Kingdom longwave UK

    Fixed some tests that use vfs:// paths by additionally allowing URIs, and fixed AttributeClassDiscovery as well by making the new method a trait. Also swapped to using \Drupal::root() as that seems more correct, injection will be painful as it means changing every single plugin manager to inject the root directory as well, unless we change container.namespaces to automatically prepend the root there instead? I assume that those paths are relative for a reason, and we can't just hardcode absolute paths into the container.

  • Comment 12 months ago →
    🇬🇧United Kingdom alexpott 🇪🇺🌍

    and we can't just hardcode absolute paths into the container.

    Well \Drupal\Core\Site\Settings::getApcuPrefix() says we can... the container cache key depends on drupal root.

  • Pipeline finished with Failed
    12 months ago
    Total: 7074s
    #150307
  • Comment 11 months ago →
    🇺🇸United States smustgrave

    Wondering what next steps are for this one?

  • Status changed to Needs work 11 months ago
  • Comment 11 months ago →
    🇬🇧United Kingdom longwave UK

    NW for #8.

  • First commit to issue fork.
  • Status changed to Needs review 10 months ago
  • Comment 10 months ago →
    🇳🇿New Zealand RoSk0 Wellington

    Changes to merge request addresses #8 , if I understood this all correctly. Sorry in advance if not.

  • Pipeline finished with Failed
    10 months ago
    Total: 556s
    #186795
  • Status changed to Needs work 10 months ago
  • Comment 10 months ago →
    🇬🇧United Kingdom longwave UK

    This broke all the tests, so there is something wrong.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024