Problem/Motivation

The documentation for this function is:

 /**
   * Executes a set of migrations in dependency order.
   *
   * @param string[] $ids
   *   Array of migration IDs, in any order.
   */

however the function executes the migrations in the order they are passed, not in dependency order

Proposed resolution

Refactor the helper function to execute in dependency order. This shouldn't break existing usages, since executing migrations out of order would cause the tests to fail. This will allow future use of this method to work properly

Remaining tasks

Commit

User interface changes

none

API changes

none, as tests are not part of the API. This method is likely used in contrib though, and it would begin working as advertised.

Data model changes

none

CommentFileSizeAuthor
#20 3015369-20.patch966 bytesquietone
#20 3015369-20-test.patch2.13 KBquietone
#20 3015369-20-fail.patch1.18 KBquietone
#17 3015369-14.patch966 bytesquietone
#17 3015369-17-fail.patch1.18 KBquietone
#15 3015369-15-fail.patch2.54 KBquietone
#14 3015369-14.patch966 bytesquietone
#2 3015369-2.drupal.MigrateTestBaseexecuteMigrations-claims-to-execute-migrations-in-dependency-order-but-does-not.patch1.1 KBmikelutz

Issue fork drupal-3015369

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

mikelutz created an issue. See original summary.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Assigned: mikelutz » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3015369-2.drupal.MigrateTestBaseexecuteMigrations-claims-to-execute-migrations-in-dependency-order-but-does-not.patch1.1 KB

Status: Needs review » Needs work

The last submitted patch, 2: 3015369-2.drupal.MigrateTestBaseexecuteMigrations-claims-to-execute-migrations-in-dependency-order-but-does-not.patch, failed testing. View results

quietone’s picture

quietone commented

@mikelutz, Nice find!

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 3015369-14.patch966 bytes

Update patch.

quietone’s picture

quietone commented
StatusFileSize
new 3015369-15-fail.patch2.54 KB

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. #3197324: Exception trace cannot be serialized because of closure.

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.

Status: Needs review » Needs work

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

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 3015369-17-fail.patch1.18 KB
new 3015369-14.patch966 bytes

The problem found above is addressed in #3371869: Fix dependencies of taxonomy term translation migrations

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 #3197324: Exception trace cannot be serialized because of closure.

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

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?

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 3015369-20-fail.patch1.18 KB
new 3015369-20-test.patch2.13 KB
new 3015369-20.patch966 bytes

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

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented

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.

smustgrave’s picture

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

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

Status: Reviewed & tested by the community » Needs work

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

smustgrave’s picture

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

Random failure.

Status: Reviewed & tested by the community » Needs work

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

smustgrave’s picture

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

Another random

quietone’s picture

quietone commented
Issue summary: View changes

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.

Status: Reviewed & tested by the community » Needs work

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

quietone’s picture

quietone commented
Status: Needs work » Needs review

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
quietone’s picture

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

Back to RTBC

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

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

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented

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.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Related issues: +#3390678: MigrationPluginManager::createInstances claims to throw a PluginException on invalid IDs, but does not

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

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented

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.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Related issues: +#3390693: MigrationPluginManager::ExpandPluginIds can lose derivative plugins under certain circumstances

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

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented

#3390678: MigrationPluginManager::createInstances claims to throw a PluginException on invalid IDs, but does not 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...

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented

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.

smustgrave’s picture

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

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

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work

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.)

quietone opened merge request !5344

quietone’s picture

quietone commented
Title: MigrateTestBase::executeMigrations() claims to execute migrations in dependency order but does not. » Fix MigrateTestBase::executeMigrations() to execute migrations in dependency order
Status: Needs work » Needs review

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

quietone’s picture

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

Tests still passing, restoring RTBC.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

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

quietone’s picture

quietone commented
Status: Needs work » Needs review

@xjm, Sorry about that. All fixed now.

smustgrave’s picture

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

drupalCi file changes removed.

xjm’s picture

xjm
she/her
Primary language English
commented

Amending attribution.

xjm’s picture

xjm
she/her
Primary language English
commented
Priority: Normal » Major

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

  • xjm committed 31911458 on 11.x 
    Issue #3015369 by quietone, mikelutz, xjm: Fix MigrateTestBase::...

  • xjm committed 4500f88b on 10.2.x 
    Issue #3015369 by quietone, mikelutz, xjm: Fix MigrateTestBase::...

  • xjm committed af93e630 on 10.1.x 
    Issue #3015369 by quietone, mikelutz, xjm: Fix MigrateTestBase::...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

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

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented

Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.1.x-dev

Status: Fixed » Closed (fixed)

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

quietone closed merge request !5344