Part of #2105863: [meta] Images, toolkits and operations

Problem/Motivation

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

Developers are not able to add image toolkit specific code to their effects. Based on the current design they can only extend a toolkit to add that code. For example, if they want to add new operations to GD toolkit, they need to extend GD toolkit. But as other modules may want to do the same, clashes can occur where multiple image toolkits exists that are derived from the GD toolkit.

To solve this we need a way to decorate toolkits instead of extend them.

Proposed resolution

Convert image toolkit operations to plugins. A few details of the implementation:

  • The plugins are defined by their operation and the toolkit they are meant for. The toolkit and the operation are elements of the annotation.
  • Core operations (resize, scale, etc.) are converted to plugins.
  • The canonical approach to calling toolkit operations is via a chain of apply() methods (on ImageInterface, ImageToolkitInterface, and ImageToolkitOperationInterface). This means that the methods resize(), scale(), etc are removed from ImageToolkitInterface.
  • For DX convenience, core methods scale(), scaleAndCrop(), resize(), crop(), rotate(), desaturate() are reintroduced in ImageInterface and Image, as specific proxies to the canonical apply()call.
  • To support some argument checking (arguments are now passed in a keyed array), the ImageToolkitOperationBase implemented a template design pattern that includes some default argument checking (by the base class) but also allows for argument checking by the child class.

API changes

  • Methods scale(), scaleAndCrop(), resize(), crop(), rotate(), desaturate() are transformed to plugins and then removed from ImageToolkitInterface.
  • apply() methods are introduced in ImageInterface and ImageToolkitInterface and implementations.
  • Core methods scale(), scaleAndCrop(), resize(), crop(), rotate(), desaturate() that were removed from ImageInterface and Image in #2103635: Remove effects from ImageInterface are reintroduced as proxies to the apply() method.

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.

CommentFileSizeAuthor
#283 interdiff.txt1.8 KBfietserwin
#283 convert_toolkit-2073759-282.patch92.4 KBfietserwin
#278 interdiff.txt21.97 KBfietserwin
#278 convert_toolkit-2073759-278.patch92.39 KBfietserwin
#277 interdiff.txt21.86 KBfietserwin
#277 convert_toolkit-2073759-277.patch104.14 KBfietserwin
#271 convert_toolkit-2073759-270.patch100.14 KBmondrake
#269 convert_toolkit-2073759-265.patch100.34 KBfietserwin
#265 interdiff-265.patch9.55 KBfietserwin
#265 convert_toolkit-2073759-265.patch100.34 KBfietserwin
#260 convert_toolkit-2073759-260.patch99.42 KBfietserwin
#257 interdiff-257-255.txt21.94 KBfietserwin
#257 convert_toolkit-2073759-257.patch99.46 KBfietserwin
#256 interdiff-256-255.txt16.63 KBfietserwin
#256 convert_toolkit-2073759-256.patch99.88 KBfietserwin
#255 interdiff-255.txt14.54 KBfietserwin
#255 convert_toolkit-2073759-255.patch98.62 KBfietserwin
#253 interdiff251-253.txt1.47 KBfietserwin
#253 convert_toolkit-2073759-253.patch102.54 KBfietserwin
#251 interdiff-2257587.txt21.04 KBfietserwin
#251 convert_toolkit-2073759-251.patch102.81 KBfietserwin
#247 2073759-toolkit_op-247.patch102.09 KBmondrake
#247 interdiff_246-247.txt10.63 KBmondrake
#246 interdiff-246.txt18.18 KBfietserwin
#246 convert_toolkit-2073759-246.patch102.09 KBfietserwin
#241 interdiff-227.txt44.13 KBfietserwin
#241 interdiff-237.txt6.38 KBfietserwin
#241 convert_toolkit-2073759-241.patch100.01 KBfietserwin
#237 interdiff.txt11.37 KBfietserwin
#237 2073759-237.patch100.37 KBfietserwin
#233 2215369-14.interdiff.txt4.15 KBfietserwin
#233 2073759-233.patch99.97 KBfietserwin
#232 interdiff.txt39.7 KBfietserwin
#232 2073759-232.patch102.32 KBfietserwin
#230 interdiff.txt38.93 KBfietserwin
#230 2073759-230.patch101.56 KBfietserwin
#227 2073759-toolkit_op-227.patch95.16 KBmondrake
#227 interdiff_225-227.txt8.21 KBmondrake
#225 2073759-toolkit_op-225.patch95.29 KBmondrake
#225 interdiff_223-225.txt11.48 KBmondrake
#223 2073759-toolkit_op-223.patch94.85 KBmondrake
#223 interdiff_221-223.txt29.41 KBmondrake
#221 2073759-toolkit_op-221.patch94.28 KBmondrake
#221 interdiff_218-221.txt11.33 KBmondrake
#220 2073759-toolkit_op-218.patch93.14 KBmondrake
#217 2073759-toolkit_op-216.patch93.45 KBmondrake
#217 interdiff_214-216.txt1.37 KBmondrake
#217 interdiff_204-216.txt11.27 KBmondrake
#214 2073759-toolkit_op-213.patch93.07 KBmondrake
#214 interdiff_204-213.txt11.19 KBmondrake
#204 toolkit-operations-2073759-204.patch91.28 KBmondrake
#203 toolkit-operations-2073759-203.patch92.74 KBmondrake
#203 interdiff_201-203.txt2.32 KBmondrake
#201 toolkit-operations-2073759-201.patch92.29 KBmondrake
#199 toolkit-operations-2073759-199.patch93.96 KBmondrake
#199 interdiff_197-199.txt2.28 KBmondrake
#197 toolkit-operations-2073759-197.patch93.55 KBmondrake
#197 interdiff_196-197.txt9.37 KBmondrake
#196 toolkit-operations-2073759-196.patch95.68 KBmondrake
#194 toolkit-operations-2073759-194.patch95.67 KBmondrake
#192 toolkit-operations-2073759-192.patch95.93 KBclaudiu.cristea
#192 interdiff.txt11.52 KBclaudiu.cristea
#189 toolkit-operations-2073759-189.patch95.57 KBclaudiu.cristea
#189 interdiff.txt869 bytesclaudiu.cristea
#188 toolkit-operations-2073759-188.patch95.56 KBclaudiu.cristea
#188 interdiff.txt14.04 KBclaudiu.cristea
#185 toolkit-operations-2073759-185.patch98.09 KBclaudiu.cristea
#184 toolkit-operations-2073759-184.patch98.77 KBclaudiu.cristea
#184 interdiff.txt678 bytesclaudiu.cristea
#183 toolkit-operations-2073759-183-combined.patch98.12 KBmondrake
#183 toolkit-operations-2073759-183-do-not-test.patch86.36 KBmondrake
#175 toolkit-operations-2073759-175-combined.patch98.12 KBmondrake
#175 interdiff_173-175.txt1.83 KBmondrake
#173 toolkit-operations-2073759-173-combined.patch98.1 KBmondrake
#173 interdiff_171-173.txt555 bytesmondrake
#171 toolkit-operations-2073759-171-combined.patch97.86 KBmondrake
#171 interdiff_166-171.txt6.55 KBmondrake
#166 toolkit-operations-2073759-166-combined.patch98.03 KBmondrake
#166 interdiff_164-166.txt1.13 KBmondrake
#164 toolkit-operations-2073759-164-combined.patch98.03 KBmondrake
#164 interdiff_161-164.txt747 bytesmondrake
#161 toolkit-operations-2073759-161-combined.patch97.94 KBmondrake
#161 interdiff_159-161.txt1.51 KBmondrake
#159 toolkit-operations-2073759-159-combined.patch97.94 KBmondrake
#159 interdiff_157-159.txt2.45 KBmondrake
#157 interdiff_150-157.txt29.48 KBmondrake
#157 toolkit-operations-2073759-157-combined.patch97.85 KBmondrake
#150 toolkit-operations-2073759-150-combined.patch82.27 KBclaudiu.cristea
#150 toolkit-operations-2073759-150-do-not-test.patch63.52 KBclaudiu.cristea
#136 toolkit-operations-2073759-136-do-not-test.patch52.47 KBclaudiu.cristea
#136 toolkit-operations-2073759-136-combined.patch72.29 KBclaudiu.cristea
#134 toolkit-operations-2073759-134-do-not-test.patch69.35 KBclaudiu.cristea
#134 toolkit-operations-2073759-134-combined.patch71.31 KBclaudiu.cristea
#134 interdiff.txt21.57 KBclaudiu.cristea
#123 toolkit-operations-2073759-123-do-not-test.patch69.22 KBclaudiu.cristea
#123 toolkit-operations-2073759-123-combined.patch71.18 KBclaudiu.cristea
#121 2073759-121.patch56.55 KBmondrake
#121 interdiff_119-121.txt5.15 KBmondrake
#119 2073759-119.patch56.46 KBmondrake
#119 interdiff_112-119.txt21.23 KBmondrake
#116 2073759-114.patch61.9 KBfietserwin
#112 2073759-111.patch56.5 KBfietserwin
#109 2073759-109.patch56.78 KBfietserwin
#109 interdiff.txt4.62 KBfietserwin
#107 2073759-107.patch56.81 KBfietserwin
#107 interdiff.txt788 bytesfietserwin
#105 2073759-105.patch56.8 KBfietserwin
#105 interdiff.txt14.87 KBfietserwin
#97 2073759-97.patch57.72 KBfietserwin
#97 interdiff.txt10.83 KBfietserwin
#85 2073759-85.patch58.86 KBfietserwin
#85 interdiff.txt33.31 KBfietserwin
#81 toolkit-operations-2073759-81.patch51.13 KBclaudiu.cristea
#81 interdiff.txt6.84 KBclaudiu.cristea
#75 toolkit-ops-2073759-75.patch51.31 KBclaudiu.cristea
#75 interdiff.txt27 KBclaudiu.cristea
#68 2073759-toolkit_operations-67.patch48.3 KBmondrake
#68 interdiff_66-67.txt2.58 KBmondrake
#66 2073759-toolkit_operations-66.patch47.15 KBmondrake
#66 interdiff_63-66.txt3.37 KBmondrake
#63 2073759-toolkit_operations-63.patch47.08 KBmondrake
#63 interdiff_61-63.txt802 bytesmondrake
#61 2073759-toolkit_operations-61.patch47.08 KBmondrake
#61 interdiff_59-61.txt1.57 KBmondrake
#59 2073759-toolkit_operations-59.patch47.15 KBmondrake
#59 interdiff_58-59.txt1.7 KBmondrake
#58 2073759-toolkit_operations-58.patch47.23 KBmondrake
#58 interdiff_54-58.txt1.55 KBmondrake
#54 2073759-toolkit_operations-54.patch47.28 KBmondrake
#54 interdiff_50-54.txt5.33 KBmondrake
#50 2073759-toolkit_operations-50.patch45.51 KBmondrake
#50 interdiff_47-50.txt4.77 KBmondrake
#47 2073759-toolkit_operations-47.patch40.74 KBmondrake
#47 interdiff_45-47.txt10.93 KBmondrake
#45 2073759-toolkit_operations-45.patch29.81 KBmondrake
#45 interdiff_29-45.txt27.01 KBmondrake
#29 2073759-toolkit_operations-29.patch33.52 KBmondrake
#29 interdiff_19-29.txt25.28 KBmondrake
#19 2073759-toolkit_operations-19.patch25.25 KBmondrake
#19 interdiff_15-19.txt29.2 KBmondrake
#15 2073759-toolkit_operations-15.patch24.81 KBmondrake
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Where did the toolkit specific code live before?

