Problem/Motivation

In #2687849: Add back rollbacks on migrate_drupal_ui the following error occurred during the rollback.

1) Drupal\Tests\migrate_drupal_ui\Functional\d6\Rollback6Test::testMigrateUpgradeExecute
Exception: Error: Call to a member function getConfigDependencyName() on null
Drupal\migrate\Plugin\migrate\destination\EntityConfigBase->rollback()() (Line: 279)

This is likely a dependency problem. In that issue the fix there was to ensure the config entity exists before attempting to delete it. This issue is to figure out the dependency problem.

For Drupal6, the problem appears when trying to rollback d6_profile_field_option_translation.

See 2687849-#151

Steps to reproduce

If not committed, apply the latest patch from #2687849: Add back rollbacks on migrate_drupal_ui and then remove the following lines from core/modules/migrate/src/Plugin/migrate/destination/EntityConfigBase.php, Then run Upgrade6Test or Upgrade7Test.

+      // Ensure the entity exists before attempting to delete.
+      if (!is_null($this->storage->load($entity_id))) {

Proposed resolution

Change the dependency in d6_profile_field_option_translation.yml from user_profile_field to user_profile_field_instance.
Add language to all the d6 user profile translation migrations
Fix an error d6_user_profile_field_instance_translation.yml

Remaining tasks

patch
review
commit

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#10 3191782-10.patch4.73 KBWim Leers
#10 interdiff.txt1.04 KBWim Leers
#7 3191782-7.patch5.66 KBquietone
#7 interdiff-5-7.txt1.73 KBquietone
#5 3191782-5.patch4.84 KBquietone
#5 interdiff-2-5.txt1.56 KBquietone
#2 3191782-2.patch3.28 KBquietone
#2 3191782-2-fail.patch2.16 KBquietone
#2 interdiff.txt760 bytesquietone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue summary: View changes
Status: Active » Needs review
FileSize
interdiff.txt760 bytes
3191782-2-fail.patch2.16 KB
3191782-2.patch3.28 KB

During rollback via the UI user_profile_field_instance was executed before d6_profile_field_option_translation. And that rollback, user_profile_field_instance, deletes the fields used by d6_profile_field_option_translation. The fix is to simply change the dependency on user_profile_field_instance to user_profile_field_instance instead of user_profile_field.

Since dependencies are being changed here, I think that language should be added to the dependencies. And also, fix a mistake in d6_user_profile_field_instance_translation where user_profile_field and user_profile_field_instance are listed in the dependencies. Only user_profile_field_instance needs to be there because it is dependent on user_profile_field.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Title: Rollback fails due to dependency problem » Fix dependency in d6 user profile translation migrations

Status: Needs review » Needs work

The last submitted patch, 2: 3191782-2.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff-2-5.txt1.56 KB
3191782-5.patch4.84 KB

I forgot to update the Migration tests. The fail patch above is still valid, the failing test is the new test, d6/MigrateUserProfileTranslationRollbackTest.php which passes in the patch in #2.

Status: Needs review » Needs work

The last submitted patch, 5: 3191782-5.patch, failed testing. View results

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff-5-7.txt1.73 KB
3191782-7.patch5.66 KB

Oh bother, I changed the wrong test.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue tags: -Migrate UI +migrate-d6-d8
mohit_aghera’s picture

mohit_aghera CreditAttribution: mohit_aghera at QED42 commented
Status: Needs review » Reviewed & tested by the community

1. Issue summary and problem statement are clear.
2. I tried to reproduce the issue from Steps to reproduce section and I was able to reproduce on local.
3. Patch is straightforward and seems to fix the issue.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#3197324: Exception trace cannot be serialized because of closure
FileSize
interdiff.txt1.04 KB
3191782-10.patch4.73 KB 
+++ b/core/modules/migrate/tests/src/Kernel/MigrateTestBase.php
@@ -189,12 +189,12 @@ protected function executeMigration($migration) {
-    array_walk($ids, function ($id) use ($manager) {
+    foreach ($ids as $id) {
       // This is possibly a base plugin ID and we want to run all derivatives.
-      $instances = $manager->createInstances($id);
+      $instances = $manager->createInstances([$id]);
       $this->assertNotEmpty($instances, sprintf("No migrations created for id '%s'.", $id));
       array_walk($instances, [$this, 'executeMigration']);
-    });
+    }

This is out of scope here.

It's a copy/paste from #3197324-3: Exception trace cannot be serialized because of closure.

Rather than marking Needs work, keeping the current status and just fixing it 😊

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

@Wim Leers, thank you. That it correct, that should be removed.

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 3191782-10.patch, failed testing. View results

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Needs work » Reviewed & tested by the community

Restoring status after HEAD was broken.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

  • larowlan committed c8b1250 on 9.2.x 
    Issue #3191782 by quietone, Wim Leers, mohit_aghera: Fix dependency in...
  • larowlan committed a5103bb on 9.3.x
    Issue #3191782 by quietone, Wim Leers, mohit_aghera: Fix dependency in...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed a5103bb and pushed to 9.3.x. Thanks!

Backported to 9.2.x as there is little risk of disruption here, other than the test we're just changing some migration dependencies.

Status: Fixed » Closed (fixed)

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