Problem/Motivation

As a site owner migrating my site from Drupal 7 running search404-7.x-1.xx to Drupal 8 or 9 running search404-2.0.0, I want to migrate configuration from search404, so that my Drupal 8/9 site has the same behavior as my Drupal 7 site.

Proposed resolution

Add migrations.

Remaining tasks

  1. Write a patch
  2. Review and feedback
  3. RTBC and feedback
  4. Commit
  5. Release

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#12 interdiff--9-12.txt1.48 KBomkar.podey
#12 3199236-12--migrate-config-from-d6-and-d7.patch20.36 KBomkar.podey
#9 interdiff--5-9.txt5.68 KBomkar.podey
#9 3199236-9--migrate-config-from-d6-and-d7.patch20.87 KBomkar.podey
#5 interdiff--3-5.txt4.44 KBomkar.podey
#5 3199236-5--migrate-config-from-d6-and-d7.patch22.44 KBomkar.podey
#3 interdiff--2-3.patch1.01 KBmparker17
#3 3199236-3--migrate-config-from-d6-and-d7.patch23.36 KBmparker17
#2 3199236-2--migrate-config-from-d6-and-d7.patch23.38 KBmparker17

Comments

mparker17 created an issue. See original summary.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 3199236-2--migrate-config-from-d6-and-d7.patch23.38 KB

Here's a patch. Reviews welcome.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
StatusFileSize
new 3199236-3--migrate-config-from-d6-and-d7.patch23.36 KB
new interdiff--2-3.patch1.01 KB

Whoops; accidentally left a reference to environment_indicator (the module I most recently wrote a settings migration for) in the migration files here.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

Note my patch adds automated tests, but it looks like automated testing is not enabled for this project... a module maintainer(s) could enable automated tests for the 4.x branch by...

  1. Go to the project page and clicking the Automated testing tab
  2. Click `Add test / retest` under the "2.x-dev" release
  3. Enter the following information on the "Add branch test" page...
    • Environment = PHP 7.3 & MySQL 5.7
    • Core = Drupal 9 Supported, currently 9.1.x
    • Schedule = Run on commit and for issues
    • Click Save & queue
omkar.podey’s picture

omkar.podey commented
StatusFileSize
new 3199236-5--migrate-config-from-d6-and-d7.patch22.44 KB
new interdiff--3-5.txt4.44 KB

Added some mappings for complete migration , updated tests.

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Needs review » Needs work

Very nice. 👍 . Data is getting migrated from D7 to D9, tested manually.

Some questions:

  1. +++ b/migrations/d6_search404_settings.yml
@@ -0,0 +1,71 @@
+    - search404_do_fuzzysearch

    What is the use of getting variable if we are not using it. 

  2. +++ b/migrations/d6_search404_settings.yml
@@ -0,0 +1,71 @@
+  search404_custom_error_message:
+    # Note the 6.x-1.x branch doesn't have this setting; so we use the default
+    # value from config/install/search404.settings.yml.
+    - plugin: default_value
+      default_value: ''
+  search404_custom_search_path: search404_custom_search_path

    Is it necessary to set default value in this case? 

  3. +++ b/migrations/d7_search404_settings.yml
@@ -0,0 +1,60 @@
+  search404_page_redirect:
+    # Note the 7.x-1.x branch doesn't have this setting; so we use the default
+    # value from config/install/search404.settings.yml.
+    - plugin: default_value
+      default_value: ''

    Same as above 

  4. +++ b/tests/src/Kernel/ValidateD6SettingsMigrationTest.php
@@ -0,0 +1,125 @@
+    $this->setUpD6Variable('search404_do_google_cse_adv', $fixtureDoGoogleCseAdv);

    This migrating is not tested, can be removed.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

@omkar.podey asked for clarifications on #6. Here they are:

  1. Indeed, we can just delete that line 👍
  2. Indeed, all these quotes lines can be deleted, because this default will already have been set when the module got installed, thanks to https://git.drupalcode.org/project/search404/-/blob/8.x-1.0/config/insta...
  3. See point 2.
  4. Indeed, this line (and the next one) for mocked a D6 variable can be deleted, because we're not going to be doing any assertions on them anyway.
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

From my perspective,

1. Agreed, delete. That was a mistake on my part.
2. and 3. I may be mis-remembering, but I thought I recall getting errors from modules when variables that are required in the D9 version and don't have a D6/D7 version are not set to their default values during the migration. I seem to recall finding out that the migration wizard overwrites the configuration objects set when the module gets installed, instead of modifying them. But, the behaviour could also have changed since I tried this last.
4. Agreed, delete.

***

I probably won't be able to get around to testing or writing a follow-up patch until December 27th, 2021 at the earliest - I must take time off from my regular job to work on the small-business website I'm trying to migrate from D7 to D9, and that's the first day I can do so.

omkar.podey’s picture

omkar.podey commented
StatusFileSize
new 3199236-9--migrate-config-from-d6-and-d7.patch20.87 KB
new interdiff--5-9.txt5.68 KB

Updated as per review , fixed tests.

omkar.podey’s picture

omkar.podey commented
Status: Needs work » Needs review

Updated as per review , fixed tests.

narendrar’s picture

narendrar
He/Him
Primary language English
Location Jaipur, India
commented
Status: Needs review » Needs work

Great 👍

Probably last thing from my side: tests/src/Kernel/ValidateD6MigrationStateTest.php is failing on my machine.

omkar.podey’s picture

omkar.podey commented
Related issues: +#3255410: Update schema type from "mapping" to "config_object"
StatusFileSize
new 3199236-12--migrate-config-from-d6-and-d7.patch20.36 KB
new interdiff--9-12.txt1.48 KB

Removing schema check exclusions in favour of #3255410: Updating schema type to config_object. , which is required for the tests to pass.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

I think #12 is ready for the D7 → D8|9 migration.

It'd be great if the maintainer of this module would enable automated tests for this project so that we could get consistent test runs here on d.o 😊🤞

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

I've tested #12 and I can confirm it migrates my D7 site settings properly without errors.

Indeed, all these quotes lines can be deleted, because this default will already have been set when the module got installed, thanks to https://git.drupalcode.org/project/search404/-/blob/8.x-1.0/config/insta...

I thought I recall getting errors from modules when variables that are required in the D9 version and don't have a D6/D7 version are not set to their default values during the migration. I seem to recall finding out that the migration wizard overwrites the configuration objects set when the module gets installed, instead of modifying them. But, the behaviour could also have changed since I tried this last.

This is apparently no longer an issue - search404_page_redirect does not appear in the patch in #12 but I can confirm that it has its default value after migration on my site. So changes #6.2 and #6.3 are warranted.

Since I had a hand in creating the original patch, I can't mark #12 as RTBC - but I would if I could.

Thanks everyone!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

Yay, #3255410: Update schema type from "mapping" to "config_object" landed, which means this is unblocked!

We've been running with #12 in production on many migrations for the past ~10 months, so … RTBC!

  • emilymathew committed 7a7050c on 2.x 
    Issue #3199236 by omkar.podey, mparker17, Wim Leers, narendraR: Migrate...
emilymathew’s picture

emilymathew commented
Status: Reviewed & tested by the community » Fixed

Thank you Wim Leers, omkar.podey, mparker17, narendraR for your contribution.

Status: Fixed » Closed (fixed)

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