Convert node_rank_ $rank variables to use configuration system

#5: 1831632-4-convert-node_rank_R-to-cmi.patch queued for re-testing.

+++ b/core/modules/comment/comment.installundefined
@@ -394,6 +394,15 @@ function comment_update_8004() {
+  update_variables_to_config('node.settings', array(
+    'node_rank_comments' => 'search_rank.comments',
+  ));

I think the config setting should go in a separate file provided by comment.module. We decided a while ago that modules should not extend config files from other modules.

Especially because comment is currently refactored to support all entities.

+++ b/core/modules/node/node.installundefined
@@ -707,6 +701,32 @@ function node_update_8013() {
+ * Moves variables node_rank_relevance, node_rank_sticky, node_rank_promote and
+ * node_rank_recent to config node.settings.search_rank.

I think this can be shortened to "node search rank variables" or something like that.

+++ b/core/modules/node/node.installundefined
@@ -707,6 +701,32 @@ function node_update_8013() {
+  $rank_variables = db_select('variable', 'v')
+    ->fields('v', array('name'))
+    ->condition('v.name', db_like($oldprefix) . '%', 'LIKE')
+    ->execute()
+    ->fetchCol();
+
+  $config = array();
+  foreach ($rank_variables as $rank_variable) {
+    $config[$rank_variable] = substr_replace($rank_variable, $newprefix, 0, $oldpfxlen);

Hm, haven't checked the older patches, is this necessary?

I thought that update_variables_to_config() is able to handle non-existing variables and defaults...

+++ b/core/modules/statistics/statistics.installundefined
@@ -173,3 +173,12 @@ function statistics_update_8002() {
+ */
+function statistics_update_8003() {
+  update_variables_to_config('node.settings', array(
+    'node_rank_views' => 'search_rank.views',

Same for this one then as for comments.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/SystemUpgradePathTest.phpundefined
@@ -99,6 +99,11 @@ public function testVariableUpgrade() {
+    $expected_config['node.settings'] = array(
+      'search_rank.promote' => 10,
+      'search_rank.custom_from_contrib' => 5,

Ah I see, that's why you did it dynamically.

That might actually be irrelevant given that those settings should not be in node.settings... I'd instead of custom_from_contrib, let's add tests for comment and statistics instead...

+++ b/core/modules/node/config/node.settings.yml
@@ -1 +1,6 @@
+search_rank:
+    promote: '0'
+    recent: '0'

Hm.

1) Would it make sense to move these settings into a new node.search config file? They are only needed when Search module is enabled. Otherwise, they're ignored/unused. E.g., how about this:

node.search.yml:
rank:
  promote: '0'
  recent: ...

2) The indentation for sub-keys should be 2 spaces instead of 4. (as usual)

+++ b/core/modules/node/node.install
@@ -707,6 +701,21 @@ function node_update_8013() {
+    'node_rank_comments' => 'search_rank.comments',
+    'node_rank_views' => 'search_rank.views',

1) node_rank_comments belongs to Comment module, not Node module. Therefore, it should be in a comment.search config file, with separate upgrade path.

2) node_rank_views belongs to Statistics module, if I'm not mistaken. It should be in a separate statistics.search config file with separate upgrade path.

Right, the settings are about nodes. But they are defined by comment and statistics module. And it was defined a while ago that yml files should only contain stuff from the module it lives in. So that they can provide meta information about it, for example.

The comment settings for node types will for example also live in a separate file.

@znerol: Thank you for clarifying the problem space!

By now, it's pretty clear that something is completely off with those node search ranking settings. ;)

Perhaps the most pragmatic way to unblock this innocent issue would be to keep those settings in node.search, but additionally move/merge the comment_ranking() and statistics_ranking() back into node_ranking(), both wrapped in a corresponding module_exists().

That would at least bring some clarity into this confusing hook_ranking() implementation space for now. I can only guess that we should/would want to actually migrate that search ranking stuff from Node module to Entity module for D8 anyway (but I'm not aware of any issue).

What do you think?

#30: 1831632-28-convert-node_rank_R-to-cmi.patch queued for re-testing.

Just a re-roll.

#33: 1831632-33-convert-node_rank_R-to-cmi.patch queued for re-testing.

I am currently re-rolling along with #1938920: Convert node_search_admin theme tables to table #type, they collide a bit.

This patch includes work done at #1938920: Convert node_search_admin theme tables to table #type, rerolled over current head.

Oops! Battery failed in the middle of my git diff'ing

Re-roll with newlines at end of files.

Crap it still doesn't seem to save the configs for the CONTENT RANKING settings...

gonna need some help on this one.

#39: 1831632-39-convert-node_rank_R-to-cmi.patch queued for re-testing.

It was saving a bad array. This fixes it!

BTW....

Is it ok to append to an array like so? (according to our coding standards?) wasn't sure...

  $ranks = array();
  foreach ($form_state['values']['content_ranking']['factors'] as $factor){
    $ranks += $factor;
  }

While changing all this lines we could replace it with Drupal::config()

It does use config(), that was just a snippet. The problem is that the form submission generates an array that doesn't match the structure of the config, so I used the foreach to simplify the array so it matches the config.

+ /**
+ * Submit callback for search admin form callback.
+ */
+function node_search_admin_submit($form, &$form_state) {
+ $ranks = array();
+ foreach ($form_state['values']['content_ranking']['factors'] as $factor){
+ $ranks += $factor;
+ }
+ config('search.node')
+ ->set('rank', $ranks)
+ ->save();
+}

Also just in general, is there a reason why we not prefix it with node instaed? So node.search instead of search.node. All this code is in node.module, which deals with that.

#42: 1831632-42-convert-node_rank_R-to-cmi.patch queued for re-testing.

.

Jon Pugh: This issue has needed a re-roll for a week, are you still working on it, or can somebody else do the re-roll? :)

@@ -115,7 +115,7 @@ function hook_search_admin() {
-  $ranks = config('node.settings')->get('search_rank');
+  $ranks = Drupal::config('search.node')->get('rank');

This was changed to:

$ranks = Drupal::config('node.settings')->get('search_rank');

So this patch needs a re-roll.

Probably makes sense to wait with a re-roll until #2003482: Convert hook_search_info to plugin system

That other issue is in... I'm not sure how much of this still needs to be done, but it's time to reroll this one.

I think we should take a slightly different approach here and use the plugin configuration system here. I'm not going with full config entities as that seems overload but patch attached will read in a search.plugin.plugin_id.yml for any search plugin and add what's contained within to the plugin's configuration array.

Doh!

ok, so I see where you are going with this, but I think it could be improved:

I think this change is wrong:

-      $container->get('config.factory')->get('search.settings'),
+      $container->get('config.factory'),

I'd either leave it as-is, or better yet maybe merge it into the configuration array used to instantiate the plugin so we totally get this out of the dependency list.

Minor/personal taste:

+    // Ensure we return an array. If the plugin has no configuration this will
+    // be null.
+    if (!is_array($configuration)) {
+      $configuration = array();
+    }

I'd personally use if (empty($configuration))

I can do a quick re-roll with those changes

This will need to be rerolled after #2086201: Use PluginFormInterface instead of one-off form methods for search plugins gets in.

Ok after talking to @pwolanin lets see if we can avoid injecting the config factory. And I just noticed the SearchExtraTypeSearch which can now also use the new search plugin configuration system.

Yes, I like this better as long as we get a config object.

Minor, but I might use $this->getPluginId() since it's guaranteed by the interface while the protected variable name is not?

I read through the code and docs, and it all looks pretty clear to me. One minor thing to fix:

+   * @param string $plugin_id
+   *   The id of the plugin to get configuration for.

This is in SearchPluginManager.php -- please use "ID" not "id" (pet peeve of mine: id is like ego, superego, id; ID means identifier... it's all over core and it's just wrong and I hate it!).

Actually, come to think of it:

+  /**
+   * Gets plugin configuration stored in the configuration management system.
+   *
+   * @param string $plugin_id
+   *   The id of the plugin to get configuration for.
+   *
+   * @return array
+   *   An array of plugin configuration.
+   */
+  protected function getConfiguration($plugin_id) {
+    $configuration = array();
+    $configuration['search.plugin.' . $plugin_id] = $this->configFactory->get('search.plugin.' . $plugin_id);
+    $configuration['search.settings'] = $this->configFactory->get('search.settings');
+    return $configuration;
+  }

Could we add something to the @return to explain that you get the search plugin and generic search settings in the return value? And the one-line description just says it is getting plugin configuration, which doesn't seem accurate.

Here's a reroll using not much more intelligence than git itself. The interdiff contains the changes I made to resolve the conflicts. Leaving at needs work until I've given it more of a test myself.

Oh, I did manage to slip in the id > ID change though.

Continuing from #71 re getConfiguration(). This function exists to provide parameters for createInstance(), but it's not a pattern that I can find anywhere else in Drupal and I think this might have been an unintentional change in #69.

FactoryInterface defines the $configuration parameter of createInstance as "An array of configuration relevant to the plugin instance.", so I think this is intended to be a potentially multi-level array, rather than an array of config objects. I think we would do this just by adding ->get() to these calls (the equivilent code before #69 did call ->get()).

+    $configuration['search.plugin.' . $plugin_id] = $this->configFactory->get('search.plugin.' . $plugin_id)<strong>->get()</strong>;
+    $configuration['search.settings'] = $this->configFactory->get('search.settings')<strong>->get()</strong>;

I'm not sure what impact this would have on the created instance though.

test

Might need some work.

+    $config = $this->configuration['search.plugin.' . $this->pluginId];

Seems like we'd want a namespace for "ranking" rather than assuming they will always be the only top-level config for node search?

@pwolain are you sure about #75

The original variables were very node specific - node_rank_promote, node_rank_views, node_rank_comments, node_rank_custom. I'm not sure that we should be generalising them here.

1. +++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.php
   @@ -272,14 +260,14 @@ protected function addNodeRankings(SelectExtender $query) {
          foreach ($ranking as $rank => $values) {
   -        // @todo - move rank out of drupal variables.
   -        if ($node_rank = variable_get('node_rank_' . $rank, 0)) {
   +        $rank = $this->configuration['search.plugin.' . $this->pluginId]->get('rank.' . $rank);
   +        if (!empty($rank)) {
              // If the table defined in the ranking isn't already joined, then add it.
              if (isset($values['join']) && !isset($tables[$values['join']['alias']])) {
                $query->addJoin($values['join']['type'], $values['join']['table'], $values['join']['alias'], $values['join']['on']);
              }
              $arguments = isset($values['arguments']) ? $values['arguments'] : array();
   -          $query->addScore($values['score'], $arguments, $node_rank);
   +          $query->addScore($values['score'], $arguments, $rank);
            }
          }
   

   $rank is used both for the index of the array being looped over and for the value returned from config. I'd suggest renaming the latter back to $node_rank.

2. +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/SystemUpgradePathTest.php
   @@ -168,6 +168,13 @@ public function testVariableUpgrade() {
   +    $expected_config['search.plugin.node_search'] = array(
   +      'rank.promote' => '10',
   +      'rank.views' => '5',
   +      'rank.comments' => '-2',
   +      'rank.custom' => '6',
   +    );
   +
   

   These should be integers now, see #1653026: [META] Use properly typed values in module configuration

It's a shame the change suggested in #67 to remove the config factory dependency wasn't split into it's own task, as it's made the patch more complicated than it needed to be, but it's probably not worth splitting them now.

Note that there are unaddressed review comments from #70 onwards.

Defaults should be provided by .yml files, not using ternary operators.

I don't think we should be dynamically adding the plugin ID in this line either:
$this->configFactory->get('search.plugin.' . $plugin_id);
I think we should be asking the plugin for it's config entity (although I'm still a bit new to this stuff).

+++ b/core/modules/search/lib/Drupal/search/SearchPluginManager.php
@@ -117,4 +117,29 @@ public function pluginAccess($plugin_id, AccountInterface $account) {
+  protected function getConfiguration($plugin_id) {
+    $configuration = array();
+    $configuration['search.plugin.' . $plugin_id] = $this->configFactory->get('search.plugin.' . $plugin_id);
+    $configuration['search.settings'] = $this->configFactory->get('search.settings');
+    return $configuration;
+  }

While I can see the point of this, it doesn't seem to be how getConfiguration is meant to be used. Looking at other plugins, I think getConfiguration should return just the configuration for the plugin as an array (i.e. the contents of $this->configFactory->get('search.plugin.' . $plugin_id)).

I don't think the plugins should be accessing search.settings directly at all, rather the search module should make the necessary settings available.

I'll keep looking at this tomorrow, in particular I couldn't see how the configuration from other plugins was actually saved to/read from disk.

I've split the problem that @alexpott and @pwolanin were working on in #69 into #2123073: Move index.cron_limit setting to NodeSearch and would really appreciate people's thoughts on that. I'm happy to write patches for both issues once there is consensus on that issue (which, if accepted, would simplify the patch for this issue).

I've got a much better understanding of what's going on here now and I don't agree with the approach taken from #65 onwards. It seems to me that you've got two basic approaches you can take:
a) Pass everything in as parameters on the constructor, store them as properties and go back later to see if they've been changed and need saving to disk (aka the config entity approach).
b) Handle everything internally using \Drupal::config()

This seems an odd hybrid of the two, in that it looks like you're passing in configuration, but really you're passing in a reference to the configuration, and that reference is always going to point to the same place! (NodeSearch will always be search.plugin.node_search)

The patch was also made more complicated by some semi-related refactoring and an odd variable name change. I've already split off the search.settings removal into it's own issue. I don't see the need for SearchExtraTypeSearch->$configuration, but it's not causing any problems so I'll leave that for now.

Once all that was undone we're left with what was actually quite a simple conversion. I've not done an interdiff, as it would be bigger than the diff. This is coded blind at this stage, but I've run out of time today. I'll hopefully get a chance to test it myself tomorrow. Setting to needs review for the test bots.

Conceptually they are very similar, although the config name has changed from node.search to search.plugin.node_search. I created #81 by removing changes from #72.

Other differences that I need to investigate:
- No longer converting node_rank_comments or node_rank_views, I assume these variables are no longer used.
- #63 has some changes that don't seem relevant to this issue, e.g. removal of theme_node_search_admin(). I'll check if any of those are still wanted.
- #63 doesn't always deal with all the variables, e.g. drupal-7.system.database.php only lists 3 of 6, #81 lists 4 of 4.

Regarding #84 - yes there are hook_ranking() implementations, but I do not think they have been working since Drupal 7.x. See
#893302: Search ranking based on the factor "number of comments" & "No of Page Views" is broken
And it looks like the patch in #81 has an upgrade hook that is converting all node_rank_* variables to config, so I don't think we are losing any information in the upgrade?

Anyway... This patch looks quite reasonable and is a straightforward conversion of variables to config, so I think we should probably just commit it.

But I have two concerns about tests:

a) It's interesting to me that the test for ranking is using a config_set() instead of testing the UI (using the Search settings page). Do we have a test that uses the UI? If not, we should add one.

b) I think I'd like to see a test added with a test module that adds a new type of ranking with hook_ranking(), and verifying that it works correctly. This should have been done in 7.

I realize that both of these tests are a bit out of scope for the issue, but it's hard to verify that the change is working properly without better tests. Thoughts? We could move these to a separate task issue...

Does anyone have any concerns about the proposed patch that would keep it from being committed?

I've compared #63 and #81 line by line, differences are:
- Changes for #1938920: Convert node_search_admin theme tables to table #type (thanks znerol)
- Config name has changed from node.search to search.plugin.node_search
- Configs are being set one at a time in a loop rather than as an array. I don't see a major difference between the two approaches, the loop is probably slightly safer in case there are already values that would be overridden but I can't see a scenario where that happens.
- The update functions are very different, but should be removed anyway as upgrades will not be supported in Drupal 8.
- $this->nodeSearchPlugin is set in each of the tests. I'm not sure what purpose this serves.
- rank_custom added to test and database upgrade in #81

It is probably important to note, that the list of variables is not exclusive. Contributed modules also may implement hook_ranking and through that mechanism contribute more variables with node_rank_-prefix.

Shouldn't they keep track of those variables in their own config objects?

Let's keep the changes to a minimum here: switching the node rankings from variables to Config, and keeping it all in the node config. Is there really a problem if it all gets into the node config? If we made it more complicated, I think we'd have to go to a plugin system for rankings, which wouldn't be a bad idea but might be a bit out of scope for this issue.

Regarding #84 - yes there are hook_ranking() implementations, but I do not think they have been working since Drupal 7.x. See
#893302: Search ranking based on the factor "number of comments" & "No of Page Views" is broken

For the sake of this issue, lets assume that they work, as they are good examples of hook_ranking(). That's a worthwhile followup though.

Shouldn't they keep track of those variables in their own config objects?

I'm not sure how this would be possible, given the way NodeSearch::addNodeRankings() is working at the moment:

I think the "correct" way of fixing this for CMI would be to replace hook_ranking with hooks to add/remove rankings from a list maintained by NodeSearch. A default value could then be supplied when the ranking was added. That's outside the scope of this issue though.

Is there really a problem if it all gets into the node config?

The problems are where do defaults come from (they can't come from a yml or an install hook), and when happens when a module is uninstalled (its config should be removed automatically).

Changes in this patch:
- Upgrade functionality and tests have been removed as that will now be handled by migration.
- All known variables documented in drupal-7.system.database.php in alphabetical order for the benefit of the migration team.
- node_ranking_custom removed as that doesn't seem to be used anywhere (I think it was just there as an example that other node_ranking_* variables could be added).
- Values are now stored as integers.
- I've gone back to creating an array and foreaching when saving the variables. This means that any ranks provided by a hook_ranking() function that has since been uninstalled will be removed from the configuration.
- A default is now provided when building the admin form, as we know there will be instances where the default is not provided in the yml. This isn't best practise, so I've added a comment to stop people copying this coding style.

Test concerns from #85 are still valid but could be addressed in a followup if necessary.

Provided the tests pass, I think this is as good as we're going to get with the current API.

Changes:
- Now aims to store all possible ranks, even if a module is not currently enabled. Previously saving the search settings page would often result in keys being added/removed to the rank array. For example, straight after an install, it would add 'comments' (because the comment module is enabled) and remove 'recent' (because cron has never run). This now saves very slightly more context, but should greatly reduce the chance of config appearing to be different when you've not changed anything.
- Improved the tests to work with whatever happens to be in the ranks array. Previously it assumed comments and views would be first, and it would only disable ranks that it knew about.
- Removed $this->nodeSearchPlugin = $this->container->get('plugin.manager.search')->createInstance('node_search'); lines. They were added in #65 without explanation, and I don't see the need for them.
- Moved the yml into the search module, since it's config for a search plugin (as indicated by the name starting search.).

Once this is committed I plan to work on some of the issues mentioned above, and may even replace hook_ranking.

Should probably have given that a txt extension ;)

I noticed that the existing SearchRankingTest does not test setting the rankings via the UI. So I don't think we have a test that verifies that this works; in which case, it is difficult to tell whether this patch might break the UI for setting rankings.

Can we add a test for this, or modify the existing SearchRankingTest so that it uses the UI instead of Config/variable_set() to set up the rankings?

I noticed that the existing SearchRankingTest does not test setting the rankings via the UI. So I don't think we have a test that verifies that this works; in which case, it is difficult to tell whether this patch might break the UI for setting rankings.

Can we add a test for this, or modify the existing SearchRankingTest so that it uses the UI instead of Config/variable_set() to set up the rankings?

I'm not entirely happy with the approach taken by #92 for a number of reasons. Configuration is supposed to be injected into plugins to reduce dependencies - this is exactly what the previous patches were doing. The rationale for going back to just getting config inside the plugin from #81 was

This seems an odd hybrid of the two, in that it looks like you're passing in configuration, but really you're passing in a reference to the configuration, and that reference is always going to point to the same place! (NodeSearch will always be search.plugin.node_search)

Well the current approach (#92) in fact are a tighter coupling between the config object name and the plugin as it is hardcoded in the plugin.

In #72 it was injected through the plugin manager. This would have allowed a plugin to be created with a reference to any configuration object as long as it was keyed by the plugin name in the configuration array. This meant that the approach allowed the configuration of any search plugin in a consistent way. Yes it is different from the config entity approach but that was deliberate since the overhead is unnecessary since there will be a 1 to 1 relationship between plugin and config object.

Furthermore if we are going to go down the approach taken in #92 then the config factory has to be injected. But I really hope we do not.

Yes the hook_ranking is weird and complicates the patch but that is not solved by either approach.

if we are going to go down the approach taken in #92 then the config factory has to be injected.

Do you mean #92 still does this for search.settings? If so, that's #2123073: Move index.cron_limit setting to NodeSearch

Or do you mean that we need to change #92 to inject search.plugin.node_search instead of calling \Drupal::config() from the plugin? If so, is there any documentation about when you should use \Drupal::config() and when you should inject configuration?

So... I'm a bit confused.

The Search Settings page is allowing individual plugins to add settings to the page, which seems logical. Node search ranking "weights" are an example of this.

They need to be saved somewhere -- either the plugin needs to save them in configuration, or they need to go into the generic search settings configuration. Which do we think makes more sense? Whichever it is, the NodeSearch plugin at some point will need to retrieve the weights that the user has set, to form the search query.

So let's step back and make that first decision: should search plugins have their own config for their specific settings, or use the central search settings? And then we can decide how best to retrieve them (where to inject etc.). The latest patch puts them in central search settings... I can't tell whether alexpott thinks that is a good idea or not?

Plugins should definitely have their own settings. As these plugins are only active when the search module is eanbled their config file should begin with "search.". Personally I think that this configuration should be injected into the plugins by the search plugin manager - in that way we can document and enforce a standard practice for all search plugins and all of them we will have names that make sense.

@jhodgdon afaics no patch here has tried to save the configuration to search.settings.yml

What I meant in #99 was to ask whether node.module should be handling the configuration for the NodeSearch plugin, or whether it should be part of search.module's config handling. I'm not sure I made myself clear there? Maybe.

Anyway, it sounds like #100 is advocating that all configuration from other modules' search plugins should be gathered by search.module, stored in search.module's configuration (search.settings.yml), and injected in a standard way (maybe in the constructor/create method?) into search plugins so that they have it available when they need it.

If I've correctly grasped #100, I think this is a good idea and it sounds like a good pattern to follow. Probably we'd need a method or two added to the SearchPluginBase class and the SearchPluginInterface (maybe?) to deal with this, so that the SearchPluginManager class could properly do the injection?

And I agree that none of the patches I've seen on this issue are doing this.

So if I've understood correctly... we'd have a section called maybe search.plugin.node_search or search.plugin.user_search, and the specific plugin could put anything it wants into that namespace for configuration.

Then during plugin instantiation, the SearchPluginManager could grab the search.plugin.[plugin id] section of search.settings configuration, and pass that in to the plugin to use as they see fit. There would also need to be a way for the plugin to give the settings to the search plugin manager to store, after the user updates them via the Search Settings Page or perhaps a plugin-specific config page.

Or something like that?

I can see arguments on both sides of this one.

Even if we enforce that a search plugin's settings are stored under search.plugin.*, the search can't specify the schema inside that or even what the valid values of * are, so I wonder what the benefit is. What do we do for plugins that don't actually have any configuration of their own? Should they ship with their own, empty, search.plugin.my_plugin.yml files?

Other than to expose an admin UI, there doesn't seem to be a need for plugins to expose that they support configuration, so why not encapsulate all configuration inside the plugin? (i.e. move back to node.search.yml). It doesn't seem like it needs to be part of the plugin API to me, although certain plugins may want to add their own methods to expose specific configuration (not that I can think of any examples).

@jhodgdon anything under search.* belongs to the search module. I think search.settings should only contain config from the search module directly, not extensions of the search module.

There's a related question in #2102521: Finish converting menu.module to CMI - what's the best way to provide defaults when you don't know the config structure beforehand?

@jhodgdon I was not suggesting that the node module's search plugin configuration defaults should be provided by the search module - the search.plugin.node.yml should definitely by in the node module's config folder. This means it'll only by installed when both the node and search module are enabled. Which is exactly what we want since this configuration only has meaning when both modules are enabled.

If the plugin has no configuration then there does not need to be a file - empty config files make no sense.

The schema for each plugin's config file if it has one should be provided by the module that provides the plugin and default config (in this case node).

@jhodgdon the patch in #72 is the last patch to inject config into the plugin using the search plugin manager. Which I thought was quite close to rtbc.

Ah, OK. Thanks for the extra explanation.

I agree search.plugin.node_search.yml should be moved back to the node module so that it is there if and only if node and search are both enabled (although it will remain if first both are enabled, and then node is disabled).

I’m still not convinced that the config needs to be injected rather than encapsulated though.

Injection enforces consistent naming across all search plugins, but means you can’t search the source code for those names (grep search.plugin.node_search will return no results) and doesn’t enforce anything for plugins of other modules. Instead, I propose we document the convention and ensure it is consistently used across core, so contrib modules copy it. We’re already doing this with the module.settings convention, and even the convention that config names start with the module name.

Configuration is supposed to be injected into plugins to reduce dependencies

Is this a coding standard? What dependencies are removed by injecting configuration?

If we are going to go down the approach taken in #92 then the config factory has to be injected.

Why is this required? Can’t we just use \Drupal::config()?

Injection allows a plugin to be created with a reference to any configuration object as long as it is keyed by the plugin name in the configuration array.

When would we have multiple configuration objects with the same keys? Is this for unit testing or something? The plugin will expect defaults to be provided, and we will are only doing this with one file, which has a “hard coded” file name (search.plugin.node_search.yml)

If the plugin has no configuration then there does not need to be a file - empty config files make no sense.

With the approach in #72, you’d still be creating an empty config object to inject though.

I had a chat with alexpott on IRC, to summarise we shouldn't call \Drupal::config() from a class because it makes unit testing harder, so we need to inject the config (ala #72). However, to make the patches simplier, it may be easier to provide the config object first in a separate issue, then add a follow up patch here to start using it.

Relevant chat text:
alexpott>the thing is we should have a single way to configure search plugins that's why moving the config loading to the manager makes sense
alexpott>so what we could do is split the patch into 2 issues... get the plugin config management right
alexpott>and then convert node ranking form variables
alexpott>perhaps that way it'll be easier to grok
...
alexpott>the search for the yml name is moot... try searching for a config entity name.
ianthomas_uk>I think to sum up, you're enforcing the config name, but not the schema or defaults.
alexpott> if we handle the injection correctly (as no patch has done this yet) we should be able to remove the plugin's dependency on the configuration system
ianthomas_uk>Searching certainly doesn't always work, but when it does it can be a big benefit. Particularly for people less familiar with the code. Not a huge issue though.
alexpott> no we should not use \Drupal::config() in classes
alexpott> it means that unit testing them - when we get round to it :) - it more difficult than it should be
alexpott> see https://portland2013.drupal.org/session/dependency-injection-drupal-8
ianthomas_uk>OK, I can see benefits in that policy, but mainly if applied consistently
alexpott>True we will be creating an empty config object with the current approach but actually that is avoidable
alexpott>we are trying to apply it consistently it has just been a challenge :)
alexpott>lots and lots of chicken and egg problems

@jhodgson I've added a test in #2132647: Add test for the settings form of the NodeSearch plugin, so it can get committed while we're working out what to do here.

I've also split injecting configuration off into #2132649: Inject a configuration into SearchPluginBase and postponing this issue on that one.

I'm rather worried about how #2132649: Inject a configuration into SearchPluginBase will actually turn out, can we not resolve this critical/beta blocker with \Drupal::config() and work through the other issue at it's own pace?

#110 I don't think so as the final solution probably will affect the configuration file name and maybe the structure of the data. So I agree with postponing this issue on that - thanks @ianthomas_uk for doing the issue queue work.

This is now postponed on #2042807: Convert search plugins to use a ConfigEntity and a PluginBag

This was fixed by the patch for #2042807: Convert search plugins to use a ConfigEntity and a PluginBag