Updated: Comment #34

This issue will not provide a patch. It's only an architectural meta issue.

Terms/definitions

  • effect: An ImageEffect plugin belonging to an ImageStyle.
  • operation: An ImageToolkitOperation plugin belonging to an ImageToolkit.

Problem/Motivation

Moving the image system to Drupal 8 was a huge undertaking. Most of image components were converted to classes and plugins. Now the whole system is more flexible, extensible and modern. However, while this part of core is starting to be "in shape" and contributors are trying to port their image modules (most of them modules that are providing new effects) to D8, we've found that some parts of the image system still lacks in terms of API consistency, extensibility, flexibility and DX.

Problems:

  1. Developers are not able to add custom operations to an image toolkit. Based on the current design they can only extend a toolkit and implement new operations. For example if they want to add new operations to GD toolkit, they need to extend GD toolkit. Other modules may want to add other operations to the same GD toolkit and they need to extend too GD (but which one?). In Drupal 7 this was achieved also in a non-canonical way: contrib modules were declaring functions like image_{toolkit}_{effect}() but code was able to call image_toolkit_invoke() and get rid of what toolkit was active.

    Fixed in #2073759: Convert toolkit operations to plugins
  2. In the current design, \Drupal\Core\Image\ImageInterface defines some basic image manipulation methods (resize, scale, crop, scaleAndCrop, desaturate), that hand over the processing to the underlying image toolkit. This makes possible to process effects directly from the image object: $image->scale(100, 200);. However, contrib cannot use the same logic without extending a new class from \Drupal\Core\Image\Image and adding a new e.g. ::blur() method. Developers need a way to hook into image system rather than extend it for any new effect/operation.
    Fixed in #2073759: Convert toolkit operations to plugins
  3. Also, the image toolkit associated to the Image object is not accessible from contrib effects, making it even more complex to access image manipulation functions.
    Fixed in #2103621: Move GD logic from ImageInterface to toolkit, tie toolkit instance to Image instance, toolkits should no longer be instantiated separately
  4. Right now most of the \Drupal\Core\Image\ImageInterface effects have a corresponding operation in toolkit, making difficult for developers to understand what belongs to \Drupal\Core\Image\Image abstract layer and what to the toolkit layer.
    Fixed in #2103635: Remove effects from ImageInterface
  5. There are parts of \Drupal\Core\Image\ImageInterface that have GD toolkit logic like ImageInterface::setResource(). The image resource is something defined and handled by PHP GD extension and should not defined in the image model. ImageMagick, for example, has no resource. We need to cleanup the API and let each toolkit define its tools in the right place.
    Fixed in #2103621: Move GD logic from ImageInterface to toolkit, tie toolkit instance to Image instance, toolkits should no longer be instantiated separately
  6. What about effect collision? How we'll handle the case when a contrib wants to add his own version of GD crop? I know, I know, it's developer responsibility but anyway we need to provide a rule for plugin selection.
  7. \Drupal\Core\Image\Image layer effects still contain some logic that is toolkit-agnostic. This refers mainly to normalizing dimensions, coordinates, etc. Example: \Drupal\Core\Image\Image::scale() effect is computing the height when only width is provided. Partially those tasks are deferred to an utility component: \Drupal\Component\Utility\Image but most of this algebra is still performed in the image layer.
  8. Image requirements (image_requirements()) are GD specific and hardcoded in image.install. A Drupal installation that uses ImageMagick as default toolkit will still get requirements for GD.
    Fixed in #1069140: Requirements should be provided by image toolkit

Proposed resolution

  • Clean up and sanitize ImageInterface and ImageToolkitInterface.
  • [fixed] Contrib modules will be able to implement new toolkits as plugins.
  • Contrib modules will be able to add new operations to toolkits. Collisions will be solved in UI but set also by code.
  • Core and contributed operations will be accessible from image object with a unified interface: $image->apply('scale', array('width' => 100, 'height'=> '200'));.
  • Move dimension and coordinates computing into utility component \Drupal\Component\Utility\Image and free toolkits from performing such tasks.
  • [fixed] Toolkit plugins will provide system requirements to be passed to image system.
  • Image toolkit operations may provide requirements too.
