Now that we've removed load plugins, doing beta-to-beta Drupal 8 updates where migrate_drupal was enabled will fail (and the patch at #2555183: Fix the filled update tests, they are broken fails, demonstrating this). We need an update path that will remove the load key from existing migration config entities.

CommentFileSizeAuthor
#5 interdiff.txt622 bytesmikeryan
#5 migrate_needs_core-2559381-5.patch836 bytesmikeryan
#3 migrate_needs_core-2559381-3.patch883 bytesmikeryan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeryan created an issue. See original summary.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Issue tags: +beta target, +D8 upgrade path
mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Active » Needs review
FileSize
migrate_needs_core-2559381-3.patch883 bytes

Well, it seems simple enough, but testing locally with the latest #2555183: Fix the filled update tests, they are broken patch still fails, can't figure why...

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented 
+++ b/core/modules/migrate/migrate.install
@@ -0,0 +1,31 @@
+    if ($migration->get('load')) {

In local testing, this never returns TRUE (as it is an empty array), so the key isn't cleared. Simply removing the conditional fixes the tests in #2555183: Fix the filled update tests, they are broken. Alternatively, using

if (migration->get('load') !== NULL) {
...

might work, but there is no harm from what I can tell on calling clear() on a non-existent property.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
FileSize
migrate_needs_core-2559381-5.patch836 bytes
interdiff.txt622 bytes

With the suggested unconditional clear, thanks @jhedstrom!

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Status: Needs review » Reviewed & tested by the community

I think this is RTBC assuming the combined patch in #2555183: Fix the filled update tests, they are broken goes green.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

I removed the message on commit - site builders don't need to know that's what we did.

Agreed this is RTBC given the implicit test coverage working in the other issue. If we later found out something was wrong with this, we could remove migrate from the test dump until we stop changing the config.

diff --git a/core/modules/migrate/migrate.install b/core/modules/migrate/migrate.install
index c99addb..5044726 100644
--- a/core/modules/migrate/migrate.install
+++ b/core/modules/migrate/migrate.install
@@ -20,8 +20,6 @@ function migrate_update_8001() {
     $migration->clear('load');
     $migration->save(TRUE);
   }
-
-  return t('Load plugin references removed.');
 }

Committed/pushed to 8.0.x, thanks!

  • catch committed 63a6753 on 8.0.x 
    Issue #2559381 by mikeryan: Migrate needs core update path

Status: Fixed » Closed (fixed)

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