Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
While working on #2500495: Upgrade path for Color 7.x I found that there isn't an upgrade path for theme settings so creating this issue. The variables are theme_themename_settings, theme_default and admin_theme. And on a first look it appears that the variables migrate to these configurations as follows.
theme_themename_settings ==> themename.settings
theme_default ==> system.theme.global
admin_theme ==> system.theme.global
Comment | File | Size | Author |
---|---|---|---|
#42 | 2620364-42.patch | 15.34 KB | bburg |
#42 | interdiff-40-42.txt | 583 bytes | bburg |
#40 | 2620364-40.patch | 15.34 KB | bburg |
#40 | interdiff-38-40.txt | 1.05 KB | bburg |
#38 | 2620364-38.patch | 15.31 KB | bburg |
Comments
Comment #2
quietone CreditAttribution: quietone commentedThis will migrate the default and admin theme settings. Although posted here, for now, it probably can be included in the patch in the parent issue.
Comment #3
quietone CreditAttribution: quietone commentedThis patch isn't complete, there aren't any assertions in the migration test. That's because it isn't writing data to the destination configuration due to a 'no schema' error.
/2620364No schema for seven.settings (/opt/sites/md82/core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php:91)
It is true that there isn't a schema for seven.settings, but theme settings form writes to a configuration with that name.
And the process plugin is named Explode and not ThemeSettings because it only does an explode and it can also be used in the Color migration. But I left it in the system module since I'm not sure if it should go into migrate or not.
Comment #5
quietone CreditAttribution: quietone commentedJust needed to install some themes. Added the insertions and some cleanup as well.
Comment #7
quietone CreditAttribution: quietone commentedModified to use the explode process plugin from #2575101: Add an explode/separator process plugin.
Postponing on that issue.
Comment #8
quietone CreditAttribution: quietone as a volunteer commented#2575101: Add an explode/separator process plugin is in, so no longer postponed.
Changed to needs review for the testbot.
Comment #10
quietone CreditAttribution: quietone as a volunteer commentedNow that explode returns multiple true, extract and get try to iterate over an array in Processrow. To avoid that handle_multiples is set to True in each, this may be wrong, but I want to see what errors are found.
Comment #12
quietone CreditAttribution: quietone as a volunteer commentedBad patch, I got interrupted and messed it up.
Again.
Comment #13
quietone CreditAttribution: quietone as a volunteer commentedAs advised by chx, created new issues and postponing this.
Postponed on #2675164: Pipeline using explode and concat fails and #2675156: Pipeline using explode and extract fails.
Comment #14
quietone CreditAttribution: quietone as a volunteer commentedComment #18
mikeryanComment #19
quietone CreditAttribution: quietone as a volunteer commentedRerolled. Moved source plugin test to MigrateSqlSourceTestBase, made the process of getting the configuration name a two step process, the same as for the color migration.
Comment #20
jofitz CreditAttribution: jofitz at ComputerMinds commentedTypo? Should this say "to the"?
Otherwise this looks great to me - just one small step away from RTBC.
Comment #21
amoebanath CreditAttribution: amoebanath at ComputerMinds commentedPatch :)
Comment #22
yogeshmpawar@amoebanath - you have added wrong patch, I have added a correct patch :)
Comment #23
jofitz CreditAttribution: jofitz at ComputerMinds commentedGreat, thanks both.
Comment #25
jofitz CreditAttribution: jofitz at ComputerMinds commentedRetests passed successfully - back to RTBC.
Comment #27
jofitz CreditAttribution: jofitz at ComputerMinds commentedTests are green - back to RTBC.
Comment #28
yogeshmpawarAny Updates on this issue ?
Comment #29
catchThis definitely doesn't construct a BlockedIP object. c&p error?
If the expected results are identical to the source data, why not do
$tests['0]['expected_data'] = $tests[0]['source_data']['variable'];
instead of duplicating the array manually?Comment #30
jofitz CreditAttribution: jofitz at ComputerMinds commentedAdjustments suggested in #29.
Comment #32
jofitz CreditAttribution: jofitz at ComputerMinds commentedAha, apparently the source data and expected results are nearly identical.
Adjusted the code to make the similarities/differences clear (and actually pass the test this time!)
Comment #33
phenaproximaLooks good. Back to RTBC.
Comment #34
Gábor HojtsyMostly just found minor things.
Make this a sentence IMHO.
Comment standards need a . at the end IMHO.
I guess we need this for the interface, can we document why we don't implement it?
Hm, if its inheritdoc, why are we defining it again? (If we need to, we need a newline above).
Comment #35
heddnTagging for the response to #34.
Comment #36
bburgComment #37
bburgLooks like the {@inheritdoc} in #3 is referring to the method defined in the MigrateDestinationInterface interface. I'm no idea why the method is just a stub. Is it because we are migrating the equivalent of Drupal variables, and there aren't any fields? Should it return an empty array?
I don't see where $variables in #4 is defined or used. Where did this come from?
Comment #38
bburgPatch for 1 and 2 only for #34.
Comment #39
phenaproximaI looked into #34.3 and #34.4:
Once these two points are done, I think this is RTBC.
Comment #40
bburgphenaproxima's suggestions applied. I used your comment for #34.3 verbatim.
I realized tha I was working off the 8.4.x branch. Everything seemed to apply cleanly though, we'll see if the tests complete.
Comment #41
phenaproximaOne small thing -- the coding standards now require us to use short array syntax in core. So this should be [], not array().
Other than that, this looks perfect.
Comment #42
bburgAh, thanks for the catch. Updated patch included.
Comment #43
bburgComment #44
phenaproximaWerd up. #40 passed Drupal CI, so I am pre-RTBCing #42 since it was just a syntax change. Nice one.
Comment #47
catchCommitted/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!