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

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

Problem/Motivation

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

This issue is a spin off of issue #2073759: Convert toolkit operations to plugins, more specifically comment #2073759-71: Convert toolkit operations to plugins.

Having image operations (crop, resize, rotate, etc) in ImageInterface makes very hard for contrib modules to add new operations. We want Image to call specific operations from the toolkit rather than define its own in interface.

The current design is confusing:

  • Some image effects are directly supported by Image (and ImageInterface) itself, while other effects (from contrib) have their full implementation in an ImageEffect class.
  • Would a contrib module decide to follow this pattern and add his own effect code to an own Image extended class, we will run into problems as toolkits also need to derive from Image to add their own stuff.

Proposed resolution

Remove the methods from ImageInterface as first step of #2105863: [meta] Images, toolkits and operations.

Remaining tasks

User interface changes

None.

API changes

The image operations are removed. But calling $image->scale() is still supported by implementing a temporary __call() just for BC.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Priority: Major » Normal

I don't see this as major. I also don't necessarily know why this is needed, since we had dedicated functions like image_rotate() in D7, and contrib still had to work around it.

claudiu.cristea’s picture

@tim.plunkett, thinking more on this I can see the reason. In fact there are 3 levels here:

Image (the abstract layer) > Toolkit (plugin) > Toolkit effect/operation (plugin)