fietserwin’s picture

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".

mondrake’s picture

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 :)

fietserwin’s picture

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.

mondrake’s picture

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....

mondrake’s picture

tagging

mondrake’s picture

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.

fietserwin’s picture

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?

mondrake’s picture

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?

fietserwin’s picture

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.

claudiu.cristea’s picture

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.

fietserwin’s picture

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.

claudiu.cristea’s picture

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.

mondrake’s picture

#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?

mondrake’s picture

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
FileSize
24.81 KB

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.

claudiu.cristea’s picture

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?

mondrake’s picture

@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?

fietserwin’s picture

[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().)

mondrake’s picture

#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

mondrake’s picture

Status: Needs work » Needs review
mondrake’s picture

Title: [decide approach] Allow contrib to implement toolkit specific code for image effects » Allow 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.

claudiu.cristea’s picture

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?

    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:

    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:

    class GDToolkitCrop extends ImageToolkitOperationBase { }
    

    and base

    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.

claudiu.cristea’s picture

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.

claudiu.cristea’s picture

Status: Needs review » Needs work

process() or apply()? :)

mondrake’s picture

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

fietserwin’s picture

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).

claudiu.cristea’s picture

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

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

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.

fietserwin’s picture

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);

mondrake’s picture

Status: Needs work » Needs review
FileSize
25.28 KB
33.52 KB

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...

fietserwin’s picture

#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.

mondrake’s picture

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?

mondrake’s picture

Status: Needs review » Needs work

Xpost

mondrake’s picture

Status: Needs work » Needs review

Can I have a review at this stage?

fietserwin’s picture

  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.

mondrake’s picture

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?

fietserwin’s picture

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.

mondrake’s picture

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....

fietserwin’s picture

+++ 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()

mondrake’s picture

#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?).

fietserwin’s picture

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.

fietserwin’s picture

Cache in the find method, not upon constructing?

mondrake’s picture

#42 yep, you're right

mondrake’s picture

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.

mondrake’s picture

Status: Needs work » Needs review
Issue tags: +DX (Developer Experience), +Plugin system
FileSize
27.01 KB
29.81 KB

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

mondrake’s picture

Status: Needs work » Needs review
FileSize
10.93 KB
40.74 KB

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

fietserwin’s picture

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.

mondrake’s picture

Status: Needs work » Needs review
FileSize
4.77 KB
45.51 KB

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

fietserwin’s picture

  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' => ..., ...)

fietserwin’s picture

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

mondrake’s picture

Status: Needs work » Needs review
FileSize
5.33 KB
47.28 KB

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.

mondrake’s picture

Status: Needs work » Needs review

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.

mondrake’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
47.23 KB
mondrake’s picture

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.

mondrake’s picture

Status: Needs work » Needs review
FileSize
1.57 KB
47.08 KB

New attempt, using at() seems too complicated

Status: Needs review » Needs work

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

mondrake’s picture

mondrake’s picture

Status: Needs work » Needs review

Runs on simplytest

mondrake’s picture

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!

mondrake’s picture

The patch...

mondrake’s picture

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.

mondrake’s picture

... and patch ...

fietserwin’s picture

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?

mondrake’s picture

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.

fietserwin’s picture

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"...

mondrake’s picture

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.

fietserwin’s picture

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.

fietserwin’s picture

Issue summary: View changes

Added related issue

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
27 KB
51.31 KB

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.

mondrake’s picture

+++ 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

mondrake’s picture

+++ 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

fietserwin’s picture

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.

fietserwin’s picture

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.

mondrake’s picture

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;
+  }
+
+}
claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
6.84 KB
51.13 KB

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.

fietserwin’s picture

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.

EclipseGc’s picture

+++ 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

fietserwin’s picture

#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.

fietserwin’s picture

Status: Needs work » Needs review
FileSize
33.31 KB
58.86 KB

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.

claudiu.cristea’s picture

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.

fietserwin’s picture

#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.

mondrake’s picture

- 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?

EclipseGc’s picture

+++ 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

claudiu.cristea’s picture

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".

mondrake’s picture

#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.

fietserwin’s picture

Title: Convert toolkit operations to plugins » Allow 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.

EclipseGc’s picture

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.