Open Issues in order of addressing them.
Consequences Issue
influences contrib #2357119: Allow image toolkit derivatives
influences contrib #2359443: Allow creating image derivatives from an Image object
blocking contrib #2168511: Allow to pass save options to ImageInterface::save
blocking contrib #1826362: ImageEffects of the same image style should be able to pass variables between them
influences toolkits in contrib #2165399: Add basic methods to ImageTookitBase
influences toolkits in contrib and some core parts #2110835: Move the GD toolkit on its own module
improves consistency, influences contrib #2109343: Unify effect/operation verb naming in image style, image, toolkit, toolkit operation
influences contrib #2106903: Image toolkit operations may provide requirements
architecture #2336811: Remove Image, we only need ImageToolkit
#2110591: API/UI for toolkit operations plugin selection
#2109459: Review image test suite
#2109465: Review docs in image system & image module code

Notes:

  • blocking contrib = difficult/impossible to start porting toolkits and/or effects to D8.
  • influences contrib = will require changes in contrib effects and/or toolkits that are already ported.

Remaining tasks

  • Agree on this architecture switch.
  • Fix child issues.

User interface changes

Will be defined by each child issue.

API changes

Will be defined by each child issue.

Comments

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

Priority: Normal » Major

"major", isn't so?

mondrake’s picture

@claudiu.cristea: excellent summary!

claudiu.cristea’s picture

@mondrake, Thank you!

What we need now is to take a breath and focus on this document. Agree or disagree, amend, modify, whatever. Let's pause a little bit the child issues till we have an agreement on the "big picture". Let the discussions begin!

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

mondrake’s picture

Ok. First proposal: shall we include also ImageStyle and ImageEffect related issues in the scope of this meta?

We have:

  • a core Image system. This partners with ImageToolkit and ImageToolkitOperation for image alteration. But, also links with other core Drupal subsystems like file, field, etc.
  • an Image styling module. This allows admins and authorized users to create and maintain, via an UI, 'styles' as sequences of effects to be applied to an image to alter its appearance before being presented to users. Drupal uses its core Image system (above) as the actual 'engine' under the hood to execute the modifications specified in the style/effects.

As a maintainer of a contrib module that provides both additional image style effects and additional image toolkit operations, I'd be interested at having one single point of reference for the evolution of the entire 'Image ecosystem' towards D8 release.

A few topics I'd like to be included here, related to image styling:
#2084985: Implement ThirdPartySettingsInterface in ImageStyle
#2098247: Provide a hook for ImageStyle::createDerivative
#1826362: ImageEffects of the same image style should be able to pass variables between them

Also, having all this together could also help with terminology: the term 'effect' is currently being used both to indicate an effect of an ImageStyle, or an effect of an ImageToolkit. These are two separate concepts. To avoid confusion, I would suggest we stick here to the following convention;

  1. effect = ImageEffect as an element of an ImageStyle
  2. operation= ImageToolkitOperation as an implementation of a ImageToolkit operation
claudiu.cristea’s picture

Well, really don't know. The first one covers the ConfigEntity in general. Next 2 are related more to ImageStyle. But, yes, I think you can add them to related issues.

fietserwin’s picture

Problems (as in issue summary)

1) Being tackled by #2073759: Convert toolkit operations to plugins.

2) Drupal is a flexible framework. This means it will be impossible to tie all data and operations within 1 class. So keep Image simple and let it only contain data and resources. Operations should stay out of Image. (and I think most of us agree on this). So, adding syntactic sugar to Image, so you can do $image->blur() is thus DX refinement. not really an architectural choice. (But being able to easily call these operations outside an ImageStyle/ImageEffect context might be handy. I'm thinking image field that resizes on upload if maximum dimensions are exceeded.)

3) Solve by removing all effect code from Image. The harder part is to subsequently decide what goes in ImageEffect what in ImageToolkitOperation and what in utility (point 6). OTOH this is not so difficult, An ImageToolkitOperation is quite strictly tied to an ImageEffect. So trying to generalize those is a rather useless exercise.

4) If image is the dataholder this belongs to Image. Meaning that Image will be tied to a toolkit and thus that swapping it by other modules will be very hard to impossible. I can live with that.

5) Don't design for the exception, create a design that feels good and intuitive for the common(ly supported) use cases, and that optionally can also cater for the exceptions. More over, don't diverge from the solutions of other Drupal subsystems. In other words: the solution we are working towards in #2073759: Convert toolkit operations to plugins is good enough to cater for this.