If we want $image->desaturate() we can still have it by implementing the magic of __call(). And there $image->toolkit will check if desaturate is implemented as a toolkit effect/operation (see #2073759: Convert toolkit operations to plugins) plugin and will pass the process to that plugin or return an exception. This is clean.

Having all that logic in D7 is not a good reason to not cleanup this. Also I agree with #2103621: Move GD logic from ImageInterface to toolkit, tie toolkit instance to Image instance, toolkits should no longer be instantiated separately. Image is "GD designed". That was inherited from D7 too, but it's wrong.

I would merge this and #2103621: Move GD logic from ImageInterface to toolkit, tie toolkit instance to Image instance, toolkits should no longer be instantiated separately to provide a unique view on this rearchitecture.

fietserwin’s picture

Added related issues in the issue summary. The work that was done in the "parent issues" was great work. I couldn't have done it as I lack the knowledge of the underlying systems (especially plugins and annotations). And of course, getting it done and working was most easily done by following D7 grouping. That kept the patch small and reviewable.

But now that the code (mainly due to your efforts) has landed and I'm having a look at it as image effect developer, I think that providing the structure and providing image effects should be separated. Thus there is a "kernel" part in core that provides the structure (image base and interface, toolkit base and interface, toolkit operation interface, and various managers and factories) and there is a part in core that provides some basic effects (IMO: image module). And as I think that core should "eat its own dog food", I think it is better if core follows the same guidelines as we (= contrib image effect/toolkit developers) in contrib have to follow when we start developing for D8. This will also give us good examples that can serve as starting point to implement our own image effects and toolkits.

mondrake’s picture

#3: +1 :))

claudiu.cristea’s picture

Looked a little bit in the code. I still cannot figure out where some pseudo-effects like scaleAndCrop() and scale() should go. They are more at logical/abstract layer rather than being "toolkit based" effects. I feel that they are belonging more to Image than toolkit because they are preparing arguments, in a toolkit-agnostic way, to be passed to real effects. There is no problem to leave them in Image but what if some contrib module wants to create other pseudo-effects? There's no other mechanism except extending Image and that we want to avoid as much as we can.

mondrake’s picture

Re #5,

There's also an Image class in Drupal\Component\Utility (yes, another one!).

To my understanding, this may contain all algebra related to images that is independent from Drupal functions.

Maybe once #2073759: Convert toolkit operations to plugins is in, scaleAndCrop() and scale()could be split like:

  1. two operation plugins implementing 'scale' and 'scale_and_crop' operations, calling
  2. the 'resize' and 'crop' operations plugins, using
  3. algebra for scaling and cropping coded in the Image component class. Since it's a component, this may stay stand alone I believe. Other modules willing to introduce additional algebra can do in separate classes, or extending the component class (for instance, I have a class to manage box rotation algebra in Textimage). But effects/toolkits will 'use' the class directly. I do not see here need for another plugin system.

Thoughts?

claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea
Status: Active » Needs work

Let's try with a patch.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
16.8 KB

Patch

tim.plunkett’s picture

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageEffectsTest.php
@@ -67,12 +67,12 @@ function testScaleEffect() {
-    $this->assertToolkitOperationsCalled(array('resize'));
+    $this->assertToolkitOperationsCalled(array('scale'));

Why the changes to the test? Ideally you should be able to make any changes to the code without changing tests like this.

claudiu.cristea’s picture

@tim.plunkett: True but in this particular patch there's no more toolkit 'resize' on scale. There's a 'scale' toolkit effect and that should be expected.

Status: Needs review » Needs work

The last submitted patch, effects-2103635-8.patch, failed testing.

claudiu.cristea’s picture

Title: Remove image effect code from Image and ImageInterface » Remove effects from ImageInterface

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

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
14.44 KB
28.66 KB

Here's new patch trying to fix the phpunit tests (nor definitive). I found that some phpunit are losing their scope. I commented out:

  • testCropWidth(), testCropHeight(): I see no way to test the width/height computation using mocks/stubs in the new design.
  • testResize(): I see no reasons why width/height should be converted from float to integer. GD functions are always doing that. Loosing that preparation step we're loosing also the reason of this test.

I would remove those tests but I would like to hear @tim.plunkett's opinion.

fietserwin’s picture

Status: Needs review » Needs work
 use Drupal\Core\Annotation\Translation;
 use Drupal\system\Plugin\ImageToolkitInterface;

Can we remove these unnecessary annotation use statements via this patch as well? This is for all ImageEffects (I think that in Test they are already no longer there)

Needs a reroll?

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
12.42 KB
25.51 KB

Notes:

claudiu.cristea’s picture

FileSize
1.47 KB
25.9 KB

Based on @mondrake suggestions, I improved docs on __call() magic method.

fietserwin’s picture

Why the __call method. Soon these methods will be removed from Toolkit as well. So I propose that we go for directly calling the operation on toolkit: $image->getToolkit()->scale(). Therefore #2108339: Image toolkit getter needs to land first. Let's focus on that one. and postpone this one.

claudiu.cristea’s picture

No, $image->scale() should still work. It's a DX "bonus" and I wouldn't drop that. There's nothing wrong with __call() what you fear about? The entity system is using also __get() to magically refer fields in a DX friendly way. See https://drupal.org/node/1532946.

This priority "0" right now because makes possible to write a smaller patch in #2073759: Convert toolkit operations to plugins.

On #2108339: Image toolkit getter I would like to hear first @tim.plunkett's opinion.

fietserwin’s picture

Hidden, automatic methods and properties are not always DX friendly: It is hard to find out what happens, hard to track when coding (e.g. find usages) and IDE's get confused as well, highlighting errors that will work after all.

But OK, then go for an explicit apply() as we are going to do in the toolkit: keep it consistent.

mondrake’s picture

I have no strong opinion on usage of __call.

If we go for an explicit method then we'll have to put it in the interface, and we'll not be able to pass a variable number of arguments, but rather an array. Which is not bad anyway, as we'll do that later in the toolkit calls to the operations' apply method. So this would force having an associative array upfront, already in the toolkit call at Image level, just to be passed on from toolkit to operation.

Maybe more reviews and input here can help.

fietserwin’s picture

Status: Needs review » Needs work

It is still possible to pass a variable number of arguments and pack them, but go for consistency: pass a keyed array everywhere:
- ImageEffect::applyEffect()
- Image::apply()
- ImageToolkit::apply()
- ImageToolkitOperation;;apply()

Note: thus we should rename ImageEffect::applyEffect() to ImageEffect::apply() (which is saner OO anyway, and DRY). Not sure in which issue to do so.

claudiu.cristea’s picture

The keyed array is the single strong reason. Indeed.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community

Well, spent some hours trying to implement apply() instead of __call(). Trying to change arguments to be passed as associative arrays is a huge work. It needs changing of ImageToolkitInterface with implementations and tests. And what for? Only to have a temporary GDToolkit::crop() & Co. that will be removed in few days, in #2073759: Convert toolkit operations to plugins?

So, I would implement apply() and arguments as keyed arrays there, because I already have to touch those files then. Let's keep this in this way as a temporary step.

mondrake’s picture

Status: Reviewed & tested by the community » Needs work

So if we reached consensus on not using __call in the destination solution, would you mind just adding that this is a temporary fix in the comment docblock?

Then I think we can RTBC this and move on to the core issue, being #2073759: Convert toolkit operations to plugins.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
1.39 KB
26.21 KB

Voilà!

tim.plunkett’s picture

So far in D8 we only use __call() on decorators.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Ok, so this is a preliminary step towards the big picture of #2105863: [meta] Images, toolkits and operations, namely to allow contrib to operate with the image system adding additional toolkit operations, deferring the actual image manipulation to the toolkit system, and making the Image class operation and toolkit agnostic.

This patch just moves operation-related functions from the Image class to the toolkit class.

As a temporary measure, a __call magic method is implemented to allow executing legacy $image->crop(...) (and its peers) methods. This will be removed in #2073759: Convert toolkit operations to plugins when a generic 'apply' method will be implemented across the Image->ImageToolkit->ImageToolkitOperation chain of calls.

RTBC

fietserwin’s picture

Priority: Normal » Critical

Other criticals are waiting on this.

fietserwin’s picture

Issue summary: View changes

added related issues

claudiu.cristea’s picture

Issue summary: View changes

Clean issue summary.

Xano’s picture

#25: effects-2103635-25.patch queued for re-testing.

dawehner’s picture

I like the idea to move the crop/scale whatever functionality to the ImageToolkit as this is the place where the actual magic happens.

In additional #2103635-27: Remove effects from ImageInterface pretty much describes everything I have to say about this patch.

catch’s picture

I'm trying to figure out exactly what the patch chain here is - it looks like #2073759: Convert toolkit operations to plugins might be split into a couple of issues?

Overall given this is blocking contrib module conversions and it's the contrib module authors working on these patches I think it's fine to change the API here, but I'm not sure what the scope of the necessary work is at the moment to get it to the point where the API is no longer in flux.

fietserwin’s picture

#31

I'm trying to figure out exactly what the patch chain here is - it looks like #2073759: Convert toolkit operations to plugins might be split into a couple of issues?

Correct. Ongoing discussions about details on the provided patches in #2073759: Convert toolkit operations to plugins made us split off parts that are relatively standalone and do provide added value on its own. This keeps the paches relatively clean and easily to review while large rerolls are prevented by chaining them and reducing the number of files touched.

(Numbers from the meta issue #2105863: [meta] Images, toolkits and operations, these are my opinions)
1.3 #2073759: Convert toolkit operations to plugins is blocking contrib development (in the sense that we want a clean solution that gives toolkit specific code from image effects a logical place. Theoretically it would be possible to hack around, but that won't allow to swap in and out specific parts, only all or nothing.
1.4: nice to have.
2,7,8,9: Will change the API, but do it now, not in D9, that will be more work for us at that moment, than it will be now.
3,4: not a necessity
5: needs to be given some thought, but it is true that contrib needs some place to add its requirements.
6: blocker: currently image effects cannot access the toolkit, e.g. to change the image format or quality parameter.
10,11: no API changes foreseen, though 11 will probably reveal DX problems in the API (inconsistencies and so).

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I'm reluctant to commit this without #2108307: Provide an abstract layer for image operations geometry - I think it's a step backwards to push the dimension calculation to all toolkits - can't we just solve this here as we're pushing the effects to the toolkit?

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: +beta blocker

@alexpott,

[...] can't we just solve this here as we're pushing the effects to the toolkit?

No, we can't while we don't have an unified interface to effects. And that one is achieved later in the issues chain. I mean, we can but that means a single patch for most of #2105863: [meta] Images, toolkits and operations. Or do you want to review a 200KB patch? This a first step split from the big issue only for patch review-ability reason.

So what's wrong having #2108307: Provide an abstract layer for image operations geometry as followup? We are pushing those geometry calculations only temporary there, to move things forward. Right now there's no other toolkit than GD, nowhere. Why? Because everybody is waiting for this architectural change (most issues of #2105863: [meta] Images, toolkits and operations). I'm working here together with @fietserwin and @mondrake. They are maintainers of the most important image contrib modules: ImageCache Actions and Textimage. Imagemagick conversion is stuck too in a sandbox at https://drupal.org/sandbox/mondrake/2110973.

We cannot release the Beta of D8 without letting modules to add contrib toolkit image operations. Tagging.

fietserwin’s picture

Though most of the arithmetic may be generic in itself, this doesn't mean that it can or should be used by all image toolkits. Bug #1554074: image scale does not work when current dimensions are unknown illustrates this. So moving the calculations to GDToolkit is not as bad as it seems, It would make the mentioned issue disappear.

But I agree with @alexpott that "really" generic computations should not be moved to the toolkits. But as the above mentioned issue shows, it is difficult to predict what computations are really generic for all toolkits and which aren't.

Note that, instead, this patch could also have moved the code from Image to the various ImageEffect classes and the ImageEffect classes could call their toolkit counterpart directly. But #2108339: Image toolkit getter is preventing that.

So, to get progress on the issues of meta issue #2105863: [meta] Images, toolkits and operations,

1) Core maintainers accept this patch as is. @catch and @alexpott have expressed their doubts. If our arguments have still not convinced you, we can continue according to plan 2:

2) We change the order of the issues beneath meta issue #2105863: [meta] Images, toolkits and operations:
-1- #2108339: Image toolkit getter
-2- This issue, but solving it as suggested above (move code from Image;;{operation}() to ImageEffect::applyEffect() (instead of to GDToolkit::{operation}()) and the ImageEffect::applyEffect() methods call their toolkit counterparts directly).
-3- Continue as in the meta issue.

@alexpott, @catch: what do you think?

claudiu.cristea’s picture

Just to give an overview on how #2108307-2: Provide an abstract layer for image operations geometry may look, I anticipated there with a proof-of-concept (not definitive, open to suggestions).

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

webchick’s picture

Assigned: claudiu.cristea » alexpott
Issue summary: View changes

I'm not sure if this should be assigned to catch or alexpott, but going with alexpott since he was the last one to say something in here. :)

Xano’s picture

25: effects-2103635-25.patch queued for re-testing.

mondrake’s picture

Dries’s picture

I reviewed the patch and it looks good. However, I don't understand why this is 'critical', or why the parent #2105863: [meta] Images, toolkits and operations is critical for that matter. It doesn't look like a regression compared to 7.x. Could be scope creep. A better explanation of why this is critical would help.

claudiu.cristea’s picture

@Dries,

We chose to split a big issue in more patches only for "reviewability" reason. This is only the first one (see the issue summary).

Without pushing this (and the rest of the chain) in, contrib modules are not able to declare/define new image operations (like desaturate, invert, etc) in a sane way. As is now, to add a custom image operation, a module should extend GD toolkit. But other module may want to add other operation and that needs to extend too the class. But which one? That's why we need to add new image operations not by extending but by plugging in. It's critical also because all other issues are stuck in this one.

I agree that maybe keeping the parent as critical should be enough.

xjm’s picture

Assigned: alexpott » catch
Issue tags: -beta blocker

@alexpott has lower availability at the moment so I've asked @catch if he can look at this.

Also, please only add the beta blocker tag in consultation with a core maintainer. Does this represent a change to the data model? In general, the only API changes tied to the beta are likely going to be critical changes to the Entity Field API, menus and routing, the configuration system, and the plugin API itself.

I'm on the fence about even the parent being critical, but I guess there's a case to be made if it's blocking contrib. Not sure though.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Discussed this with the other core maintainers and agreed to proceed with this patch - thanks for all the work @fietserwin and @claudiu.cristea - it's great that module maintainers are making core better :) and better before Drupal 8's beta :)

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.php
@@ -152,6 +166,40 @@ public function desaturate(ImageInterface $image) {
+  public function scaleAndCrop(ImageInterface $image, $width, $height) {
+    // @todo Dimensions computation will be moved into a dedicated functionality
+    //   in https://drupal.org/node/2108307.
+    $scale = max($width / $image->getWidth(), $height / $image->getHeight());
+    $x = ($image->getWidth() * $scale - $width) / 2;
+    $y = ($image->getHeight() * $scale - $height) / 2;
+
+    if ($this->resize($image, $image->getWidth() * $scale, $image->getHeight() * $scale)) {
+      return $this->crop($image, $x, $y, $width, $height);
+    }
+  }

This should be returning FALSE if $this->resize returns FALSE.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
26.23 KB
631 bytes

@alexpott, Thank you for addressing this chain of issues.

Indeed, we need a return FALSE there.

claudiu.cristea’s picture

44: effects-2103635-44.patch queued for re-testing.

claudiu.cristea’s picture

So, what's going on here?

catch’s picture

Status: Needs review » Reviewed & tested by the community

HEAD was broken. But should be fixed now. Back to RTBC pending the bot.

claudiu.cristea’s picture

44: effects-2103635-44.patch queued for re-testing.

claudiu.cristea’s picture

The latest test started ran before fixing the HEAD.

catch’s picture

Title: Remove effects from ImageInterface » Changte notice: Remove effects from ImageInterface
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed/pushed to 8.x, thanks!

Looking forward to the other issues being green.

claudiu.cristea’s picture

Title: Changte notice: Remove effects from ImageInterface » Remove effects from ImageInterface
Issue tags: -Needs change record

@catch,

I think the change notice should describe the whole architectural change. What to describe exactly for this issue? $image->scale() is still $image->scale() after this commit. I think we should raise the change notice when $image->scale() will be $image->apply('scale', ...). I think that would be relevant as change notice.

claudiu.cristea’s picture

Assigned: catch » Unassigned
Priority: Major » Normal
Status: Active » Fixed

Added 'Needs change notification' tag to the end of this conversion chain, in #2073759-138: Convert toolkit operations to plugins so that we can fix this.

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -Needs change record