/**
 * @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:


/**
 * @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

claudiu.cristea’s picture

Title: Allow contrib to implement toolkit specific code for image effects » Convert 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.

fietserwin’s picture

Title: Allow contrib to implement toolkit specific code for image effects » Convert 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)?

mondrake’s picture

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?

fietserwin’s picture

Status: Needs work » Needs review
FileSize
10.83 KB
57.72 KB

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.

fietserwin’s picture

Status: Needs work » Needs review

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

fietserwin’s picture

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

claudiu.cristea’s picture

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.

mondrake’s picture

#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.

claudiu.cristea’s picture

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.

fietserwin’s picture

Status: Needs work » Needs review
FileSize
14.87 KB
56.8 KB

#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.

fietserwin’s picture

Status: Needs work » Needs review
FileSize
788 bytes
56.81 KB

Ouch!

Status: Needs review » Needs work

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

fietserwin’s picture

Status: Needs work » Needs review
FileSize
4.62 KB
56.78 KB

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.

mondrake’s picture

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?

fietserwin’s picture

Status: Needs work » Needs review
FileSize
56.5 KB

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?

claudiu.cristea’s picture

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
claudiu.cristea’s picture

Status: Needs review » Needs work

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

fietserwin’s picture

Status: Needs work » Needs review
FileSize
61.9 KB

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]

claudiu.cristea’s picture

@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?

fietserwin’s picture

mondrake’s picture

FileSize
21.23 KB
56.46 KB

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.

mondrake’s picture

Status: Needs work » Needs review
FileSize
5.15 KB
56.55 KB

This passes locally.

mondrake’s picture

Status: Needs review » Postponed

Back to postponed as per #118

claudiu.cristea’s picture

Status: Postponed » Needs work
Issue tags: -Needs tests
FileSize
71.18 KB
69.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.

claudiu.cristea’s picture

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?

claudiu.cristea’s picture

Status: Needs work » Needs review

Ah, the status...

fietserwin’s picture

Status: Needs work » Postponed

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

claudiu.cristea’s picture

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

fietserwin’s picture

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.

mondrake’s picture

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.

fietserwin’s picture

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.

mondrake’s picture

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.

claudiu.cristea’s picture

@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.

fietserwin’s picture

#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.

claudiu.cristea’s picture

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.

claudiu.cristea’s picture

Issue summary: View changes

Added related issue

claudiu.cristea’s picture

Issue summary: View changes

Clean the issue summary.

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

Status: Needs review » Postponed

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

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

Status: Postponed » Needs review
FileSize
72.29 KB
52.47 KB

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

Patches:

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

Tagging.

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

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.

mondrake’s picture

Issue summary: View changes
mondrake’s picture

Issue summary: View changes
mondrake’s picture

mondrake’s picture

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

xjm’s picture

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.

jamesbenison’s picture

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:

$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

xjm’s picture

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!

fietserwin’s picture

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.

jamesbenison’s picture

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

All good.

fietserwin’s picture

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.

claudiu.cristea’s picture

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: Unify the call and interfaces of image toolkit operations => 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: Convert toolkit operations to plugins => 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: Unify the call and interfaces of image toolkit operations => 39 comments, 4 IRC mentions
[1:39pm] Druplicon: https://drupal.org/node/2073759 => Convert toolkit operations to plugins #2073759: Convert toolkit operations to plugins => 156 comments, 7 IRC mentions
[1:39pm] claudiu_cristea: alexpott++
claudiu.cristea’s picture

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.

tim.plunkett’s picture

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.

fietserwin’s picture

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.

claudiu.cristea’s picture

#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.

fietserwin’s picture

#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).

claudiu.cristea’s picture

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

mondrake’s picture

Status: Needs work » Needs review
FileSize
97.85 KB
29.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.

mondrake’s picture

Status: Needs work » Needs review
FileSize
2.45 KB
97.94 KB

#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.

mondrake’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
97.94 KB

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.

mondrake’s picture

Status: Needs work » Needs review
FileSize
747 bytes
98.03 KB

Status: Needs review » Needs work

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

mondrake’s picture

FileSize
1.13 KB
98.03 KB

this should fix the last test failure...

mondrake’s picture

Status: Needs work » Needs review
mondrake’s picture

#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.
mondrake’s picture

Status: Needs review » Needs work

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

mondrake’s picture

Status: Needs work » Needs review
FileSize
6.55 KB
97.86 KB

Status: Needs review » Needs work

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

mondrake’s picture

Status: Needs work » Needs review
FileSize
555 bytes
98.1 KB

Status: Needs review » Needs work

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

mondrake’s picture

Status: Needs work » Needs review
FileSize
1.83 KB
98.12 KB
aspilicious’s picture

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.

claudiu.cristea’s picture

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.

aspilicious’s picture

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

fietserwin’s picture

#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.

aspilicious’s picture

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

claudiu.cristea’s picture

#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.

claudiu.cristea’s picture

@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.

mondrake’s picture

claudiu.cristea’s picture

claudiu.cristea’s picture

FileSize
98.09 KB

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.

fietserwin’s picture

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.

claudiu.cristea’s picture

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.

claudiu.cristea’s picture

FileSize
869 bytes
95.57 KB

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.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
11.52 KB
95.93 KB

Ah, forgot to fix the phpunit test too.

mondrake’s picture

IMHO, type checking would be useful during development, but probably less on a prod site. Maybe the toggle being introduced in #2226761: Change all default settings and config to fast/safe production values could be leveraged here?

mondrake’s picture

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());
<    }
<  
<    /**
mondrake’s picture

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.

mondrake’s picture

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']
mondrake’s picture

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.

mondrake’s picture

Status: Needs work » Needs review
FileSize
2.28 KB
93.96 KB

Fix for PHPUnit tests.

mondrake’s picture

mondrake’s picture

Status: Needs review » Needs work

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

mondrake’s picture

Status: Needs work » Needs review
FileSize
2.32 KB
92.74 KB

The PHPUnit test fix in #199 got lost.

mondrake’s picture

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

I'm going to rtbc this as we need the plugify this as soon as possible. (in order to prevent major api changes after beta)
I would like to see a followup to discuss the fact that "arguments" is part of the annotation, I still don't like that but I feel that I'm blocking this issue which isn't my intention.

mondrake’s picture

fietserwin’s picture

Status: Reviewed & tested by the community » Needs work

I started to review and saw at least 2 doc references to the non (no longer?) existing \Drupal\Component\Plugin\Exception\UnknownPluginException. Unfortunately I have no more time today to do a serious full review.

I also found it a bit strange to have an ImageToolkitOperationManagerInterface. Do we really expect this to be re-implemented by contrib? If so, why don't we have a similar ImageToolkitManagerInterface. Looking at other (plugin) managers, they also do not always have an accompanying interface.

I hope to be able to do a full review tomorrow.

mondrake’s picture

fietserwin’s picture

First batch of my review: Image(Interface), ImageToolkit(Base/Interface/Manager), ImageToolkitOperationManager(Interface). Later on more (a.o. the toolkit operations itself (base, interface, annotation, GD operations) and all other changes outside the Image subsystem).

  1. +++ b/core/lib/Drupal/Core/Image/ImageInterface.php
    @@ -79,6 +79,22 @@ public function getToolkit();
    +   *   An associative array with required arguments to be passed to the toolkit
    +   *   operation. E.g. array('width' => 50, 'height' => 100, 'upscale' => TRUE).
    

    Remove the word required, the arguments may be optional as well.

  2. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitBase.php
    @@ -7,15 +7,73 @@
       /**
    +   * The image toolkit operation manager.
    +   *
    +   * @var \Drupal\Core\ImageToolkit\ImageToolkitOperationManagerInterface
    +   */
    +  protected $operationManager;
    +
    

    - As written in a previous comment: do we need an interface for this or should we just type it as a ImageToolkitOperationManager?
    - if so: the interface should extend PluginManagerInterface (or at least FactoryInterface) because we are calling createInstance(...) on it.

  3. +++ b/core/lib/Drupal/Core/Image/ImageInterface.php
    @@ -79,6 +79,22 @@ public function getToolkit();
    +  public function apply($operation, array $arguments = array());
    +
    

    Document that it may throw a PluginException instead of returning false. Document so for the whole chain call of apply(). Alternatively, we may catch a plugin exception and return false. Current behavior (D7) if an image effect from contrib is disabled/removed is to silently skip that effect and continue to create the derivative. I'm not sure at which level to catch the exception: at the image style create derivative level or in the image(toolkit) system?

  4. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitBase.php
    @@ -7,15 +7,73 @@
    +use Drupal\Core\ImageToolkit\ImageToolkitOperationManagerInterface;
     use Drupal\Core\Plugin\PluginBase;
    

    Superfluous: remove line.

  5. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
    @@ -259,4 +147,20 @@ public static function isAvailable();
    +   * Applies a toolkit specific operation to an image.
    +   *
    

    Within the context of the toolkit class. the word specific does not make much sense to me.

  6. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
    @@ -259,4 +147,20 @@ public static function isAvailable();
    +   * @param array $arguments
    +   *   An associative array with required arguments to be passed to the toolkit
    +   *   operation. E.g. array('width' => 50, 'height' => 100, 'upscale' => TRUE).
    +   *
    

    Remove the word required, the arguments may be optional as well.

  7. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitBase.php
    @@ -7,15 +7,73 @@
    +  protected function getToolkitOperation($operation) {
    +    $plugin_id = $this->operationManager->getToolkitOperationPluginId($this->getPluginId(), $operation);
    +    return $this->operationManager->createInstance($plugin_id, array(), $this);
    +  }
    +
    

    IMO, this method belongs to the manager, so I propose to move it to that class (including adapting the interface: it does not need to extend the PluginManagerInterface interface anymore).

  8. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitBase.php
    @@ -7,15 +7,73 @@
    +   * @inheritdoc
    +   */
    

    {@inheritdoc}

  9. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitManager.php
    @@ -10,8 +10,10 @@
    +use Drupal\Core\ImageToolkit\ImageToolkitOperationManagerInterface;
     use Drupal\Core\Language\LanguageManager;
    

    Superfluous: remove line.

  10. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationManager.php
    @@ -0,0 +1,102 @@
    +use Drupal\Component\Plugin\Exception\UnknownPluginException;
    

    Change to PluginNotFoundException.

  11. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationManager.php
    @@ -0,0 +1,102 @@
    +        throw new PluginException($message);
    +      }
    

    We could throw a more specific InvalidPluginDefinitionException, which IMO seems to be more to the point here.

  12. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationManager.php
    @@ -0,0 +1,102 @@
    +      throw new UnknownPluginException('', $message);
    +    }
    

    Change to PluginNotFoundException.

  13. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationManager.php
    @@ -0,0 +1,102 @@
    +      throw new UnknownPluginException('', $message);
    +    }
    

    Change to PluginNotFoundException.

  14. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationManager.php
    @@ -0,0 +1,102 @@
    +      // @todo In https://drupal.org/node/2110591 we'll return here the UI
    +      //   selected plugin or the first found if missed.
    +      $definition = reset($definitions);
    

    I'm not sure that that issue will make it into core eventually. So we'd better not refer to it.

  15. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationManagerInterface.php
    @@ -0,0 +1,30 @@
    +   * @throws \Drupal\Component\Plugin\Exception\UnknownPluginException
    +   */
    

    Change to PluginNotFoundException.

fietserwin’s picture

Second batch of my review: ImageToolkitOperation(Base/Interface and the annotation). More to come.

  1. +++ b/core/lib/Drupal/Core/ImageToolkit/Annotation/ImageToolkitOperation.php
    @@ -0,0 +1,76 @@
    +  /**
    +   * The plugin ID.
    +   *
    +   * Use {toolkit}.{operation} pattern.
    +   *
    +   * @var string
    +   */
    +  public $id;
    +
    

    I am not sure that we want id's like this. Id's are free format and should not clash, so should normally include the module name. (See also further on regarding the proposal to add a toolkit field to the annotation.)

  2. +++ b/core/lib/Drupal/Core/ImageToolkit/Annotation/ImageToolkitOperation.php
    @@ -0,0 +1,76 @@
    +   * - default: (optional) The default value of this argument when the argument
    +   *   is not required.
    +   *
    

    We should better describe that his argument is required when 'required' is false (and ignored when 'required' is true).

  3. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationBase.php
    @@ -0,0 +1,75 @@
    +   * Builds am image toolkit operation plugin.
    +   *
    

    - constructs instead of builds
    - an instead of am

  4. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationBase.php
    @@ -0,0 +1,75 @@
    +   * @var ImageToolkitInterface $toolkit
    +   *   The image toolkit.
    +   */
    

    - @param instead of @var
    - fully namespaced class name according to https://drupal.org/node/1354#types.

  5. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationBase.php
    @@ -0,0 +1,75 @@
    +  public function prepare(ImageInterface $image, array &$arguments) {
    +    $arguments = $this->prepareArguments($arguments);
    +  }
    +
    

    Are actual operations supposed to override the public prepare method and/or the protected prepareArguments method?

    If the latter, the image parameter should be passed to that method as well. Otherwise it may better be made private or, even better, be inlined because actual operations are kind of required to call the parent implementation anyway and they may do so at any time

    If the former, the 'template method' design pattern which I would prefer here, we should make prepare final, inline the code of prepareArguments, and make that an empty method here in the base class to be overridden by actual operations for their additional validation code. That way operations cannot 'forget' to call the basic validation code.

  6. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationInterface.php
    @@ -0,0 +1,52 @@
    + * An image toolkit operation provides image manipulations like scaling,
    + * cropping, rotating, etc.
    + */
    ...
    +   * Validates, normalizes and adds processing to arguments before sending them
    

    - 1 operation provides 1 manipulation (correct the sentence)
    - for a specific toolkit (add something like this to the sentence)

  7. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationInterface.php
    @@ -0,0 +1,52 @@
    +   *
    

    This seems better:
    Validates, normalizes and adds defaults to arguments before sending them to the apply method.
    (we are already in the operation object)

  8. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationInterface.php
    @@ -0,0 +1,52 @@
    +   * @return bool|null
    +   *   Tells if the image still needs this operation to be applied. FALSE means
    +   *   that after applying the operations the image will remain unchanged.
    +   *
    

    - when is null returned?
    - false means that the operation will not be applied because the image would remain unchanged.

  9. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationManager.php
    @@ -0,0 +1,102 @@
    +    // Get the toolkit ID from the image operation plugin class namespace. Each
    +    // plugin is placed under Plugin\ImageToolkit\{plugin_id}.
    +    $path = explode('\\', $definition['class']);
    +    $definition['toolkit'] = $path[count($path) - 2];
    +
    

    I am not sure that we want the toolkit id in the namespace. The id is normally fully lowercase and uses underscores, whereas our namespaces normally are camel cased and thus at least start with a capital and do not use underscores (see the only namespace we have without that: Drupal\system\Plugin\ImageToolkit\Operation\gd).

    Furthermore, I do not see the need for repetition of the toolkit name/id in the namespace: ImageMagick\operations seems a more logical place than ImageMagick\im (same for the GD toolkit if and when that becomes a separate module).

    So I propose to add a separate specific toolkit field to the annotation, so that the annotation fully defines the operation, not a mix of annotation and namespace.