6) Move to ImageEffect. If a more general use is foreseen, it can be moved to the utility as well. But looking at our image effect code in D7 (i.e. module imagecache_actions) i see that most effects indeed do some data massaging before handing it off to the toolkit specific code. But this massaging is often so specific to the effect and the options provided to the user in its UI, that I hardly see any cases for generalization.

7) This is simply a bug, not an architectural issue I think.

Generally speaking, i would say it is good to have a central place for discussion and oversight of different issues, but I'm more a developer that likes small refactorings than 1 Big Design Up Front (BDUF). This happens to coincide with the way Drupal is developed: by (small, user reviewable) patches.

Architecturally my view is: split data and operations, a flexible framework like Drupal needs it in this case. This may seem contrary to OO principles but it is not. Martin Fowler in his "Patterns of Enterprise Application Architecture" mentions this choice as a separate valid pattern: the transaction script (versus the domain model).

mondrake’s picture

#6:

my 2 cents:

1) agree

2) while using __call in Image to have the magic of $image->blur() seems sexy, I believe this could cause documentation/understanding issues: you will not have documentation for an Image::blur method. If we have $image->getToolkit()->process('blur', (...)) this seems to me enough and clear, and documentation of the 'blur' operation will be tied in to the class' process method documentation.

3) agree

4) to me, I would strive to get the 'resource' property of GD and the 'parameters' of Imagemagick managed by the relative ImageToolkit plugin classes. ImageToolkitOperations need to access that, and will do since we inject the toolkit to them. But IMO Image should be really toolkit agnostic. This may require some more refactoring, but I believe it would be the cleanest way.

5) agree, sigh

6) agree, fully

7) as already commented in #1069140: Requirements should be provided by image toolkit, I believe that there's also an opportunity here: we could use the mechanism introduced by #2073759: Convert toolkit operations to plugins to add additional toolkit operations so that modules could provide more requirements. Example, GD core checks PNG and rotate/desaturate support. I can add a FreeType support check by adding a 'requirements.textimage' operation plugin, and avoid having to implement that in the module .install.

I support this meta; on the other hand, I do not see why we cannot also discuss in the detail ones. But the summary here, and (hopefully) its updates would be very useful for anyone joining at later stages.

fietserwin’s picture

#7.4) It still does not feel good to me, if we let a toolkit create an Image it may be of a type tied to that toolkit (thus GDImage, ImagemagickImage). That looks quite natural to me.

MOREOVER: When we move the data, we then need to guarantee that we always will have exactly 1 toolkit instance for 1 Image instance. Without an exact 1-1 relationship that will eventually fail subtly: quality parameter specific for image1.jpg applied when saving image2.jpg.

Alternatively, if we don't want extension by inheritance, we could implement the __get and __set magic methods and store the properties the toolkit needs, as in a stdClass (in fact, you don't need to do so, each object is a stdClass and is thus open by definition to add any property you want).

claudiu.cristea’s picture

@mondrake,

4) to me, I would strive to get the 'resource' property of GD and the 'parameters' of Imagemagick managed by the relative ImageToolkit plugin classes. ImageToolkitOperations need to access that, and will do since we inject the toolkit to them. But IMO Image should be really toolkit agnostic. This may require some more refactoring, but I believe it would be the cleanest way.

...but responding also to #2103621-2: Move GD logic from ImageInterface to toolkit, tie toolkit instance to Image instance, toolkits should no longer be instantiated separately.

What I'm thinking, is that we need to keep in Image a storage for toolkit operations instead of resource with setters and getters in the ImageInterface. Name it ::$toolkitData or ::$toolkitStorage (or?). IMO that should be an associative array. Then image toollkits will implement specific tasks against that storage. E.g. GDToolkit wants to implement its methods set/getResource(). There's no problem to pass the Image itself to those methods, so that they are able to access the storage.

@mondrake,

I do not see why we cannot also discuss in the detail ones

Course, there's no problem to discuss details. I just wanted to say that we shouldn't go ahead with coding without an overview :)

mondrake’s picture

What I'm thinking, is that we need to keep in Image a storage for toolkit operations instead of resource with setters and getters in the ImageInterface. Name it ::$toolkitData or ::$toolkitStorage (or?). IMO that should be an associative array. Then image toollkits will implement specific tasks against that storage. E.g. GDToolkit wants to implement its methods set/getResource(). There's no problem to pass the Image itself to those methods, so that they are able to access the storage.

