Problem/Motivation

Right now, many simple configuration (variable->config) migrations record nothing in the map table, and return 0 for the source count. This is confusing from a UX perspective, making it seem like nothing is being migrated in those cases. It also means that rerunning the migration will, well, rerun the migration - normal migrations do nothing with items that are previously migrated (i.e., recorded in the map table), but these migrations will reset the configuration again from the source (which may be unexpected if an administrator and manually changed confiugration).

On the other hand, it looks like some variable->config migrations do have proper source counts/map saves, so it seems not an entirely general issue. Also, I vaguely recall some discussion of this behavior a long time ago, and there may be a good reason for it. At any rate, it needs investigation.

Proposed resolution

TBD

Remaining tasks

Investigate.

User interface changes

N/A

API changes

?

Data model changes

N/A

CommentFileSizeAuthor
#12 2570541-12.patch5.12 KBphenaproxima
#9 interdiff.txt1.77 KBmikeryan
#9 track_simple-2570541-9.patch4.87 KBmikeryan
#3 track_simple-2570541-3.patch2.66 KBmikeryan
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

OK, I've come to some understanding here. migrate_drupal's variable plugin normalizes the incoming variables into a single row, with each field name being a variable name. That makes it simple to directly map the variables to their corresponding D8 configuration. So, this plugin will show 0 rows when none of the listed variables are in the source variable table, and exactly 1 row when one or more of them are present. This is normal - the issue we need to deal with is that the variable plugin returns nothing from getIds(), so nothing is recorded in the map table. Thus, after running a variable migration, you see something like

$ drush ms d6_search_settings
 Group: default      Status  Total  Imported  Unprocessed  Last imported       
 d6_search_settings  Idle    1      0         1            2015-09-23 16:06:45

This is confusing, and also will prevent #2554003: isComplete() should not rely on RESULT_COMPLETED from working properly - all variable migrations with at least one real variable will not appear complete even when they are. So, I think it should be a simple matter to add a dummy ID field that will get saved to the map table.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Active » Needs review
Issue tags: +blocker
FileSize
track_simple-2570541-3.patch2.66 KB

The ultimate patch is pretty simple, although there was a bit of trial-and-error to get there. Variable migrations will now make a record in the map table, so the new isComplete() will work correctly. I had to fix one test, MigrateSearchPageTest - it ran the same variable migration twice and expected the second run to actually do something, which it only did because of the empty map table.

#2554003: isComplete() should not rely on RESULT_COMPLETED is actually a fail test for this - when isComplete() works correctly, then the broken behavior here breaks tests all over the place, which this test fixes.

quietone’s picture

quietone CreditAttribution: quietone commented

Works great, thanks. Did a migration with and without the patch and diffed the output of 'drush ms' for each. Definitely seeing 1 row migrated for the variable to config migrations.

Just one question.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/Variable.php
@@ -53,7 +53,10 @@ protected function initializeIterator() {
+    $values['id'] = reset($this->variables);
+    return $values + array_map('unserialize', $this->prepareQuery()->execute()->fetchAllKeyed());

Wouldn't this be a problem if array_map contained an 'id' key?

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented

Wouldn't this be a problem if array_map contained an 'id' key?

The keys here are the variable names being migrated, so it would only be a problem in practice if the migration were trying to import a variable named 'id'. I'm willing to take that chance...

quietone’s picture

quietone CreditAttribution: quietone commented

Ok. But seems like something to document somewhere to prevent a problem for someone in the future? Would this mostly effect contrib?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

In an ideal world, we'd be able to map every migrated variable to its fully-qualified destination key in config, but I see why that's not really possible -- at least, not without significantly refactoring (and in the process, convoluting) a lot of existing migrations.

I was going to mark this issue as needing tests, but as @mikeryan mentioned in #3 and explained on IRC, #2554003: isComplete() should not rely on RESULT_COMPLETED breaks -- horribly, I'm told -- without this patch. If that patch serves as an implicit test of this one, I'm pretty much satisfied.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

It looks like this is missing test coverage.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
track_simple-2570541-9.patch4.87 KB
interdiff.txt1.77 KB
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

DrupalCI approves. So do I.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Shoot. This one needs a re-roll.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2570541-12.patch5.12 KB

Done!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Und beck to RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed eac02c7 on 8.0.x 
    Issue #2570541 by mikeryan, phenaproxima: Track simple configuration...

Status: Fixed » Closed (fixed)

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