fietserwin’s picture

Batch 3: the gd operations and the changes to the gd toolkit itself. More to do.

  1. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Crop.php
    @@ -0,0 +1,77 @@
    +    // Preserve aspect.
    +    $aspect = $this->toolkit->getHeight($image) / $this->toolkit->getWidth($image);
    +    $arguments['height'] = empty($arguments['height']) ? $arguments['width'] * $aspect : $arguments['height'];
    +    $arguments['width'] = empty($arguments['width']) ? $arguments['height'] / $aspect : $arguments['width'];
    +
    

    - So, the arguments width and height are required but may be empty?
    - What if both are empty?

  2. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Crop.php
    @@ -0,0 +1,77 @@
    +    if ($arguments['width'] <= 0 || $arguments['height'] <= 0) {
    +      return FALSE;
    +    }
    +
    

    Should become part of prepare as that method is responsible for validation.

  3. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Resize.php
    @@ -0,0 +1,66 @@
    +    if ($arguments['width'] <= 0 || $arguments['height'] <= 0) {
    +      return FALSE;
    +    }
    +
    

    Should be part of prepare.

  4. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Scale.php
    @@ -0,0 +1,77 @@
    +    $width =& $arguments['width'];
    +    $height =& $arguments['height'];
    +
    

    - I read this as: $a = $a & $b, though it probably is an assignment by reference: $a = &$b. Please place the spaces differently.
    - As the parent method is not yet called, the width or height keys do not have to exist yet: better first call the parent.

In general:
- almost all prepare methods of the gd operations are lacking a return statement (in all flows) while a @return is specified. I don't like this. Let's always return true (parameters prepared successfully) or false (operation does not need to be applied) or throw an exception (parameter error).
- A lot of code is calling GdToolkit specific methods, which is of course fully legal and logical. PHP allows to redefine/override a property which would allow us to make the type info more specific. So, to improve DX (code checking, code completion), we could add the following lines to all GD operations:

  /**
   * The image toolkit.
   *
   * @var \Drupal\system\Plugin\ImageToolkit\GDToolkit
   */
  protected $toolkit;

or create an intermediate \Drupal\system\Plugin\ImageToolkit\Operation\gd\Base base operation class for that, or (my preference) create a trait for that. This will make many warnings from my IDE code checker go away.
- This issue is about moving operation code to separate plugin classes, so this issue should not be bothered with this, but I find the error checking/handling and argument handling not good nor logical nor complete. So, I will file a follow-up to improve robustness of the GD operations.
- Same for what comprises an operation. Ideally an operation should not be much more than a wrapper around a function/method call of the actual toolkit in use. This means that calculations should be moved to the image effect as much as possible. e.g. the crop operation should not be smart about retaining the aspect ratio if a parameter is missing, the image effect should decide so and only pass concrete numbers to the operation. So, I will file a follow-up issue to make the operations dumber and the image effects smarter, thereby probably making #2108307: Provide an abstract layer for image operations geometry superfluous (which is already closed as a duplicate of this, but is not handled here in a way as suggested over there).
- Same for effects that build on other effects: the operations should not be a strict 1-to-1 copy of the effects, the effect should call the different operations. This holds for the operations Scale (based on resize) and ScaleAndCrop (based on scale and crop).

mondrake’s picture

Assigned: Unassigned » mondrake

Ok I will take a first stab at these comments.

fietserwin’s picture

I edited the comments and reviewed the tests and changes outside the image system: no remarks, those changes are all OK. So that completes my review of the patch.

mondrake’s picture

Status: Needs work » Needs review
Related issues: -#2189985: Rename UnknownPluginException to PluginNotFoundException
FileSize
11.19 KB
93.07 KB

@fietserwin thanks for detailed review. This patch addresses comments in #209 only; more when time allows, please wait for more complete work before further comments.

1. Ok
2. Ok initially added PluginManagerInterface to ImageToolkitOperationManagerInterface, then removed see point 7 :). As to why an interface see @tim.plunkett comment #151 above.
3. I'd say at the toolkit level. In D7 a watchdog entry for a missing function is logged in image_toolkit_invoke(), so for consistency this is now in the space of the ToolkitBase implementation. Done here.
4. Ok
5. Ok
6. Ok
7. Indeed. Done.
8. Ok
9. Ok
10. Ok
11. Ok. This is only going to be used at plugin discovery, so I am not catching this exception at run-time. It will fail hard but that's more of an exception for developers.
12/13. Ok
14. Let's leave it there and only remove if requested by a core committer. I can point you to todos in the codebase expected for Drupal 9...
15. Ok

Also, the patch adds a test to verify that invoking a missing operation plugin, the toolkit apply() method will return FALSE.

Setting to NR for the testbot to say yeah or nope at this stage, will reset to NW later.

Status: Needs review » Needs work

The last submitted patch, 214: 2073759-toolkit_op-213.patch, failed testing.

fietserwin’s picture

I agree with all of your answers in #214.

I want to add a further insight to #210.5:
If we decide to go for the 'template method' pattern, we better take the apply method as the template method and make it final and have it do something like:

public final function apply(ImageInterface $image, array $arguments = array()) {
  prepareArgumentsBase(); // the current prepareArguments
  preprareArguments(); // no-op y base, to be overridden by the actual operations to add operation specific parameter validation
  execute(); // abstract for base, required to be implemented by actual operations
}

This way we can be sure that, unlike with the current (public) apply, the arguments are really completed and validated. It would also make the interface simpler.

[EDIT: corrected the name of the design pattern]

mondrake’s picture

Status: Needs work » Needs review
FileSize
11.27 KB
1.37 KB
93.45 KB

Need to keep a getToolkitOperation() proxy in ImageToolkitBase to allow PHPUnit testing. Interdiff both against #204 and against #214

mondrake’s picture

Status: Needs review » Needs work
mondrake’s picture

Issue summary: View changes
Status: Needs work » Needs review

Just a reroll of #217 due to #2281905: Stop caching plugin discovery/info hooks by language. Also cleaned up issue summary a bit.

mondrake’s picture

FileSize
93.14 KB

Whoops

mondrake’s picture

Assigned: mondrake » Unassigned
FileSize
11.33 KB
94.28 KB

#210:

1. I think we could just say that this pattern is the recommendation for the *base* implementation of a plugin operation of a toolkit, meaning: if you are developing a brand new toolkit, use this pattern to provide the id of your operations. If you are a developer that wants to implement an override of an operation plugin, use a {module}.{toolkit}.{operation} pattern.
(In any case, discovery of the appropriate operation plugin is based on the values of the 'toolkit' and 'operation' definition keys, not on the id itself, so in the end this is just a convention.)
2. Ok
3. Ok
4. Ok
6. Ok
7. Ok
8. Let's make it more clear - my suggestion is that the prepare method returns

   * @return bool
   *   TRUE if the operation has to be applied to the image. FALSE if the
   *   image has to remain unchanged.

9. A $toolkit annotation key was in the patches of this issue till #136, then removed (from the annotation) in the patch in #150, see comments there. However, even if it is not in the annotation, it still gets in the plugin definition via processDefinition(), and is used for the selection of the operation plugin to be used. I like the current solution because it matches the class namespace with the file structure, and a single module can provide operations for multiple toolkits just by putting plugins in separate subdirectories of my_module/src/Plugin/ImageToolkit/Operation/{toolkit_id}.
Supposing we have a 'my_op' operation in a MyOp class and two toolkits, 'gd' and 'imagemagick', this will result in:

my_module/src/Plugin/ImageToolkit/Operation/gd/MyOp.php             --> class \Drupal\my_module\Plugin\ImageToolkit\Operation\gd\MyOp
my_module/src/Plugin/ImageToolkit/Operation/imagemagick/MyOp.php    --> class \Drupal\my_module\Plugin\ImageToolkit\Operation\imagemagick\MyOp

#211:

1. Ok, added a check.
2/3. Ok.
4. Fixed the coding standards; cannot move the call to parent::prepare() as in this case parent is Resize, where width and height are required, and the code before just really prepares that.

Also, all prepare() methods of the gd operations now return TRUE.

So if I am not mistaken we have open:
210.1 - need agreement on my proposal above
210.5 - need to think more about it; in the meantime unassigning myself if anyone wants to step in. In any case, I can tell that the reason to split prepare and prepareArguments was to be able to get PHPUnit tests working. My preference would be to merge them back into one.

claudiu.cristea’s picture

9. A $toolkit annotation key was in the patches of this issue till #136, then removed (from the annotation) in the patch in #150, see comments there. However, even if it is not in the annotation, it still gets in the plugin definition via processDefinition(), and is used for the selection of the operation plugin to be used. I like the current solution because it matches the class namespace with the file structure, and a single module can provide operations for multiple toolkits just by putting plugins in separate subdirectories of my_module/src/Plugin/ImageToolkit/Operation/{toolkit_id}.
Supposing we have a 'my_op' operation in a MyOp class and two toolkits, 'gd' and 'imagemagick', this will result in:

We use previously a very ugly name-spacing like GDCrop. Now that became gd/Crop. There are few benefits:

  • More natural name-spacing. The operation name is nice (e.g. Crop).
  • A module can provide an operation for several toolkits while keeping the structure simple and clear.
  • DX: The toolkit doesn't need to be annotated anymore. It magically extracts the toolkit ID from the namespace.

Related to prepare stuff: Yes, we can force the default validation but also we can go the way is now designed. I preferred the flexible way where the developer is able to completely swap the default preparation/validation. This is how usually OOP works. It let's you extend a default implementation OR totally replace with your own. I don't have right now a use case for a full swapping of default preparation but who knows in the future. It's a matter of policy: do we want to be conservative or flexible? I don't have a strong opinion.