I think we are thinking more or less the same :)

My point is why to have that in Image, and not in ImageToolkit. When an Image object is instantiated, it builds and store an instance of the default ImageToolkit with it. So we can let the toolkit manage it own storage needs. ::$resource, getResource(), hasResource() and setResource() stay in the GDImageToolkit. An array of ::$parameters stays in the ImagemagickToolkit. ToolkitOperations access those properties via their getters/setters through the ImageToolkit object injected to them during a process call.

I've looked at usage of getResource(), hasResource(), setResource() currently. Apart from tests, and outside of GDImageToolkit, hasResource() is only used by Image, and getResource() by file.module and ImageStyle. AFAICS, these are just loading an image, or checking that an image is loaded. So IMHO we could just have a load() and a isLoaded() method in Image that would call some ImageToolkit level counterparts to respond in an abstract way.

For what other purposes we would let these properties accessed outside of Image?

fietserwin’s picture

My point is why to have that in Image, and not in ImageToolkit.

The relationship: is it thinkable that 1 toolkit instance is attached to 2 Image instances? The answer is yes. So, image instance related storage, even if it is only used by the toolkit belongs to image. i think that the idea of having a ::$toolkitData property (introduced in #9) on Image is a good idea.

I didn't have a detailed look at the use of get/set/hasResource(), but am not surprised that there are hardly any real usages for that outside the GD effect operations that need access to the resource.

And if it is used for a status check then, yes, we should introduce a method to check the status or to check for a specific status (getStatus() === Image::Loaded,or IsLoaded()).

fietserwin’s picture

Practical consideration: I see patches being introduced in the various issues linked to this meta. These patches are all changing more or less the same code. To prevent the need for rerolls, can we prioritize the issues and fix them more or less 1 by 1?

tim.plunkett’s picture

You have a major meta but many major/critical child issues. Please make this critical and demote the rest to normal.

tim.plunkett’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

Issue summary: View changes

Adding new proposal to list.

claudiu.cristea’s picture

Issue summary: View changes

Added priorities, terms, new issues.

claudiu.cristea’s picture

Priority: Major » Critical

Setting priority to "critical" as per #13.

Updated issue summary:

claudiu.cristea’s picture

Issue summary: View changes

Fix.

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

Issue summary: View changes

fix

mondrake’s picture

Updated issue summary to reflect better agreed terminology.

mondrake’s picture

Issue summary: View changes

Aligned summary to terms

claudiu.cristea’s picture

Issue summary: View changes

Fix

mondrake’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

claudiu.cristea’s picture

Issue summary: View changes

New child issues. Changed priority.

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

Added #2108339: Image toolkit getter to issue summary.

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

mondrake’s picture

Since we agreed in #2103635: Remove effects from ImageInterface that we will not use magic __call() method in Image, and rather implement an apply() method across the chain of Image->ImageToolkit->ImageToolkitOperation calls, the point 2. in the issue summary and point 3. in the proposed resolution need to be revised. We will not have $image->scale(100, 200); but rather $image->apply('scale', array('width' => 100, 'height'=> '200'));.

I'll update later if @claudiu.cristea won't beat me on time ;)

mondrake’s picture

Issue summary: View changes

order

mondrake’s picture

Issue summary: View changes

Updated issue summary based on #18

claudiu.cristea’s picture

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

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

mondrake’s picture

Shall we leave the core GDToolkit implementation in the system module? I guess it's there because it can not be in the Image module, as we may use it even if Image module is not installed. So in D7 the system module was a good option - but now with a Core lib, is there a better place?

fietserwin’s picture

Spin off a separate issue for that: lib makes more sense to me, image module as alternative. System never made sense to me. Why not require to enable the image module even if you only want to do so some very basic image handling (thus without image field, image styles, and image effects).

I guess in D7 load order had to do with that decision as well. That is not a problem anymore.

claudiu.cristea’s picture

Well, GD toolkit is an implementation of ImageToolkitInterface and that one is a core interface, right? GD is the most popular toolkit and is installed in almost all cases with PHP. That's why Drupal is shipped with GD. But being shipped with Drupal, GD is still a particular implementation. Where are usually located such plugin implementations? Not in modules? If you agree, then what is shipped with Drupal should stay in core modules, what is contrib in contrib modules. And having GD in a module is a good example for module developers of how to provide their own implementation.

