Make derivatives work

Yeah we need to kill the constructor entirely from feedback from eclipsegc. getting the base discovery class right to cover the right logic is proving hard but I'm close.

OK, I don't remember which patch effulgentsia had me apply before I started working, perhaps he can glance at it and make a note to clarify my stupidity for not documenting it myself. That being said, I have a 8 tests for derivatives in this. I _THINK_ this will work, but I'm going to have to actually give blocks a try to see how this pans out, still this is a good first try I think.

Eclipse

+++ b/core/lib/Drupal/Component/Plugin/Derivative/DerivativeInterface.php
@@ -31,6 +28,6 @@ interface DerivativeInterface {
-  public function getDerivatives();
+  public static function getDerivatives();

This is static because it IS a 1 to 1 relationship between derivative classes and their corresponding plugin definition (which is why I favored consolidating the classes), however, I can't think of a situation where the getDerivatives() method would need anything internal to the class itself, so from a utility standpoint, having the method as a static means we can utilize it in the UI without a full class instance. I'm not sure if that's a good enough reasoning for it to be static, especially since I'm advocating we pick one method or another (either plugins provide their own derivative support, or we have a separate class for that, but both seems to turn the code to spaghetti imo).

In short, no two plugins are likely to ever have the same discovery methodology. The only ones that would come close are field based plugins, but I don't believe I've written two of those with the same getDerivatives() method yet, so I'm pretty sure the logic here is sound.

+++ b/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryAbstract.php
@@ -0,0 +1,55 @@
+    if (isset($this->plugin_definitions)) {

You're totally right, I dunno why I changed that.

+++ b/core/lib/Drupal/Component/Plugin/Discovery/StaticDiscovery.php
@@ -17,14 +17,35 @@ class StaticDiscovery implements DiscoveryInterface {
   public function getPluginDefinition($plugin) {
-    return isset($this->plugin_definitions[$plugin]) ? $this->plugin_definitions[$plugin] : array();
+    list($plugin_id, $derivative) = $this->derivePluginId($plugin);
+    if (isset($this->plugin_definitions[$plugin_id])) {
+      if ($derivative && !empty($this->plugin_definitions[$plugin_id]['derivative'])) {
+        return $this->getDerivative($this->plugin_definitions[$plugin_id], $derivative);
+      }
+      elseif (!empty($this->plugin_definitions[$plugin_id]['derivative'])) {
+        return array();
+      }
+      return $this->plugin_definitions[$plugin_id];
+    }
+    return array();
   }

   /**
    * Implements DiscoveryInterface::getPluginDefinitions().
    */
   public function getPluginDefinitions() {
-    return isset($this->plugin_definitions) ? $this->plugin_definitions : array();
+    $definitions = array();
+    if (isset($this->plugin_definitions)) {
+      foreach ($this->plugin_definitions as $id => $definition) {
+        if (isset($definition['derivative'])) {
+          $definitions += $definition['derivative']::getDerivatives();
+        }
+        else {
+          $definitions[$id] = $definition;
+        }
+      }
+    }
+    return $definitions;
   }

I simplified this further still, but it actually buys us a lot since every discovery method has a different way of getting the plugins, but roughly the same way of getting derivatives, and this helps us there. New code is better, take a look.

+++ b/core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php
@@ -19,6 +19,7 @@ class DefaultFactory implements FactoryInterface {
+  protected $plugin_definition;

   public function __construct(DiscoveryInterface $discovery, $class_key) {
     $this->discovery = $discovery;
@@ -45,7 +46,9 @@ class DefaultFactory implements FactoryInterface {

@@ -45,7 +46,9 @@ class DefaultFactory implements FactoryInterface {
     else {
       $instance = new $plugin_class();
     }
-    $instance->setConfig($configuration);
+    if (!empty($configuration['config'])) {
+      $instance->setConfig($configuration['config']);
+    }

     return $instance;
   }
@@ -64,12 +67,12 @@ class DefaultFactory implements FactoryInterface {

@@ -64,12 +67,12 @@ class DefaultFactory implements FactoryInterface {
       throw new PluginException("The plugin type did not specify a valid key for determining the plugin instance class.");
     }

-    $plugin_definition = $this->discovery->getPluginDefinition($plugin_id);
-    if (empty($plugin_definition[$this->class_key])) {
+    $this->plugin_definition = $this->discovery->getPluginDefinition($plugin_id);
+    if (empty($this->plugin_definition[$this->class_key])) {
       throw new PluginException("The plugin did not specify an instance class.");
     }

-    $class = $plugin_definition[$this->class_key];
+    $class = $this->plugin_definition[$this->class_key];

Yes, to get tests passing I had to do a little in the factory, so it's definitely relevant.

+++ b/core/lib/Drupal/Core/Plugin/Discovery/ConfigDiscovery.php
@@ -22,7 +22,7 @@ class ConfigDiscovery implements DiscoveryInterface {
-      $derviative_mapper = new $config['derivative']($this->definition_root, $plugin_id, $config);
+      $derviative_mapper = new $config['derivative']();

Quite a few more changes beyond this in the new code base, I still don't have tests for config discovery yet, but I'm pretty confident they could be written at this point easily enough, and the 10 tests I have working against static are a good map of what needs testing.

And, another derivatives patch:

I'm still not convinced of the static methods. It seems like we're making them static because ctools did and I can't find a good reason to do it.

I also like the the approach in my branch which seems to leave a much simpler discovery class and which the change I just pushed a simple conversion process of extending DerivativeDiscoveryBase and renaming your getPluginDefinition/getPluginDefinitions functions to loadPluginDefinition/loadPluginDefinitions.

I'm still not convinced. committing to a static method means we can never do anything like pass the global container or a database object to the constructor.

Hmmm... can we solve this by just passing an options array to both static methods that we know we won't need 99% of the time? I hear what you're saying here though, but Alex's point about us not being able to use this on the plugin class w/o static methods is really valid, and I'd REALLY like to not lose that now that I've got it :-D

Eclipse

+++ b/core/lib/Drupal/Component/Plugin/Discovery/StaticDiscovery.phpundefined
@@ -14,17 +14,33 @@ class StaticDiscovery implements DiscoveryInterface {
+  public function __construct(DerivativeDiscoveryInterface $derivativeDiscovery = NULL) {
+    $this->derivativeDiscovery = isset($derivativeDiscovery) ? $derivativeDiscovery : new NullDerivativeDiscovery();
+  }

I'm a little uncertain of this derivative class for figuring out derivatives and then a different class and interface for each plugin instance. It seems harder to understand. The null fallback is fairly clever though.

+++ b/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscovery.phpundefined
@@ -0,0 +1,88 @@
+  /**
+   * Returns the object that implements DerivativeInterface or class that implements StaticDerivativeInterface on behalf of the plugin.
+   *
+   * Returns NULL if the plugin doesn't define a value for this class, or if the
+   * class doesn't implement the required interface. Not throwing an error for
+   * either of these cases allows for flexibility in allowing but not requiring
+   * plugin classes to directly handle their own derivative fetching.
+   */
+  protected function getDerivativeClassOrObject($basePluginId, $baseDefinition) {
+    if (!isset($this->implementors[$basePluginId])) {
+      $this->implementors[$basePluginId] = FALSE;
+      if (isset($baseDefinition[$this->classKey])) {
+        $class = $baseDefinition[$this->classKey];
+        $reflector = new \ReflectionClass($class);
+        if ($reflector->implementsInterface('Drupal\Component\Plugin\Derivative\DerivativeInterface')) {
+          $this->implementors[$basePluginId] = new $class($basePluginId, $this->pluginType);
+        }
+        elseif ($reflector->implementsInterface('Drupal\Component\Plugin\Derivative\StaticDerivativeInterface')) {
+          $this->implementors[$basePluginId] = $class;
+        }
+      }
+    }
+    return $this->implementors[$basePluginId] ? $this->implementors[$basePluginId] : NULL;

I'm really not a fan of this, especially the "maybe we call a static method maybe we call a normal method" runtime on the calling side.

Kris and I are going to discuss this more tomorrow but just wanted to jot down some notes on looking over this patch.

merlin, eclipse and I had a call today and we decided supporting both static and instantiated methods is making things a bit of a mess. Eclipse conceded(perhaps a bit reluctantly) that using statics means we can not easily use any sort of constructor or injection if we go with static methods. It'd require a soft options array sort of argument. (side note, crell will also be happy)

So action items, look at at effulgentsia's last patch and see if we can't merge it into eclipse's earlier patch. Maybe using some of the branch I was working on if it makes sense.

Timeframe, we'll see how the weekend goes and if I'm able to review, if not eclipse and I will try and work on it during the week, worst case eclipse will probably work on it Friday during his SCOTCH time.

Sorry, I totally wanted to post a more in-depth review here (some days ago), but only got this far. Got totally distracted with CMI and Testing system work. :-/ I'm not sure whether this helps in any way, but well...:

+++ b/core/lib/Drupal/Component/Plugin/Derivative/DerivativeInterface.php
@@ -1,36 +1,49 @@
+ * Interface implemented by plugins needing derivative support.

Is there any reason for why you guys chose to go with the name "derivatives" instead of "variants", which sounds much more human and pronounceable to me? At least for me, as a non-native English speaker, and as a non-native OO speaker, that would be much more clear.

+++ b/core/lib/Drupal/Component/Plugin/Derivative/DerivativeInterface.php
@@ -1,36 +1,49 @@
+   *   The derivative id. The id must uniquely identify the derivative within a
+   *   given base plugin, but derivative ids can be reused across base plugins.

It's no clear to me how a variant/derivative of one base plugin could ever be re-used for another base plugin, and why we actively try to support that.

+++ b/core/lib/Drupal/Component/Plugin/Derivative/DerivativeInterface.php
@@ -1,36 +1,49 @@
+   *   Implementations of
+   *   Drupal\Component\Plugin\Discovery\DerivativeDiscoveryInterface combine
+   *   the base plugin id with $derivativeId to construct the derivative's full
+   *   plugin id.

No idea what this means, needs more context.

Ok, spent the day reviewing this and I like it. I'm not sure the fetcher helper buys us a lot but it probably doesn't hurt so I didn't touch it. Did some cleanups and a couple tweaks:

1. Convert args and function variables to _ seperated.
2. Convert some class variables to camelCase.
3. Some doc cleanups.
4. Split encode/decode of plugin/derivative id into functions so it /could/ be modified and is better documented.

If no one has any objections I'll commit it and we'll put this to rest at long last.

oh, the split encode/decode logic has a slightly cheaper set of string functions when dealing with derivative id's(from some goofy synthetic benchmarks). they where generally close though and this seems easier to read.

I can't express how thankful I am for driving this home effulgentsia.

YAYYYY!! Great job, guys!!

+1000000

Please link the core issue that will follow this patch (or whatever) here.