Search Settings (d7_search_settings) migration is ignored if any of the variables it would use is missing.

Re #3:

Only one nit (the first point).

1. +++ b/core/modules/search/migrations/d7_search_settings.yml
   @@ -8,19 +8,51 @@ source:
      variables_no_row_if_missing:
   +    - search_default_module
   +  variables:
        - minimum_word_size
        - overlap_cjk
        - search_cron_limit
        - search_tag_weights
        - search_and_or_limit
   -    - search_default_module
   

   search_default_module also should be a "simple" variable:

   https://git.drupalcode.org/search?search=variable_get%28%27search_defaul...

   So please add default_value: node to the default_page process pipeline's static_map configuration, and move search_default_module to the variables key in the source plugin's config!

2. +++ b/core/modules/search/migrations/d7_search_settings.yml
   @@ -8,19 +8,51 @@ source:
   +  'index/minimum_word_size':
   +    plugin: default_value
   +    source: minimum_word_size
   +    strict: true
   +    default_value: 3
   

   👍minimum_word_size is 3 in Drupal 7: https://git.drupalcode.org/search?search=variable_get%28%27minimum_word_...

3. +++ b/core/modules/search/migrations/d7_search_settings.yml
   @@ -8,19 +8,51 @@ source:
   +  'index/overlap_cjk':
   +    plugin: default_value
   +    source: overlap_cjk
   +    default_value: true
   

   👍 overlap_cjk's default value is TRUE in Drupal 7: https://git.drupalcode.org/search?search=variable_get%28%27overlap_cjk%2...

4. +++ b/core/modules/search/migrations/d7_search_settings.yml
   @@ -8,19 +8,51 @@ source:
   +  'index/cron_limit':
   +    plugin: default_value
   +    source: search_cron_limit
   +    strict: true
   +    default_value: 100
   ...
   +  and_or_limit:
   +    plugin: default_value
   

   👍 search_cron_limit's default value is 100 in Drupal 7: https://git.drupalcode.org/search?search=variable_get%28%27search_cron_l...

5. +++ b/core/modules/search/migrations/d7_search_settings.yml
   @@ -8,19 +8,51 @@ source:
   +  'index/tag_weights':
   +    plugin: default_value
   +    source: search_tag_weights
   +    strict: true
   +    default_value:
   +      h1: 25
   +      h2: 18
   +      h3: 15
   +      h4: 12
   +      h5: 9
   +      h6: 6
   +      u: 3
   +      b: 3
   +      i: 3
   +      strong: 3
   +      em: 3
   +      a: 10
   

   👍 This default value also matches Drupal 7's search_tag_weights default value: https://git.drupalcode.org/project/drupal/-/blob/bb5b229a4f845a18ef18895...

6. +++ b/core/modules/search/migrations/d7_search_settings.yml
   @@ -8,19 +8,51 @@ source:
   +  and_or_limit:
   +    plugin: default_value
   +    source: search_and_or_limit
   +    strict: true
   +    default_value: 7
   

   👍 search_and_or_limit's default value is 7 in Drupal 7: https://git.drupalcode.org/search?search=variable_get%28%27search_and_or...

Review of #6

+++ b/core/modules/search/migrations/d7_search_settings.yml
@@ -16,11 +16,42 @@ source:
   default_page:
     plugin: static_map
@@ -29,6 +60,7 @@ process:

@@ -29,6 +60,7 @@ process:
     map:
       node: node_search
       user: user_search
+    default_value: node

Although the Drupal 7 default value of search_default_module is node, it should be mapped to node_search.

Nit: could you please provide an interdiff next time?

Tested manually and found that 2 settings are not migrated from D7 if not checked.

• overlap_cjk
• search_logging

This is because their default value is set true. But I think if they are not checked in source site, same should be migrated to destination.

Tested manually and settings are migrated properly. 👍

I am not sure about this.

This patch is adding default values from Drupal 7 and I think there are cases where we don't do that and instead take the default values of the destination site. It is too late for me to investigate further on that. If the IS is correct and the implementation of #3151993: Search settings migration (d7_search_settings) assumes that the search_default_module variable is always set is incorrect, then maybe we should revert that and fix the original problem.

Assigning to myself to review.

@quietone do you still plan to review this?

Oops, forgot about this one. Un-assigning myself so mikelutz can review.

I think this change is mostly fine. On my wishlist for the migrate system is a way for a migration to dynamically remove properties from the destination during processing, such that it would be unnecessary to hard code defaults here, and a non-existant destination property could simply pick up the default config from D9, but we don't currently have that. At best the property has to be null, and null is still a value that will override the default, so I don't see a way around hard coding some default in situations like this.

This does need more dedicated tests though. while the current test included with this patch gives me some confidence it's working right now, it needs to be expanded at least a little.

Currently the variable you delete has the same value in the fixture as the default you are applying, so I can't guarantee in the test that the value is coming from the new default rather than the fixture. For all I know we've deleted the variable incorrectly and the test is still picking up the value from the fixture.

Also, because we've hijacked the existing settings test, we are no longer testing that the cron limit value is actually migrated from the test fixture. The ability to migrate that value could potentially be broken, but because we are only testing the default override, we wouldn't know.

So test wise, we need to restore the original test and add additional testing around this. And we should adjust the fixture and the original test assertions to migrate values other than the new defaults, so we can be sure that when the values DO exist they are actually migrated. Then we need separate tests showing that if the values are deleted that we end up with the defaults.

The last issue is what to do if ALL the variables are missing. the issue description suggests that at least one should exist for us to migrate this, and as it sits, we are migrating even if none of these values exist. This may be okay, the config is created with default d9 values when the search module is installed anyway, and these defaults seem to match, but I'd also like a test showing explicitly what happens if none of them are set.

Just updating tags,

The Migrate Drupal Module was approved for removal in #3371229: [Policy] Migrate Drupal and Migrate Drupal UI after Drupal 7 EOL.

This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

The deprecation work is in #3522602: [meta] Tasks to remove Migrate Drupal module and the removal work in #3522602: [meta] Tasks to remove Migrate Drupal module.

Migrate Drupal will not be moved to a contributed project. It will be removed from core after the Drupal 12.x branch is open.

The Migrate Drupal Module and Migrate Drupal UI are deprecated and they are not in Drupal 12.0.0.

Issues for these modules should now be on the 11.x branch. And the changes are limited to critical and major bug fixes. Other changes are allowed at the discretion of the core Release Managers in consultation with the Migrate subsystem maintainers.