Updated: Comment #135

Part of #2105863: [meta] Images, toolkits and operations being the 3rd issue in the next sequence:

Priority Issue
1.1 #2103635: Remove effects from ImageInterface
1.2 #2110499: Unify the call and interfaces of image toolkit operations Assigned to: claudiu.cristea
1.3 #2073759: Convert toolkit operations to plugins
1.4 #2111263: Toolkit setup form displays settings for multiple toolkits
1.5 #2110591: API/UI for toolkit operations plugin selection

Problem/Motivation

Read first #2105863: [meta] Images, toolkits and operations!

Developers are not able to add custom operations to an image toolkit. Based on the current design they can only extend a toolkit and implement new operations. For example if they want to add new operations to GD toolkit, they need to extend GD toolkit. Other modules may want to add other operations to the same GD toolkit and they need to extend too GD (but which one?).

Proposed resolution

Convert image toolkit operations to plugins.

API changes

Methods scale(), scaleAndCrop(), resize(), crop(), rotate(), desaturate() will be removed from ImageToolkitInterface and transformed in plugins.

Original report by @fietserwin

The image system has now largely been converted to a plugin based system. As co-maintainer for imagecache_actions this triggered me to start thinking about how to convert our module (that implements a set of image effects) to D8.

The image effects itself will be rather straightforward. but these effects also contain toolkit specific code. Where should I put this code?

Straight forward solution: extend from GDToolkit (and ImageMagickToolkit when available) and put our code in there. Register it as toolkit to use.

However, this has a major problem: if our module extends from GDToolkit and another module that implements image effects does so as well, we have competing toolkits that neither support the full range of image effects.

Files: 
CommentFileSizeAuthor
#199 toolkit-operations-2073759-199.patch93.96 KBmondrake
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,001 pass(es).
[ View ]
#199 interdiff_197-199.txt2.28 KBmondrake
#197 toolkit-operations-2073759-197.patch93.55 KBmondrake
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#197 interdiff_196-197.txt9.37 KBmondrake
#192 interdiff.txt11.52 KBclaudiu.cristea
#184 interdiff.txt678 bytesclaudiu.cristea

Comments

Where did the toolkit specific code live before?

In functions named image_{toolkit}_{effect}(). We would typically add these functions to our own code but still be able to use image_toolkit invoke() (thus not having to know which toolkit is active).

Possible solution?
- The code stays within the image effect class
- Our effects support a limited and known number of toolkits
- We define a class that extends ImageEffectBase and that adds a method applyToolkitSpecificEffect (which probably does some dynamic method lookup and calling).
- Our effects derive from that "intermediary base class".

Issue tags:+Plugin system

:)

I was raising basically the same question in #2071153: Contrib extensions to Image Toolkits, I closed that now. I agree we should not create a separate plugin just to add more methods to the underlying toolkit, just 'extend' it with the more image mgmt methods we need, in a way that also other modules may add theirs on top of the same underlying one.

Just thinking aloud: how about another plugin type for the image management functions, somehow dependent on the specific image toolkit plugin? Then when image toolkit is instantiated, it will pick all the 'extensions' available to that toolkit. Then 'core' image mgmt function could be just a case of that.

To make it visible, from the change record:

D7

<?php
  $image
= image_load('sites/default/files/example.jpg');
 
image_toolkit_invoke('rotate', $image, array(90));
 
image_save($image, 'sites/default/files/example.jpg');
?>

D8 (current)

<?php
  $image
= image_load('sites/default/files/example.jpg');
 
$image->toolkit->rotate(90);
 
image_save($image, 'sites/default/files/example.jpg');
?>

D8 (potentially, as an example)

<?php
  $image
= image_load('sites/default/files/example.jpg');
 
$image->toolkit->image->rotate(90);
 
$image->toolkit->imagecache_actions->roundedcorners(array(...));
 
image_save($image, 'sites/default/files/example.jpg');
?>

No clue how to achieve that though :)

Thanks for joining. I did some search on issues that already raised the same question but did not found yours (I probably searched on "effect" (as well as other keywords) in D8 image system issues).

