Problem/Motivation

Found on https://git.drupalcode.org/project/drupal/-/jobs/2356950

core/modules/field/migrations/d6_field_formatter_settings.yml depends on Drupal\migrate_drupal\Plugin\migrate\FieldMigration

For some reason this isn't picked up by the test when it runs locally via phpunit, or when it runs on HEAD, but it is picked up by that MR and it looks like a real bug - the plugin class is provided by migrate_drupal but the migration is defined in migrate.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3466289

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review

Getting the test to only create the migration instance it actually needs still passes locally. Given we're deprecating migrate_drupal altogether for removal in Drupal 12, this might be as far as we want to take this - the bug has been hidden for years.

catch opened merge request !9101

quietone’s picture

quietone commented
Status: Needs review » Active

What is the relation between the test and d6_field_formatter_settings.yml? It should not be involved in this test at all. Without migrate_drupal installed only the migration provided by the test should be discovered.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review

The test installs node, which depends on text, which depends on field module, which provides this migration.

This is the backtrace from the test:

https://project.pages.drupalcode.org/-/drupal/-/jobs/2356950/artifacts/s...

Drupal\Core\Plugin\Factory\ContainerFactory->createInstance('d6_field_formatter_settings', Array) (Line: 118)
Drupal\migrate\Plugin\MigrationPluginManager->createInstances(Array) (Line: 33)
Drupal\migrate_no_migrate_drupal_test\Controller\ExecuteMigration->execute()
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 593)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 183)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 37)
Drupal\Core\Test\StackMiddleware\TestWaitTerminateMiddleware->handle(Object, 1, 1) (Line: 53)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 191)
Drupal\page_cache\StackMiddleware\PageCache->fetch(Object, 1, 1) (Line: 128)
Drupal\page_cache\StackMiddleware\PageCache->lookup(Object, 1, 1) (Line: 82)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 709)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

The call to getInstances('') discovers and creates a plugin instance for every migration.

What in MigratePluginManager is supposed to stop the migration from being discovered?

benjifisher made their first commit to this issue’s fork.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

I think @catch is 100% right. It has always worked this way, it creates other problems, and it is kind of crazy: we instantiate every migration plugin way too often.

In migration projects, I implement hook_migration_plugins_alter() to filter out all the D6 migrations.

A Nightwatch test failed. After 10 minutes, I cannot figure out how to trigger a re-test (grr). So I rebased and force-pushed.

smustgrave’s picture

smustgrave commented

I cannot figure out how to trigger a re-test

if the MR is opened by a core committer even getting push access you can't re-run tests, super inconvenient!

catch’s picture

catch
he/him
Primary language English
commented

Nightwatch was probably #3471104: Nightwatch and Functional JavaScript fails since selenium/standalone-chrome:128 but also we get regular enough random fails on nightwatch anyway.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Change seems small enough though and tests are all green.

quietone’s picture

quietone commented

#4 Ah, of course. I was clearly not thinking.

  • quietone committed 835708cc on 10.3.x 
    Issue #3466289 by catch, benjifisher, smustgrave:...

  • quietone committed b8047dec on 10.4.x 
    Issue #3466289 by catch, benjifisher, smustgrave:...

  • quietone committed dc2ab59a on 11.0.x 
    Issue #3466289 by catch, benjifisher, smustgrave:...

  • quietone committed 984772a9 on 11.x 
    Issue #3466289 by catch, benjifisher, smustgrave:...
quietone’s picture

quietone commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 11.x, 11.0.x, 10.4.x, and 10.3.x.

Thanks!

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

+1 for the implemented fix.

The previous code instantiated all migrations, then used just the one from the test module. The fix instantiates just the one that is used.

We also could have used createInstance() instead of createInstances().

It is a little odd that the migration in the test module has an ID that does not match its file name: from node_migration_no_upgrade.yml,

id: node_migration_no_migrate_drupal

The id is what matters, not the file name.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.