The migrate list command calls $manager->createInstances() with an empty array, which instantiates all the migration plugins.

It would be better to filter on migration IDs, groups, and tags based on the plugin definitions, and then only instantiate the plugins actually needed.

Firstly, it would improve performance, and second, instantiating migrations causes their requirements to be checked, which might not be desirable. (Though a bug in core means that some migrations check requirements even without being instantiated: #3187480: D6NodeDeriver calls checkRequirements() when building migration plugin definitions.)

CommentFileSizeAuthor
#5 migrate_tools.2969227.backport-8.5.0.patch4.46 KBjoachim

Issue fork migrate_tools-2969227

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

joachim created an issue. See original summary.

joachim opened merge request !2

joachim’s picture

joachim commented
Version: 8.x-4.x-dev » 8.x-5.x-dev
Status: Active » Needs review

MR: https://git.drupalcode.org/project/migrate_tools/-/merge_requests/2

This rearranges the filtering on migration IDs, groups, and tags, so all that is done with the plugin definitions. Plugins are only instantiated once the list of plugins IDs has been filtered.

joachim’s picture

joachim commented
Issue summary: View changes
joachim’s picture

joachim commented
StatusFileSize
new migrate_tools.2969227.backport-8.5.0.patch4.46 KB

Here's a backport of the MR for 8.x-5.0.

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

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Needs work

The failing test is a legit failure. We should the tests expectations for migrate status with an invalid migration.

joachim’s picture

joachim commented
Status: Needs work » Needs review
heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Needs work

Tests are still not passing.

joachim’s picture

joachim commented

Found the problem. The kernel test is doing

> $this->commands->status('invalid_plugin');

which is calling the status() method on MigrateToolsCommands directly. I had assumed this call was doing some sort of magic to simulate a Drush call, but it's just:

    $this->commands = new MigrateToolsCommands( // etc

The problem is this:

  public function status($migration_names = '', array $options = [
    'group' => self::REQ,
    'tag' => self::REQ,
    'names-only' => FALSE,
    'continue-on-failure' => FALSE,
  ]) {

The default values self::REQ are there to tell Drush's command registration system that the group option is required. (Though I don't understand that part -- the --group option is NOT required!)

The command processing in Drush/Symfony presumably pass the $options array so that the default value of the $options parameter is never actually seen by the status() method.

Except in the kernel test, which bypasses all that.

The best fix would be to properly simulate Drush commands, which is out of scope here.

Quick fix would be to pass in an empty array as the $options parameter.

What do you think?

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

Quick fix seems fine to me.

joachim’s picture

joachim commented
Status: Needs work » Needs review
heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Needs work

I think we have to pass in actual default values as well.

joachim’s picture

joachim commented
Status: Needs work » Needs review
joachim’s picture

joachim commented
Version: 8.x-5.x-dev » 6.0.x-dev

Rebased the MR on the 6.0.x branch.

joachim’s picture

joachim commented

Rebased.

joachim’s picture

joachim commented

Drupal\Tests\migrate_tools\Kernel\MigrateImportTest::testImport passed for me locally.

ronchica’s picture

ronchica commented

FWIW, I needed this patch in order to be able to import or check the status of a migration group.

Due to a dependency, I needed to enable the Migrate Drupal module, which appears to add some migrate plugins to the available list of plugins. I kept getting a '/SQLSTATE[HY000] [2002] Connection refused' error when trying to run the migration group (a single migration ran fine).

After a bunch of time thinking the issue was with my DDEV setup, I debugged using xdebug, found all those plugins trying to connect to a non-existent db, which led me to this issue. Applying the patch enables the status and import commands to work on a migration group, and the import looks good. I still get the connection error once in a while though, so there might be something else that needs to be fixed in my case.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented

There's still test failures on the MR.

  • heddn committed 8d0deabf on 6.1.x authored by joachim 
    fix: #2969227 'migrate status MIGRATION' loads every migration

By:...
heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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

unstatu’s picture

unstatu commented

Hello everyone :)

I think that this patch had a side effect. I'm not sure if it was desired or not.

Since 6.1.3, the migrations listed with the migrate:status drush command are not sorted in the same order as they are run. That is, taking into account their dependencies.

fchometon’s picture

fchometon
Primary language French
Location Lyon
commented

I’m seeing the same behavior.
Since 6.1.3, migrations are no longer sorted according to their dependencies.
This comes from migrationsList(), which no longer uses $manager->createInstances([]) and therefore never calls buildDependencyMigration().
As a result, dependency-based ordering (including migration_dependencies.optional) is lost.

I have prepared a patch that restores dependency-based ordering for filtered migrations by calling buildDependencyMigration() on the already instantiated migrations after requirements checks :
https://www.drupal.org/project/migrate_tools/issues/3572002