Allow image toolkit derivatives

This is a big architectural issue of the toolkit system. Don't think that this is the right way

My approach is to allow declaration of derivative plugins for image toolkits. The effect is the same but it's more elegant. A derivative is declared id = "gd:whatever". If you want to override operations, just implement new one having the toolkit definition pointing to the derivative id.

This issue is blocked by #2359391: The deriver class should receive the decorated plugin discovery. Attached 2 patches: one for this issue and the other combined with #2359391: The deriver class should receive the decorated plugin discovery for testing.

Additional check added.

It seems that we don't need a deriver class. Derivatives are discovered exactly as the base toolkits and the whole logic goes into ImageToolkitOperationManager::getToolkitOperationPluginId() where we need to check also the base plugin for operations. Now the patch is more compact and does not depend on #2359391: The deriver class should receive the decorated plugin discovery anymore. Somehow we are back to @attiks initial solution but instead of having the "base toolkit" key, we are using the derivative like ID prefixing.

The interdiff is against the combined patch.

Small fix: removed unused "use" statement.

Changed the existing change request to reflect this improvement: https://www.drupal.org/node/1993056/revisions/view/7175637/7745505

Those are not comments. Those are making your life easier in PhpStorm by giving you rich autocomplete features. And are also good for documenting the types of objects that we are using there.

Ok, ok :)

Hey @attiks, this is here for a long time waiting a review. Is this still needed? I guess, yes.

PS: Rerolled.

+++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationManager.php
@@ -77,6 +88,14 @@ function ($definition) use ($toolkit_id, $operation) {
+      if (($toolkit_id != $base_toolkit_id) && !empty($base_toolkit_id)) {
+        $base_toolkit = $this->toolkitManager->createInstance($base_toolkit_id);
+        return $this->getToolkitOperationPluginId($base_toolkit, $operation);
+      }

1. Should check if $base_toolkit_id is available before creating an instance? The base toolkit may have been uninstalled.

+++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationManager.php
@@ -92,17 +111,17 @@ function ($definition) use ($toolkit_id, $operation) {
-  public function createInstance($plugin_id, array $configuration = array(), ImageToolkitInterface $toolkit = NULL) {
+  public function createInstance($plugin_id, array $configuration = array(), ImageToolkitInterface $toolkit = NULL, ImageToolkitManager $toolkit_manager = NULL) {
     $plugin_definition = $this->getDefinition($plugin_id);
     $plugin_class = DefaultFactory::getPluginClass($plugin_id, $plugin_definition);
-    return new $plugin_class($configuration, $plugin_id, $plugin_definition, $toolkit, $this->logger);
+    return new $plugin_class($configuration, $plugin_id, $plugin_definition, $toolkit, $this->logger, $this->toolkitManager);
   }

2. Why? We are not passing the ImageToolkitManager to ImageToolkitOperation objects.

+++ b/core/modules/system/tests/modules/image_test/src/Plugin/ImageToolkit/DerivedToolkit.php
index 0000000..780dbe5
--- /dev/null

--- /dev/null
+++ b/core/modules/system/tests/modules/image_test/src/Plugin/ImageToolkit/Operation/Bar.php

+++ b/core/modules/system/tests/modules/image_test/src/Plugin/ImageToolkit/Operation/Bar.php
+++ b/core/modules/system/tests/modules/image_test/src/Plugin/ImageToolkit/Operation/Bar.php
@@ -0,0 +1,21 @@

@@ -0,0 +1,21 @@
+<?php

3. To keep consistency with gd toolkit, I think this should go under file core/modules/system/tests/modules/image_test/src/Plugin/ImageToolkit/Operation/test/Bar.php and class \Drupal\image_test\Plugin\ImageToolkit\Operation\test\Bar. Same for Foo. Not sure about FooDerived though.

Other than that, nice and useful if anyone wants to support additional image file formats without rewriting entire toolkits. Test coverage is good.

• #18.1: If a bad $base_toolkit_id is passed, a \Drupal\Component\Plugin\Exception\PluginNotFoundException is thrown. An if() {} doesn't help. But I'm happy with the exception because no plugin derivative should exists without its base plugin.
• #18.2, 3: Fixed, thank you.

+++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationManager.php
@@ -92,7 +111,7 @@ function ($definition) use ($toolkit_id, $operation) {
-  public function createInstance($plugin_id, array $configuration = array(), ImageToolkitInterface $toolkit = NULL) {
+  public function createInstance($plugin_id, array $configuration = array(), ImageToolkitInterface $toolkit = NULL, ImageToolkitManager $toolkit_manager = NULL) {
     $plugin_definition = $this->getDefinition($plugin_id);

1. This is also not necessary, then.

+++ b/core/modules/system/tests/modules/image_test/src/Plugin/ImageToolkit/Operation/test/Bar.php
@@ -0,0 +1,21 @@
+ * Contains \Drupal\image_test\Plugin\ImageToolkit\Operation\Bar

2. \Drupal\image_test\Plugin\ImageToolkit\Operation\test\Bar

+++ b/core/modules/system/tests/modules/image_test/src/Plugin/ImageToolkit/Operation/test/Bar.php
@@ -0,0 +1,21 @@
+namespace Drupal\image_test\Plugin\ImageToolkit\Operation;

3. Drupal\image_test\Plugin\ImageToolkit\Operation\test;

2. and 3. also in the other files moved

Added beta evaluation.

.

Thank you.

Thanks @claudiu.cristea.

@attiks is happy too, quoting myself from #18

nice and useful if anyone wants to support additional image file formats without rewriting entire toolkits. Test coverage is good.

RTBC

#27 seems a testbot failure.

This issue does not meet the criteria for allowed changes during the beta phase (see https://www.drupal.org/core/beta-changes).

Since the patch is BC-compatible (API addition with an added service via constructor injection), it can be made once the 8.1.x branch is open for development, so postponing to that branch. Thanks!

It was RTBC for 1 months :(

#24 is still green.

+++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationManager.php
@@ -67,7 +77,8 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
-  protected function getToolkitOperationPluginId($toolkit_id, $operation) {
+  protected function getToolkitOperationPluginId(ImageToolkitInterface $toolkit, $operation) {
+    $toolkit_id = $toolkit->getPluginId();

I think we should be able to make the change without changing the protected method signature which is a BC break that should only be done in major releases unless the method is agreed to be internal. Protected methods are not like service construction which we've agreed can change (within reason) in minor releases.

According to @xjm we protected methods are automatically considered internal see https://www.drupal.org/node/2562903. I've debated whether or not we should avoid the signature change but I think in this case it keeps the code more maintainable and sub classing the ImageToolkitOperationManager should be extremely rare.

Committed 3ef563d and pushed to 8.1.x. Thanks!