Convert search settings to configuration system

Here's a first stab at this.

There is some missing white space

$limit = (int)config('search.settings')->get('cron_limit');

should be (This happens in number of places)

$limit = (int) config('search.settings')->get('cron_limit');

You can remove the extra white space that you have in your xml eg

<overlap_cjk> TRUE </overlap_cjk>

should be

<overlap_cjk>TRUE</overlap_cjk>

Also each sub tag should be idented 2 characters

eg

<config>
  <minimum_word_size>3</minimum_word_size>
</config>

You can't have 2 keys with the same name

+<active_modules> node </active_modules>
+<active_modules> user </active_modules>

Instead we need something like

<active_modules>
  <node_module>node</node_module>
  <user_module>user</user_module>
</active_modules>

There are some layout issue in the update hook

$variable_names = array(
+            'minimum_word_size' => 'minimum_word_size',
+            'overlap_cjk' => 'overlap_cjk',
+            'sample_search_force_keywords' => 'sample_force_keywords',
+            'search_active_modules' => 'active_modules',
+            'search_and_or_limit' => 'and_or_limit',
+            'search_cron_limit' => 'cron_limit',
+            'search_default_module' => 'default_module',
+            'search_embedded_form_submitted' => 'embedded_form_submitted',
+            'search_tag_weights' => 'tag_weights',
+    );

Okay, fixed the whitespace issues. Haven't done anything about the issues with the upgrade path yet, but let's see what this does.

should actually attach a patch.

Oops. This one should apply.

The latest patch seems to be missing the xml file but all the white space problems look like they are fixed and changes seem good. Leaving for needs to review for the bot but I am assuming the lack of the config file is going to use failures.

Uploading again to see if it gets picked up by the test bot this time

With the config file

Fixed up the search tests some.

#13: search-1496510-13.patch queued for re-testing.

Converting to new helper update hook.

Hopefully fixes some issues

+++ b/core/modules/search/config/search.settings.xmlundefined
@@ -0,0 +1,28 @@
+	<user>user</user>

Whitespace