Now the question is: why system module? I have no idea. Looking now in Drupal I see that image.module can be uninstalled (we have no more disable module now). And while it provides basic functionality for images, needed by Drupal they didn't had other option? What about a core required gd.module?

fietserwin’s picture

Separate module is an option but IMO too much overhead. I had a look at /core/lib/Drupal/core and it is not only interfaces, base classes, managers, exceptions and such. You can find actual implementations that - apparently - are deemed to be available at all times;

- Cache backends: database and memory
- Database drivers for mysql, pgsql and sqlite
- PhpMail
- PhpassHashedPassword
- Various stream wrappers
- Lots of Validation Constraints

So moving the GD toolkit to lib is not that strange, it is required for Drupal to work out of the box.

mondrake’s picture

OK. IMHO, a stub module for just implementing a few plugins seems a bit overkill. I'd suggest to leave it where it is. At least we have your rationale in #24. It has be available to Drupal core, and it has to be implemented by a module; it can't be the Image module because Image can be uninstalled. So system module seems still the best option.

mondrake’s picture

Issue summary: View changes

Updated issue summary.

webchick’s picture

Just a reminder that we are post-API freeze. We can still make API changes if they're required to "complete" APIs, or for tremendous DX benefits, but we should not break peoples' code unnecessarily. So please apply a discerning eye towards these clean-ups and which ones will actively create a ton of pain if they're not fixed now vs. ones we could postpone to D9.

rivimey’s picture

I found this issue because I was looking for ways to extend the image style options in D7. I've read what is happening and it looks good... only thing I can see not noted otherwise is please be aware of image colourspaces - sRGB, AdobeRGB, ProPhotoRGB etc.

Can anyone point towards whatever approved way there may be for adding new styles, (e.g. "sepia", or "add frame") in D7 as I haven't yet found anything other than hackery...

fietserwin’s picture

#28: I assume you mean defining new image effects. Image styles can be added via the UI. Defining a new image effect is nothing hackery, have a look at e.g. imagecache_actions.

fietserwin’s picture

#27: Maintainers of image effect contrib modules (@mondrake and I) are active in these issues, so we can start working on porting our modules. Other image effect modules I know of show little to no activity, certainly not regarding D8. So, I guess that nobodies code will break by cleaning up the image handling API.

fietserwin’s picture

Issue summary: View changes

changed prerequisite

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

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

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

mondrake’s picture

mondrake’s picture

Issue summary: View changes

Updated 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

Added "Is blocking" column to issues table.

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

claudiu.cristea’s picture

Issue summary: View changes

Updated issue summary.

mondrake’s picture

Issue summary: View changes
mondrake’s picture

mondrake’s picture

Issue summary: View changes
claudiu.cristea’s picture

mondrake’s picture

Issue summary: View changes
fietserwin’s picture

Issue summary: View changes
fietserwin’s picture

Issue summary: View changes

I reordered the priorities by giving:
- higher (beta blocking) priority to API changing issues like changing names, changing locations, (re)moving methods
- lowering the priority of issues that provide API additions, introduce new features, introduce only UI changes, and mere refactorings.

webchick’s picture

Thanks a lot! That's very helpful to see the roadmap laid out like that.

fietserwin’s picture

Issue summary: View changes

Added new issue #2168511: Allow to pass save options to ImageInterface::save. Being a regression from D7 I gave it a quite high priority.

claudiu.cristea’s picture

Issue summary: View changes

Updated the issue summary.

catch’s picture

Priority: Critical » Major

None of the issues marked 'is blocking beta' should be marked as such - small changes to the image toolkit API will be accepted after beta. If you're concerned they might not be, then tagging as 'beta target' works - those will get reviewed before the beta actually gets tagged most likely.

Also there's no priority #1 items left, here, so unless priority #2 is also critical (in which case the issues should be marked as such on their own merits), I'm downgrading this to major.

mondrake’s picture

@catch, my 2 cents.

Priority #1, #2 and #3 were an artificial split just for the sake of making smaller patches. In fact they combined would deliver a plugin-ified approach to extending toolkits with additional operations. #1 alone being in, makes no difference.

