Problem/Motivation

https://www.drupal.org/pift-ci-job/1162569
There's a test over in #3023504: Add test coverage for drush config export. The reason why this seems to be failing is that we are trying to generate a config entity in core inside of the profile field migration's getProcess method and that is failing. Config entities can not be stubbed, and that's what it is trying to do.

Proposed resolution

Don't throw a skip row exception. Return an empty process in getProcess if requirements aren't met. In this case, requirements are that the previous migrations have finished.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#17 interdiff.3023747.9-17.txt3.25 KBmikelutz
#17 3023747-17.drupal.D6-profile-migrations-assume-stubs-which-fail.patch4.78 KBmikelutz
#11 3023747-11.patch4.23 KBheddn
#11 interdiff_9-11.txt720 bytesheddn
#9 interdiff.3023747.3-9.txt2.38 KBmikelutz
#9 3023747-9.drupal.D6-profile-migrations-assume-stubs-which-fail.patch4.23 KBmikelutz
#9 3023747-9.TESTS-ONLY.patch3.14 KBmikelutz
#5 3023747-4.drupal.D6-profile-migrations-assume-stubs-which-fail.patch4.09 KBmikelutz
#3 3023747-3.drupal.D6-profile-migrations-assume-stubs-which-fail.patch3.03 KBmikelutz
#2 3023747.patch1.24 KBheddn
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn created an issue. See original summary.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Active » Needs review
FileSize
3023747.patch1.24 KB
mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer commented
FileSize
3023747-3.drupal.D6-profile-migrations-assume-stubs-which-fail.patch3.03 KB

Test Only Patch. Migration::getProcess() should never throw an exception. It should return a process, or an empty array, but not an exception.

Status: Needs review » Needs work

The last submitted patch, 3: 3023747-3.drupal.D6-profile-migrations-assume-stubs-which-fail.patch, failed testing. View results

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer commented
FileSize
3023747-4.drupal.D6-profile-migrations-assume-stubs-which-fail.patch4.09 KB

whoops. test patch again.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer commented
Status: Needs work » Needs review
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented 
+++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/MigrateDrupal6TestBase.php
@@ -6,6 +6,7 @@
+ *

+++ b/core/modules/user/src/Plugin/migrate/ProfileValues.php
@@ -2,6 +2,7 @@
+use Drupal\Core\Queue\RequeueException;

Seems there are some stray changes in here.

Status: Needs review » Needs work

The last submitted patch, 5: 3023747-4.drupal.D6-profile-migrations-assume-stubs-which-fail.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer commented
Status: Needs work » Needs review
FileSize
3023747-9.TESTS-ONLY.patch3.14 KB
3023747-9.drupal.D6-profile-migrations-assume-stubs-which-fail.patch4.23 KB
interdiff.3023747.3-9.txt2.38 KB

I don't know why my diffs got borked, but let's try one last time...

The last submitted patch, 9: 3023747-9.TESTS-ONLY.patch, failed testing. View results

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Reviewed & tested by the community
FileSize
interdiff_9-11.txt720 bytes
3023747-11.patch4.23 KB 
+++ b/core/modules/user/src/Plugin/migrate/ProfileValues.php
@@ -59,7 +60,8 @@ public function getProcess() {
+        // not exist on the source site, or if the requirement migrations have

I think this should be 'required'.

Fixed here in new patch. And marked RTBC as most of these changes are not mine, except for the minor doc improvement. The solution opted for here is a much cleaner/better route than what was there before.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Issue summary: View changes
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Issue summary: View changes
mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer commented

Testing against 8.6 as well, this should be backported.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Issue tags: +needs backport to 8.6.x
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Reviewed & tested by the community » Needs work

Re-reviewing things here. Having second thoughts.

+++ b/core/modules/migrate_drupal/tests/src/Kernel/d6/MigrationProcessTest.php
@@ -0,0 +1,44 @@
+      catch (MigrateSkipRowException $e) {

Might want to catch for Exception?

+++ b/core/modules/user/src/Plugin/migrate/ProfileValues.php
@@ -45,15 +45,19 @@ public function getProcess() {
            throw new MigrateSkipRowException("Can't migrate source field $name.");

This is still getting thrown. Is that fine or should we log an error and continue? Just because one field cannot migrate doesn't mean we should bail the whole migration? Or should it? I checked, and the skip row will cause the entire migration to fail.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer commented
FileSize
3023747-17.drupal.D6-profile-migrations-assume-stubs-which-fail.patch4.78 KB
interdiff.3023747.9-17.txt3.25 KB

I caught the skipRow exception in particular because I wanted the fail message to tell me which migration was failing, but any Exception would fail the test. Still, might as well have the message for all exceptions.

We really need to inject loggers into the migration system in general, but I don't want to add one for this issue. I don't see any real way for that exception to be thrown, we are looping through the same source data as the migration we are looking up, and we know that migration ran, so if we don't get a result, there's something corrupt with the source data. I think it is reasonable to just skip the field in that case, because if the lookup failed then the field didn't get created anyway, so there's nothing to migrate to.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer commented
Status: Needs work » Needs review
heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Reviewed & tested by the community

Assuming this comes back green, we be good to go. The rational for removing the skip exception (and not replacing with logging) make sense. No need for a follow-up in here, because we shouldn't see the situation. We have a check requirements call earlier and the sources are the same.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -needs backport to 8.6.x

Fixed this on commit:

core/modules/user/src/Plugin/migrate/ProfileValues.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 7 | WARNING | [x] Unused use statement
----------------------------------------------------------------------

Committed and pushed 95b3b17867 to 8.7.x and 305fd92394 to 8.6.x. Thanks!

  • catch committed 95b3b17 on 8.7.x 
    Issue #3023747 by mikelutz, heddn: D6 profile migrations assume stubs,...

  • catch committed 305fd92 on 8.6.x 
    Issue #3023747 by mikelutz, heddn: D6 profile migrations assume stubs,...

Status: Fixed » Closed (fixed)

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