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
Comment | File | Size | Author |
---|---|---|---|
#12 | 2570541-12.patch | 5.12 KB | phenaproxima |
#9 | interdiff.txt | 1.77 KB | mikeryan |
#9 | track_simple-2570541-9.patch | 4.87 KB | mikeryan |
Comments
Comment #2
mikeryanOK, 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
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.
Comment #3
mikeryanThe 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.
Comment #4
quietone CreditAttribution: quietone commentedWorks 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.
Wouldn't this be a problem if array_map contained an 'id' key?
Comment #5
mikeryanThe 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...
Comment #6
quietone CreditAttribution: quietone commentedOk. But seems like something to document somewhere to prevent a problem for someone in the future? Would this mostly effect contrib?
Comment #7
phenaproximaIn 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.
Comment #8
alexpottIt looks like this is missing test coverage.
Comment #9
mikeryanComment #10
phenaproximaDrupalCI approves. So do I.
Comment #11
webchickShoot. This one needs a re-roll.
Comment #12
phenaproximaDone!
Comment #13
phenaproximaUnd beck to RTBC.
Comment #14
webchickCommitted and pushed to 8.0.x. Thanks!