Fix MigrateTestBase::executeMigrations() to execute migrations in dependency order

Update patch.

That is good, no core tests are broken by this change.

So, I thought it would be good to confirm that re-arranging the order of the migrations in a Kernel test does work. I selected MigrateTaxonomyTermTranslationTest. I started by moving the last migration to the top and running the test. That work. I repeated that and eventually got a failure. You will see it in the patch and well as the absolutely useless serialization error message. 🐛 Exception trace cannot be serialized because of closure Needs work .

When you install the fix for that you get

1) Drupal\Tests\taxonomy\Kernel\Migrate\d7\MigrateTaxonomyTermTranslationTest::testTaxonomyTermTranslation
'language_content_settings' entity with ID 'taxonomy_term.test_vocabulary' already exists. (/var/www/html/core/lib/Drupal/Core/Entity/EntityStorageBase.php:519)
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'status'
+'error'

I then debugged and found that d7_language_content_vocabulary_settings is last in the dependency list but it should be further up. I added a requirement on d7_taxonomy_vocabulary to it and re-ran the test. And d7_language_content_vocabulary_settings was still last! There is something more going on here and I am not seeing it.

The last submitted patch, 15: 3015369-15-fail.patch, failed testing. View results →

The problem found above is addressed in 🐛 Fix dependencies of taxonomy term translation migrations Fixed

Thinking about how to test this I decided to make a fail patch by changing the order of migrations in an existing test. I chsse d7/MigrateNodeTest and put the node translation migration before the node migration. That fails as expected with

1) Drupal\Tests\node\Kernel\Migrate\d7\MigrateNodeTest::testNode
Migration d7_node_translation:article did not meet the requirements. Missing migrations d7_node:article.
Failed asserting that two strings are equal.
--- Expected

You won't see that in the testbot results because of 🐛 Exception trace cannot be serialized because of closure Needs work .

Then I re-uploaded #14 which is the desired change so this doesn't get set back to NW.

The last submitted patch, 17: 3015369-17-fail.patch, failed testing. View results →

For this one going to rely on the test-only patch.

But can the full patch contain the tests to see if it fixed it?

Fair enough. Here is a -fail and -test patch where the difference between them is the addition of the fix.

The patch intended for commit is the other one, which has just the fix. It does not have the change to the test that proved the fix works.

The last submitted patch, 20: 3015369-20-fail.patch, failed testing. View results →

Just reiterating here that we do not need to include the test with the committed patch here because this method is a test helper function, not core code. We don't need to test the tests. This should be good to go, but alas I wrote the original patch and can't rtbc.

In that case think #20 should be good then. Thanks for uploading those @quietone!

The last submitted patch, 20: 3015369-20.patch, failed testing. View results →

Random failure.

The last submitted patch, 20: 3015369-20.patch, failed testing. View results →

Another random

Reviewing the RTBC queue. The issue summary is update to date and complete. There are no unanswered questions in the issue summary. There are tests to prove the change works in #20 but note that those tests are not to be committed.

The last submitted patch, 20: 3015369-20.patch, failed testing. View results →

Random fail, retesting.

1) Drupal\Tests\Core\Command\QuickStartTest::testQuickStartCommand
unlink(/var/www/html/sites/simpletest/35649434/files/.sqlite-shm): No such file or directory

Back to RTBC

I think this looks pretty good -- the new implementation is much cleaner -- but what about the loss of the assertNotEmpty() coverage?

I don't feel like it's necessary or useful. It was added here: https://www.drupal.org/project/drupal/issues/3021395#comment-12905204 → in response to a bug in a test helper function for d6 node translations where an incorrect migration id was passed, resulting in no migrations being created. This again falls back into the category of 'testing the tests' to ensure we don't write a test calling for a plugin to be created that doesn't exist. Seems to me it would be more useful (in a follow-up) to add error checking and exceptions in MigrationPluginManager::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;
  }

Which will silently allow you to request plugins that don't exist and return an empty array. MigrationPluginManager::createInstances() claims it

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

but in reality, ::expandPluginIds filters the requested plugin IDs against the discovered plugin IDs, so we are only ever calling $factory->createInstance() with a valid plugin id, and won't ever bubble up a PluginException from it.

I opened 📌 MigrationPluginManager::createInstances claims to throw a PluginException on invalid ids, but does not. Needs review . Just to see what breaks if we allow the plugin manager to throw exceptions on the invalid plugin ids.

Now that I look at that expandPluginIds closely, it looks buggy... using the += on the derivatives is fine, preg grep will return the numeric keys from $all_ids which are unique, but then we throw $plugin_ids[] = $id in there, and if that gets assigned an index used by a derivitive in a later loop, that derivative won't get added... I think.

Yup, that's a nasty little bug.. Opened 🐛 MigrationPluginManager::ExpandPluginIds can lose derivative plugins under certain circumstances Needs work

📌 MigrationPluginManager::createInstances claims to throw a PluginException on invalid ids, but does not. Needs review exposed even more issues... because expandPluginIds silently filters out non-existent migration ids, and we run the migration requirements through expandPluginIds before checking that the rows are processed, we actually aren't checking that required migrations exist when checking requirements. We only check that if they do exist that they have been fully processed...

All that to say, I think we can commit this without the assertNotEmpty check. The point of that was to make it harder to write tests with mistakes, but digging into it today, they fact that we can call createInstances() with invalid plugin ids and have it silently fail has caused mistakes in runtime code and migrations, so we need to fix those, and in doing so will make this check redundant anyway.

per @mikelutz in #38 sounds like this can move forward.

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Update title, convert to MR, hiding all patches. The patch still applied so no changes needed to be made.

Tests still passing, restoring RTBC.

The MR is currently kind of sort of turning off all tests but the one.

@xjm, Sorry about that. All fixed now.

drupalCi file changes removed.

Amending attribution.

This is probably actually major as some nasty data integrity bugs could potentially result from them being out of order.

Committed to 11.x, 10.2.x, and 10.1.x as a data integrity bugfix. Thanks!

@xjm This is only a helper function for PHPUnit tests. The regular plugin manager will properly sort migrations via a dependency graph when requested. I don't believe it can cause data integrity issues on sites.

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