Migrate D7 global theme settings variable, theme_settings, to configuration, system.theme.global.

CommentFileSizeAuthor
#29 2621798-29.patch4.89 KBjofitz
#29 interdiff_25-29.txt895 bytesjofitz
#25 interdiff-23-25.txt1.36 KByogeshmpawar
#25 2621798-25.patch4.9 KByogeshmpawar
#23 interdiff-2621798-17-23.txt1.15 KBkekkis
#23 2621798-23.patch4.92 KBkekkis
#17 interdiff.txt3.36 KBquietone
#17 2621798-17.patch4.92 KBquietone
#15 interdiff.txt2.22 KBquietone
#15 2621798-15.patch4.98 KBquietone
#6 2621798-6.patch4.29 KBhussainweb
#6 interdiff-5-6.txt2.04 KBhussainweb
#5 2621798-5.patch5.65 KBhussainweb
#5 interdiff-2-5.txt717 byteshussainweb
#2 2621798-2.patch5.64 KBquietone

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone commented
Status: Active » Needs review
StatusFileSize
new 2621798-2.patch5.64 KB

And a patch.

benjy’s picture

benjy commented 
+++ b/core/modules/system/src/Plugin/migrate/destination/d7/GlobalThemeSettings.php
@@ -0,0 +1,49 @@
+ * @MigrateDestination(
...
+class GlobalThemeSettings extends DestinationBase {

Why do we need a custom destination here? Can we not use the config destination?

quietone’s picture

quietone commented

The existing method, theme_settings_convert_to_config seems a more robust option. It was written specifically for converting D7 theme settings to config. It was likely looked at by those who know theming better than me and it does some minor checking of the data that would not be done if using the config destination.

/**
 * Converts theme settings to configuration.
 *
 * @see system_theme_settings_submit()
 *
 * @param array $theme_settings
 *   An array of theme settings from system setting form or a Drupal 7 variable.
 * @param Config $config
 *   The configuration object to update.
 *
 * @return
 *   The Config object with updated data.
 */
function theme_settings_convert_to_config(array $theme_settings, Config $config) {
hussainweb’s picture

hussainweb
Primary language English
commented
StatusFileSize
new interdiff-2-5.txt717 bytes
new 2621798-5.patch5.65 KB

I think $config->save(); was being called twice. I fixed that here and also return an id in consistency with Config destination base.

hussainweb’s picture

hussainweb
Primary language English
commented
StatusFileSize
new interdiff-5-6.txt2.04 KB
new 2621798-6.patch4.29 KB

I am thinking that we could still do this with just the config plugin. I saw the theme_settings_convert_to_config and don't see it doing any checks, but just translating the keys to D8 equivalents (and skipping logo_upload. I commented out logo_upload in the yml for that reason. Let's see what the tests say.

quietone’s picture

quietone commented
Issue tags: +migrate-d7-d8

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

benjy’s picture

benjy commented
Status: Needs review » Reviewed & tested by the community

Looks good.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/system/migration_templates/d7_global_theme_settings.yml
@@ -0,0 +1,29 @@
+#  logo_upload: theme_settings/logo_upload

Why is this commented out?

hussainweb’s picture

hussainweb
Primary language English
commented

@catch: I don't exactly remember why. I checked the theme settings again and don't see a logo/upload setting.

logo:
  path: ''
  url: ''
  use_default: true

I am not sure if logo/url is the same as logo/upload. I'll have to check that in detail which I could do later. But someone else is welcome to pick it up now. I am not sure if I will get time this week.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Version: 8.1.x-dev » 8.2.x-dev

Resurrecting, submitted an 8.2.x test. Given the last patch was 8.0.x, this will almost certainly need a reroll, at least for the fixture.

Status: Needs review » Needs work

The last submitted patch, 6: 2621798-6.patch, failed testing.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented

Surprisingly, the patch still applies - it needs to add the new migration ID to the list in MigrateUpgradeForm, though.

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 2621798-15.patch4.98 KB
new interdiff.txt2.22 KB

Add this migration to MigrateUpgradeForm.

I did a test to see how logo_upload is used. Before uploading an logo file these are the values in the variable.

s:9:"logo_path";s:0:"";
s:11:"logo_upload";s:0:"";

After uploading an image the values are:

s:9:"logo_path";s:38:"core/modules/simpletest/files/logo.png";
s:11:"logo_upload";s:0:"";

So, no change at all. And as pointed out in #6 logo_upload isn't converted in theme_settings_convert_to_config.

I didn't dive into the D6 code.

Status: Needs review » Needs work

The last submitted patch, 15: 2621798-15.patch, failed testing.

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 2621798-17.patch4.92 KB
new interdiff.txt3.36 KB

Moved the test to Drupal\Tests\system\Kernel\Migrate\d7.

phenaproxima’s picture

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

I'm not seeing any red flags here...

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Version: 8.2.x-dev » 8.3.x-dev

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2621798-17.patch, failed testing.

svendecabooter’s picture

svendecabooter
he/him
Primary language Dutch
Location Gent
commented
Status: Needs work » Needs review

The failing test seems unrelated:
cURL error 28: Operation timed out after 30001 milliseconds with 0 bytes received (see http://curl.haxx.se/libcurl/c/libcurl-errors.html)

Testing again against 8.x-3.x

kekkis’s picture

kekkis
he/him
Primary language Finnish
Location Pirkkala
commented
Assigned: Unassigned » kekkis
Status: Needs review » Needs work

Patch #17 applies with offset.

kekkis’s picture

kekkis
he/him
Primary language Finnish
Location Pirkkala
commented
Assigned: kekkis » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 2621798-23.patch4.92 KB
new interdiff-2621798-17-23.txt1.15 KB

Patch rerolled.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work 
+++ b/core/modules/system/tests/src/Kernel/Migrate/d7/MigrateGlobalThemeSettingsTest.php
@@ -0,0 +1,49 @@
+    $this->assertIdentical('image/png', $config->get('favicon.mimetype'));
+    $this->assertIdentical('public://somefavicon.png', $config->get('favicon.path'));

Use assertSame(), not assertIdentical(). Once this is fixed, it's RTBC.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
StatusFileSize
new 2621798-25.patch4.9 KB
new interdiff-23-25.txt1.36 KB

Rerolled the patch against #24, also posted a interdiff.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Status: Needs work » Needs review
phenaproxima’s picture

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

Excellent.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

The functionality looks great - just one very minor thing.

+++ b/core/modules/system/migration_templates/d7_global_theme_settings.yml
@@ -0,0 +1,29 @@
+#  logo_upload: theme_settings/logo_upload

This needs explaining why it is commented out - is it the same reason as the others? I think it is good to include these to explain what is not migrated.

jofitz’s picture

jofitz
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_25-29.txt895 bytes
new 2621798-29.patch4.89 KB

Moved commented line because, as expected, it is not migrated.

phenaproxima’s picture

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

I think this looks OK.

  • catch committed 50e815d on 8.3.x 
    Issue #2621798 by quietone, hussainweb, kekkis, Yogesh Pawar, Jo...

  • catch committed 598eac9 on 8.2.x 
    Issue #2621798 by quietone, hussainweb, kekkis, Yogesh Pawar, Jo...
catch’s picture

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

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

Status: Fixed » Closed (fixed)

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