I think before beta we should at least take a decision on #2110499: Unify the call and interfaces of image toolkit operations and #2103621: Move GD logic from ImageInterface to toolkit, tie toolkit instance to Image instance, toolkits should no longer be instantiated separately. These would introduce API changes that would impact developers of image effects and of toolkits. The actual pluginfication of toolkit operations could actually be somehow happening under the hood even later as that would not change interfaces.

Unfortunately, fact is that on those two issues there is lack of consensus on how to proceed. They would certainly benefit more reviews.

fietserwin’s picture

You should read #priority as order. API changes can be found even in #17, but also in #7, #6 and #5. API additions are everywhere but are less intrusive and thus may be added in beta (I guess).

To get some progress (and consensus), I think we should make a step back and look at what is absolutely needed to:

  • arrive at feature parity with D7.
  • allow contrib to start porting.

As there are ideas to be more relaxed about API changes/additions in 8.1 and further, including marking parts that are still in flux as not being an API, we could start by defining what is the currently agreed API and what should not be marked as such (and thus be used at your own risk). If this means (partially) rolling back #2103635: Remove effects from ImageInterface, so be it.

Feature parity with D7:

No consensus on:

  • Should core also use the toolkit operations or should the core effects remain special and directly implemented in Image and ImageToolkitInterface?
  • how should the interface of toolkit operations look like? (formalize parameter checking or not?, explicitly describe parameters in the plugin annotation?)
  • ... (other?)
claudiu.cristea’s picture

Consensus? There was a consensus and a road map on this. This ticket is, itself, the expression of the consensus we had. If something has changes that needs a new plan with very clear concept. Now we are discussing about rolling back? Common!

Just because core committers are not having enough time resources to take this part of Drupal intro consideration it doesn't mean that we had a bad approach. All Drupal parts are moving to this kind of architecture.

I see "new ideas" around: Core effects having other architecture than contrib effects? While core needs to serve also as example for contrib this is pure discrimination and it's against the direction where Drupal is moving. I'm not doing things like this. I'm not developing shit!

catch’s picture

think before beta we should at least take a decision on #2110499: Unify the call and interfaces of image toolkit operations and #2103621: Move GD logic from ImageInterface to toolkit. These would introduce API changes that would impact developers of image effects and of toolkits. The

Just to reiterate, beta doesn't mean zero API changes like this - they might need to be reviewed a bit more closely in case there's an unexpected impact on already-ported modules, but the effect/toolkit API only really affects authors of effects/toolkits - it's not like routing or fapi.

fietserwin’s picture

