Inject a configuration into SearchPluginBase [#2132649]

Following much discussion in #1831632: Convert node_rank_ $rank variables to use configuration system it was agreed that configuration must be injected into search plugins, as the alternative is to call \Drupal::config() inside object-oriented code, which would make unit testing much harder.

I've opened this issue for us to inject the configuration, so the patch isn't complicated by variable_get/set conversions.

Comment	File	Size	Author
#1	2132649-inject-plugin-config-1.patch	5.16 KB	ianthomas_uk
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

ianthomas_uk

he/him

English

Brighton, UK

CreditAttribution: ianthomas_uk commented 10 November 2013 at 18:48

File	Size
2132649-inject-plugin-config-1.patch	5.16 KB

This patch contains the relevant parts of #1831632-72: Convert node_rank_ $rank variables to use configuration system as a starting point.

I still really don't like the way this adds a config object to the $configuration array. Everywhere else I've seen it used $configuration just contains primative types, and the plugin doesn't worry about how/when they are saved. One argument for it was that it removes a dependency on the config system, but surely if your class contains lines like $this->configuration['search.plugin.' . $this->pluginId]->get('boost') then you've got a dependency on config to implement the get method (although it can sneak past the method signature because it's hiding in the $configuration array).

How about we just generalise the $this->configSettings style that SearchExtraTypeSearch is already using instead?

A related discussion (which doesn't look to have an imminent solution) is happening on #1764380: Merge PluginSettingsInterface into ConfigurableInterface

Comment #2

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 13 November 2013 at 18:08

Issue tags:

+beta blocker

This issue is blocking #1831632: Convert node_rank_ $rank variables to use configuration system, which is tagged "beta blocker". Therefore, this issue is also a beta blocker.

Comment #3

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 13 November 2013 at 19:12

Issue tags:

-beta blocker

+Plugin system

I have some concerns about this issue, so I've unpostponed #1831632: Convert node_rank_ $rank variables to use configuration system.

+++ b/core/modules/search/lib/Drupal/search/Plugin/SearchPluginBase.php
@@ -9,6 +9,7 @@
+use Symfony\Component\DependencyInjection\ContainerInterface;

@@ -39,6 +40,17 @@
+  static public function create(ContainerInterface $container, array $configuration, $plugin_id, array $plugin_definition) {
+    return new static(
+      $configuration,
+      $plugin_id,
+      $plugin_definition
+    );
+  }

Not needed.

+++ b/core/modules/search/lib/Drupal/search/SearchPluginManager.php
@@ -119,4 +119,28 @@ public function pluginAccess($plugin_id, AccountInterface $account) {
+    $configuration = array_merge($configuration, $this->getConfiguration($plugin_id));
+    return $this->factory->createInstance($plugin_id, $configuration);
...
+    $configuration = array();
+    $configuration['search.plugin.' . $plugin_id] = $this->configFactory->get('search.plugin.' . $plugin_id);
+    return $configuration;

This doesn't seem right to me. Why bother with the array merge and second method? This is the plugin manager itself, not the plugin, so swapping it out would be rare.

Also, we should likely utilize ConfigurablePluginInterface to decide if this even needs to be done (NodeSearch and UserSearch don't need this, just the *test plugin*)

Comment #4

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 14 November 2013 at 17:08

Title:	Inject a config object into SearchPluginBase	» Inject a configuration into SearchPluginBase
Issue summary:	View changes
Issue tags:		+beta blocker

Configuration system data structures are beta blockers so we should get this one done and then use it in #1831632: Convert node_rank_ $rank variables to use configuration system. I've updated the issue summary to reflect the fact that we probably won't end up injecting the configuration object but an array of configuration.