+++ b/core/modules/search/search.admin.incundefined
@@ -50,13 +50,18 @@ function _search_get_module_names() {
+  $active_modules = config('search.settings')->get('active_modules');
+  if (!empty($active_modules)) {
+    foreach (config('search.settings')->get('active_modules') as $module) {

You can reuse $active_modules variable again instead of calling config() again.

+++ b/core/modules/search/search.admin.incundefined
@@ -128,10 +134,13 @@ function search_admin_settings($form) {
+  $active_modules = $config->get('active_modules');
+  if (!empty($active_modules)) {
+    foreach ($config->get('active_modules') as $module) {

Reuse $active_modules instead of calling with $config again.

+++ b/core/modules/search/search.admin.incundefined
@@ -156,13 +165,18 @@ function search_admin_settings_validate($form, &$form_state) {
+	$config->set('overlap_cjk', $form_state['values']['overlap_cjk']);

Whitespace

+++ b/core/modules/search/search.admin.incundefined
@@ -171,9 +185,15 @@ function search_admin_settings_submit($form, &$form_state) {
+	  $enabled_modules[$module] = $module;
+	}
+	$config->set('active_modules', $enabled_modules);

Whitespace issues

-29 days to next Drupal core point release.

New patch fixing comments above and fixing up some tests. Still having an issue with the config folder not being created as part of the upgrade test, which is the main blocker to last test passing as far as I can tell.

Giving to the bot to see where this leaves us.

This will make the tests pass.

It needed a change it config.inc to make sure the simpletest directory existed. The same fix is also used in #1496458: Convert maintenance mode settings to configuration system, so as this or the other is committed (with that fix or another way than the current one), this or the other patch needs to be updated.

Ah that makes sense but shouldn't we be using

file_prepare_directory($path, FILE_CREATE_DIRECTORY);

instead of direct calls to file_exists and mkdir.

Absolutely, good catch!

Meh, that's what you getting trying to manually update a patch :)

Looks good. Tempted to mark this rtbc. But if maintenance gets committed first this needs a reroll.

#1517138: SimpleTest does not create configuration folder causing test failures has now been committed to get round the folder issue this likely needs a re-roll.

New patch

Reroll is just minus the test folder element so happy to RTBC.

+function search_update_8000() {
+  config_install_default_config('search');

This makes me uncomfortable. What happens if default config for search changes during the Drupal 8 lifecycle? Seems like it should explicitly lay out the default configuration as it is now, install that, then it'll be guaranteed to be the same regardless of when any given 8.x site gets upgraded.

This patch rerolls the previous patch and includes the edits called for in #37. Also, I applied a few coding standards that have been pointed out to me in the past. Namely use the fluid syntax when possible on store the config object in a variable whenever that variable can be reused.

Found my mistake, new patch.

Fixed the fails by discovering that one of the config gets was using the wrong key name. Also changed the way the incrementing was coded to be absolutely sure that the number of submits is incrementing.

I think an htaccess diff has crept into the patch.

Also I think we can make a minor code style improvement:

+  $active_modules = $config->get('search_active_modules');
+  if (!empty($active_modules)) {
+    foreach ($active_modules as $module) {

should be

+  $active_modules = $config->get('search_active_modules');
+  foreach ($active_modules as $module) {

Basically checking if $active_modules is empty is unnecessary... There are two instances of this. To prove the point, there is other code in this patch the makes the assumption the $config->get('search_active_modules') will return an array - such as:

+  foreach (config('search.settings')->get('search_active_modules') as $module) {

Thanks Alex, this patch fixes that.

Now that you do this:

-  foreach (variable_get('search_active_modules', array('node', 'user')) as $module) {
+  foreach (config('search.settings')->get('search_active_modules') as $module) {

You can't use $active_modules here....

   // Per module settings
-  foreach (variable_get('search_active_modules', array('node', 'user')) as $module) {
-    $added_form = module_invoke($module, 'search_admin');
-    if (is_array($added_form)) {
-      $form = array_merge($form, $added_form);
+  if (!empty($active_modules)) {
+    foreach ($active_modules as $module) {
+      $added_form = module_invoke($module, 'search_admin');
+      if (is_array($added_form)) {
+        $form = array_merge($form, $added_form);
+      }
     }
   }

I suggest you make the following change:

   // Per module settings
-  foreach (variable_get('search_active_modules', array('node', 'user')) as $module) {
-    $added_form = module_invoke($module, 'search_admin');
-    if (is_array($added_form)) {
-      $form = array_merge($form, $added_form);
+  foreach (config('search.settings')->get('search_active_modules') as $module) {
+    $added_form = module_invoke($module, 'search_admin');
+    if (is_array($added_form)) {
+      $form = array_merge($form, $added_form);
+    }
    }

Unassign not enough time available

Taking

Patch attached. config file is now in YAML and converted the variable names, let's hope this turns green!

Talked this through first a bit with alexpott on IRC and at first we thought splitting these up in search.settings and search.indexing. However, that turned out to be a little awkwardly and overcomplicated, especially in the admin form.

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchEmbedFormTest.phpundefined
@@ -43,7 +43,7 @@ class SearchEmbedFormTest extends SearchTestBase {
+    config('search.settings')->set('search_embedded_form_submitted', $this->submit_count)->save();

search_embed_form_submitted is kinda verbose. And don't we need a default value? (or is it not needed in this test)

+++ b/core/modules/search/search.admin.incundefined
@@ -50,10 +50,12 @@ function _search_get_module_names() {
 function search_admin_settings($form) {

Don't we need to pass $form_state (for config_settings_form) ?

-13 days to next Drupal core point release.

New patch with $form_state variable

The default value of search_embed_form_submitted is set in the test itself, no need to have a default value for that for core itself as it's not used.

do we no longer need to modify the search_update_8000 function? I still find the variables we're modifying elsewhere there.

I think I might need to catch up with the documentation on this.

This looks good to me.

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchEmbedFormTest.phpundefined
@@ -67,7 +67,7 @@ class SearchEmbedFormTest extends SearchTestBase {
-    $count = variable_get('search_embedded_form_submitted', 0);
+    $count = config('search.settings')->get('search_embedded_form_submitted');

I don't see these in the search.settings. Also, these appear to be test related search variables so do we want them there? Third, should we drop the 'search_' prefix?

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchKeywordsConditionsTest.phpundefined
@@ -26,8 +26,7 @@ class SearchKeywordsConditionsTest extends SearchTestBase {
     // Test with all search modules enabled.
-    variable_set('search_active_modules', array('node' => 'node', 'user' => 'user', 'search_extra_type' => 'search_extra_type'));
-    menu_router_rebuild();
+    config('search.settings')->set('active_modules', array('node' => 'node', 'user' => 'user', 'search_extra_type' => 'search_extra_type'))->save();    menu_router_rebuild();

Missing return before menu_router_rebuild().

New patch. Fixed the return. Dropped the 'search_' prefix as well.
I changed search.settings to search.testing as the embedded variable is indeed only used in testing.
However, really strictly speaking, I think 'embedded_form_submitted' a state, thus not belonging to the config system, so should we wait untill #1175054: Add a storage (API) for persistent non-configuration state gets in ?

All config system conversions are API changes, so tagging as such.

From another cmi issue

Additionally, update_variables_to_config() "installs" (merges) the default configuration of a module already (if appropriate), so this call to config_install_default_config() should be removed.

New patch without update_variables_to_config()

Is that correct? Since the update_variables_to_config() is in an update hook it seems appropriate.

Sorry, patch is without the config_install_default_config() function :)

That's what I mean. I think that the patch needs the update_variables_to_config function. I've seen that function in other update hooks.

However, your patch went green. so........

Thanks for working on this! :)

+++ b/core/modules/search/config/search.settings.yml
@@ -0,0 +1,21 @@
+minimum_word_size: 3
+overlap_cjk: 1
+cron_limit: 100
+tag_weights:
+    h1: 25
+    h2: 18
+    h3: 15
+    h4: 14
+    h5: 9
+    h6: 6
+    u: 3
+    b: 3
+    i: 3
+    strong: 3
+    em: 3
+    a: 10
+active_modules:
+    node: node
+    user: user
+and_or_limit: 7
+default_module: 'node'

I wonder whether there is some room for improving the keys? It looks like a good chunk of them belongs to indexing, and another chunk to searching.

So how about this?

index:
    minimum_word_size: '3'
    overlap_cjk: '1'
    cron_limit: '100'
    tag_weights:
        h1: '25'
        ...
search:
    active_modules:
        - node
        - user
    and_or_limit: '7'
    default_module: node

Would that make sense?

Please also note that all non-strings need to be enclosed in single-quotes. All strings are not quoted.

+++ b/core/modules/search/config/search.settings.yml
@@ -0,0 +1,21 @@
+active_modules:
+    node: node
+    user: user

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchKeywordsConditionsTest.php
@@ -26,7 +26,7 @@ class SearchKeywordsConditionsTest extends SearchTestBase {
+    config('search.settings')->set('active_modules', array('node' => 'node', 'user' => 'user', 'search_extra_type' => 'search_extra_type'))->save();

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchPageOverrideTest.php
@@ -29,7 +29,7 @@ class SearchPageOverrideTest extends SearchTestBase {
+    config('search.settings')->set('active_modules', array('node' => 'node', 'user' => 'user', 'search_extra_type' => 'search_extra_type'))->save();

+++ b/core/modules/search/search.admin.inc
@@ -49,11 +49,13 @@ function _search_get_module_names() {
-  foreach (variable_get('search_active_modules', array('node', 'user')) as $module) {
+  foreach (config('search.settings')->get('active_modules') as $module) {

It looks like this should be an indexed array instead of an associative/hashmap (and actually has been that way before); i.e.,

active_modules:
    - node
    - user

+++ b/core/modules/search/search.admin.inc
@@ -49,11 +49,13 @@ function _search_get_module_names() {
+function search_admin_settings($form, $form_state) {

&$form_state needs to be taken by reference.

+++ b/core/modules/search/search.admin.inc
@@ -156,24 +158,38 @@ function search_admin_settings_validate($form, &$form_state) {
+  if ($config->get('cron_limit') != $form_state['values']['cron_limit']) {
+    $config->set('cron_limit', $form_state['values']['cron_limit']);
+  }
+  if ($config->get('default_module') != $form_state['values']['default_module']) {
+    $config->set('default_module', $form_state['values']['default_module']);
+  }

For these two, we do not have to check the existing value and can just simply set the new value.

+++ b/core/modules/search/search.api.php
@@ -69,7 +69,7 @@ function sample_search_conditions_callback($keys) {
-  if ($force_keys = variable_get('sample_search_force_keywords', '')) {
+  if ($force_keys = config('search.settings')->get('sample_search_force_keywords')) {

The idea of this API hook example code is to demonstrate a module that integrates with Search module, so the sample config object actually belongs to sample_search.module; i.e.:

config('sample_search.settings')->get('force_keywords')

+++ b/core/modules/search/tests/modules/search_embedded_form/search_embedded_form.module
@@ -30,7 +30,7 @@ function search_embedded_form_menu() {
+  $count = config('search.testing')->get('embedded_form_submitted');

@@ -56,8 +56,10 @@ function search_embedded_form_form($form, &$form_state) {
+  $search_config = config('search.testing');

The config object name needs to be in search_embedded_form's namespace; i.e.:

config('search_embedded_form.settings')->get('submitted')

I'm off for a few days - so if anyone else wants to re-roll with sun's remarks, feel free! If it's not done by wednesday, I'll take it back then!

Made changes proposed in #63 and did a couple of code tidy ups.

re: search_embedded_form.module and it's config value "submitted"... I think this really is state but in absence of the state system and the fact that this is a test module I've converted it anyway.

#65: 1496510-65.search_cmi.patch queued for re-testing.

Personally this looks to verbose
$limit_combinations = config('search.settings')->get('search.and_or_limit');

I understand the reason for grouping stuff in "index" but why group again in search. Everything not grouped is part of "search" by default. No need to create an extra layer.

Looking at this again I agree with @aspilicious - the yml file should look like this:

active_modules:
    - node
    - user
and_or_limit: '7'
default_module: node
index:
    cron_limit: '100'
    overlap_cjk: '1'
    minimum_word_size: '3'
    tag_weights:
        h1: '25'
        h2: '18'
        h3: '15'
        h4: '14'
        h5: '9'
        h6: '6'
        u: '3'
        b: '3'
        i: '3'
        strong: '3'
        em: '3'
        a: '10'

Do I need to reroll this patch with the changes is #69?

@cosmicdreams: The answer to such questions is always "yes." :-D

Rerolled patch with changes suggested by @aspilicious

AWESOME, friends! Thank you so much for getting this done! :)

Rock! So great to have another one of these off our plate! :D

Committed and pushed to 8.x. Thanks!