RE #48: No, there was no consensus before on each and every aspect of this meta.:

  • This meta was created out of the single most important issue here: #2073759: Convert toolkit operations to plugins. This parent issue was thus created long after the creation of its most important child issue, not the other way round. And so, at once, many other issues became related to the original issue. Not by consensus but by the desire to relate all problems with the image API into a single meta. Around that same time (but only first written down in a comment on 11 October), I already concluded that there is no consensus. Not between the 3 people that initially started the toolkit operation issue, and in a later stadium also not with people that joined later (e.g. neclimdul on not adapting core, at least not now)).
  • No, there was no consensus on over-engineering versus step by step improvements. (From a different point of view also described as good design versus developing shit.)
  • No, there was no consensus on the approach: many small patches or few large patches.
  • No, there was no consensus on the order of tackling issues. Rerolls were needed all the time all over the place, everybody was working on the number 1 priority of its own agenda, not of this list.
  • No, there was no consensus on the high level concepts:
    • Should we combine Image and ImageToolkit, i.e. combine data and operation in 1 class?
    • Or should we leave the 2 classes apart, but more clearly separate their responsibilities: make Image a data carrier only and let the toolkit only do operations. Thus no apply() on Image and probably making it toolkit dependent (GdImage has a resource, ImageMagickImage has an ops array.
    • Or should we continue as we do now and ignore all the code smells like a.o. setWidth(), setHeight(), gerResource() on ImageInterface?

In #47, I was only looking at the bare minimum that is needed to let contrib start porting, not at the desired end point. So don't worry about developing shit, nobody here wants to do that. But to make the best use of scarce resources, both core committers time and our own development and review time, I wanted to discuss the order of doing things and what we can postpone until after beta1 (and that is a lot according to #49). If we are honest, we have to acknowledge that converting core to its own new plugin system for toolkit operations is not absolutely necessary for starting to port and thus, IMO, can be postponed to post-beta1. But it should still be done, I fully agree with you on that.

mondrake’s picture

Issue summary: View changes

Marking #2108339: Image toolkit getter won't fix, as #2103621: Move GD logic from ImageInterface to toolkit, tie toolkit instance to Image instance, toolkits should no longer be instantiated separately includes a fix for this, and it's needed there for both tests and to expose the toolkit to contrib code.
Also removing 'beta blocker' references according to @catch input in #45.

fietserwin’s picture

Issue summary: View changes

Progress is slow, but there is progress and @catch assured that these changes will be allowed past beta1. Therefore, and after some conversations with @mondrake (which does not necessarily mean that he agrees with everything below), I:
- changed the table into a list of open issues only.
- moved fixed issues to the list below.
- reordered the table of open issues.
- renamed column 'Priority' to 'Order' (indicating order of solving).
- renamed column 'Is blocking' to 'Is blocking contrib' (in the sense that these are must haves for contrib to be able to properly add image effects).

While the main issue here remains #2073759: Convert toolkit operations to plugins, that issue has been moved down. My considerations for doing so:
- I noticed that in almost all issues we are working on, we discover bugs or not so handy things in the interface specification, leading to ever growing patch sizes and/or follow-up issues.
- the 'main' issue did already provoke a lot of discussion and follow-up issues.
- the current (green) patch for the 'main' issue is quite large and will be difficult to review for non insiders.
- to get that patch in, I expect many more discussions, objections, new bugs (and thus lots of rerolls and a lot of work for the patch writers).

Therefore I think it is better to first:
- get the interface in a stable and sane state.
- solve structural issues as well (though I doubt if #2110835: Move the GD toolkit on its own module is purely structural, I think that issue has more consequences than discussed over there so far).
- get patches in that will reduce the size of the patch for the 'main' issue.
- solve known bugs.

Th result for the patch for the 'main issue:
- reduced patch size.
- will be to the point.
- will contain a minimum of side effects that needed to be changed as well to keep the system in a working state.

Resulting in:
- better reviewability for non-insiders
- hopefully less discussions that are not to the point
- easier and faster acceptance of the patch
- less rerolls and thus work involved

fietserwin’s picture

Issue summary: View changes

List of already fixed issues (formerly related issues) contained some open issues,some not even mentioned in the table above. Fixed the list and table for that.

fietserwin’s picture

Issue summary: View changes

Sometimes, a preview would be handy, lists of plain issue numbers are not easy to read... 1 more issue removed.

fietserwin’s picture

fietserwin’s picture

Issue summary: View changes
mondrake’s picture

Issue summary: View changes
mondrake’s picture

Issue summary: View changes

Updated issue summary to show the problems that have been solved already.

mondrake’s picture

Issue summary: View changes

One more.

mondrake’s picture

Issue summary: View changes
mondrake’s picture

Issue summary: View changes

Updated issue summary.

fietserwin’s picture

Issue summary: View changes
mondrake’s picture

I propose to change the sequence of issues.

mondrake’s picture

fietserwin’s picture

Issue summary: View changes

Updated list of open issues.

mondrake’s picture

fietserwin’s picture

Issue summary: View changes
mondrake’s picture

Issue summary: View changes
mondrake’s picture

Issue summary: View changes

Removed #2084985: Implement ThirdPartySettingsInterface in ImageStyle as it is not relevant to this meta.

mondrake’s picture

Issue summary: View changes
mondrake’s picture

Issue summary: View changes
mondrake’s picture

mondrake’s picture

Issue summary: View changes
mondrake’s picture

Issue summary: View changes
mondrake’s picture

claudiu.cristea’s picture

The problem described in #2357119: Allow image toolkit derivatives seems critical to me leading to reconsidering the architecture of toolkits > toolkit-operations.

mondrake’s picture

Updated issue summary

mondrake’s picture

mondrake’s picture

Status: Active » Fixed

I would suggest to close this at this stage

so let's close and deal with individual issues in the main d.o. queues 'image system' and 'image module'.

If we need we can create a new meta plan for 8.1 at later stage.

If anybody disagree just revert status.

Status: Fixed » Closed (fixed)

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