mondrake’s picture

Ok. About 210.5:

Here I dropped prepareArguments(). Now everything is into prepare() and apply(), as per initial approach. Again, the split off of prepareArguments was needed to enable PHPUnit tests. Since there were calls to methods to get annotation stuff in there, PHPUnit was failing as the operation class (in PHPUnit tests) is instantiated by the mock builder and not by the plugin manager, hence no annotation definition available, hence failure.

In order to get here though, I stumbled upon @aspilicious points in #176 and #205 - to get the arguments' definitions we can not rely on annotations if we want some sort of unit testability. So - removed the $arguments annotation and introduced a new method defaultConfiguration() that takes the same data. I used this method name so we can be open to implement ConfigurablePluginInterface if that makes sense at some point. I believe that one additional benefit of this approach now is that it can allow implementing base classes per operation, e.g. to have, given an operation, common defaultConfiguration() and prepare() methods across different toolkits, and just leave the toolkit specific apply() -> reduce boilerplate code.

aspilicious’s picture

Whoeha...

Looking at your changes I would name the function different after all.
It doesn't provide the actual configuration it defines the arguments.
So yeah maybe just call it arguments()

But yeah general +1

mondrake’s picture

FileSize
11.48 KB
95.29 KB

Renamed defaultConfiguration() to argumentsDefiniton(), and added some documentation to the annotation class to suggest the pattern for building the plugin ID string (re #210.1)

claudiu.cristea’s picture

Why not just arguments() as @aspilicious suggested? +1 for that name.

mondrake’s picture

FileSize
8.21 KB
95.16 KB

Done

fietserwin’s picture

Status: Needs review » Needs work

Thanks for all these changes. It really starts to look good. Still, a few points though:

  1. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitBase.php
    @@ -7,15 +7,87 @@
    +  public function apply($operation, ImageInterface $image, array $arguments = array()) {
    +    // Get the plugin to use for the operation. If there are no plugins
    +    // available, log an error message and return.
    +    try {
    +      $plugin = $this->getToolkitOperation($operation);
    +    }
    +    catch (PluginNotFoundException $e) {
    +      watchdog('image', 'The selected image handling toolkit %toolkit can not process operation %operation.', array('%toolkit' => $this->getPluginId(), '%operation' => $operation), WATCHDOG_ERROR);
    +      return FALSE;
    +    }
    +
    +    // Validate and prepare arguments before applying the operation. The
    +    // apply operation will not be executed if prepare() decides that the
    +    // final image will not change. If there are errors in the arguments
    +    // passed, log a warning message and return.
    +    try {
    +      $execute_apply = $plugin->prepare($image, $arguments);
    +    }
    +    catch (\InvalidArgumentException $e) {
    +      watchdog('image', $e->getMessage(), array(), WATCHDOG_WARNING);
    +      return FALSE;
    +    }
    +    if (!$execute_apply) {
    +      return TRUE;
    +    }
    +
    +    // Apply the image operation and return its result.
    +    return $plugin->apply($image, $arguments);
    +  }
    +
    

    Can we refactor this to 1 try with multiple catches and 1 exit point (1 return) (also see the next remark, where it is explained why this code should be moved to the ImageToolkitOperationBase class).

  2. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationInterface.php
    @@ -0,0 +1,53 @@
    +interface ImageToolkitOperationInterface extends PluginInspectionInterface {
    +
    

    I have been looking once more at this interface. (with the remarks about the template pattern and avoiding boilerplate code (including the need to call the parent method a the begin of the method). What is really needed here? only the apply() method! The other declared methods are boiler plate for the base implementation that core delivers, but are not really part of the interface.

    • You can totally swap the base toolkit operation out and define your own validation within apply(), e.g. using getters (that do the checking and defaulting) for the arguments you need at any moment. (#222)
    • If you go with the core provided base implementation, you don't want to introduce errors as below. Therefore a template pattern approach is preferred as that better protects the developer.
    • The current apply in ImageToolkitBase has far too much knowledge of the current ImageToolkitOperationBase whereas it should better only forward the call to the operation. This also prevents a total swap of the operation system.

    Thus, to further decouple the (base) toolkit and the operation, to simplify the interface to what is strictly needed, I propose to:

    • Reduce ImageToolkitOperationInterface to the apply method only (simpler interface, containing only what is really needed).
    • Thus moving the code from the current ImageToolkitBase::apply() to ImageToolkitOperationBase::apply() (better decoupling). (as in remark #209.7: if a method calls multiple methods from another object, that part probably needs to be in the other object in the first place.)
    • Implement the pattern of prepare(), defaultConfiguration() and apply() in the apply() method of the (core provided) ImageToolkitOperationBase, (introducing an abstract protected execute()) thereby assuring that deriving classes cannot "forget" to follow that pattern (error below).

    Whether we make the apply method final, or just document how it should be used (do not override!) is not that important.

  3. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Scale.php
    @@ -0,0 +1,78 @@
    +  public function prepare(ImageInterface $image, array &$arguments) {
    +    $width = &$arguments['width'];
    +    $height = &$arguments['height'];
    +
    

    The keys width and height are not necessarily defined at this point here, leading to an undefined index error. That's why the call to the parent method needs to be before these lines.

mondrake’s picture

@fietserwin -

The current apply in ImageToolkitBase has far too much knowledge of the current ImageToolkitOperationBase whereas it should better only forward the call to the operation.

I am sorry but I am not with you on this, ImageToolkitBase just (a) finds the plugin to use, (b) cleans up the arguments, (c) executes the operation, and does it for any operation. I tried moving around some code as per your point 1 and 2 in #228, without coming to anything that would be significantly better. But please feel free to take it over.

On #228.3 - that's exactly the point: height and width may not be set at this stage (it's the scale operation), and by using the & operator $arguments['width'] and $arguments['height'] will get defined, set to null, and associated to the corresponding $width and $height variables, that are used in the code afterwards to determine the ratio of the scale. See the second note in this link.

fietserwin’s picture

Status: Needs work » Needs review
FileSize
101.56 KB
38.93 KB

After an off-line conversation with @mondrake, to further explain my ideas, we decided that these changes indeed are an improvement and that, due to lack of time at @mondrake's side, I would implement them on top of the current patch.

This patch implements/changes:

  • (#228.2) Reduces the ImageToolkitOperationInterface to its minimum, thus just an apply (with now a required arguments parameter: for toolkit base they are optional but that apply will always pass an array, so we can make them required as well).
  • (#228.1/2) By reducing the interface, part of the code of ImageToolkitBase::apply() was moved to ImageToolkitOperationBase::apply(). This really cleans up ImageToolkitBase::apply()and decouples toolkit and operation.
  • ImageToolkitOperationBase::apply() utilizes the template pattern, thus child classes only need to implement certain (abstract) methods and do not need to worry about calling the parent method.
  • To protect the template method (apply), it was made final. this was not done for prepareArguments(). These decisions are open for discussion.
  • To force operations to "document" the absence of parameters, the parameters() method was made abstract.
  • A GetToolkit() getter was added to ImageToolkitOperationBase, this allows to "retype" the toolkit for better code analyzing and completion. See the trait that was added to the folder with the gd operations.
  • ImageToolkitOperationBase::prepare() was renamed to prepareArguments() as there is now also a validateArguments(). It does no longer return a bool, but the array of arguments. (this meant that the check on upscaling in the scale operation was moved to execute(), where it IMO belongs.
  • ImageToolkitOperationBase::prepareArguments() can throw a plugin exception, as not defining a default for an optional parameter is an error of the plugin, not an argument error on the calling side. This exception is not documented (implementation detail, so it should not be documented on the interface), nor caught. By rewriting the method a bit, it will always be thrown, even if the (optional) parameter is provided by the calling side. This way it will be thrown on the first test during development.
  • ImageToolkitOperationBase::validateArguments() is added for child class validation and completion. This way child classes do not have to call the parent method and can be assured that all parameters are available.
  • As ImageToolkitOperationBase::apply is now final, another (abstract) method, execute(), was added. This is were child classes should add the actual image manipulation code.
  • In the GD operations, I renamed some variables to a more logical/consistent name.

To get the test locally working, PHP 5.5 on (Windows 8.1), I also needed to change the following:

  • I discovered some (already existing) errors in the rotate operation: if an image does not already have a transparent color, imagecolortransparent() returns -1, not 0! Apparently, PHP 5.5 is more strict in checking color indices, leading to something already reported in issue #2215369: Various bugs with PHP 5.5 imagerotate(), including when incorrect color indices are passed in
  • Solving the above problem, revealed another problem: using 'fully opaque white' as transparent color. I had to change this to rgba(0, 0, 0, 127), the "definition" of transparent as used in ToolkitGdTest.
  • Lastly, imagerotate() in PHP 5.5. can result in images that are 1 pixel smaller in both dimensions than in PHP 5.4. This can be due to a new feature that allows to set the interpolation algorithm: imagesetinterpolation(). I could not find any hint about what interpolation constant covers the algorithm used in PHP 5.4.

As the problems in the tests seem to be PHP 5.5 related, they can be separated from this patch. I can do so in a next patch and will use the above mentioned issue for that.

Status: Needs review » Needs work

The last submitted patch, 230: 2073759-230.patch, failed testing.

fietserwin’s picture

Status: Needs work » Needs review
FileSize
102.32 KB
39.7 KB

The new trait file was not in the patch, nor interdiff.

BTW:
- Moved issue #2215369: Various bugs with PHP 5.5 imagerotate(), including when incorrect color indices are passed in to the core image system queue.
and as a follow-up to my last remarks in #211, I created:
- #2284589: Error handling strategy for image toolkit operations.
- #2284577: Make image effects smarter, image toolkit operations dumber.

fietserwin’s picture

Testbot refuses to test #232, so I'm posting a new version of it, without the changes regarding the PHP 5.5 rotation problems. I moved these changes to #2215369: Various bugs with PHP 5.5 imagerotate(), including when incorrect color indices are passed in. Also removed a debug statement that was left in the previous patch.

Let's see if we can get the testbot to test the new patch.

Status: Needs review » Needs work

The last submitted patch, 233: 2073759-233.patch, failed testing.

The last submitted patch, 232: 2073759-232.patch, failed testing.

fietserwin’s picture

233: 2073759-233.patch queued for re-testing.

fietserwin’s picture

Status: Needs work » Needs review
FileSize
100.37 KB
11.37 KB

Thanks to @mondrake for pointing out possible failures in the unit tests :) No thanks to the testbot for not indicating what went wrong :(

I solved the failing tests in ImageTest unit tests but found out that they are actually no longer needed or at least are at the wrong place: The unit test for the Image class mocks the toolkit and the toolkit operations. But it is not the business of the Image to know that a toolkit uses image toolkit operations to forward calls to the ImagetoolkitInterface::apply() method. If we had decided that the GD toolkit would keep the core operations in the GDToolkit itself (which has been suggested) then the Image class would still be exactly the same but the test would miserably fail again. So, IMO, we are testing implementation details that we should no longer test here and that we are already testing in 2 other places, see my comment in #2257587-14: Remove $image parameter in calls between and within Image and toolkit. Instead this unit test should test the Image::apply() method, not all currently known possible calls to it via the now (already) no longer existing methods scale(), resize(), etc.

Another consequence of testing implementation details of other classes is that testScaleSame() failed: it is no longer prepareArguments() that decides if the operation should be executed or not, but execute() itself.

I will add a reference to this comment in issue #2109459: Review image test suite, but prefer to solve it here in this issue.

The last submitted patch, 233: 2073759-233.patch, failed testing.

fietserwin’s picture

Ignore #238, that was not the *last* submitted patch ...

aspilicious’s picture

  1. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Desaturate.php
    @@ -0,0 +1,48 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function arguments() {
    +    // This operation does not use any parameters.
    +    return array();
    +  }
    

    Thats why we have base classes.
    They should implement this method and return an empty array.

  2. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/GDToolkitOperationTrait.php
    @@ -0,0 +1,21 @@
    +trait GDToolkitOperationTrait {
    +
    +  /**
    +   * The correctly typed image toolkit for GD operations.
    +   *
    +   * @return \Drupal\system\Plugin\ImageToolkit\GDToolkit
    +   */
    +  protected function getToolkit() {
    +    return parent::getToolkit();
    +  }
    +
    +}
    

    I don't like this structure.
    We should have a GDImageToolkitOperationBase that has this metod.

    Traits should be used to share functionality if we can't use inheritance (because the class already extends another class). In this case we can perfectly create a base class for GD.

fietserwin’s picture

FileSize
100.01 KB
6.38 KB
44.13 KB

RE #240.1:
IMO:
- This method serves an important documentation goal as well, that's why I explicitly made the base method abstract, thereby forcing operation implementers to explicitly document the (absence of ) parameters.
- There is only one operation that has no parameters and, afaik, contrib also will not have any operation without parameters (the autorotate effect (the only effect in contrib I know without parameters) will use the rotate operation).
- This operation, desaturate, is either wrongly named (should be called grayscale or black & white), or should be changed to use the IMG_FILTER_COLORIZE filter and consequently accept a parameter per color channel. This would e.g. allow to create sepia pictures instead of b&w.

RE #240.2:
I guess you are right. I had been thinking about creating a base class per operation that implements the parameter specifying and validating stuff as that will (probably) be equal for all toolkits. But not having done that, we could (and should) use a base class for GD operations instead of the trait. I still like the idea of a base class per operation though.

So, this patch only changes #240.2. But if people agree that we'd better create a base class per operation, I will change that part again.

I added an interdiff against #237 and @mondrake's #227 to give an overview of my changes.

aspilicious’s picture

In core (other places) we usually put empty functions in the base class anyway...
But in this case I don't think it hurts...

Close to RTBC now, no time to go over it again.

mondrake’s picture

+1 for RTBC from my side, even though I do not think I am in position to set that given my work on the patch :(
This would certainly unblock work on contrib toolkit and image effect modules. See ImageCache Actions' #2228361: Drupal 8 port issue summary as evidence.

I like the idea of base classes per operation as arguments(), prepareArguments() and validateArguments() methods would likely be one and the same, or very similar, across different toolkits. But that could live in a followup as this patch is big enough already, and such a followup would not imply an API change like this does.

fietserwin’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

OK, all remarks from #210, #211 and #212 (representing a detailed and exhaustive review) are satisfactorily answered or implemented by @mondrake and a few last remarks (#228) were, due to lack of time by @mondrake, implemented by myself (#230 to #237). My changes were reviewed by @aspilicious and @mondrake.

So, I think that with 3 people who are OK with the patch, even if 2 of them actively developed the patch (though my development contributions to the part are very small, I was more on the review side all that time), we can say RTBC.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Plugin system

None of the plugin maintainers have reviewed this in the past 9 months, and I have some concerns about the structure and naming of the plugins.

Additionally, there are some other nitpicks.

  1. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitBase.php
    @@ -7,15 +7,72 @@
    +   * Constructs a Drupal\Component\Plugin\ImageToolkitBase object.
    

    Either put a leading \ on that, or just have it be ImageToolkitBase

  2. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitBase.php
    @@ -7,15 +7,72 @@
    +   * Get a toolkit operation plugin instance.
    

    Gets

  3. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitBase.php
    @@ -7,15 +7,72 @@
    +      watchdog('image', 'The selected image handling toolkit %toolkit can not process operation %operation.', array('%toolkit' => $this->getPluginId(), '%operation' => $operation), WATCHDOG_ERROR);
    ...
    +      watchdog('image', $e->getMessage(), array(), WATCHDOG_WARNING);
    

    watchdog() is deprecated, we have a logger service (here and throughout the patch).

  4. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitInterface.php
    @@ -259,4 +147,21 @@ public static function isAvailable();
    +   * @return mixed
    

    bool?

  5. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationBase.php
    @@ -0,0 +1,183 @@
    +   * An array whose keys are the names of the arguments (e.g. "width",
    +   * "degrees") and each value is an associative array having the following
    +   * keys:
    +   * - description: A string with the argument description. This is used only
    +   *   internally for documentation purposes, so it does not need to be
    +   *   translatable.
    +   * - required: (optional) A boolean indicating if this argument should be
    +   *   provided or not. Defaults to TRUE.
    +   * - default: (optional) When the argument is set to "required" = FALSE,
    +   *   this must be set to a default value. Ignored for "required" = TRUE
    +   *   arguments.
    +   *
    +   * @return array
    

    This paragraph should be nested under the @return.

  6. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationBase.php
    @@ -0,0 +1,183 @@
    +          throw new \InvalidArgumentException("Argument '$id' expected by plugin '" . $this->getPluginId() . "' but not passed.");
    ...
    +          throw new InvalidPluginDefinitionException("Default for argument '$id' expected by plugin '" . $this->getPluginId() . "' but not defined.");
    

    These should use String::format()

  7. +++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationManager.php
    @@ -0,0 +1,106 @@
    +    // Get the toolkit ID from the image operation plugin class namespace. Each
    +    // plugin is placed under Plugin\ImageToolkit\{plugin_id}.
    +    $path = explode('\\', $definition['class']);
    +    $definition['toolkit'] = $path[count($path) - 2];
    

    I'm not sure if this is a good idea. Part of the goals of plugins was to reduce magic code. Why not just require them to place it in the annotation? Also, isn't Plugin\ImageToolkit\Operation\{plugin_id}?

  8. +++ b/core/modules/file/file.module
    @@ -469,7 +469,7 @@ function file_validate_image_resolution(File $file, $maximum_dimensions = 0, $mi
    -        if ($image->scale($width, $height)) {
    +        if ($image->apply('scale', array('width' => $width, 'height' => $height))) {
    

    So this is the crux of the patch. Right now we have good DX for core operations, and terrible/impossible DX for custom operations, and this issue provides the lowest common denominator for both.

    Was there any discussion about leaving the core operations as methods on Image and having them pass through to apply()?

  9. +++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Crop.php
    @@ -0,0 +1,95 @@
    + * Contains \Drupal\system\Plugin\ImageToolkit\Operation\gd\Crop.
    ...
    +namespace Drupal\system\Plugin\ImageToolkit\Operation\gd;
    ...
    + *   id = "gd.crop",
    + *   operation = "crop",
    

    So back to what I said before, I think toolkit should be an explicit part of this annotation.

    Then this could just be Plugin\ImageToolkit\Operation\GDCrop, with no additional magic

    Also, we usually don't use . in plugin IDs, usually underscores.

fietserwin’s picture

Status: Needs work » Needs review
FileSize
102.09 KB
18.18 KB

RE #245:

  1. done
  2. done
  3. done (also on places were we are only moving existing code and evenin the image effects., though I took the Drupal::logger shortcut, no injection. Let's change that when we are updating the test sets to unit tests.)
  4. done
  5. done
  6. done
  7. I asked the same in #210.9: A $toolkit annotation key was in the patches of this issue till #136, then removed (from the annotation) in the patch in #150, see comments there. I'm in favor of adding it to the annotation, but are not going to decide on that my self. So if someone else can decide.
  8. There indeed was discussion but no real consensus came out of that. I want to mention 2 things:
    • Use of apply versus __call. With __call we can add @method doc tags to the class/interface, nice but these can't have parameter documentation, so that does not bring us a lot.
    • Leave the core methods as they are, use apply() only for contrib: I oppose that for reasons of consistency, the "eat your own dog food" principle and the tests it gives us. I personally take the loss of DX here for granted.
  9. changed . to _; for the rest see point 7.
mondrake’s picture

Hi,

this re-introduces the toolkit as an *explicit* annotation key as per #245.7 and #245.9, plus fixes the new naming convention also for the test toolkit. I am still in favour of keeping the toolkit in the class namespace as per @claudiu.cristea comment in #222, i.e. have a gd/Crop class instead of a GDCrop one.

As to the 'crux' in #245.8: IMHO, the DX issue is smaller than what appears. In the end, image operations are mainly called from image effects, and there the arguments are already represented as an array within the effect's $configuration property. In fact, we could even think about just passing $this->configuration, or a copy of it if needs to be manipulated, to the image operation...

That said, if we are still strong on keeping specific scale(), resize(), ... methods on ImageInterface and this is the only way to move forward, then I think we could just re-implement them, kind of reverting #2103635: Remove effects from ImageInterface. These methods would just prepare the method's arguments in an array format and return $this->apply('operation', $arguments).

claudiu.cristea’s picture

Keeping $image->scale() or $image->resize() it's not DX. It's exactly the opposite. Everywhere we are abstracting and cleaning except the image system. Why "scale" (provided by core) would benefit of its own method and "blur" (provided by a contrib) not? Need a fancy $image->scale()? Write a contrib module and wrap ->apply(). That is not DX, that it's about being fancy.

And I'm also keeping my opinion about the need to have a clear, logical namespacing and use gd/Crop instead of GDCrop. Why this hurts?

fietserwin’s picture

#247:
- Good catch regarding the . versus _ in tests and docs.
- So we go for toolkit in annotation: fixed, so no more discussion please.

#248:
- Why would GD use either gd/Crop or GDCrop and not just Crop? IMO, all GD classes should be in a namespace having GD somewhere earlier in the namespace: \Drupal\GD\Plugin\ImageToolkit\toolkit for the toolkit class, and \Drupal\GD\Plugin\ImageToolkit\operation\scale, etc. for the operation classes. But that is for issue #2110835: Move the GD toolkit on its own module. So, IMO, for now we should stick with GD in the class name (toolkit) resp. namespace (GD operations). Image effects in contrib can use the name of the toolkit in the namespace of their operations but should not have to (and they no longer have to).

#247/#248:
- That's the 3 of us not wanting the scale(), resize(), etc back: for me that stands as a decision.

For me, it's back to RTBC.

fietserwin’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs a reroll due to #2257587: Remove $image parameter in calls between and within Image and toolkit. I'm going to work on that asap.

fietserwin’s picture

Status: Needs work » Needs review
FileSize
102.81 KB
21.04 KB

Rerolled (actually, due to too many merge conflicts, it was more like redoing the other issue on top of this patch, so a good review is needed again.)

Status: Needs review » Needs work

The last submitted patch, 251: convert_toolkit-2073759-251.patch, failed testing.

fietserwin’s picture

Status: Needs work » Needs review
FileSize
102.54 KB
1.47 KB

These 2 lines in ScaleAndCrop are very very well test covered :)

Locally I'm running PHP 5.5, leading to many failing tests due to different dimensions after rotation and other errors, so testing locally is a bit difficult, but I think that I solved the problems that caused the above fails.

Status: Needs review » Needs work

The last submitted patch, 253: convert_toolkit-2073759-253.patch, failed testing.

fietserwin’s picture

Status: Needs work » Needs review
FileSize
98.62 KB
14.54 KB

I started cleaning up some tests. We are testing that parameters are correctly passed to the toolkit or even to its operations on various levels:
- image effect (core\modules\image\src\Tests\ImageEffectsTest.php)
- image (core\tests\Drupal\Tests\Core\Image\ImageTest.php)
- image toolkit (core\modules\system\src\Tests\Image\ToolkitTest.php)

The 1st is more end-to-end tests, as it tests the apply chain from ImageEffect::apply(), via Image::apply(), ImageToolkitBase::apply() and ImageToolkitOperationBase::apply() to the execute() methods of test operations. The 2nd is a unit test, thus preferred over the other and also contains some more intelligent tests (float width of 30.4 rounded to 30 pixels). The 3rd contains exact copies of a number of tests in the 1st, while the methods (resize(), scale(), etc.) do no longer exist on the toolkit as they are all replaced by the apply method.

Furthermore, the 1st test set is testing from ImageEffect all the way down to individual operations on a test toolkit with test operations. But as a remainder of how it currently is, thus before this patch, it only check at the toolkit::apply level what was passed in.

Therefore I did the following cleaning-up:
- Removed the test operations, made the logCall protected, again and had TestToolkit override the apply() method to log the call to it.
- Removed the duplicate tests from ToolkitTest.

Note that:
- Operations and the apply methods from the toolkit base and toolkit operation base are tested in the ImageTest unit test.
- The GD operations are tested in the ImageDimensionsTest (core\modules\image\src\Tests\ImageDimensionsTest.php) and the GD toolkit test (core\modules\system\src\Tests\Image\ToolkitGdTest.php).

So these changes do not remove any test coverage and there's still a lot to do for the test cleanup issue.

I think that I had no more local fails other then those due to PHP 5.5.

The interdiff is w.r.t #251 but does not contain the deletion of the test operations.

fietserwin’s picture

@mondrake pointed out some omisssions and further optimizations in the reroll.

I found some minors in the documentation and had Image switch to using getToolkit() instead of directly using $toolkit (If we allow Image to be overridden, we should use getters internally as well).

Interdiff is w.r.t. #255 except that it refuses to pick 1 change in the ImageToolkitOperationInterface.php (removing $image as argument from apply),so that is not in the interdiff, but is in the patch.

fietserwin’s picture

Issue tags: -Needs reroll
FileSize
99.46 KB
21.94 KB

And I should of course had adapted the tests :(

The last submitted patch, 256: convert_toolkit-2073759-256.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 257: convert_toolkit-2073759-257.patch, failed testing.

fietserwin’s picture

Status: Needs work » Needs review
FileSize
99.42 KB

Something went wrong, the interdiff did contain my test corrections,the patch didn't. Let's try again (thus interdiff of previous post is actually for this post.

claudiu.cristea’s picture

+++ b/core/lib/Drupal/Core/ImageToolkit/Annotation/ImageToolkitOperation.php
@@ -0,0 +1,76 @@
+   * @ingroup plugin_translatable

+++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationBase.php
@@ -0,0 +1,178 @@
+          throw new \InvalidArgumentException(String::format("Argument '!argument' expected by plugin '!plugin' but not passed.", array('!argument' => $id, '!plugin' => $this->getPluginId())));
...
+          throw new InvalidPluginDefinitionException(String::format("Default for argument '!argument' expected by plugin '!plugin' but not defined.", array('!argument' => $id, '!plugin' => $this->getPluginId())));

I have to disagree with @tim.plunkett here (comment #245.6). The reason why we are not translating exception errors is not necessary related to language and human readability but also we want to avoid calling other components of the system in the middle of an exception throwing. Those calls might throw themselves other exceptions and we might hide the real problem. We want to exit as soon as we can without passing the control to other methods. I'm quoting from #2055851-3: Remove translation of exception messages:

"Additional uncaught exception while handling exception." Calling a whole huge codepath in our exception is liable to obscure the real problem in many situations.

fietserwin’s picture

String::format() does not translate, it more acts as sprintf(), and @tim.plunkett just gave a +1 on the issue you mention. so we all agree to not translate exception messages and we don't.

claudiu.cristea’s picture

I know what String::format() is doing. The idea is to avoid calls as much as we can when throwing exceptions.

EDIT:

  • Note that it's not only the method call but also all the autoloading stuff around.
  • Also, I see that you used the "!" placeholders. If we are still using ::format() we have to use at least "@" prefixed placeholders.
mondrake’s picture

I went through the patch in #260, and I could only find a doc nitpick below.

With regards to usage of String::format - if I look at the code in the String class it seems to me that further errors happening within that class during an exception handling are unlikely.

#260 addresses concerns both from @asplicious re. operation arguments handling and from @tim.plunkett re. structure of the operations plugins, plus removes all $image argument from the calls within Image<->ImageToolkit<->ImageToolkitOperation.

There is consensus between the three people that participated in developing this patch to keep the operation arguments as an associative array, see #249.

I think this is ready to go but can't RTBC - @asplicious @tim.plunkett any taker?

+++ b/core/lib/Drupal/Core/ImageToolkit/Annotation/ImageToolkitOperation.php
@@ -0,0 +1,76 @@
+   * of the 'operation' definition keys. The 'toolkit' definition key is
+   * derived from the class namespace, while the 'operation' key is annotated.
The 'toolkit' definition key is derived from the class namespace, while the 'operation' key is annotated.

This is no longer true and should be removed.

fietserwin’s picture

#263: I am not so afraid that String is not already loaded or will fail to load when we arrive at this point in code. However, normally it will be during image derivative generation, so it will be hard to catch and display the exception on a admin faced screen. So the message will normally arrive in the configured log sinks of which we cannot assume that they will display html correctly. Therefore, I decided to not use the %-placeholder that is normally used to display object names and id's. We can use the @-placeholder, although for now they are not user entered values.

Attached patch solves this in a more consistent way: all exceptions and log messages now use '@...'.

#264: done.

[EDIT: the interdiff should of course have extension .txt, so please ignore test failures on that file]

Status: Needs review » Needs work

The last submitted patch, 265: interdiff-265.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review

Well, I'm keep my point of view but, anyway, this is only a point of view.

fietserwin’s picture

FileSize
100.34 KB

Let's get rid of that failing interdiff.patch by just reposting the patch here. Read the comment at and see the interdiff from #265.

fietserwin’s picture

Status: Needs review » Reviewed & tested by the community

Considering that:
- nobody besides @claudiu.cristea, @mondrake and myself (the 3 people that actively worked on the development side of this patch) have posted any remarks since #254 and #265 (replaced by #269).
- @claudiu.cristea, @mondrake and myself do consider this RTBC (even if we would have done details differently).
- the patch has been extensively reviewed in #209, #210 and #211 and that all points from that review are resolved by mondrake (up to #227) and myself (as of #227), leading to RTBC in #244
- @tim.plunkett did an in-depth review in #245, of which all points raised were
* solved by myself (#246) and @mondrake (#247)
* reviewed by @mondrake and @claudiu.cristea
* answered
* or were discussed leading to reaching consensus by the 3 active developers
- the resulting patch was rerolled, due to #2257587: Remove $image parameter in calls between and within Image and toolkit leading to more reviews by @claudiu.cristea and resulting in the current patch
- the current patch has gotten
* green light from @mondrake
* green light from the testbot
* perhaps no explicit green light, but also no red light from @claudiu.cristea

I dare say that this is RTBC again.

And no, I'm not RTBC'ing my own work. ALL my work has been reviewed by others, all work from others has been reviewed by me and others.

I hope we can get this patch in sooner then later, as many other issues are awaiting to be rerolled or even are not started yet because of the changes that this patch brings, let alone that it will unblock contrib.

mondrake’s picture

mondrake’s picture

neclimdul’s picture

I still agree with Tim about disagree with removing the core methods. I see contrib as an edge case and their removal is just unneeded complexity and DX regressions. Write good tests and you'll get all the benefits of "eat your own dogfood", we can support contrib, and we don't have to loose the DX.

I'll leave it to a core committer to make the final call here because its clear we aren't going to change each others minds but I wanted to make my opinion clear one last time.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

@claudiu.cristea String::format () is not translating and is consistently used in our exception messages - it has 0 dependencies other than PHP

That said, if we are still strong on keeping specific scale(), resize(), ... methods on ImageInterface and this is the only way to move forward, then I think we could just re-implement them, kind of reverting #2103635: Remove effects from ImageInterface. These methods would just prepare the method's arguments in an array format and return $this->apply('operation', $arguments).

Let's do that. I agree with neclimdul

Write good tests and you'll get all the benefits of "eat your own dogfood", we can support contrib, and we don't have to loose the DX.

alexpott’s picture

Also

+++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitOperationBase.php
@@ -0,0 +1,178 @@
+          throw new \InvalidArgumentException(String::format("Argument '@argument' expected by plugin '@plugin' but not passed.", array('@argument' => $id, '@plugin' => $this->getPluginId())));
...
+          throw new InvalidPluginDefinitionException(String::format("Default for argument '@argument' expected by plugin '@plugin' but not defined.", array('@argument' => $id, '@plugin' => $this->getPluginId())));

+++ b/core/modules/system/src/Plugin/ImageToolkit/Operation/gd/Crop.php
@@ -0,0 +1,95 @@
+      throw new \InvalidArgumentException("At least one dimension ('width' or 'height') must be provided to the image 'crop' operation.");
...
+      throw new \InvalidArgumentException(String::format("Invalid width (@value) specified for the image 'crop' operation.", array('@value' => $arguments['width'])));
...
+      throw new \InvalidArgumentException(String::format("Invalid height (@value) specified for the image 'crop' operation.", array('@value' => $arguments['height'])));

Exception messages should not contain a fullstop since they are part of a longer message produced by PHP's exception handler. And there are probably more instances of this than those I've highlighted....

alexpott’s picture

  /**
   * Get an image with a mocked toolkit, for testing.
   *
   * @param array $stubs
   *   (optional) Array containing toolkit methods to be replaced with stubs.
   *
   * @return \Drupal\Core\Image\Image
   *   An image object.
   */
  protected function getTestImage(array $stubs = array()) {

$stub is never user.

fietserwin’s picture

Assigned: Unassigned » fietserwin
Status: Needs work » Needs review
FileSize
104.14 KB
21.86 KB

#274: done, I added convenience methods to Image (and interface) for the core provided operations. However, for some effects it is easier to just forward the configuration array, so they are not used for all effects

#275: done (in all image system related code).

#276: done (stubs is now forwarded to getToolkitMock()).

@claudiu: A side note on exception handling: when, as here, exceptions are used for application error handling and not system errors, I think you can do what you want when handling them, so no need to worry about unavailable services or exceptions in exceptions. We are not handling an out of memory or disk full error.

fietserwin’s picture

FileSize
92.39 KB
21.97 KB

Discussing the latest patch with @mondrake, @mondrake suggested that at the Image level nothing actually changes, so all calls from outside the image system to Image should be restored to their current state. this leads to the following changes compared to #271 (not #277!)

  • (#274) Add methods scale(), resize(), etc back to Image(Interface). These are convenience methods that internally call apply().
  • Earlier changes to file,image effects and validator test are now restored and are no longer part of the patch. So, for the image effects we now need a follow-up issue for the watchdog => logger and the full stop on exception messages changes.
  • ImageEffect tests are also restored. this needed some hacks to log calls and the check on expected and unexpected calls.
  • (#275) Exception messages within the image system no longer have a full stop at their end.
  • (#276) $stubs is now passed on to gettoolkitMock().
  • In core\modules\system\src\Plugin\ImageToolkit\Operation\gd\Scale.php\Scale.php an empty line between the namespace line and the use line was added.

[EDIT: That the files outside the Image system are no longer part of the patch (image effects, file.module, ...) is not reflected in the interdiff!]

mondrake’s picture

Status: Needs review » Needs work

Thanks @fietserwin. A few more small things

1. To keep alignment with original method signatures in ImageInterface before #2103635: Remove effects from ImageInterface,

+++ b/core/lib/Drupal/Core/Image/Image.php
@@ -157,48 +157,59 @@ public function save($destination = NULL) {
+  public function scale($width, $height = NULL, $upscale = FALSE) {

should be

+  public function scale($width = NULL, $height = NULL, $upscale = FALSE) {

and in the interface too; also

+++ b/core/lib/Drupal/Core/Image/Image.php
@@ -157,48 +157,59 @@ public function save($destination = NULL) {
+  public function crop($x, $y, $width, $height = NULL) {

should be

+  public function crop($x, $y, $width, $height) {

2. (minor) Empty line between the namespace line and the use line needs to be added to Crop, Resize and ScaleAndCrop too.

3.

+++ b/core/modules/system/src/Tests/Image/ToolkitGdTest.php
@@ -250,7 +250,7 @@ function testManipulations() {
-        call_user_func_array(array($image, $values['function']), $values['arguments']);
+        $image->apply($values['function'], $values['arguments']);

We are testing image manipulations with core operations here, so I think we should revert this now, as we said to use the direct Image methods instead of apply. This also means reverting the changes in the $operations array above.

claudiu.cristea’s picture

@alexpott, @neclimdul,

That said, if we are still strong on keeping specific scale(), resize(), ... methods on ImageInterface and this is the only way to move forward, then I think we could just re-implement them, kind of reverting #2103635: Remove effects from ImageInterface. These methods would just prepare the method's arguments in an array format and return $this->apply('operation', $arguments).

Well, I totally disagree with those privileged operations. Everywhere in Drupal core we are abstracting, and normalising the API. We are moving core implementations to swappable, pluggable, overridable implementations. Plugins, components, objects, entities added by contrib are more and more equal as treatment with those provided by core. We are moving in D8 from first class citizens to equal citizens. And this is the spirit of a clean API, this is Drupal.

Did I said everywhere? No. Everywhere except the Image System. There we still want to keep the old Drupal flavor.

That being said (for the very last time) I need to add that I can live with those methods if this is the "political compromise" we need to make to move forward in a chain of issues. The whole image system work is stuck for 1 year in this critical architectural change. But why adding those methods to interface? I can imagine an edge case where I want to extend the Image object and I don't need those methods at all. I still think that they are a fancy thing. OK, let's add them in Image but not in the interface. So we can say that we satisfied the need of keeping the DX fancy but we are still accurate as "equal citizens API".

Just a point of reflection.

mondrake’s picture

Issue summary: View changes

Updated issue summary.

mondrake’s picture

Re. #278

So, for the image effects we now need a follow-up issue for the watchdog => logger

there is already an issue for that #2272481: Remove usages of watchdog() from forms, plugins and controllers.

fietserwin’s picture

#279.1 (scale): I deliberately changed that. Yes, we can pass NULL for width, no we cannot pass no parameter at all as we must pass in at least width or height, and because we are back to ordered parameters, we must pass in at least the 1st parameter.

#279.1 (crop): I was actually a bit flabbergasted by the fact that width and height are not required for crop. If only 1 of them is passed in, the crop calculates the other based on aspect ratio. So we may leave out height.

#279.2: Done.

#279.3: I am a bit hesitant about this one. A toolkit test should test the toolkit. That it is using Image for that is already dubious (but understandable due to the strong coupling of these 2). So this suggests to use the apply chain. Using apply(), also allows us to test not passing in optional parameters, which is impossible with the convenience methods (they always pass all parameters, albeit with their default value). So I prefer to keep these tests as they are. Can you agree with that?

#280: I agree with you, but removing them from the interface also removes any DX advantages.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community
Related issues: +#2272481: Remove usages of watchdog() from forms, plugins and controllers

Thanks.

because we are back to ordered parameters, we must pass in at least the 1st parameter

Makes sense, also for crop.

Using apply(), also allows us to test not passing in optional parameters, which is impossible with the convenience methods

Makes sense, too.

Setting this to RTBC, I also did some manual testing with effects and were OK. I have no (more) arguments as to point raised in #280, so leaving that to @neclimdul and @alexpott for any comment.

claudiu.cristea’s picture

@fietserwin,

#280: I agree with you, but removing them from the interface also removes any DX advantages.

Can you give an example? We are not losing the docs because those will be moved in the class (the methods not being documented in the interface, it's mandatory to document them in the implementation). What else?

mondrake’s picture

Updated the draft change record Image operations are plugins to reflect the latest developments.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Awesome work mondrake, fietserwin, claudiu.cristea - I know this has taken ages - thanks for all the hard work.

Committed 697991f and pushed to 8.x. Thanks!

+++ b/core/modules/system/tests/modules/image_test/src/Plugin/ImageToolkit/TestToolkit.php
--- a/core/tests/Drupal/Tests/Core/Image/ImageTest.php
+++ b/core/tests/Drupal/Tests/Core/Image/ImageTest.php

Can we get a followup to add the proper @covers annotations to this PHPUnit test

Edit: fixed commit hash

fietserwin’s picture

Thanks for picking this up quickly after it reached RTBC. I added a comment to #2109459: Review image test suite to add the @covers annotations.

fietserwin’s picture

The link to commit 48419e1 gives me a "Bad object id: 48419e1" and when I locally pull, I don't see this commit either???

tim.plunkett’s picture

Status: Fixed » Reviewed & tested by the community

This was not pushed.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
git push origin 8.x
fatal: remote error: Permission denied when accessing 'drupal' as user 'alexpott'

Well it was... but d.o

  • alexpott committed 697991f on 8.x
    Issue #2073759 by mondrake, fietserwin, claudiu.cristea: Convert toolkit...
cosmicdreams’s picture

Confirmed pushed. Saw it when I updated today.

fietserwin’s picture

Assigned: fietserwin » Unassigned

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.