Contrib.social

MigrationPluginManager::createInstances claims to throw a PluginException on invalid IDs, but does not

Open on Drupal.org →
Created on 29 September 2023, about 2 years ago
Updated 12 November 2023, almost 2 years ago

Problem/Motivation

The docblock for MigrationPluginInterface::createInstances like most plugin managers has a @throws tag:

   *
   * @throws \Drupal\Component\Plugin\Exception\PluginException
   *   If an instance cannot be created, such as if the ID is invalid.
   */

to throw an exception if invalid plugin ids are requested. This exception would typically bubble up from $factory->createInstance($plugin_id... However MigrationPluginManager runs the passed plugin ids through MigrationPluginManger::expandPluginIds() which is used to find derived plugins. The code in expandPluginIds

  /**
   * {@inheritdoc}
   */
  public function expandPluginIds(array $migration_ids) {
    $plugin_ids = [];
    $all_ids = array_keys($this->getDefinitions());
    foreach ($migration_ids as $id) {
      $plugin_ids += preg_grep('/^' . preg_quote($id, '/') . PluginBase::DERIVATIVE_SEPARATOR . '/', $all_ids);
      if ($this->hasDefinition($id)) {
        $plugin_ids[] = $id;
      }
    }
    return $plugin_ids;
  }

effectively builds a new list of plugin ids filtered through the discovered plugins, silently removing any invalid ids without trying to create them, leaving no opportunity for the exception to bubble up.

Steps to reproduce

Proposed resolution

We should have expandPluginIds throw an exception if neither the requested id or derivatives on it are found, or it should remove the original plugin id only if derivitives are found, ensuring that $factory::createInstance() has an opportunity to throw an exception on the invalid id.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

📌 Task
Status

Needs work

Version

11.0 🔥

Component
Migration  →

Last updated about 1 month ago

Created by

🇺🇸United States mikelutz Michigan, USA

Live updates comments and jobs are added and updated live.
  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

  • Needs subsystem maintainer review

    It is used to alert the maintainer(s) of a particular core subsystem that an issue significantly impacts their subsystem, and their signoff is needed (see the governance policy draft for more information). Also, if you use this tag, make sure the issue component is set to the correct subsystem. If an issue significantly impacts more than one subsystem, use needs framework manager review instead.

Sign in to follow issues

Comments & Activities

  • Issue created by @mikelutz
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.2 & MySQL 8
    last update about 2 years ago
    30,347 pass, 18 fail
  • @mikelutz opened merge request.
  • Status changed to Needs review about 2 years ago
  • Comment about 2 years ago →
    🇺🇸United States mikelutz Michigan, USA

    Testing to see what this breaks

  • Comment about 2 years ago →
    🇺🇸United States mikelutz Michigan, USA

    So this seems to open up a whole can of worms before and after 📌 $migration_dependencies has inconsistent structure Fixed . Some of these tests just need an adjustment to expecting the exception instead of checking that createInstances returns an empty array. But more importantly, this shows migrations like the d6 block which has 

    migration_dependencies:
  required:
    - menu
    - d6_custom_block
    - d6_user_role

    There is no 'menu' migration. There are d6_menu and d7_menu migrations. But the plugin manager pumps the migration_requirements through expandPluginIds before building the requirements array, and thus stripping out any non-existent migrations. So migrations requirements check that migrations that exist have run to completion, but never checks that the migrations listed in the requirements actually exist. If they don't exist, they are just skipped over...

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.2 & MySQL 8
    last update about 2 years ago
    Custom Commands Failed
  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.2 & MySQL 8
    last update about 2 years ago
    30,357 pass, 6 fail
  • Status changed to Needs work about 2 years ago
  • Comment about 2 years ago →
    🇺🇸United States smustgrave

    Appears to have test failures.

    Have not yet reviewed

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.2 & MySQL 8
    last update about 2 years ago
    30,358 pass, 4 fail
  • Comment about 2 years ago →
    🇺🇸United States mikelutz Michigan, USA

    Yeah, I'm still working through the ramifications of all this, trying to see if we can fix it, or if we are going to need to add in a BC layer.

  • Comment about 2 years ago →
    🇺🇸United States mikelutz Michigan, USA

    Sigh.. it's never good when it's down to just the full upgrade tests failing.. mean something isn't being tested at the kernel level that should be..

  • Comment about 2 years ago →
    🇺🇸United States mikelutz Michigan, USA

    My mistake, those aren't the primary full upgrade tests, those are the full upgrade tests for the forum module. Those failures come from not enabling the language module despite the language migration being required for some of the migrations being run. The test itself will need a few updates to pass with that module enabled, which I will try to work through.

  • Comment almost 2 years ago →
    🇺🇸United States xjm
  • Comment almost 2 years ago →
    🇺🇸United States xjm
contrib.social Blog FAQ Discussions
Production build 0.71.5 2024