Make mappers runtime swappable

Here's a patch that demonstrates the cache use-case. There's a @todo in there pointing out that one approach is to make it the responsibility of the plugin type class to get the mapper from a dependency injection container. (Doing so requires #1538712: Merge latest 8.x HEAD into plugins-kernel).

In IRC, pounard suggested that maybe mapping should be decoupled from plugin type entirely. Anyone care to submit a patch for what that could look like?

Updated issue summary.

Here's a reroll for plugins-next, thanks to #1538712: Merge latest 8.x HEAD into plugins-kernel.

This fixes some copy/paste errors.

+++ b/core/lib/Drupal/Core/Cache/Cache.php
@@ -0,0 +1,21 @@
+class Cache extends PluginType {
+
+  public function __construct() {
+    $this->discovery = NULL;
+    $this->factory = NULL;
+    $this->mapper = drupal_container()->has('cache.mapper') ? drupal_container()->get('cache.mapper') : new DefaultCacheMapper();
+  }
+}

Just to clarify, this is the important part of the patch. I'm okay with this being our answer to making mappers runtime swappable: i.e., place the onus on the plugin type class to do so when that's what's appropriate. But if someone wants to propose a better idea, please do so.

This might be a non-issue given the latest patch on #1529162: Decouple plugin type discovery from plugin discovery where we settled on each plugin type provider adding its own wrapper function and/or using drupal_container() for instantiating its plugin type objects if appropriate to that type. However, I'm leaving this open for comment from pounard, given that the plugin type / mapper relationship is something he's expressed interest in. Others with opinions are encouraged to comment too.

I don't think I get how "each plugin type provider adding its own wrapper function" makes it possible to swap the mapper that gets instanciated within PluginType::__construct() ?

Because per #1529162-28: Decouple plugin type discovery from plugin discovery, the wrapper function can use the DI container as appropriate for the type. For example, if a plugin type wants a swappable mapper (or a swappable anything else), it can take those objects as constructor arguments, and configure the plugin type in the DI container accordingly. merlinofchaos has been against forcing all plugin types to use the DI container, and he's basing that on his experience of there being many plugin types that don't require dependency injected factories, discovery, or mapping. For those that do require it (like cache routing), I think the DI container is the right tool for achieving it, but we can see if that bears out when we start converting various systems to the plugin architecture, and adjust if necessary.

Its kinda a blocker too because without it we end up just proxying around the plugin system like cache router and that sort of defeats some of the purpose of having a core plugin system.

I don't quite follow. Can you elaborate?

Is there anything actionable or do we just say "you can do it" for now?

I think we're all good on the plugin system code with respect to this. It might be nice to get the cache system converted to using the technique we settled on, so that when we post the next patch to #1497366: Introduce Plugin System to Core, we have a demonstration use-case. I can reroll #3 to do that, but can you merge 8.x HEAD to plugins-next first? I'm particularly wanting to use the DI changes that just landed an hour ago (note, that link is the 2nd of 2 commits on that today).

This could use some docs, but perhaps we can defer that to #1512588: [meta] Fill in plugin documentation and commit this if you think the code itself is good and demonstrates the decision we reached..

+++ b/core/includes/cache.inc
@@ -27,16 +27,7 @@ function cache($bin = 'cache') {
+  return drupal_container('cache')->getPluginInstance(array('bin' => $bin));

Fixed in this patch to drupal_container()->get('cache')->...

Upping to major, because this patch contains our first use-case for mappers, and ideally the initial patch for getting plugins into core should demonstrate use-cases for all major components.

On second thought, maybe this is a better way to plugin'ify the cache system? With this approach, the mapper isn't runtime swappable directly, but the top-level cache plugin type is (by virtue of being in drupal_container()), so the goal of this issue is still preserved. As a bonus, this demonstrates a plugin type that uses all 3 components (discovery, factory, and mapper), solves #1512594: Figure out what a default mapper implementation would look like., and gives us a reusable PersistentVariableDiscovery class that EclipseGc asked for in IRC.

Implemented #18.1, #19, and some other small improvements.

Regarding #18.2 (DI for Drupal/Core/Cache/Cache::__construct()), it's a chicken and egg problem. DefaultFactory and DefaultMapper require either $this or $this->discovery, so they can't be passed as constructor arguments unless the discovery object is also created by the caller. If we make all 3 constructor arguments, then there's no logic at all left in Drupal/Core/Cache/Cache. Since Drupal/Core/Cache/Cache itself is in the DI, I think it's okay for its constructor to hard-code all 3 components, as it seems easy enough for someone to just override the entire class.

I agree that it's a bit weird for bin to equal plugin_id, but that's how the cache system currently works. As per:

function cache_invalidate(array $tags) {
-  foreach (cache_get_backends() as $bin => $class) {
     cache($bin)->invalidateTags($tags);
   }
 }
 
 /**
- * Returns a list of cache backends for this site.
- *
- * @return
- *   An associative array with cache bins as keys, and backend classes as value.
- */
-function cache_get_backends() {
-  return variable_get('cache_classes', array('cache' => 'Drupal\Core\Cache\DatabaseBackend'));
-}
-

So this patch keeps that. Perhaps we need a follow-up to organize cache plugins differently, but is that a short term blocker?