The search settings at admin/config/search/settings need to be converted to use the new configuration system.

Tasks

  • Create a default config file and add it to the search module.
  • Convert the admin UI in search.admin.inc to read to/write from the appropriate config.
  • Convert any code that currently accesses the variables used to use the config system.
  • Write an upgrade path from D7 -> D8 to migrate existing data to the new system and drop the appropriate variables.

I am tagging this config novice but it is actually a bit more complex than the other novice issues. There are a lot more settings and its more far reaching, but if you're feeling brave go for it!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jvns’s picture

Assigned: Unassigned » jvns
jvns’s picture

Status: Active » Needs review
FileSize
20.46 KB

Here's a first stab at this.

Status: Needs review » Needs work

The last submitted patch, search-1496510-1.patch, failed testing.

marcingy’s picture

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',
+    );
jvns’s picture

Status: Needs work » Needs review

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

jvns’s picture

FileSize
1.3 KB

should actually attach a patch.

Status: Needs review » Needs work

The last submitted patch, search-1496510-6.patch, failed testing.

jvns’s picture

Status: Needs work » Needs review
FileSize
19.48 KB

Oops. This one should apply.

marcingy’s picture

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.

jvns’s picture

FileSize
19.48 KB

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

jvns’s picture

FileSize
20.35 KB

With the config file

Status: Needs review » Needs work

The last submitted patch, search-1496510-11.patch, failed testing.

jvns’s picture

Status: Needs work » Needs review
FileSize
20.98 KB

Fixed up the search tests some.

Status: Needs review » Needs work
Issue tags: -Configuration system, -Config novice

The last submitted patch, search-1496510-13.patch, failed testing.

jvns’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +Configuration system, +Config novice

The last submitted patch, search-1496510-13.patch, failed testing.

marcingy’s picture

FileSize
21.1 KB

Converting to new helper update hook.

marcingy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, search-1496510-17.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
22.17 KB

Hopefully fixes some issues

Status: Needs review » Needs work

The last submitted patch, search-cmi.patch, failed testing.

marcingy’s picture

Assigned: jvns » Unassigned
swentel’s picture

+++ 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.

marcingy’s picture

Status: Needs work » Needs review
FileSize
22.24 KB

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.

Status: Needs review » Needs work

The last submitted patch, search-cmi.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
22.71 KB

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.

marcingy’s picture

Status: Needs review » Needs work

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.

swentel’s picture

Status: Needs work » Needs review
FileSize
22.71 KB

Absolutely, good catch!

Status: Needs review » Needs work

The last submitted patch, 1496510-28.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
22.71 KB

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

aspilicious’s picture

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

marcingy’s picture

Status: Needs review » Needs work

#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.

swentel’s picture

Status: Needs work » Needs review
FileSize
22.24 KB

New patch

marcingy’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

Status: Reviewed & tested by the community » Needs work

+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.

gdd’s picture

Status: Needs work » Reviewed & tested by the community

The problem if we do that is then when things change, we have to update it in two places instead of just in the config file, and things are pretty likely to be changing at this point (we're already talking about, for instance, changing the file format.) Until such a time as we are supporting head-to-head upgrades, I don't really consider this a problem.

gdd’s picture

Status: Reviewed & tested by the community » Needs work

One thing I only just realized about this patch is that it removes the 'search_' prefix from all the variable names. I am not sure how I feel about this. We have not been doing it on the other conversion patches, and I would like to keep it consistent. We can attack changing the variable names in a followup if we want.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
20 KB

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.

Status: Needs review » Needs work

The last submitted patch, 1496510_38_search_cmi.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
20 KB

Found my mistake, new patch.

Status: Needs review » Needs work

The last submitted patch, 1496510_40_search_cmi.patch, failed testing.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
22.23 KB

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.

alexpott’s picture

Status: Needs review » Needs work

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) {
cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
21.62 KB

Thanks Alex, this patch fixes that.

Status: Needs review » Needs work

The last submitted patch, 1496510_44_search_cmi.patch, failed testing.

alexpott’s picture

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);
+    }
    }
gdd’s picture

Two things need to happen to this patch now:

- It need to be updated to YAML instead of XML
- The naming needs to be adjusted to match the conventions defined at http://drupal.org/node/1667896

aspilicious’s picture

Assigned: Unassigned » aspilicious
aspilicious’s picture

Assigned: aspilicious » Unassigned

Unassign not enough time available

swentel’s picture

Assigned: Unassigned » swentel

Taking

swentel’s picture

Status: Needs work » Needs review
FileSize
25.65 KB

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.

aspilicious’s picture

+++ 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.

swentel’s picture

FileSize
25.74 KB

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.

cosmicdreams’s picture

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.

Dries’s picture

Status: Needs review » Needs work

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().

swentel’s picture

Status: Needs work » Needs review
FileSize
25.65 KB

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 ?

sun’s picture

Issue tags: +API change

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

aspilicious’s picture

Status: Needs review » Needs work

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.

swentel’s picture

Status: Needs work » Needs review
FileSize
25.61 KB

New patch without update_variables_to_config()

cosmicdreams’s picture

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

swentel’s picture

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

cosmicdreams’s picture

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........

sun’s picture

Status: Needs review » Needs work

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')
swentel’s picture

Assigned: swentel » Unassigned

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!

alexpott’s picture

Status: Needs work » Needs review
FileSize
26.13 KB
21.5 KB

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.

Status: Needs review » Needs work
Issue tags: -API change, -Configuration system, -Config novice

The last submitted patch, 1496510-65.search_cmi.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +API change, +Configuration system, +Config novice

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

aspilicious’s picture

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.

alexpott’s picture

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'
cosmicdreams’s picture

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

sun’s picture

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

alexpott’s picture

Rerolled patch with changes suggested by @aspilicious

sun’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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

Committed and pushed to 8.x. Thanks!

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