Thinking aloud further:
- We have effects that are independent from each other and don't need to know about each other (except when choosing to do so, like the singe effect "scale and crop")
- Effects should not need to know about toolkits, though they shuld be able to call/execute code specific for the currently active toolkit.
- We have toolkits that do not have to know about each other
- (IMO) Toolkits do not have to know about the effects, they should just provide access to the underlying tools that do the actual work (GD functions versus calling imagemagick's convert) and provide some basic operations (think of load, info, save (to given image format), quality)
- Currently, toolkits also know about the basic effects as provided by core. given the former point, this should be moved.

This seems to direct into some toolkit-effect combination pliugins (RoundedCornersGd plugin, RoundedCornersImagemagick plugin, AutotrotateGD pugin, etc.

But 1 plugin per effect-toolkit combination seems overkill as it would only have 1 (public) method, so combining them as #3 suggests could be OK. But, indeed, how?

I did not read much yet about annotations, but could they be of help here (i.e. can you find methods/functions based on annotations and as such call the {effect}{toolkit} specific code.

Title:How should contrib implement toolkit specific code for image effects?Allow contrib to implement toolkit specific code for image effects
Category:support» task
Priority:Normal» Major

I suggest to make this into a 'task', and raise priority to 'major' as this would be a blocker for contrib modules conversion to D8. If anybody disagrees, please revert.

#4

- We have effects that are independent from each other and don't need to know about each other (except when choosing to do so, like the singe effect "scale and crop")

Agree

- Effects should not need to know about toolkits, though they shuld be able to call/execute code specific for the currently active toolkit.

Agree - 'effects' are where you put the high level logic, calling toolkit image mgmt methods as needed. Within the effect, you should not care to check which is the current toolkit.

- We have toolkits that do not have to know about each other

Agree

- (IMO) Toolkits do not have to know about the effects, they should just provide access to the underlying tools that do the actual work (GD functions versus calling imagemagick's convert) and provide some basic operations (think of load, info, save (to given image format), quality)

Agree, see above

- Currently, toolkits also know about the basic effects as provided by core. given the former point, this should be moved.

Yes that was some thinking - separate toolkit basics (load, save, getInfo, the admin side of the toolkit) from image manipulation (resize, rotate, crop, desaturate, etc.). I believe 'core' could also just stay where they are to avoid API changes.

I am not proficient at all in the new plugins concept, so we'll definitely need some help from gurus :)

Some basic thinking:

  • (in core) create a new plugin type, say ImageManipulationFunctions; in the annotations, allow to specify the ImageToolkit plugin each plugin will be an extension of;
  • (in contrib) create the plugin classes, say ImageCacheActionGDManipulationFunctions and ImageCacheActionImagemagickManipulationFunctions; they will reference 'gd' and 'imagemagick' image toolkits in the annotations, and contain the toolkit specific methods;
  • (in core) when the image toolkit is instantiated, it 'discovers' the ImageManipulationFunctions plugins available, and create a public property for each one
  • (in contrib) refer to that property to call the underlying methods

Need more thoughts....

tagging

Title:Allow contrib to implement toolkit specific code for image effects[no patch, decide approach] Allow contrib to implement toolkit specific code for image effects
Status:Active» Needs review

No patch yet, but some more thinking. Setting to 'needs review' to get opinions if this could be a plan.

  • (in core) create a new plugin type, say ImageToolkitExtension; in the annotations, allow to specify the 'parent' ImageToolkit plugin id (i.e. 'gd', 'imagemagick')
  • (in contrib) create the plugin, say ImageCacheActionGDExtension; in this case, reference 'gd' parent toolkit in the annotations. Put the custom methods in there.
  • (in core) use a magic __get method in the parent toolkit. When a method $image->toolkit->my_module->my_method() is invoked, __get will look for the ImageToolkitExtension plugins provided by 'my_module' that have as parent toolkit id the same image toolkit id as the toolkit itself ('gd' in this case), instantiate it, and call its 'my_method' method

If this is acceptable, then I *hope* I may be able to start writing an initial patch for this.

Overall a good proposal. I would like to change the 3rd point to something like:
(in core and contrib) The (base) toolkit should implement the magic _call() method. When a method $image->toolkit->my_effect() is invoked, __call will look for an ImageToolkitExtension plugin that implements the my_effect() method for the current toolkit, instantiate it, and call the 'my_effect' method.

The idea of this change is that the module that implements it is removed from the algorithm. We are only interested in the toolkit specific implementation of a given effect, not what module implements it. E.g. Imagemagick module may decide to support (some of) the effects provided by imagecache_actions, or another module may want to overrule a given implementation because it (thinks it) has a better or faster algorithm (for a given situation).

I am not sure if in my proposal the list of my_effect() methods must be "duplicated" into the annotation or that we can find out if a given method is implemented by further inspection (probably after instantiating).

Questions:
- This may mean that we want to implement a base toolkit that supplies an implementation for the __call method().
- May the _call magic method be added to an interface?

Hm. You have a valid point on abstracting from the implementing modules. However I believe going for #8 would be too complicated (require reflection to see which classes implement which methods, probably having to cache results for performance, etc...). Also, how would we be resolving conflicts if more plugins implement the same method? Collision may happen at method level, not just at plugin level.

An option could be to declare in annotation a 'method group' as well, being a group of methods we are implementing in the plugin. This could also be subject to interface definition for the class. Then the __get could pick the plugin implementation of that 'method group' having the highest weight.

Summarising:

  • (in core) create a new plugin type, say ImageToolkitExtension; in the annotations, allow to specify the 'parent' ImageToolkit plugin id (i.e. 'gd', 'imagemagick', etc.), and the 'method group' of methods it is implementing
  • (in contrib) create the plugin, say TextimageGDToolkitExtension; in this case, reference 'gd' parent toolkit in the annotations. Put the 'method group' in the annotations as well, and the custom methods in the plugin class. Something like

    /**
       * Defines GD2 toolkit extension for methods needed by Textimage module.
       *
       * @ImageToolkitExtension(
       *   id = "textimage_textimage_gd",
       *   title = @Translation("GD2 toolkit extension for methods needed by Textimage module'),
       *   toolkit_id = 'gd',
       *   method_group = 'textimage',
       *   weight = 0
       * )
       */
    class TextimageGDToolkitExtension implements TextimageImageToolkitExtensionInterface {
      public function getFontFilePath(....) {
        ....
      }
    }
  • (in core) use a magic __get method in the parent toolkit. When a method $image->toolkit->my_method_group->my_method() is invoked, __get will look for the ImageToolkitExtension plugins (a) implementing 'my_method_group' method group, that (b) have as parent toolkit_id the same image toolkit id as the toolkit itself ('gd' in this case), (c) find the one with highest weight, (d) instantiate it, and (e)call its 'my_method' method

So if we have two plugins for the same combination toolkit/method group, the one with higher weight will take over. Both plugins should implement the same interface so we can be sure all the methods within that 'method group' will be implemented.

Thoughts?

I guess you are right about the reflection. So, a solution should prevent needing to load, instantiate and reflect upon an extension just to find out if it can be used.

However, for the sake of DX, I also want to have more or less the same syntax as with the core effects: thus:
$image->toolkit->my_effect();
This leads to an annotation that lists the methods the extension implements. Thus a list instead of a single method_group. note that method_group would be a new concept as well. which we should prevent if possible.

/**
   * Defines GD2 toolkit extension for methods needed by Textimage module.
   *
   * @ImageToolkitExtension(
   *   id = "textimage_textimage_gd",
   *   title = @Translation("GD2 toolkit extension for methods needed by Textimage module'),
   *   toolkit_id = 'gd',
   *   method_list = {
   *    "getFontFilePath",
   *    "myMethod2"
   *   },
   *   weight = 0
   * )
   */
class TextimageGDToolkitExtension {
  public function getFontFilePath(....) {
    ....
  }
}

The method_list should be a non-empty subset of the public methods in the class (containing possibly all public methods).

Using weight to define priority when there are multiple implementors is a good idea. But I think (in Drupal) it is more intuitive to take the one with the lowest weight (first found in the list ordered on weight). See e.g. the list of filters and the one that will be selected by default for users (D7: admin/config/content/formats).

Another note: not sure why you would want to implement an interface for an extension whose methods will be "added" to another class and will be called via a magic method.

Priority:Major» Critical

Guys, this is critical.

I would call them "operations" or "toolkit operations" (ImageToolkitOperation?). Introducing this new plugin means that we have to convert also existing scale(), rotate(), etc.

Let's have more opinions here as it's an architectural switch.

No, the base toolkit implementations (GDToolkit, ...) can and will still have the toolkit specific implementations for the effects that are in core (scale, crop, rotate,...) or for that matter, any operation they would want to support. The extensions will only add methods that are not already available in the class itself.

Regarding the naming, extension versus operation: the idea is that 1 plugin can handle multiple operations (effects). So, IMO, extension is more appropriate.

No, the base toolkit implementations (GDToolkit, ...) can and will still have the toolkit specific implementations for the effects that are in core (scale, crop, rotate,...) or for that matter, any operation they would want to support.

We need to unifiy and abstract the API. If we will open for arbitrary operations then the existing (scale, crop, etc.) will need to follow the same path but will be shipped with Drupal core.

Regarding the naming, extension versus operation: the idea is that 1 plugin can handle multiple operations (effects). So, IMO, extension is more appropriate.

This is how the old, "module way" used to implement this and does not follow the "new world order". Why not taking benefit of autoloading? Each effect/operations is contained in his own plugin, having his own class file and is loaded only on request? Course that they will be packed in contrib modules, under lib/MODULENAME/Drupal/..., and a module would provide one or many effects/operations.

#13 I personally agree on a unified API - with core shipping with an implementation of the plugin that will contain createTmp, crop, desaturate, resize, rotate. The other methods (getInfo, isAvailable, load, save, settingsForm, settingsFormSubmit, supporteTypes) will remain in the ImageToolkit plugin.

So the new ImageToolkitOperations plugin could be sth like

/**
  * Defines GD2 operations supported by Drupal core.
  *
  * @ImageToolkitOperations(
  *   id = "core_gd_operations",
  *   title = @Translation("GD2 operations supported by Drupal core'),
  *   toolkit_id = 'gd',
  *   operations_group = 'core'
  * )
  */
class GDToolkitOperations implements ImageToolkitOperationsCoreInterface {
  public function createTmp(....) {
    ....
  }
  public function crop(....) {
    ....
  }
  public function desaturate(....) {
    ....
  }
  public function resize(....) {
    ....
  }
  public function rotate(....) {
    ....
  }
}

and ImageToolkitOperationsCoreInterface would provide declaration for createTmp, crop, desaturate, resize, rotate methods.

Then, in the effects, you would invoke

$image->toolkit->core->rotate()

for core operations, and

$image->toolkit->my_operations_group->my_method()

for contrib operations.

'core' operations can be overriden by another plugin having lower weight as per suggestion in #10.

How does this sound?

Title:[no patch, decide approach] Allow contrib to implement toolkit specific code for image effects[decide approach] Allow contrib to implement toolkit specific code for image effects
StatusFileSize
new24.81 KB
FAILED: [[SimpleTest]]: [MySQL] 58,816 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

Very first stab at implementing #14. This implements the new 'core operations' plugin - moving core GDToolkit operations to it, implements the plugin manager, and a __call magic method to load the 'core' operations plugin. Calls to $xx->toolkit->method(args): in Image.php and GDToolkit.php have been changed to:

$xx->toolkit->core('method', args);

'core' is still hardcoded here, but before going in any further steps I just wanted to share for visualization and comments.

Status:Needs review» Needs work

@mondrake, @fietserwin, I don't like this approach at all. As an image effect is a plugin, in the same way a "toolkit-effect" should be a plugin. A plugin for each effect/operation. Same is on the more abstract ImageEffect level, same should be here. Then we can use the plugin discovery mechanism to get all available effects/operations and so on. What would be the logic of loading and parsing the code for rotate if in the current request we need only scale?

@claudiu.cristea - good point, makes sense, we would also avoid the 'operations_group' in the annotation. I will adjust accordingly.

BTW - I was thinking that instead of using __call, we could have a specific method in the toolkit to invoke the 'toolkit operation', and hence a cleaner API. I mean:

instead of $image->toolkit->my_operation(args);

something like $image->toolkit->operation('my_operation', 'args');

What do you think?

[EDIT: cross posted with #17: we have a number of more or less the same remarks]

- I agree with removing all effect methods (operations) from GDToolkit and thus any image toolkit in contrib as well and thus going for a unified effect-toolkit plugin system.
- createTmp() is not an effect operation and thus should IMO remain in GDToolkit.
- As to if we want exactly 1 effect-toolkit combination per plugin or allow for more: I was in favor of allowing more than 1 (therefore extension instead of operation). But if we are really unifying all this, in core as well, I don't really have a preference anymore. BTW: I'm not afraid of loading a few additional methods that may not be used, discovery and autoloading have performance impacts as well.

[BTW: Loading all of an image effect just to be able to call transformDimensions() (just to be able to add a width and height attribute to an already created image derivative, and introduced in D7 for performance reasons) may have a far worse impact on performance.]

- If we go for 1 effect-toolkit combination per plugin, do we adopt a strict naming scheme and just instantiate the plugin class by name or do we still use annotations. The latter allows for providing multiple implementations (prioritized by weight); where the former may not even be feasible in a namespaced environment like D8.

- I remain opposed to introducing a concept of 'core' or whatever in the call syntax. So $xx->toolkit->core('method', args); should IMO become the more natural $xx->toolkit->method(args); (or $xx->toolkit->method(arg1, arg2);) where method can be found in the annotation specification (and thus making use or knowledge of who implements it superfluous.

- If we don't want to rely on the magic __call() we can introduce an explicit invoke method: $xx->toolkit->invoke('method', arg1, arg2);. (Hmmm, reminds me of image_toolkit_invoke().)

StatusFileSize
new29.2 KB
new25.25 KB
FAILED: [[SimpleTest]]: [MySQL] 58,795 pass(es), 32 fail(s), and 75 exception(s).
[ View ]

#18 :))

This patch goes along comments #16, #17 and #18.

One plugin per each combination toolkit/operation.

No more 'core' operations group concept.

Methods are called like $xx->toolkit->operation('method', args). No more usage of __call.

No auto discovery (hardcoded loading of plugins), no tests. But if we agree this is the right approach these could be next steps.

Cheers

Status:Needs work» Needs review

Title:[decide approach] Allow contrib to implement toolkit specific code for image effectsAllow contrib to implement toolkit specific code for image effects
Issue tags:+API change

Tagging with 'API change' to raise hands in this perspective. Removed [decide approach] from issue title as I think we are getting closer at this stage. Feel free to revert if not in agreement.

Status:Needs work» Needs review

Let's try to fix naming convention first:

  1. +++ b/core/modules/system/lib/Drupal/system/Annotation/ImageToolkitOperation.php
    @@ -0,0 +1,53 @@
    +  /**
    +   * The title of the image toolkit.
    +   *
    +   * The string should be wrapped in a @Translation().
    +   *
    +   * @ingroup plugin_translatable
    +   *
    +   * @var \Drupal\Core\Annotation\Translation
    +   */
    +  public $title;

    I would name it label instead title to be consistent with ImageEffect plugin.

    Also I would add a description info to annotation.

  2. +++ b/core/modules/system/lib/Drupal/system/Annotation/ImageToolkitOperation.php
    @@ -0,0 +1,53 @@
    +  public $toolkit_id;

    Let's name it simple: $toolkit.

  3. +++ b/core/modules/system/lib/Drupal/system/Annotation/ImageToolkitOperation.php
    @@ -0,0 +1,53 @@
    +  public $operation;

    Drop this. We'll use the same "effect" method in each plugin. And, yes, we need an interface to standardize this plugin implementation. Add an ImageTookitOperationInterface and define there a process() method. Then each plugin (e.g. GDToolkitCrop should implement process() (instead of crop(), resize(), etc.).

  4. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
    @@ -293,4 +146,15 @@ public static function isAvailable() {
    +  public function operation($name, $arguments) {

    Let's rename it to process(). Possible code?

    <?php
    public function process($name) {
     
    $plugin = \Drupal::service('image.toolkit.operation.manager')->createInstance($name);
     
    $arguments = func_get_args();
      unset(
    $arguments[0]);
      return
    call_user_func_array('process', array($plugin, $arguments));
    }
    ?>

    And will be invoked in this way:

    <?php
    return $this->toolkit->process('resize', $this, $width, $height);
    ?>

    Hmm... And it seems that we need to add this method to ImageToolkitInterface and add an abstract ImageToolkitBase with the implementation of process() method as this will never change in GD or ImageMagick.

  5. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitOperation/GDToolkitCreateTmp.php
    @@ -0,0 +1,71 @@
    + *   id = "gd_create_tmp",
    ...
    +  public function createTmp(ImageToolkitInterface $toolkit, ImageInterface $image, $width, $height) {

    Remove "gd_" prefix, we don't need that as we have the "toolkit" value.

  6. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitOperation/GDToolkitCreateTmp.php
    @@ -0,0 +1,71 @@
    +  public function createTmp(ImageToolkitInterface $toolkit, ImageInterface $image, $width, $height) {

    Instead of passing $toolkit for each plugin, let's define an abstract class ImageToolkitOperationBase with a protected $toolkit; property. Then in constructor, get the toolkit object from annotation (not sure we can do that...) and set $this->toolkit property. So, will be available to all implementations. An implementation will look like:

    <?php
    class GDToolkitCrop extends ImageToolkitOperationBase { }
    ?>

    and base

    <?php
    abstract class ImageToolkitOperationBase extends PluginBase implements ImageToolkitOperationInterface { }
    ?>

And finally... I'm not very sure that "toolkit operation" (aka ImageToolkitOperation) is the right naming. What you think? Toolkit effect? Toolkit process? Open discussion here but let's go with "ImageToolkitOperation" for now.

Status:Needs review» Needs work

Changed the status + an addition to the last review.

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitOperationManager.php
@@ -0,0 +1,35 @@
+  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, LanguageManager $language_manager) {
+    parent::__construct('Plugin/ImageToolkitOperation', $namespaces, 'Drupal\system\Annotation\ImageToolkitOperation');
+    $this->setCacheBackend($cache_backend, $language_manager, 'image_toolkit_operation');
+  }

Not sure if we don't have to add here also an alteration point, before cache.

Status:Needs review» Needs work

process() or apply()? :)

Status:Needs work» Needs review

@claudiu.cristea thanks. Cool, let me process #22 somehow and post a new patch.

#3 - if we make an ImageTookitOperationInterface and declare 'process', then we cannot have a variable number of arguments, and would need to pass those as an array a la

public function process(ImageToolkitInterface $toolkit, array $variables);

fine for me but just wanted to make sure.

#5 - my understanding was that id must be unique - if we just have id = "create_tmp", then we would have two same plugins id for GD and ImageMagick (apart from the other annotation key)? Or am I wrong (sorry I am pretty new to plugin system).

Thanks again

Status:Needs review» Needs work

In addition (or response) to the review of #22:

#22.3/#24: Are there existing examples in Drupal? Other options are exec() or execute()?

#22.5: Leave this method in GdToolkit it is a helper method that other methods in GdToolkit and all of its effect-toolkit plugins may call.

#22.6: Can the toolkit be passed to the effect-toolkit plugin upon creation? I consider toolkit as a typical property for an effect-toolkit plugin, not a parameter to one of its methods. This probably means it should become part of the interface and thus not a property but a getter/setter method (I think that properties cannot be specified in an interface in PHP).

public function process(ImageToolkitInterface $toolkit, array $variables);

I'm OK with this. Then we can avoid call_user_func_array() and call with

<?php
return $plugin->process($this, $arguments);
?>

#5 - my understanding was that id must be unique - if we just have id = "create_tmp", then we would have two same plugins id for GD and ImageMagick (apart from the other annotation key)? Or am I wrong (sorry I am pretty new to plugin system).

You're right. Let's keep the prefix now but we need somehow to fix this.

Crossposted again. But the status is NW anyway. @claudiu will react even if it is NW. If it becomes an interface (and it should) we indeed go for 1 array with all data in a single argument, but $image would better be a separate argument:

public function process(ImageInterface $img, array $data);

Status:Needs work» Needs review
StatusFileSize
new25.28 KB
new33.52 KB
FAILED: [[SimpleTest]]: [MySQL] 59,158 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

OK then. Patch here:

  • covers all of #22, with the exception of 6. The relevant code is in ImageToolkitOperationBase, commented out. I'm reluctant as I feel like re-instantiating the image tollokit upon creation of the operation plugin seems to me overkill. We do not need another instance of the same plugin, we have that open already. Also there's a risk of endless loops. Last, passing the toolkit to the operation 'process' method only occurs between the image toolkit plugin and the operation plugin itself. However, code is there, easy to activate if we are convinced it is the right thing to do.
  • Added the alter decorator in ImageToolkitOperationManager
  • The image is a separate argument to the process call
  • We need to pass toolkit to process if we want an operation to be able to call another operation in the same toolkit (we have Resize and Crop calling createTmp in core, but in contrib there is more (in Textimage for sure))

Still no work on plugin discovery based on weights/reference toolkit etc (can you help here?), nor tests. Also not much doc...

#29.1/29.4: Imagemagick toolkit stores a a list of arguments it passes to the shell when invoking convert. This only happens at saving the image. So multiple instances will NOT work. Nor will overrides of quality and format settings, as those are also not used until ImageToolkit::save() is called. Can't we pass the toolkit via the service instantiator? createInstance() accepts a $config array.

#29.2: Not sure if an alter decorator is still needed when we have discovery based on weight and other annotation content. Hook_alter is (was) very useful in D7 and before but I'm not so sure about that in this environment.

Status:Needs review» Needs work

The last submitted patch, 2073759-toolkit_operations-29.patch, failed testing.

Status:Needs work» Needs review

Thanks @fietserwin.

So based on #30, creating a new instance of toolkit in the operation constructor is a no go. The options left are to pass the 'master' toolkit either as (1) a variable to the 'process' method of the operation toolkit (current patch), or (2) as a key/value in the $configuration array passed to the ImageToolkitOperationManager createInstance method. I see little difference between the two, every ImageToolkitOperation plugin will have only one public 'process' method exposed anyway, and will not be called directly from the image style effects plugins but always via $xx->toolkit->process. Any preference??

On the alter decorator, I assume it would be good to leave to modules the option to change the weight of plugins provided by other modules. Is that the concept @claudiu.cristea?

Status:Needs review» Needs work

Xpost

Status:Needs work» Needs review

Can I have a review at this stage?

  1. +++ b/core/modules/system/lib/Drupal/system/Annotation/ImageToolkitOperation.php
    @@ -0,0 +1,53 @@
    +  /**
    +   * The label of the image toolkit operation.
    +   *
    +   * @var string
    +   */
    +  public $label;
    +

    I had a look at some other annotations: label is indeed the term to use but it should be translatable and is always described with "The human-readable name ...":
    /**
    * The human-readable name of the image toolkit operation.
    *
    * @ingroup plugin_translatable
    *
    * @var \Drupal\Core\Annotation\Translation
    */
    public $label;

    BTW: image toolkit annotation (Drupal\system\Annotation\ImageToolkit) uses title. That should be changed in a separate issue.

  2. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
    @@ -231,50 +129,6 @@ public function getInfo(ImageInterface $image) {
    -  public function createTmp(ImageInterface $image, $width, $height) {
    -    $res = imagecreatetruecolor($width, $height);

    Leave this method in GDToolkit.

  3. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitBase.php
    @@ -0,0 +1,75 @@
    +abstract class ImageToolkitBase extends PluginBase implements ImageToolkitInterface {
    +

    It is an abstract class, so its methods may be abstract instead of returning some default value.

    Moreover, I think hat the base class does not have to implement the interface, unless we can come up with some sane default implementations for some (preferably more than one) of the methods in the interface that can be used by both GD and Imagemagick.

    If we stick with implementing the interface it does not have to be repeated on the derived classes.

  4. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitInterface.php
    @@ -189,4 +117,19 @@ public static function isAvailable();
    +  public function process($operation, ImageInterface $image, array $variables = NULL);
    }

    Should we rename $variables to $data? ImageEffectBase uses the term data.

  5. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitInterface.php
    @@ -189,4 +117,19 @@ public static function isAvailable();
    }

    Change NULL into empty array?
    public function process($operation, ImageInterface $image, array $variables = array());

    (Same for ImageToolkitOperationInterface and other places where it is used.)

  6. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitOperation/GDToolkitCrop.php
    @@ -0,0 +1,65 @@
    +  /**
    +   * Crops an image.
    +   *
    +   * @param \Drupal\Core\Image\ImageInterface $image
    +   *   An image object. The $image->resource, $image->info['width'], and
    +   *   $image->info['height'] values will be modified by this call.
    +   * @param int $x
    +   *   The starting x offset at which to start the crop, in pixels.
    +   * @param int $y
    +   *   The starting y offset at which to start the crop, in pixels.
    +   * @param int $width
    +   *   The width of the cropped area, in pixels.
    +   * @param int $height
    +   *   The height of the cropped area, in pixels.
    +   *
    +   * @return bool
    +   *   TRUE or FALSE, based on success.
    +   *
    +   * @see image_crop()
    +   */
    +  public function process(ImageToolkitInterface $toolkit, ImageInterface $image, array $variables = NULL) {
    +    $res = $toolkit->process('createTmp', $image, array('width' => $variables['width'], 'height' => $variables['height']));

    There is now 1 $data parameter: change the doc to describe the expected/required content of the array.

  7. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitOperation/GDToolkitDesaturate.php
    @@ -0,0 +1,51 @@
    +  public function process(ImageToolkitInterface $toolkit, ImageInterface $image, array $variables = NULL) {
    +    // PHP installations using non-bundled GD do not have imagefilter.

    Lacking parameter doc: specify it should be empty.

  8. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitOperationBase.php
    @@ -0,0 +1,40 @@
    +  public function process(ImageToolkitInterface $toolkit, ImageInterface $image, array $variables = NULL) {
    +    return FALSE;
    +  }
    +

    Make it abstract and remove the method body.

Thank you @fietserwin!

I am really struggling with what should go in the annotation. I do not think we will will have a UI for these plugins, do we? So what would be the purpose of human readable and translatable 'label' and 'description'?

The information we need there, I believe, is:

  • toolkit - the toolkit for which the operation is implemented (gd, imagemagick, etc.);
  • operation - the operation being implemented by the process() method in the plugin (resize, desaturate, etc.);
  • weight - an index to be able to prioritize toolkit/operation implementations, and pick the one with lower weight;
  • id - a unique id for the plugin. I wonder if this could be built programmatically, something like {toolkit}_{operation}_{module}. This way we could have a standard naming to enable discovery. So if we need to run a desaturate operation via gd toolkit, and we have two implementations (say gd_desaturate_system and gd_desaturate_mymodule we can take the one with lower weight. I do not know if this is possible though.

Thoughts?

I don't have any experience with it either. But by just looking at other annotations, I see that (almost?) all annotations have a translatable label, so apparently there is some use for it foreseen, albeit in contrib.

Description is less used, but if we stick with a fixed naming scheme, it is also not that important, ImageEffects should have a description but we are talking about plugins that are strictly linked to ImageEffects, so these plugins should not be looked at in isolation.

Looks like if we implement a findDefinitions method in ImageToolkitOperationsManager, we can enforce the id and plugin_id to the naming standard as per #36.

Like (rough)

  protected function findDefinitions() {
    $definitions = parent::findDefinitions();
    foreach ($definitions as $plugin_id => &$definition) {
      $definition['id'] = $definition['toolkit'] . '_' . $definition['label'] . '_' . $definition['provider'];
    }
    $new_definitions = array();
    foreach ($definitions as $plugin_id) {
      $new_definitions[$plugin_id['id']] = $plugin_id;
    }
    return $new_definitions;
  }

A bit hacky though, as it still requires an id in the annotation which just get overridden by findDefinitions....

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitBase.php
@@ -0,0 +1,75 @@
+  public function process($operation, ImageInterface $image, array $variables = NULL) {
+    $plugin_id = ($operation == 'createTmp') ? 'gd_create_tmp' : 'gd_' . $operation;
+    $plugin = \Drupal::service('image.toolkit.operation.manager')->createInstance($plugin_id);
+    return $plugin->process($this, $image, $variables);
+  }
+

Becomes

  public function process($operation, ImageInterface $image, array $variables = NULL) {
    $manager = \Drupal::service('image.toolkit.operation.manager');
    $plugin_id = $manager->findPlugin($this->getPluginId(), $operation);
    $plugin = $manager->createInstance($plugin_id);
    return $plugin->process($this, $image, $variables);
  }

And in the manager class

  public function findPlugin($toolkitName, $operation) {
    $result = NULL;
    $resultWeight = PHP_INT_MAX;
    $definitions = parent::findDefinitions();
    foreach ($definitions as $plugin_id => $definition) {
      if ($definition['toolkit'] === $toolkitName && $definition['operation'] === $operation) {
        if ($definition['weight'] < $resultWeight) {
          $result = $plugin_id;
          $resultWeight = $definition['weight'];
        }
      }
    }
    return $result;
  }

Naming of this method is tbd, may also be something like findMatchingPlugin or findImplementingPlugin or just find() or findByToolkitOperation()

#39 sounds reasonable, we do not even have to change the original id. Just scan the operation plugins for toolkit, operation and weight. I wonder shall we cache that somehow during ImageToolkit class construction (a plugin bag?).

Status:Needs review» Needs work

Not sure what plugins will be returned and thus if we first should check if the array keys exists. 2 if's can be combined. I also don't get why we don't receive a list of objects of our new annotation class but keyed arrays instead.

Cache in the find method, not upon constructing?

#42 yep, you're right

Ok, I'll update and post a new patch with input so far. I'll leave 'label' and 'description' in the annotation, we'll decide later if we want to keep that.

Status:Needs work» Needs review
Issue tags:+DX (Developer Experience), +Plugin system
StatusFileSize
new27.01 KB
new29.81 KB
FAILED: [[SimpleTest]]: [MySQL] 58,436 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

Should have more or less everything from #30 to #44. Finally implemented a getToolkitOperationPluginId method in ImageToolkitOperationManager, but of course let's discuss.

Status:Needs review» Needs work

The last submitted patch, 2073759-toolkit_operations-45.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.93 KB
new40.74 KB
FAILED: [[SimpleTest]]: [MySQL] 58,725 pass(es), 11 fail(s), and 0 exception(s).
[ View ]

Just implemented ImageToolkitOperations for the 'test' image toolkit in the 'test_image' module. Not touched the tests themselves.

Some tests are a bit weird, they test that applying the scale/rotate/... effect calls the toolkit method scale/rotate/.... So that is going to be a lot of rework, or just delete such futile tests.

Status:Needs review» Needs work

The last submitted patch, 2073759-toolkit_operations-47.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.77 KB
new45.51 KB
FAILED: [[SimpleTest]]: [MySQL] 58,645 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

I know nothing about PHPUnit tests but let's see if this is OK.

Status:Needs review» Needs work

The last submitted patch, 2073759-toolkit_operations-50.patch, failed testing.

  1. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitBase.php
    @@ -0,0 +1,53 @@
    +      $plugin = $manager->createInstance($plugin_id);
    +      $this->operationPlugins[$operation] = $plugin;
    +    }
    +    return $plugin->process($this, $image, $data);
    +  }

    By removing the local $plugin and directly assigning and returning $this->operationPlugins[...] this code becomes much shorter and cleaner (if branch becomes superfluous).

  2. +   * @see image_crop

    Does not exist anymore?!

  3. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitOperationBase.php

    Is this file needed? it does not seem to contain any feature anymore.

    OK, if we pass $toolkit to the createInstance, we should add a $toolkit property and a constructor (or is it something like initialize?).

  4. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitOperationManager.php
    @@ -0,0 +1,53 @@
    +  public function getToolkitOperationPluginId($toolkitName, $operation) {
    +    $result = NULL;

    My fault: we'd better use $toolkitId instead of name as that suggest something human readable and translatable.

  5. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitOperationManager.php
    @@ -0,0 +1,53 @@
    +  }
    +  ¶
    +}

    White space on empty line.

  6. +++ b/core/modules/system/tests/modules/image_test/lib/Drupal/image_test/Plugin/ImageToolkit/TestToolkit.php
    +  public function logCall($op, $args) {

    public function logCall($op, $img, $data) ?? to make life easier and then do perhaps an array_unshift to add $img to $data or store both separately: $result[$op][] = array('img' => ..., ...)

I do not directly understand the test failures, but can have a better look at it if you don't find it either.

Status:Needs work» Needs review
StatusFileSize
new5.33 KB
new47.28 KB
FAILED: [[SimpleTest]]: [MySQL] 58,721 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Follow up on #50, no time for #52. Tomorrow.

Status:Needs review» Needs work
Issue tags:-DX (Developer Experience), -API change, -Plugin system

The last submitted patch, 2073759-toolkit_operations-54.patch, failed testing.

Status:Needs work» Needs review

#54: 2073759-toolkit_operations-54.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+DX (Developer Experience), +API change, +Plugin system

The last submitted patch, 2073759-toolkit_operations-54.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.55 KB
new47.23 KB
FAILED: [[SimpleTest]]: [MySQL] 58,333 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

StatusFileSize
new1.7 KB
new47.15 KB
FAILED: [[SimpleTest]]: [MySQL] 58,340 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Sorry for the mess, I cannot run PHPUnit tests locally so I am using simplytest

Status:Needs review» Needs work

The last submitted patch, 2073759-toolkit_operations-59.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.57 KB
new47.08 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/tests/Drupal/Tests/Core/Image/ImageTest.php.
[ View ]

New attempt, using at() seems too complicated

Status:Needs review» Needs work

The last submitted patch, 2073759-toolkit_operations-61.patch, failed testing.

StatusFileSize
new802 bytes
new47.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,564 pass(es).
[ View ]

Yes

Status:Needs work» Needs review

Runs on simplytest

Input from #52.

52.1 - OK

52.2 - OK

52.3 - I am leaving as is. Having a base class implementation can be useful for future purpose and/or to inject the toolkit at instantiation as @fietserwin says. Let's get more feedback here.

52.4 - OK in fact $toolkit_id as camelCase is not for method args

52.5 - OK

52.6 - In fact I am trying not to touch existing tests, I'd leave that to separate issues in case. The PHPUnit test I had to change as it is testing a specific structure of the methods/arguments which we changed by implementing the ImageToolkitOperation plugins.

Thanks @fietserwin!

StatusFileSize
new3.37 KB
new47.15 KB
PASSED: [[SimpleTest]]: [MySQL] 58,663 pass(es).
[ View ]

The patch...

Just found out... the $toolkit property in the Image class is protected, which makes impossible to invoke $image->toolkit->process(...) from within an image effect (which would be the norm in contrib for non-core operations).

Added a public method toolkitProcess($operation, $data) to the Image class that wraps the call to toolkit->process.

StatusFileSize
new2.58 KB
new48.3 KB
PASSED: [[SimpleTest]]: [MySQL] 58,769 pass(es).
[ View ]

... and patch ...

And why wouldn't I directly call $toolkit->process from within an ImageToolkitOperation.

This raises the question: what is an Image? Just a data holder that is passed along with the operations or has it (effect) logic built in as well? Why would a core effect be able to call its sister method on an image and contrib not, while we just decided that at toolkit level all effects are the same: core and contrib (we removed crop, resize, etc. from toolkit).

Should/can we remove crop, resize, etc from images as well?

And why wouldn't I directly call $toolkit->process from within an ImageToolkitOperation.

Well that one you can as $toolkit is explicitly passed to the ImageToolkitOperation.

Why would a core effect be able to call its sister method on an image and contrib not

Fair point, I have no opinion.

So, I'm in favor of ignoring the changes from #68, and removing the methods scale, scaleAndCrop, crop, resize, desaturate, and rotate from Image (and ImageInterface) as well.

Furthermore, in D7, the Imagemagick toolkit module add an ops[] property to images (being a StdClass this is no problem at all). In D8, this probably means that the imagemagick module has to create its own Image class and ImageFactory (as that does instantiate an object of type Image on get()). Subsequently, this means that the setResource(), hasResource() and getResource() methods from Image are useless, as they are GD specific. So they should be removed from ImageInterface as well and Image should probably be split into a BaseImage and a GdImage.

Wow, this one becomes really critical.

As ImageFactory becomes toolkit dependent, shouldn't the task of creating an Image become a task of the toolkit itself, thereby making ImageFactory redundant.

Shall we split off a separate issue "Allow contrib to implement a toolkit"...

Yeah @fietserwin, let's have a spin-off here. This issue is already complicated enough....

We can't roll-back #68 though, not unless we make $toolkit public which, I believe, is not a recommended practice.

Status:Needs review» Needs work

Oops, I thought ImageToolkitOperation could not call another operation. So forget my 1st remark from #69.

Indeed, image effects need to be able to call toolkit specific features, thus must have access to the toolkit. So I propose to change the getToolkitId() method on ImageInterface to getToolkit(). All usage, only watchdog message logging, should then be changed to $image->getToolkit()->getPluginId(). Why would image expose properties of other objects (toolkit in this case) anyway? That is not clean OO.

Issue summary:View changes

Added related issue

Status:Needs work» Needs review
StatusFileSize
new27 KB
new51.31 KB
FAILED: [[SimpleTest]]: [MySQL] 58,679 pass(es), 0 fail(s), and 12 exception(s).
[ View ]

This is based on #68.

  • Moved operations under ImageTookit to cleanup.
  • Renamed operation classes.
  • DX: Dropped "id" from annotation (!!!). The plugin ID is build later in manager.
  • Cleaned up a little.

Now, we rely on weight for plugin selection. I don't know if this is the right approach.

+++ b/core/lib/Drupal/Component/Plugin/Discovery/AnnotatedClassDiscovery.php
@@ -115,6 +115,8 @@ public function getDefinitions() {
+                // Allow ID to be built later. Set an index ID as fallback.
+                $definition['id'] = isset($definition['id']) ? $definition['id'] : count($definitions);

Bit of an edge case, but it could be possible that there's already an id == count($defintions) in the array. Append some random token? Very clever otherwise :)

+++ b/core/lib/Drupal/Core/Image/Image.php
@@ -225,6 +225,13 @@ public function getToolkitId() {
   /**
    * {@inheritdoc}
    */
+  public function toolkitProcess($operation, array $data = array()) {
+    return $this->toolkit->process($operation, $this, $data);
+  }
+

It seems you missed this from #68 (and in the interface). Was thinking again on it. Should toolkitProcess() be better named just operation() ? Then we have $image->operation('crop', array(...));. At the end that's what we're doing, applying an operation to the image, and within the Image class we are abstract from the underlying toolkit.

EDIT - seems #75 is from #66 not from #68

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/Operation/GDCrop.php
@@ -0,0 +1,66 @@
...
+use Drupal\Core\Annotation\Translation;
...
+use Drupal\system\Annotation\ImageToolkitOperation;
...
+   * Crops an image.

Ah, and I think we do not need this anymore, see #2090353: Don't require to put the use statement into plugin classes

Status:Needs review» Needs work

Thus the case described in #67 will fail. I prefer #73 to use to solve that, but we can defer that to #2103635: Remove effects from ImageInterface and use the solution of #68 for the moment.

I'm not happy with altering AnnotatedClassDiscovery. Though it is questionable to require each plugin to have a unique ID, i would not hack with it. If a unique Id is required/expected provide it. That discovery based on annotation values (instead of only id) so far was not needed surprise me more.

I'm not happy with altering AnnotatedClassDiscovery. Though it is questionable to require each plugin to have a unique ID, i would not hack with it. If a unique Id is required/expected provide it. That discovery based on annotation values (instead of only id) so far was not needed surprise me more.

Now if you miss id in the annotations, you simply get an error. Not sure if providing a default fallback is a good or bad thing. I believe this is a broader topic.

In any case for our specific case I was wondering if we can still do something like this (taken from earlier discussion in #38 and #39), which would alter the ids and cache them at findDefinitions time, and remove the need for finding the most appropriate plugin at runtime (conceptual, untested)

+  /**
+   * Returns the plugin ID for the plugin having the lowest weight, for a given
+   * toolkit and operation.
+   *
+   * @param string $toolkit
+   *   The toolkit ID.
+   * @param string $operation
+   *   The operation (e.g. "crop").
+   *
+   * @return string
+   *   A full image toolkit operation plugin ID (e.g. "system.gd.crop").
+   */
+  public function getToolkitOperationPluginId($toolkit, $operation) {
+    $definitions = $this->getDefinitions();
+    uasort($definitions, '\Drupal\Component\Utility\SortArray::sortByWeightElement');
+    $pattern = '|\\.' . $toolkit . '\\.' . $operation . '$|';
+    foreach ($definitions as $plugin_id => $definition) {
+      if (preg_match($pattern, $plugin_id)) {
+        return $plugin_id;
+      }
+    }
+  }
+

Becomes

  /**
   * Builds the list of plugin IDs, altering the original value.
   *
   * Return the plugin having the lowest weight, for a given
   * toolkit and operation.
   *
   * @return array
   *   An array of image toolkit operation plugin definitions.
   */
  public function findDefinitions() {
    $definitions = parent::findDefinitions();
    uasort($definitions, '\Drupal\Component\Utility\SortArray::sortByWeightElement');
$toolkit = $operation = '';
$new_definitions = array();
    foreach ($definitions as $plugin_id => $definition) {
      if ($toolkit != $definition['toolkit'] || $operation != $definition['operation']) {
$toolkit = $definition['toolkit'];
$operation = $definition['operation'];
$id = $toolkit . '.' . $operation;
$definition['id'] = $id;
$new_definitions[$id] = $definition;
      }
    }
return $new_definitions;
  }

and in ImageToolkitBase

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitBase.php
@@ -0,0 +1,53 @@
+  /**
+   * Processes a toolkit operation.
+   *
+   * @param string $operation
+   *   The toolkit operation to be processed.
+   * @param \Drupal\Core\Image\ImageInterface $image
+   *   An image object.
+   * @param array $data
+   *   An array of variables to be used by the toolkit operation.
+   *
+   * @return mixed
+   *   The return value of the image toolkit operation.
+   */
+  public function process($operation, ImageInterface $image, array $data = array()) {
+    // Check if operation plugin has been instantiated already. If not,
+    // create an instance.
+    if (!isset($this->operationPlugins[$operation])) {
+      $manager = \Drupal::service('image.toolkit.operation.manager');
+      $plugin_id = $manager->getToolkitOperationPluginId($this->getPluginId(), $operation);
+      $this->operationPlugins[$operation] = $manager->createInstance($plugin_id);
+    }
+    // Return the the value returned by the operation process method.
+    if (isset($this->operationPlugins[$operation])) {
+      return $this->operationPlugins[$operation]->process($this, $image, $data);
+    }
+    return FALSE;
+  }
+
+}

becomes

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitBase.php
@@ -0,0 +1,53 @@
+  /**
+   * Processes a toolkit operation.
+   *
+   * @param string $operation
+   *   The toolkit operation to be processed.
+   * @param \Drupal\Core\Image\ImageInterface $image
+   *   An image object.
+   * @param array $data
+   *   An array of variables to be used by the toolkit operation.
+   *
+   * @return mixed
+   *   The return value of the image toolkit operation.
+   */
+  public function process($operation, ImageInterface $image, array $data = array()) {
+    // Check if operation plugin has been instantiated already. If not,
+    // create an instance.
+    if (!isset($this->operationPlugins[$operation])) {
+      $manager = \Drupal::service('image.toolkit.operation.manager');
+      $this->operationPlugins[$operation] = $manager->createInstance($this->getPluginId() . '.' . $operation);
+    }
+    // Return the the value returned by the operation process method.
+    if (isset($this->operationPlugins[$operation])) {
+      return $this->operationPlugins[$operation]->process($this, $image, $data);
+    }
+    return FALSE;
+  }
+
+}

Status:Needs work» Needs review
StatusFileSize
new6.84 KB
new51.13 KB
PASSED: [[SimpleTest]]: [MySQL] 58,344 pass(es).
[ View ]

I fixed here only tests from #75. Didn't had time to address #76, #77.

@fietserwin,

I'm not happy with altering AnnotatedClassDiscovery. Though it is questionable to require each plugin to have a unique ID, i would not hack with it. If a unique Id is required/expected provide it. That discovery based on annotation values (instead of only id) so far was not needed surprise me more.

That is not hack, it's a fix :)

But anyway, because we're touching the Holly Plugin let's see what plugin system gurus have to say. That's why I dropped #2104017: Let discovery work even if no plugin ID was annotated in for discussions and feedback.

Status:Needs review» Needs work

TODO:
- The annotation class needs a property weight
- undo the removal of id
- undo the annotation "hack" (see #2104017: Let discovery work even if no plugin ID was annotated)
- incorporate #77 (if not already done)

Other remarks:
- I'm OK with moving the operations to a subdirectory, and renaming them, so that can stay in.
- re #76: We are already applying the operation, this is about calling the toolkit specific part of it, so just process() or apply() seems a bit too vague.

+++ b/core/core.services.ymlundefined
@@ -527,6 +527,9 @@ services:
   image.toolkit.manager:
     class: Drupal\system\Plugin\ImageToolkitManager
     arguments: ['@container.namespaces', '@cache.cache', '@language_manager']
+  image.toolkit.operation.manager:
+    class: Drupal\system\Plugin\ImageToolkitOperationManager

Any reason we're not providing alterability by passing the module handler?

Also it seems like there's an interdependency between these... no? I'm having trouble finding out what the actual process() method does, but it seems to invoke a toolkit operation plugin, which means that toolkit operation manager should probably be getting injected somewhere. But that's totally just a guess based on what I THINK is going on here.

+++ b/core/lib/Drupal/Component/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -115,6 +115,8 @@ public function getDefinitions() {
+                // Allow ID to be built later. Set an index ID as fallback.
+                $definition['id'] = isset($definition['id']) ? $definition['id'] : count($definitions);

I mentioned this on the separate issue that was filed for this specific code, but you absolutely can't do this. You have no control over the order in which plugins will be discovered meaning that you're literally doing something like $manager->createInstance(1); and that's completely unreliable. On a cache clear that was because of new modules or removal of existing ones, new plugins could be added or removed and 1 would be a completely different plugin that you just instantiated. Not ok.

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitOperationBase.phpundefined
@@ -0,0 +1,25 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\system\Plugin\ImageToolkitOperationBase.
+ */
+
+namespace Drupal\system\Plugin;
+
+use Drupal\Core\Plugin\PluginBase;
+use Drupal\Core\Image\ImageInterface;
+use Drupal\system\Plugin\ImageToolkitInterface;
+use Drupal\system\Plugin\ImageToolkitOperationInterface;
+
+/**
+ * Base class implementation of a image toolkit operation.
+ */
+abstract class ImageToolkitOperationBase extends PluginBase implements ImageToolkitOperationInterface {
+
+  /**
+   * {@inheritdoc}
+   */
+  public abstract function process(ImageToolkitInterface $toolkit, ImageInterface $image, array $data = array());
+

Completely useless. You already have an interface. It has all your documentation, there's no reason to create a base class that then makes an abstract of the only method on the interface. Delete this.

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitOperationManager.phpundefined
@@ -0,0 +1,71 @@
+  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, LanguageManager $language_manager) {
+    parent::__construct('Plugin/ImageToolkit/Operation', $namespaces, 'Drupal\system\Annotation\ImageToolkitOperation');
+
+    // Create valid IDs for plugin definitions.
+    $definitions = array();
+    foreach ($this->discovery->getDefinitions() as $definition) {
+      $id = $definition['provider'] . '.' . $definition['toolkit'] . '.' . $definition['operation'];
+      $definition['id'] = $id;
+      $definitions[$id] = $definition;
+    }
+    $this->definitions = $definitions;
+
+    $this->setCacheBackend($cache_backend, $language_manager, 'image_toolkit_operation');
+  }

WAT?!? No.

Everything this does is already handled by calling getDefinitions() under normal working conditions. There's exactly 0 reason to ever do this in the constructor, that incurs unnecessary penalty for instantiating this class. Also, calling to $this->discovery->getDefinitions() bypasses all the benefits that using the DefaultPluginManager got you. Just check another plugin manager that's extending that base class, you don't have to do much of anything in the __construct().

Just setCacheBackend() and probably alterInfo() so that you can drop in replacements for toolkit operations with better plugins as necessary.

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitOperationManager.phpundefined
@@ -0,0 +1,71 @@
+  public function getToolkitOperationPluginId($toolkit, $operation) {
+    $definitions = array_filter($this->getDefinitions(), function ($definition) use ($toolkit, $operation) {
+      return $definition['toolkit'] == $toolkit && $definition['operation'] == $operation;
+    });
+    if ($definitions) {
+      if (count($definitions) > 1) {
+        uasort($definitions, '\Drupal\Component\Utility\SortArray::sortByWeightElement');
+      }
+      $definition = reset($definitions);
+      return $definition['id'];
+    }

So I was going to suggest that your plugin ids should be essentially $toolkit$operation concatenated together. This removes any question as to what plugin is in charge of what operation and puts you back into the situation where if you need to change out how say... desaturation is working, you just replace the existing plugin for it, or you make up a new operation to handle your custom desaturation needs.

This removes the funky need for weight and really... this whole function. Or you can keep this function and just do $this->getDefinitions($toolkit . $operation);

I like having the toolkit and operation stuff notated separately as it could give you some cool options long term, but you have to provide sane defaults and let other modules worry about the interaction pattern of providing multiple plugins for the same operation on the same toolkit. I'm not sure how common that's even likely to be.

Eclipse

#83: Thanks for joining. this is some input from the plugin/annotation experts we were waiting for.

What we would like to accomplish:
- Separate image toolkit specific code for image effects from both image effects plugins and the toolkit class. If multiple contrib modules want to add an image effect, they cannot all rely on class inheritance: they would clash. So extending an ImageToolkit or Image is a no go for image effect developers: hence, a plugin structure for ImageEffect's and image toolkit specifc code for these seems perfect for this.

- Allow to swap (replace or whatever you would like to name it) implementations at the image toolkit operation level (This allows to cater for e.g. specific usages or different versions of the underlying toolkit ("Help, Imagemagick toolkit does not work with imagemagick 4.2.8 and my hoster refuses to upgrade").

What we did:
- Define an ImageToolkitOperation plugin type with associated annotation that defines what operation (effect) for what toolkit it implements.
- Move the effect code from the GD image toolkit class to these new operation classes.
- Add a method in ImageToolkit(Interface) that allows an effect to call the toolkit specific code. This method, process(), should load the correct plugin and call it.

Where went our efforts wrong:
- How does this discovery of plugins work? How can the process() method get the correct plugin returned.

Some hints about the design (what classes and methods we should develop) to do that in the way as envisioned by the plugin and annotation system would be nice. I will rework the patch if you (or tim.plunkett) could give these hints.

Status:Needs work» Needs review
StatusFileSize
new33.31 KB
new58.86 KB
PASSED: [[SimpleTest]]: [MySQL] 58,773 pass(es).
[ View ]

New patch, based on #81,with the following changes:

#82:
- Added (reintroduced) property id to ImageToolkitOperation annotation. Pattern = {toolkit}.{operation}
- Added property weight to ImageToolkitOperation annotation.

#83
- Added @module_handler in core.services.yml and thus also to the constructor of ImageToolkitOperationManager and the constructor now calls the alterInfo() method.
- Removed id assigining code from AnnotatedClassDiscovery.
- Removed ImageToolkitOperationBase.
- Removed the id juggling code from the constructor of ImageToolkitOperationManager.

#77
- Removed the use of annotation related namespaces.

#67
- Re-added a solution for this problem but the solution is like depicted in #73 ($image->getToolkit()->getPluginId()). In Image, changed all uses of $this->toolkit to $this->getToolkit().

Overall:
- Improved some comments.
- Placed unreachable watchdog code before the return FALSE;
- Specified the exact type of the toolkit in the doc comments of the GD and Test toolkit operations. Changing the method signature is not allowed.

TODO:
- I did not touch the weight based selection yet. But it looks like we have to dictate the id, and that as long as that pattern contains the {toolkit} and {operation}, we can instantiate based on plugin id solely. Overruling an operation from contrib is done via the alterInfo(), something like $definitions[{toolkit}.{operation}] = $definitions[{id.of.my.plugin}]?

- I was wondering: a toolkit operation is tied both to the image effect and to a toolkit. So why did we place these operations under the toolkit and not under (or close to) the image effects. In contrib they will likely be placed close to the image effects . So I would propose to move them to namespace \Drupal\image\Plugin\ImageEffect\ToolkitOperation. But in core this is part of the image module, while the current location \Drupal\system\Plugin\ImageToolkit\Operation is part of the system module.

But it looks like we have to dictate the id, and that as long as that pattern contains the {toolkit} and {operation}, we can instantiate based on plugin id solely. Overruling an operation from contrib is done via the alterInfo(), something like $definitions[{toolkit}.{operation}] = $definitions[{id.of.my.plugin}]?

If we rely on {toolkit}.{operation} id pattern, then you'll not be able to replace an operation from contrib by providing a plugin with a weight lower that the core (or other module). You'll need to provide the new definition by altering. And this is ugly. That was the whole idea of using annotated plugins. You can have even an empty module but having the plugin under lib/Drupal/... That's why I would add the module name too as prefix to the id pattern. This would solve this issue: system.gd.resize. With this the developer needs only to take care of weight.

I still didn't dropped the idea of auto-namespacing the id with provider followed by toolkit and then by operation. Didn't had time to look deep into plugin stuff (decorators, derivatives, mappers). That would have been the real and clean DX solution.

Also I'm not very happy with weight selection too. What are the possible alternatives? UI selection? UI may have the advantage that can let the user/admin interact and choose but allows also contrib to set programatically the desired plugin in case of id collision while weight can be overwritten in alter hooks invoked by the plugin manager constructor.

And now the patch became to big to be reviewed. We need to make one of #2073759: Convert toolkit operations to plugins, #2103621: Move GD logic from ImageInterface to toolkit, tie toolkit instance to Image instance, toolkits should no longer be instantiated separately or this one a meta issue and agree first on the image system architecture. Just continuing with this without knowing exactly how we want to look the whole system doesn't make any sense.

#86: altering may be ugly, but if that is how it is done in other places we better follow that practice. How can contrib replace an image effect in the current version of D8? Replacing an image effect toolkit operation should be done the same way.

To reduce patch size:

Start an issue with a bit of cleaning:
- method getToolkitId() => method getToolkit() including all its uses
- removal of that ; after "Contains \Drupal\system\Plugin\ImageToolkit\GDToolkit";
This removes some 12k of the patch and gives a patch of about 12K on its own and very easily reviewed

Continue with an issue introducing the concept of a "toolkit operation" without moving core to it. At least one test effect should be moved to prove the concept. This patch mainly adds code and will be conceptually the most difficult to review.

Finish this issue by moving core effects to use toolkit operations. This is mainly code moving and adapting the test to that. Large patch, but not difficult.

Continue with the other related issues [##2103621] and #2103635: Remove effects from ImageInterface.

- Removed ImageToolkitOperationBase.

Not sure. True that for 4 core operations this is not big deal. But with contrib we'll get dozens of toolkit/op combinations. If for any reason we need to get some code common to them all it will be difficult. I was seeing ImageToolkitOperationBase kinda 'foundational'. We have PluginBase, ControllerBase, FormBase, ToolkitBase (with this patch), etc. Why not here?

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/Operation/GDDesaturate.phpundefined
@@ -0,0 +1,55 @@
+  public function process(ImageToolkitInterface $toolkit, ImageInterface $image, array $data = array()) {
+    // PHP installations using non-bundled GD do not have imagefilter.
+    if (!function_exists('imagefilter')) {
+      watchdog('image', 'The image %file could not be desaturated because the imagefilter() function is not available in this PHP installation.', array('%file' => $image->getSource()));
+      return FALSE;
+    }
+
+    return imagefilter($image->getResource(), IMG_FILTER_GRAYSCALE);

Is it just me? or is $toolkit NEVER used here? In fact, in most of these, $toolkit doesn't seem to ever be used, and... that feels really wrong.

Looking at the php documentation, I had a hard time finding any sort of image class that can do the various imagefilter, imagedestroy, etc calls. I guess that makes this more a failing of php, but that's exactly what I'd expect the ImageToolkitInterface to provide (a set of methods that standardizes that across toolkits) and I'm becoming increasingly confused by these plugins. If they exist to provide contrib the ability to swap individual "method" implementations, then... ok that makes some sense, but at the end of the day, resizing an image (or whatever) is pretty standardized. Adding new implementations for undefined methods makes good sense to me though. Some clarification on this I think might help.

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitOperationManager.phpundefined
@@ -0,0 +1,68 @@
+  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, LanguageManager $language_manager, ModuleHandlerInterface $module_handler) {
+    parent::__construct('Plugin/ImageToolkit/Operation', $namespaces, 'Drupal\system\Annotation\ImageToolkitOperation');
+
+    $this->alterInfo($module_handler, 'image_toolkit_operation');
+    $this->setCacheBackend($cache_backend, $language_manager, 'image_toolkit_operation');

MUCH BETTER! Thank you.

@mondrake,

We shouldn't make completely useless classes. An abstract method of an interface enforced method on a base class is completely nonsensical. If at some point we end up with code that makes sense to put on a base class, then we can reopen the discussion at that time, but there's no reason to have one just because of some convention (which, there's not a convention so...).

@claudiu.cristea, @fietserwin,
I don't find the patch difficult to review in the slightest. The code is pretty clean for the most part and generally easy to follow. Other than the confusion I outlined in this comment above, I think this generally makes great sense, and is not particularly large. If the patch were to double in size, we might should look for some alternatives, but even then, only if it made a ton of sense. The patch currently seems pretty cohesive in its topic, so I don't see a reason to break it up at all.

Yes, altering is how this is done elsewhere in core, and it's really not particularly ugly at all. I'd argue it's a lot better than having 10 implementations of the same basic image operation but only being able to use 1 of them. Likewise, having real plugin ids supports getting at ANY of the image operations that are provided. I would expect these operations to be available during image effect building, so your discussion of a UI is ++ from me if that's not already happening.

As a last note, I sort of wish there were imagemagick code here as well. Having more than one implementation always helps to reinforce that the path taken is sane. That may not be possible (I'm not sure whether we can provide that in core) but example code, or a contrib module to compare against that just had the toolkit and operations in it might be really nice. Just a thought.

Eclipse

Yes, altering is how this is done elsewhere in core, and it's really not particularly ugly at all

Maybe it's a matter of taste but what I found very nice to the new plugin system (and thank you for that!) is also that adding a new implementation of "something" is a clean process in terms of DX. Wanna provide your own version of "gd.crop"? No problem, just create a new empty module and drop your plugin in lib/Drupal/Plugin/ImageTookit/Operation/ and that's all. Maybe it needs an UI additional step if there's a UI to select the variant (which I prefer instead of weight mechanism). Altering is something dedicated when we really want to ALTER something not to PROVIDE a new "something".

#89 I am co-maintaining Textimage, and you can take a look at the current implementation in 7.x-3.x of GD toolkit functions, mainly around font management, that is totally absent from core (determination of bounding boxes, laying out text over image, etc.). You can also see instances of a toolkit function calling another toolkit function (currently nothing like that in core AFAICS). I am sure @fietserwin can point you to even better examples in Imagecache Actions.

I am really waiting for the solution here to move Textimage conversion to D8 forward. I would need 10 operation plugins for that.

So if there is consensus on this one (+1 from me, this is enough to start with), please move this on.

There is more discussion needed for sure on other topics, I agree on @claudiu proposal on a meta.

Title:Convert toolkit operations to pluginsAllow contrib to implement toolkit specific code for image effects

@EclipseGc
We have ImageEffect that defines an operation on an Image. An ImageEffect plugin defines the UI and some other things, so an ImageEffect can be added to an ImageStyle. However, to actually apply the operation to an image we need to call functions of an actual image handling library (e.g. GD) or executable (e.g. Imagemagick).

Thus the apply method of ImageEffect Scale eventually need to execute code that is specific for the toolkit (imagecopyresampled() for GD; add parameters -resize {width}x{height}! when calling convert(.exe).

The ImageToolkitOperation plugin we are defining here is about being able to execute that toolkit specific code for an ImageEffect. And it is about being able to do so without extending Image and ImageToolkit, because that will clash when multiple contrib modules start adding ImageEffects and/or ImageToolkits. Hence our plugin based solution that defines this code in a place separate from ImageEffect and ImageToolkit.

answering your question about $toolkit never being used: I guess it will more often not be used than be used, because most of the time the operation is straightforward: just a single or a few GD functions or adding a few Imagemagick parameters. But D7 contrib does define Image effects that e.g. allow to alter the image format (e.g. from jpg to png to add transparency to a rotated image) or to overrule the default quality parameter. In these cases we certainly need access to the toolkit.

I'd expect the ImageToolkitInterface to provide (a set of methods that standardizes that across toolkits)

Correct, but the interface that core defines will never be enough for all needs of the ImageEffect's that contrib may come up with. So we need a way to extend ImageToolkit without taking the traditional inheritance path (as that will lead to clashes).

re #89, #90: I have the same ideas as claudiu.cristea about easily adding your plugin, even if it replaces an already existing one. But apparently, that is not the way it is currently done. I'm fine with requiring an alterInfo hook for those cases, that admittedly will be rare. Most of the times it will be new plugins that come with new ImageEffect's from the same contrib module, or in the case of Imagemagick toolkit, that will come with its own imagemagick specific implementations for the core effects and they don't overrule the GD operations either.

Ok, it sounds like we're actually pretty close over all here in terms of expectations and desired end result, so I'm going to spend a little time explaining some plugin specific stuff just to make sure we're all on the same page.

<?php
/**
* @ImageToolkitOperation(
*   id = "gd.crop",
*   toolkit = "gd",
*   operation = "crop",
*   label = @Translation("Crop"),
*   description = @Translation("GD2 toolkit crop")
* )
*/
?>

I've removed weight here, because you don't need it. This is the gd toolkit crop operation. Odds are it'll NEVER need to change cause it does what is necessary, but if it DID need to change, the limited nature of this plugin dictates that you'd likely only ever need ONE override for it. Explicitly putting this together in an alter statement is far superior to a weight management solution because you actually have no clue which plugin is getting used in the case of weight, and more to the point, modules could unintentionally hijack what you're doing there by simply typoing their weight, or not really understanding what the weight means.

Likewise, if you wanted a customized version of crop, having:

<?php
/**
* @ImageToolkitOperation(
*   id = "gd.my_module_crop",
*   toolkit = "gd",
*   operation = "my_module_crop",
*   label = @Translation("My Module Crop"),
*   description = @Translation("GD2 toolkit crop custom to My Module")
* )
*/
?>

This is TOTALLY sane and should be accessible in the UI. If we wanted to go one step further and figure out how to have both of them's operation just say "crop" so that you could bring up all crop operations that are available and choose the one you want from the UI w/o intermixing it with other operations, that's also probably very doable (though I'm going to have to investigate your ImageToolkitOperationManager a little bit but I don't imagine that this would be difficult code wise, it's probably the UI where this becomes interesting).

Your general explanation of why you're doing what you're doing was what I was hoping you'd say, so ++ to that. I really like where this is going, we can probably just finish up these last few things and rtbc this.

Eclipse

Title:Allow contrib to implement toolkit specific code for image effectsConvert toolkit operations to plugins

Created a meta-issue for all image work here #2105863: [meta] Images, toolkits and operations. Also changed the title to be more descriptive.

Title:Allow contrib to implement toolkit specific code for image effectsConvert toolkit operations to plugins

re #93:
Clear. To summarize, and to provide a bit of a head start for the change notice:

A contrib module that defines a new ImageToolkit:
- Defines an ImageToolkit plugin with an id, e.g. 'im'.
- Has to provide toolkit specific implementations of the core effects (scale, resize, crop, desaturate, rotate, and scaleAndCrop):.To this end it defines a number of ImageToolkitOperations plugins for these effects and gives them ids: im.crop, im.scale, etc.

A contrib module that defines a new ImageEffect:
- Defines an ImageEffect plugin with an id, e.g. 'text'.
- For all toolkits it wants to support, it defines an ImageToolkitOperation plugin and gives them ids: gd.text, and im.crop,

A contrib module that wants to overrule an existing ImageEffect (from core):
- Defines an ImageEffect plugin and give its an id it wants to, e.g. 'mymodule-scale'.
- Defines one (or more) ImageToolkitOperation plugins and gives them ids it wants to, e.g. 'gd.mymodule-scale' (and optionally im.mymodule-scale).
These ids suggest that the effect is meant to replace the scale effect from core but without the following point this will just be an additional image effect that the user can choose from. Given the id's, it will work.

To overrule the scale efect from core this module:
- implements the alterInfo hook/event (not sure about D8 naming here) for both ImageEffect and ImageToolkitOperation to let definitions['scale'] resp. definitions['gd.scale'] point to its own plugin definition:

// for the image effect alterInfo:
definitions['scale'] = definitions['mymodule-scale'];
unset(definitions['mymodule-scale']);
// and for the toolkit operation alterInfo:
definitions['gd.scale'] = definitions['gd.mymodule-scale'];
unset(definitions['gd.mymodule-scale']);

A contrib module that just wants to overrule the operation part for a specific toolkit, not the rest of the ImageEffect, like e.g. the UI:
- Only defines its own ImageToolkitOperation with id e.g. gd.mymodule-scale.
- implements the alterInfo hook/event for ImageToolkitOperation to let definitions['gd.scale'] point to its own plugin definition:

definitions['gd.scale'] = definitions['gd.mymodule-scale'];
unset(definitions['gd.mymodule-scale']);

A contrib module that only wants to overrule the UI of an ImageEffect (e.g. to provide a nice color picker):
- Defines an ImageEffect plugin and gives it an id it wants to, e.g. 'mymodule-rotate'. This plugin may extend from RotateImageEffect from core, but does not have to.
- implements the alterInfo hook/event for ImageEffect to let definitions['rotate'] point to its own plugin definition:

definitions['rotate'] = definitions['mymodule-rotate'];
unset(definitions['mymodule-rotate']);
// if necessary, see question below:
definitions['rotate']['id'] = 'rotate';

Question about the last use case: will this work? What will be the id of the plugin for Drupal? rotate or mymodule-rotate (as definitions[rotate] points to an array that defines a plugin with id=mymodule-rotate)?

Status:Needs review» Needs work

So this is NW as per #93, to remove the weight keys from the plugins' annotations. @fietserwin are you going to post a patch here?

Status:Needs work» Needs review
StatusFileSize
new10.83 KB
new57.72 KB
FAILED: [[SimpleTest]]: [MySQL] 58,388 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Let's do so.

- Removed weight.
- Adapted documentation to emphasize the id construction convention.
- Changed toolkit operation plugin creation to create based on id.
- Corrected/improved some documentation.

Status:Needs review» Needs work

The last submitted patch, 2073759-97.patch, failed testing.

Status:Needs work» Needs review

Failures are not caused by this patch (and if they were, Drupal would have far too much coupling).

#97: 2073759-97.patch queued for re-testing.

Issue tags:+Needs tests

@fietserwin, thank you!

I still don't like the idea to provide a new operation (even if overrides other) by altering. That's fugly. We have plugins and that means that we should provide them and then select the preferred by UI or/and API. Hook alter should be used to really alter (= small changes to an already provided functionality) and NOT to provide new features.

Right now I stand for:

  1. Adopting {provider}.{toolkit}.{operation} as naming convention for ID. Of course this is only a recommendation while plugins having the same toolkit and operation are competing. So, the developer may choose to use an arbitrary ID.
  2. Implementing API/UI to allow setting the active operation for a given toolkit and operation.

Going this way, a contrib module will have only to provide the plugin in the right place, and then the site builder will have to set the desired operation (only if there are competitors on a given toolkit & op) or, alternatively, modules may set the "winner" through provided API.

  1. +++ b/core/core.services.yml
    @@ -527,6 +527,9 @@ services:
    +  image.toolkit.operation.manager:

    plugin.manager.image.toolkit.operation to be consistent with naming conventions for the rest of plugin managers ( image.toolkit.manager will be renamed too in #2048827: Move Image toolkit API from system.module to Drupal\Core).

  2. +++ b/core/lib/Drupal/Core/Image/Image.php
    @@ -218,8 +218,8 @@ public function getSource() {
    -  public function getToolkitId() {
    -    return $this->toolkit->getPluginId();
    +  public function getToolkit() {
    +    return $this->toolkit;

    Is getToolkit() critical for this patch to work? Because, if not, the let's split this in a new followup issue and keep the patch clean.

  3. +++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/ResizeImageEffect.php
    @@ -38,15 +38,6 @@ public function applyEffect(ImageInterface $image) {
    -  public function transformDimensions(array &$dimensions) {
    ...
    -  }

    Cannot understand. It's simple relocation inside the same file or I'm missing something? If it's just relocation let's drop it for now.

  4. +++ b/core/modules/system/lib/Drupal/system/Annotation/ImageToolkitOperation.php
    @@ -0,0 +1,70 @@
    +   * For discovery to work, the id must be composed as {toolkit}.{operation}.
    +   *
    +   * If that will lead to an already existing id, you are apparently overruling
    +   * an existing operation. In that case make up a unique id and use alterInfo()
    +   * to swap your plugin into the place of the existing one.

    Keeping my point of view.

  5. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitBase.php
    @@ -0,0 +1,28 @@
    +    $manager = \Drupal::service('image.toolkit.operation.manager');

    Let's inject the toolkit manager service. We need to extend the constructor for that.

  6. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitInterface.php
    @@ -189,4 +117,19 @@ public static function isAvailable();
    +  public function process($operation, ImageInterface $image, array $data = array());

    I was the "author" of naming this method process(), but thinking more I think I like more apply(). But it's a matter of taste. If you, guys, are fans of process(), I can live with that :)

  7. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitOperationManager.php
    @@ -0,0 +1,59 @@
    +  public function getToolkitOperationPluginId($toolkit, $operation) {
    +    return "$toolkit.$operation";
    +  }

    If my point of view will be accepted, here we need to read the selected plugin.

Tagging with "Needs tests". Regardless of how we'll go with plugin ID and collisions, I think we need to test various scenarios. Maybe we need a test module implementing some custom operations with, or without collisions.

#101.1 - plugin.manager.image_toolkit_operation with underscores and not plugin.manager.image.toolkit.operation with dots if we want to be consistent with naming in #2048827: Move Image toolkit API from system.module to Drupal\Core (it's plugin.manager.image_toolkit there)

#101.2 - It's critical. No work possible in contrib without that. See #67.

#101.6 - whatever...

#101.7 - if no change ahead, then I'd just drop that method. It's pointless like that.

It seems there's no consensus yet on plugin altering/discovering, not sure how to proceed.

Priority:Critical» Normal

@mondrake, you're right with getToolkit(). It's used in Image but those will be removed in #2103635: Remove effects from ImageInterface. If that will get in first, the we will split this for readability.

Changing priority because of #2105863-13: [meta] Images, toolkits and operations.

Status:Needs review» Needs work

The last submitted patch, 2073759-97.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.87 KB
new56.8 KB
FAILED: [[SimpleTest]]: [MySQL] 57,915 pass(es), 95 fail(s), and 21 exception(s).
[ View ]

#101.1, #102-#101.1: Done

#101.2: unchanged, answered in #102-#101.2.

#101.3: Good catch, no idea how and when it ended up in the patch.

#101.4: Unchanged , also see explanation below. The id should not contain the provider because we do not know and do not want to know the provider.

#101.5: Why? We already have a toolkit instance? Am I missing a use case?

#101.6: Done. A toolkit operation is really part of an ImageEffect, so using naming as in ImageEffect is IMO better. BTW: my rename refactoring partly failed because ImageToolkitBase was not implementing ImageToolkitInterface. So its process methd was not seen as implementing the now renamed apply method of the interface. Moved the "implements ImageToolkitInterface" from GDToolkit to ImageToolkitBase.

#101.7 Unchanged. If someone wants to change plugin selection, he should do so in the ImageToolkitOperationManager. Thus leaving id selection (currently based on simple construction) in that manager,how futile that method currently looks.

Why should we leave the plugin selection as is:
Bottom line, this patch is about allowing contrib ImageEffect providers to give their toolkit specific code a place. Toolkit operations will thus be strictly tied to an ImageEffect 95+% of the time (and with less than 20 known image effects in D7....:). So there is no reason to come up with a fancy UI or whatsoever.

If one is inclined to do so, we can do so in a follow-up issue. So, I propose to proceed with the current solution. But if one wants to provide a UI or so for it: start with that at ImageEffect level.

Status:Needs review» Needs work

The last submitted patch, 2073759-105.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new788 bytes
new56.81 KB
FAILED: [[SimpleTest]]: [MySQL] 58,828 pass(es), 11 fail(s), and 0 exception(s).
[ View ]

Ouch!

Status:Needs review» Needs work

The last submitted patch, 2073759-107.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.62 KB
new56.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2073759-109.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Bah, tests all over the place, findability zero :(

interdiff is w.r.t #105.

Status:Needs review» Needs work
Issue tags:-Plugin system

The last submitted patch, 2073759-109.patch, failed testing.

After #2048827: Move Image toolkit API from system.module to Drupal\Core, it seems that

  1. +++ b/core/core.services.yml
    @@ -511,6 +511,9 @@ services:
    +  plugin.manager.image_toolkit_operation:
    +    class: Drupal\system\Plugin\ImageToolkitOperationManager
    +    arguments: ['@container.namespaces', '@cache.cache', '@language_manager', '@module_handler']
       image.toolkit:

    Need to be reverted to image.toolkit.operation.manager as initially proposed. See #2048827-20: Move Image toolkit API from system.module to Drupal\Core.

  2. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitBase.php

    should go to core/lib/Drupal/core/ImageToolkit/ImageToolkitBase.php, namespace Drupal\Core\ImageToolkit

  3. +++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitOperationInterface.php

    should go... where? a new core subdir like core/lib/Drupal/core/ImageToolkitOperation, namespace Drupal\Core\ImageToolkitOperation?

Status:Needs work» Needs review
StatusFileSize
new56.5 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

This is what I was afraid for. Moving to other namespaces and changing the contents of files: Let's see what this reroll does. Local installation fails to start

question": what to do after services has been moved/renamed and a local install fails with fatals?

Let's fix first file locations and namespaces. I would do it in this way:

  • \Drupal\system\Annotation\ImageToolkit let's move it to \Drupal\Core\ImageToolkit as it's agnostic to what toolkit is used and all those are in core (Shame! This should had been moved in #2048827: Move Image toolkit API from system.module to Drupal\Core). Don't forget to update the ImageToolkitManager. Better create a followup in #2048827: Move Image toolkit API from system.module to Drupal\Core, this is already too complex and will not affect this in terms of rerolling.
  • \Drupal\system\Annotation\ImageToolkitOperation => \Drupal\Core\ImageToolkit\ImageToolkitOperation.
  • \Drupal\system\Plugin\ImageToolkitBase => \Drupal\Core\ImageToolkit\ImageToolkitBase.
  • \Drupal\system\Plugin\ImageToolkitOperationInterface => \Drupal\Core\ImageToolkit\ImageToolkitOperationInterface
  • \Drupal\system\Plugin\ImageToolkitOperationManager => \Drupal\Core\ImageToolkit\ImageToolkitOperationManager

Created follwup for annotation class move #2108077: ImageToolkit annotation object left under system module.

Status:Needs review» Needs work

The last submitted patch, 2073759-111.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new61.9 KB
FAILED: [[SimpleTest]]: [MySQL] 58,882 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

I indeed guess that now one of your other patches is in, we need to move stuff here as well.

- In this patch the GDToolkit and its operations remain in the system module for now.
- The annotation classes are moved under a sub-directory Annotation. This seems to be the standard, except for Action (Core\Annotation instead of \Core\Action\Annotation).
- The previous patch was a reroll after Claudiu's issue (moving imagetoolkit from system to core), but also not more than that. I forgot to adapt a whole lot of use statements to that change.
- More unused annotation use statements removed (#77).

Let's see. patch is becoming bigger and bigger.

[EDIT: missed #114, is at this moment part of this patch]

@fietserwin, annotation stuff is handled in #2108077: ImageToolkit annotation object left under system module. Do less here, don't touch annotation to keep the patch readable. Also what you think about moving #2103635: Remove effects from ImageInterface as first priority? That should drop (for this patch only) the getToolkit() stuff and remove a lot of code from diff?

StatusFileSize
new21.23 KB
new56.46 KB
FAILED: [[SimpleTest]]: [MySQL] 59,080 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

Guys do you mind if I give a try. If it fails, it should at least be closer :)

Interdiff from #112.

Status:Needs review» Needs work

The last submitted patch, 2073759-119.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.15 KB
new56.55 KB
FAILED: [[SimpleTest]]: [MySQL] 58,893 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

This passes locally.

Status:Needs review» Postponed

Back to postponed as per #118

Status:Postponed» Needs work
Issue tags:-Needs tests
StatusFileSize
new71.18 KB
PASSED: [[SimpleTest]]: [MySQL] 58,528 pass(es).
[ View ]
new69.22 KB

Just to move forward I reworked this patch in top of #2103635: Remove effects from ImageInterface.

  • toolkit-operations-2073759-123-do-not-test.patch -- the patch for this issue
  • toolkit-operations-2073759-123-combined.patch -- both issues merged for testing.

This not a final patch. I just fixed the Image::apply() stuff together with passing arguments as an associative array.

Status:Needs review» Needs work

Removed the tag. We have tons of tests, we need to review each one and see if they cover all scenarios. Maybe a follow up for this?

Status:Needs work» Needs review

Ah, the status...

Status:Needs work» Postponed

Be patient :)As per #118, let's first concentrate on #2108339: Image toolkit getter.

Status:Postponed» Needs review

That it's nice to have, I agree, but is not blocking the main conversion. IMO that should remain a low-priority now. Let's follow this as it turned green.

It still needs attention

I still want that indirection via image out of the apply chain, thus for me it is a prerequisite to be able get the toolkit from image.

Stupid question: is it big deal to do #2108339: Image toolkit getter straight here instead? We have a green patch here, none there, I do not think it would change this one too much.

Let's face it, we are unable to get agreement on a patch of this size. Claudiu is over engineering the problems here, i'm more of the quick fixes (that do have a design rationale). We also do not agree on the role of Image. Rerolling or just changing, and reviewing a large patch costs us all much more time then doing it step by step.

So let's focus on 1 issue at a time, and these patches, green or not are to be rerolled anyway.

Oh and yes, #2108339: Image toolkit getter is part of the main conversion as we do not want to calll image to apply something. Image effects call the toolkit directly, so we need that property.

Sorry but what's wrong with that? Why calling $image->getToolkit()->apply('desaturate', $image); is better than calling $image->apply('desaturate'); from the effect? Maybe I am missing something.

@fietserwin, I don't like big patches too. But we cannot split a patch only based on the need to have it in small chunks. Each small patch must have a real issue behind and that real issue has to make sense itself. #2103635: Remove effects from ImageInterface is such an issue.

Let me check this weekend if I can draw some demarcation lines inside this issue and split it in way that each sub-issue to have its story.

#131: Because we are removing all operations from image, turning it into a data holder object, we should also remove the apply() method. getToolkit() is data, apply() is an operation.

(Alternatively, the toolkit instance could be set directly in an image effect, but that would be a separate issue as well.)

#132: The issue is still a nice small API cleanup in itself, even if we would only change that 1 method.

StatusFileSize
new21.57 KB
new71.31 KB
PASSED: [[SimpleTest]]: [MySQL] 58,984 pass(es).
[ View ]
new69.35 KB

Before any decision on how to split this patch I just wanted to add an idea introduced in #22.6 but never applied: introducing ImageToolkitOperationBase that keeps a reference to the toolkit. That is simplifying the ImageToolkitOperationInterface::apply() by removing the $toolkit argument from method interface.

Issue summary:View changes

Added related issue

Issue summary:View changes

Clean the issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Status:Needs review» Postponed

Cleaned up the issue summary. Postponing because #2110499: Unify the call and interfaces of image toolkit operations was inserted before.

Issue summary:View changes

Updated issue summary.

Status:Postponed» Needs review
StatusFileSize
new72.29 KB
PASSED: [[SimpleTest]]: [MySQL] 58,966 pass(es).
[ View ]
new52.47 KB

Let's see what this became. This is a work in top of:

Patches:

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes
Issue tags:+Needs change record

I'm marking this with 'Needs change notification' tag because after committing this a change notice for all issue chain will really make sense. See comment #2103635-54: Remove effects from ImageInterface.

Issue summary:View changes

Issue summary:View changes

Related, #2118991: Use abstract service definitions to minimize copy & pasted service definitions.. We should change the service definition for image.toolkit.operation.manager

Issue tags:-beta blocker

Please check with a core maintainer before adding the beta blocker tag. Only certain types of changes related to the data model or certain low-level APIs are beta blockers.

About a month ago I ported a small image effect module to drupal 8 and got familiarized with what's up here.

Fact is we have a bit of a mess. It's possible to throw together image effect code that gets the job done, but it's fugly (example). Now let's think hard about what we are proposing as the solution here.

So far the leading idea is to take image effects, which are already a plugin, and then throw on top of that additional plugins to actually perform the effect based on the toolkit. That's kinda crazy, but I see what has led people down that road.

I played around with getting the Imagine library working with d8. Actually I have the Imagine library working with d8. For people who aren't familiar Imagine is a php5 image manipulation library that uses polymorphism to abstract it from the server image manipulation software. Basically it makes it so you can write one effect one time using a powerful collection of tools and it will work whether you are using GD2, Imagick, or Gmagick.

I've been reluctant to bring it up because it's a significant API change. The way I did it blew unit testing to smithereens. But it worked really well. It also pointed out glaring problems in the current image API.

I'll use this as a flagrant example:

<?php
$image
->getResource();
?>

That method is pointless unless you are using the GD2 library. It's kinda lazy to depend on it.

Anyways, after batting this around for a few weeks I think using the Imagine library is probably less work, will solve lots of related problems (remove operations from interfaces, operations as plugins...blah...blah) and result in a much better drupal 8. In fact it kinda makes "toolkits" obsolete.

My code is a couple weeks old and the patch probably won't apply. It will blow up the tests. If people are interested I will re roll for eval.

Best

Jim

Issue tags:-Needs change record

For now we leave the "Needs change notification" tag off until the issue has been committed. (This policy will change soon, but untagging for now for accurate metrics.) Thanks!

I'm back from a few weeks holiday. I will try to catch up with the image issue queue and review patches the coming week.

RE #144:
Your observation that the current image system is a kind of a mess is correct. That is why we are working hard here to make the necessary changes so that D8 will ship with an acceptable API and without any regressions from D7.

The idea behind the Imagine library looks good. I did not have a look at it yet, but certainly will in the near future. However, i'm afraid that we are too late for D8 for such large API changes. This mainly because we could only start after the image style/effect system was converted to plugins which was already after "D8 API freeze". (We =the people currently active in the image system issue queue, among them other contrib image effect implementers.) So what we should foucs on now is making the D8 code acceptable and usable (consistent, easy to use, etc.) for image effect implementers.

Your remark about getResource() has already its own issue: #2103621: Move GD logic from ImageInterface to toolkit, tie toolkit instance to Image instance, toolkits should no longer be instantiated separately.

Your remark about plugin on op of plugins is correct, but stms from the fact that Drupal always tries to provide a flexible system that allows contrib to contribute but also to override existing behavior. So, yes, most of the times the module providing an image effect will also provide the toolkit specific parts of them, but that is not necessarily always true: imagemagick toolkit must provide the imagemagick parts for the core effects and if someone wants to implement a brand new toolkit, he should probably start by providing all implementations for that toolkit for all popular image effects from contrib as well. That's why we have to decouple.

If Imageine can take away that part, great! But I'm afraid not for D8 core. We could try to make it a D8 contrib module and that try to get it into core for D9.

You are very welcome to help us on the current issues and to further discuss the future of the image system.

I think d8 is already taking on a lot right now and image effects are pretty fringe in the overall picture.

All good.

I had a look at Imagine:

Cons:
- No imagemagick support other then via the Imagick extension which is not enabled by default and thus not enabled on many hosters.
- Looks not easy to extend, I saw lots of finals on classes and methods (even on methods in final classes ...).
- Arbitrary splitting of "operations" over the image class (a.o. resize, rotate, mask), an effects class (a.o. gamma, sharpen) and filter classes (a.o. rotate, border). I did not fully grab the (conceptual) differences between these.

Pros:
- I do like the API: more powerful, yet not more difficult then current Drupal.
- The API exhibits good OOP practices, uses PSR-0 namespaces and would thus be easy to add.
- The library is well documented.
- The actual toolkit is only hinted at on image creation, effectively this means that the Drupal Image and ImageToolkit classes are merged, which I find logical.
- image effects can be built on top of this (and, in Imagine terms, can be seen as filters with UI).

All in all, I think we can reach at least parity with D7 image features, both core and contrib (as far as I know the image related contrib modules, and I know some).

We should try this in a contrib sandbox (I think it will be possible to overrule core by defining our own image factory and extend the image effects to overrule their apply).

EDIT: there is already an issue for this: #1561832: Replace Image toolkit API with Imagine.

IRC discussion with @alexpott on new methods that needs to be implemented in image operations plugins.

[1:05pm] alexpott: claudiu_cristea: just was thinking if there is any way around the $data array in https://drupal.org/node/2110499
[1:05pm] Druplicon: https://drupal.org/node/2110499 => Unify the call and interfaces of image toolkit operations [#2110499] => 39 comments, 3 IRC mentions
[1:06pm] claudiu_cristea: alexpott: (1) implement a "defaults()" method in the future ImageToolkitOperation plugin
[1:07pm] claudiu_cristea: alexpott: and in apply() check all $data keys against
[1:08pm] claudiu_cristea: alexpott: cons: We'll have to duplicate that in each toolkit op for the same operation
[1:08pm] alexpott: claudiu_cristea: I was wondering if we could put the core supplied methods on the Image Interface and have them call the apply
[1:09pm] claudiu_cristea: alexpott: I don't get it
[1:09pm] claudiu_cristea: alexpott: You mean "resize()", "scale()", etc?
[1:10pm] alexpott: claudiu_cristea: yep
[1:10pm] claudiu_cristea: we just remove them
[1:10pm] claudiu_cristea: removed
[1:11pm] claudiu_cristea: alexpott: We want flexibility in adding operations. This is the whole idea. Those ops were hardcoded. We are movin them into plugins
[1:12pm] alexpott: claudiu_cristea: I get that
[1:12pm] claudiu_cristea: alexpott: so, what are cons to $data?
[1:12pm] claudiu_cristea: alexpott: (1) poor documentation
[1:12pm] alexpott: claudiu_cristea: it's a small array of doom
[1:13pm] claudiu_cristea: alexpott: and (2) IDE not friendly
[1:13pm] alexpott: claudiu_cristea: and we're trying to move away from array based apis
[1:13pm] claudiu_cristea: alexpott: I see
[1:13pm] alexpott: claudiu_cristea: (2) IDE not friendly = developer not friendly
[1:13pm] claudiu_cristea: alexpott: In D6 we moved to array in theme()
[1:14pm] claudiu_cristea: alexpott: in D7
[1:14pm] claudiu_cristea: alexpott: We need a way to unify the interface to ops
[1:14pm] alexpott: claudiu_cristea: I know but the theme system has moved to OO yet
[1:14pm] claudiu_cristea: alexpott: I'm open
[1:15pm] alexpott: claudiu_cristea: so the before the rotate method on Image did return $this->toolkit->rotate($this, $degrees, $background);
[1:15pm] alexpott: claudiu_cristea: it could do return $this->toolkit->apply('rotate', array($degrees, $background));
[1:16pm] claudiu_cristea: alexpott: indexed array insetead assoc?
[1:16pm] claudiu_cristea: *instead
[1:18pm] alexpott: claudiu_cristea: I'm just saying that we can still have rotate() etc on the Image AND have a consistent interface to apply effects on the Image
[1:19pm] claudiu_cristea: alexpott: Treat core ops different? Or I'm missing something
[1:20pm] alexpott: claudiu_cristea: just provide helpers for core ops - this wouldn't preclude new ops being added via plugins and invoked via apply
[1:21pm] claudiu_cristea: alexpott: Honestly, I don't like this "discrimination"
[1:22pm] alexpott: claudiu_cristea: also atm we are trusting what's in $data far too much
[1:22pm] alexpott: claudiu_cristea: in all the toolkit operations we are just assuming the required values are there
[1:22pm] alexpott: claudiu_cristea: looks dodgy to me
[1:22pm] claudiu_cristea: alexpott: Let me understand: You're aware about having $data (assoc array)
[1:23pm] alexpott: claudiu_cristea: yep
[1:23pm] claudiu_cristea: alexpott: But if we we check the $data before sending to tookit->apply()
[1:23pm] claudiu_cristea: alexpott: against tookit->defaults()
[1:24pm] alexpott: claudiu_cristea: but each operations requirements are different
[1:24pm] claudiu_cristea: alexpott: yes
[1:24pm] claudiu_cristea: alexpott: but in the next issue from chain ops will be plugins
[1:25pm] claudiu_cristea: implementing their own defaults()
[1:25pm] claudiu_cristea: 1sec
[1:25pm] alexpott: claudiu_cristea: ah so we can guarantee the that the default values exist
[1:26pm] claudiu_cristea: alexpott: https://drupal.org/node/2073759
[1:26pm] Druplicon: https://drupal.org/node/2073759 => Convert toolkit operations to plugins [#2073759] => 156 comments, 6 IRC mentions
[1:27pm] claudiu_cristea: alexpott: here's the patch https://drupal.org/comment/7960081#comment-7960081
[1:27pm] alexpott: claudiu_cristea: yep just having a look
[1:27pm] claudiu_cristea: is built in top of "Unify..."
[1:28pm] claudiu_cristea: alexpott: we can define defaults() in \Drupal\Core\ImageToolkit\ImageToolkitOperationInterface
[1:28pm] claudiu_cristea: or getDefaults() or whatever
[1:29pm] claudiu_cristea: now we have only ::apply()
[1:29pm] claudiu_cristea: that patch is not finalised. I created just to prove the whole transformation process
[1:29pm] alexpott: claudiu_cristea: well I'd have requiredData or something like that - so we can through an error in apply if data does not have the expected keys
[1:30pm] claudiu_cristea: alexpott: agree
[1:30pm] alexpott: claudiu_cristea: ok
[1:30pm] claudiu_cristea: alexpott: Can I add this last part of IRC as comment to that issue?
[1:31pm] alexpott: claudiu_cristea: of course
[1:31pm] claudiu_cristea: and switch to needs work
[1:31pm] alexpott: claudiu_cristea: so the annotation should defined what is required in $data (and maybe what is optional)
[1:32pm] claudiu_cristea: alexpott: hmmm.
[1:32pm] claudiu_cristea: good point
[1:32pm] alexpott: claudiu_cristea: and ImageToolkitOperation should implement a validateData method
[1:33pm] claudiu_cristea: alexpott: There is a CONS to this approach
[1:33pm] alexpott: claudiu_cristea: and Image::apply calls validate and then apply - validate throws an exception if something is missing
[1:33pm] alexpott: claudiu_cristea: like what?
[1:33pm] claudiu_cristea: alexpott: Example: 'resize" implemented by core for GD
[1:34pm] claudiu_cristea: alexpott: and then the same 'resize' is implemented by ImageMagick in contrib
[1:34pm] claudiu_cristea: alexpott: bot needs requiredData() — kinda code duplication
[1:35pm] claudiu_cristea: alexpott: because I didn't want to add an additional toolkit-agnostic layer
[1:36pm] alexpott: claudiu_cristea: it's a minor issue imo
[1:36pm] claudiu_cristea: alexpott: agree
[1:36pm] alexpott: claudiu_cristea: and one that affects very few people
[1:36pm] claudiu_cristea: alexpott: traits?
[1:36pm] alexpott: claudiu_cristea:
[1:36pm] claudiu_cristea: alexpott: I'm thinking.
[1:36pm] alexpott: claudiu_cristea: we need to do serious testbot work for that
[1:37pm] claudiu_cristea: alexpott: I know
[1:37pm] claudiu_cristea: alexpott: Then adding the conclusion of this chat to that issue as a comment and switch to "needs work"
[1:39pm] alexpott: claudiu_cristea: ok - I'll move ahead with https://drupal.org/node/2110499 on the proviso that https://drupal.org/node/2073759 needs to implement a validation of $data
[1:39pm] Druplicon: https://drupal.org/node/2110499 => Unify the call and interfaces of image toolkit operations [#2110499] => 39 comments, 4 IRC mentions
[1:39pm] Druplicon: https://drupal.org/node/2073759 => Convert toolkit operations to plugins [#2073759] => 156 comments, 7 IRC mentions
[1:39pm] claudiu_cristea: alexpott++

Status:Needs work» Needs review
StatusFileSize
new63.52 KB
new82.27 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

This is worked din top of #2110499-33: Unify the call and interfaces of image toolkit operations with a combined patch.

Based on discussion with @alexpott I reworked the solution to address all concerns related to "loss of DX" expressed by @neclimdul, @fietserwin & others an provide a proper (1) way to document image operation (2) image operation arguments validation and preprocess.

What this patch brings

  • Now, image operations arguments are annotated in the plugin definition.
  • An image operation must provide a list of arguments having next keys: description, type (as TypedData plugin id), required, default. Each argument needs to be documented, type hinted using TypedData. Arguments may be declared optional and defaults can be provided.
  • The toolkit validates all arguments before passing them to the operation. Is doing even a type check based on the TypedData plugin id.
  • When arguments are not conforming the validation exceptions are thrown.
  • Each image operation toolkit is able to extend this validation/preprocess and provide additional processing to arguments.
  • Applied #2118991: Use abstract service definitions to minimize copy & pasted service definitions. to image.toolkit.operation.manager.
  • As #2103155: Pick up plugins in subdirs is in, I namespaced operations by moving them under a directory named by the plugin ID. By doing this I dropped "toolkit” from image operations annotation.
  • Renamed the operation arguments associative array from $data to $arguments as it describes better its usage.

What isn't solved yet?

The PHPUnit tests are not reworked.

ImageToolkitOperationManager needs an interface, getToolkitOperationPluginId needs to be on it.

You're calling \Drupal::service('image.toolkit.operation.manager') in ImageToolkitBase, that should be injected. That will start to uncover the issues with the PHPUnit failures.

I really can't help more with the tests, since I cannot comprehend the new architecture. The tests should have been written first, as they would help identify architectural issues.

Status:Needs review» Needs work

The last submitted patch, 150: toolkit-operations-2073759-150-combined.patch, failed testing.

Please let's keep it simple.

1) How does attempting to formalize the parameter requirements aid the DX or, for that matter, the system?

2) The proposed changes can only document some of the requirements, e.g. they cannot document "width or height must be present", "0 < quality <= 100" or "if keepAspectRatio is false then width and height must be present". So do we want a complete system; a half baked system or should we just document it with plain text and not try to formalize this?

3) Should an implementation of a public interface method trust the caller that it has done requirements checking before calling it? (compare with: should we extract the options checking of url() into a separate checkUrlOptions() and let url() assume that that method has been called and thus does not have to be called again?

The answers are: not, no, and no.

So please let's go back to the basic problem: give an image effect defined in contrib a place to put its toolkit specific code. In other words. let's define that toolkit operation plugin with 1 public method apply() and don't make matters more difficult than they are (and feel free to create follow-up issues for that).

BTW: to be honest, this suggests to also leave the core operations in ImageInterface and ImageToolkitInterface. Although I'm in favor of moving them to toolkit operations as well, it might be better to postpone that to a followup issue.

BTW 2: I'm fine with the last 3 bullet points of #150.

#153.1: Sorry? I think you mean by DX only IDE autocomplete or "find source/class". The DX improvement is that everything is documented and if a dev will miss an argument he is able to debug with no effort because of thrown exceptions. And the IDE stuff? No IDE autocomplete. Do you have autocomplete or hyperlink on services (like 'image.toolkit.managers')? The answer is NO. Do you have validation on services names? Still NO. Do you want a list with tons similar cases? Extra-DX-precautions are starting to hide the real DX problems. And the real DX problem here is: let developers extend the image system with a clean API.

#153.2: In the annotation — don't you see it? Yes with plain text as it was documented till now. Or better let me respond with a question: How this was documented BETTER in legacy Image::scale()?

#153.3: Not so clear to me. Are you talking about prepare() and the fact that operations can overwrite that method and bypass the base implementation? If so, my answer is: we should provide the basic validation/processing but still keep this process flexible. Yes, developers must call the parent::prepare() before or after their specific processing. Everywhere in " the OO world" methods that extends are manually calling their ancestor. Why? because so they are able also to allow the flexibility of totally overwriting the parent.

BTW: to be honest, this suggests to also leave the core operations in ImageInterface and ImageToolkitInterface. Although I'm in favor of moving them to toolkit operations as well, it might be better to postpone that to a followup issue.

Olala! Don't you see that you're contradicting your self in each comment? I'm sick of this. We agreed on this approach months ago. And you are telling me now that we better go back to core-contrib "discrimination"? So, you're very disappointed because documenting operations is in "plain text" (!!!) and for that reason you consider this a "half baked system"? Right? But having the whole system handling in one way for core operations and other way the contrib operations doesn't smell to you as a "half backed system"? C'mon!

I'm not gonna work for any half-step or half-backed solution on this issue. It's a pleasure for me to contribute to Drupal. I understand that in the most of cases compromise is needed just to make wheels rolling. This huge amount of work is to get a more modern, stable and clean Drupal image system. The "half-backed" solution for extending image with operations is in HEAD right now. You don't need this issue to be committed for that.

#154.1:

let developers extend the image system with a clean API.

Correct, we agree. But then you start to further complicate the API by adding additional methods and runtime requirements regarding the order of execution between them???

#154.2: I'm not suggesting that the old system is better, but your question suggests that it will be better with the new system: it won't. So no reason to further complicate matters: keep it clean and allow imperfections that we can't solve completely anyway.

#154.3: Yes. I'm talking about prepare(). Checking arguments (and thereby throwing exceptions and filling in defaults) is the sole responsibility of a method itself (in this case the apply() method), not something else. A clean API places these responsibilities where they belong.

#154.4 I was perfectly aware of this, but still mentioned this as to keep this patch small and to the point. Read the original post, problem/motivation, proposed solution and first 10 comments and then look at the monster we created out of that. So I'm suggesting to take a step back, resolve the real problem at hand in this issue and then continue to improve the API via follow-up issues.

I disagree with the remark that a solution in HEAD is available for the problem at hand. The only thing we (contrib image effect developers) can do right now is ignore the toolkit code in core completely (we can't even access the toolkit object linked to an image) and build our own toolkit support directly into the image effects. (We would have to do so, using a switch statement, or use of some magic methods!). Where this might work with GD, it probably won't even work with ImageMagick (we would need alter access to the $ops array but, as stated above, don't even have access to the toolkit object itself).

@fietserwin: Single reply: You should read above all comments (including yours) but especially the discussion with @alexpott from #149.

Status:Needs work» Needs review
StatusFileSize
new97.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 63,404 pass(es), 206 fail(s), and 117 exception(s).
[ View ]
new29.48 KB

Reroll of #150 + fixed PHPUnit tests + implemented points from @tim.plunkett in #151. More details later.

Status:Needs review» Needs work

The last submitted patch, 157: toolkit-operations-2073759-157-combined.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.45 KB
new97.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,564 pass(es), 90 fail(s), and 161 exception(s).
[ View ]

#2053153: Allow contrib modules to provide plugins on behalf of optional modules : DefaultPluginManager::__construct() now has the module handler injected.

Status:Needs review» Needs work

The last submitted patch, 159: toolkit-operations-2073759-159-combined.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.51 KB
new97.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,837 pass(es), 0 fail(s), and 7 exception(s).
[ View ]

alterInfo() signature also changed...

Status:Needs review» Needs work

The last submitted patch, 161: toolkit-operations-2073759-161-combined.patch, failed testing.

The last submitted patch, 161: toolkit-operations-2073759-161-combined.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new747 bytes
new98.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,805 pass(es), 0 fail(s), and 7 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 164: toolkit-operations-2073759-164-combined.patch, failed testing.

StatusFileSize
new1.13 KB
new98.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch toolkit-operations-2073759-166-combined.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

this should fix the last test failure...

Status:Needs work» Needs review

#166 is now green, so let me summarise what this patch brings:

  1. Image toolkit operations are changed to the plugin system. This means that, given a toolkit, core and contrib can extend the toolkit capabilities by implementing 'operation plugins'.
  2. Core implements the basic GD toolkit operations: crop, desaturate, resize, rotate, scale_and_crop, scale
  3. In order to apply an operation to an image via the toolkit, a unified interface is established: $image->apply($operation, $data);
  4. $data is an associative array or arguments, like $image->apply('crop', array('x' => $x, 'y' => $y, 'width' => $width, 'height' => $height));
  5. Each operation lists its expected arguments in the plugin annotation. An image operation must provide a list of arguments having next keys: description, type (as TypedData plugin id), required, default. Each argument needs to be documented, type hinted using TypedData. Arguments may be declared optional and defaults can be provided.
  6. The toolkit validates all arguments before passing them to the operation. Is doing even a type check based on the TypedData plugin id.
  7. When arguments are not conforming the validation exceptions are thrown.
  8. Each image operation toolkit is able to extend this validation/preprocess and provide additional processing to arguments.

what it adds since #150:

  1. ImageToolkitOperationManagerInterface defines an interface for ImageToolkitOperationManager, with getToolkitOperationPluginId on it (comment in #151)
  2. 'image.toolkit.operation.manager' is injected to ImageToolkitManager constructor and from there to the instantiated toolkit plugins (comment in #151)
  3. the ImageTest PHPUnit tests have been reworked. The logic starts from what I assume was the original intent of the test, i.e. testing the functionality of the Image class. Before #2103635: Remove effects from ImageInterface, the calculation of the arguments for the operations was done in the Image class itself, and the PHPUnit test was stubbing the toolkit methods actually performing the image manipulation to check that the input given to the methods was as expected. Now, with this patch, both argument validation and operation execution move down from Image, onto the toolkit, and from there onto the operation itself. Each operation has two public methods, prepare and apply, the first validating the arguments and the second executing the image manipulation. So in this patch the PHPUnit test is mocking both the toolkit and the toolkit operation 'apply' method, and the arguments input reaching the apply stub (from the non-stubbed prepare) are returned back to the test. This way we can ensure that the values of the arguments as they are validated through the 'prepare' method (which were in the Image class originally, and now moved to the operation plugin) still are as expected.

Status:Needs review» Needs work

The last submitted patch, 166: toolkit-operations-2073759-166-combined.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.55 KB
new97.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Status:Needs review» Needs work

The last submitted patch, 171: toolkit-operations-2073759-171-combined.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new555 bytes
new98.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 43,936 pass(es), 920 fail(s), and 1,737 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 173: toolkit-operations-2073759-173-combined.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.83 KB
new98.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,336 pass(es).
[ View ]

Im nog a fan of the argument being part of the anotation. Only stuff needed for Discovery should be there. Cant we put it inside a defaultArgument function.

Im nog a fan of the argument being part of the annotation. Only stuff needed for Discovery should be there.

Why? Aren't we provide default settings for fields & field instances in annotation? And I can give a lot of other examples.

EDIT: Annotations are not used only for discovery but to provide meta information and to describe the plugin. And this is what we do here: we provide a description of expected operation arguments.

Im just giving an opinion based on the experience I have had with plugins the last 2 years. Lets see what others think.

#178: can you explain your (bad) experiences a bit more (or give some links to issues or comments)? I have not made up my mind yet, so I would like to hear the arguments from both sides.

im on a phone hard tot type. here is at least one link i can sharehttps://drupal.org/node/2136197

#2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition is related to fields/instances/widgets/formatters. I read carefully the summary there. We are on other page. I don't have anything agains moving them into static methods — this would be a simple operation — but I don't get the reason.

@mondrake, can you post also the non-combined patch here? Typically we need:

* The interdiff.
* The issues patch with *-do-not-test.patch suffix >>> That should be reviewed.
* The combined patch >>> That should pass tests.

Thank you.

StatusFileSize
new86.36 KB
new98.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,810 pass(es).
[ View ]

Here we go.

StatusFileSize
new678 bytes
new98.77 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

New patch.

StatusFileSize
new98.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,805 pass(es).
[ View ]

Sorry, the latest patch has also an unwanted file at the end. Here's the good one. Interdiff is OK.

The last submitted patch, 184: toolkit-operations-2073759-184.patch, failed testing.

I'm away this weekend so unfortunately, I won't be able to do any reviewing this weekend. I'm fine with forwarding on this, provided we:
- Get at least 1 additional external review, I understood that @sun is willing to do so.
- Stay responsive afterwards. though this is the single most important issue from the issues in the meta issue, many issues remain open in that meta issue and new issues will probably arise once this lands. (So far, almost each step we made forward, gave lead to new, smaller, issues.
- II don't have any opinion on the contents of the patch as I did not have a look at it for a long time.
- Nor do I have any substantiated opinion on argument validation other than that on first impression it seems like overkill to me, even though in general I love more formal solutions resp. more formal ways of documenting.
- To make any review more easy, it would be fine if @claudiu.christea or @mondrake could update the issue summary with a more complete and detailed list of what this patch does.

StatusFileSize
new14.04 KB
new95.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

I want to underline again the reason why we need to convert image toolkit operations to plugins: Right now a contributed module is able to add a new operation (e.g. "blur") only by extending the toolkit class. This seems reasonable at first look but what if there's another module wanting to add also an image operation? That 2nd module needs to extend the toolkit too. Who's gonna win? The problem here is that we need a system where modules are able to plug in rather than extend classes. And these are plugins in D8.

Other worries were addressed by the patch. Anyway, some are complaining that using TypedData for checking arguments is "too heavy". I'm not very sure but here's a patch without type checking, so without TypedData support.

StatusFileSize
new869 bytes
new95.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,880 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

Small fix relative to #188.

The last submitted patch, 188: toolkit-operations-2073759-188.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 189: toolkit-operations-2073759-189.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new11.52 KB
new95.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,831 pass(es).
[ View ]

Ah, forgot to fix the phpunit test too.

IMHO, type checking would be useful during development, but probably less on a prod site. Maybe the toggle being introduced in #2226761: Make Drupal 8 fast by default: settings are optimized for production, but can easily be overridden using settings.development.php could be leveraged here?

StatusFileSize
new95.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,647 pass(es).
[ View ]

Reroll of #192 after #2217783: Remove $extension and $mimeType from Image, use image_type_to_xxx function in the getters instead, merge conflicts resolved. It was @cache.cache replaced by @cache.default in core services, and the testGetExtension method in PHPUnit test removed. No interdiff.

Edit

here relevant portion of the diff-diff:

9,10c9,10
< -    arguments: ['@container.namespaces', '@cache.cache', '@language_manager', '@config.factory', '@module_handler']
< +    arguments: ['@container.namespaces', '@cache.cache', '@language_manager', '@config.factory', '@module_handler', '@image.toolkit.operation.manager']
---
> -    arguments: ['@container.namespaces', '@cache.default', '@language_manager', '@config.factory', '@module_handler']
> +    arguments: ['@container.namespaces', '@cache.default', '@language_manager', '@config.factory', '@module_handler', '@image.toolkit.operation.manager']
2214,2222d2213
<     * Tests \Drupal\Core\Image\Image::getExtension().
<     */
<    public function testGetExtension() {
< -    $this->assertEquals($this->image->getExtension(), 'png');
< +    $this->getTestImage();
< +    $this->assertEquals('png', $this->image->getExtension());
<    }
<
<    /**

Issue tags:+beta target

I suggest to tag this issue as 'beta target' as per @catch suggestion in #2105863-45: [meta] Images, toolkits and operations.

AFAICS, no proper porting of image effect modules to D8 can really start without this. See e.g. #2228361: Drupal 8 port.

StatusFileSize
new95.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,607 pass(es).
[ View ]

Reroll after #2228291: Move all plugin and other discovery data into a cache_discovery bin

In core.services.yml

-    arguments: ['@container.namespaces', '@cache.default', '@language_manager', '@config.factory', '@module_handler', '@image.toolkit.operation.manager']
+    arguments: ['@container.namespaces', '@cache.discovery', '@language_manager', '@config.factory', '@module_handler', '@image.toolkit.operation.manager']

StatusFileSize
new9.37 KB
new93.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Reroll due to #2196067: Remove setWidth and setHeight from ImageInterface, plus made better usage of the injected toolkit in the toolkit operations (instead of using $image->getToolkit()->...). Partial interdiff for the toolkit operations only.

Status:Needs review» Needs work

The last submitted patch, 197: toolkit-operations-2073759-197.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.28 KB
new93.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,001 pass(es).
[ View ]

Fix for PHPUnit tests.