Split ImageStyle into the config entity and a separate event-based image processing service

Drive-by review

+++ b/core/modules/image/src/Entity/ImageStyle.php
@@ -174,130 +164,37 @@ protected static function replaceImageStyle(ImageStyleInterface $style) {
-    return "$scheme://styles/{$this->id()}/$source_scheme/{$this->addExtension($path)}";
+    @trigger_error('The ' . __METHOD__ . ' method is deprecated since version 9.x.x and will be removed in y.y.y.', E_USER_DEPRECATED);
+    return \Drupal::service('image.processor')->createInstance('core')
+      ->setImageStyle($this)
+      ->setSourceImageUri($uri)
+      ->getDerivedImageUri();
   }
 

I'm not sure if I like the API, having a service with a state like that is a IMHO a bad design.

maybe it should be like a factory that creates an object for each image style + source image?

Another thing, in light of #2669074: Convert file_create_url() & file_url_transform_relative() to service, deprecate it, we might want to make this API return URL objects, this currently not doing that is one reason why we had to go back to having 3 different methods on the new service there.

Sounds like media already made kind of it to redo thumbnails processing, and features are for 9.1

Per latest MR test failures; looks not necessarily related to the feature but the test setup.

I updated from 9.4.x and made a few very minor fixes while reviewing.

1. While the event having setVariable and related methods is certainly flexible, I don't like how it's similar to arrays in that a single typo in a key can be hard to debug. My first thought is that those should be pulled out into an interface, and then instead of ->hasVariable() ? ... it changes to $pipeline instanceof DerivativeImageUrlInterface.
2. It feels odd to have the Derivative plugin, but only a single implementation, and no interface. I think to meet the "Why a plugin" goals in the issue summary we at least need an interface. Once that's done, I think the \Drupal::service(..) calls in the ImageStyle entity should be pulled out into a method to assist type checking and method autocomplete in IDEs.
3. The ImageDerivativeSubscriber class seems rather large, with many constructor arguments and imports. I noticed that flushing is the only place using the module handler, so perhaps that and other methods could be split out into a few different subscribers.
4. It would be good to split ImageProcessException, perhaps make it a base exception with a few subclasses like MissingSourceException, CachedImageExistsException, etc.
5. Reading the interface comments at \Drupal\image\ImageStyleInterface::createDerivative() for example, it's not clear what to do instead of that method. Further docs could help here.

I haven't reviewed any test code. The only failure is due to calling a newly marked deprecated method.

Regardless of the above, I would like to take a pass at implementing what's here to support alternative image processing methods. I have time reserved December 3rd, so if any of the above are clear next steps I can work on them then.

I have time reserved December 3rd

This is next-level organization. I am in awe.

I only have time because Lullabot gives us a few days to dedicate each year, and I've not used them yet! Thanks for the kind words.

I've got several commits locally that refactors to use interfaces in many places, but I realized there's a gap in the current architecture.

In general I see the combination of plugin + events may seem overwhelming but my rationale was to get to enable overriding at the highest granularity possible

Agreed! However, as is it's not actually possible for a contrib module to easily replace what core's image styles do, but only alter them. For example, in the current patch's buildUri method in ImageStyle and similarly in template_preprocess_image_style():

  /**
   * {@inheritdoc}
   */
  public function buildUri($uri) {
    @trigger_error('The ' . __METHOD__ . ' method is deprecated since version 9.x.x and will be removed in y.y.y.', E_USER_DEPRECATED);
    return \Drupal::service('image.processor')->createInstance('derivative')
      ->setImageStyle($this)
      ->setSourceImageUri($uri)
      ->getDerivativeImageUri();
  }

Since the derivative plugin is completely tied to core's image style implementation, it has functionality to set clean URLs, deal with derivative tokens, etc. An external image service may not have any of these concepts at all.

As well, the "check for null, dispatch an event, set the value" loop seems like an easy way to accidentally introduce recursion (which I've done locally a few times now).

My thinking is that we reorganize the plugin and events so that:

1. The image.processor service becomes a normal service and not a plugin type. It would only have very basic methods, like setImageStyle, getDerivativeImageUrl, and so on.
2. The processor service loads all implemented plugins (like the current derivative plugin), and calls a method to determine if that plugin can handle the request.
3. Each plugin type can choose to expose events for itself, with derivative being the example. That way, contrib modules that need just very targeted changes can listen to those events, and site-specific code that doesn't need events (or contrib that doesn't want to allow altering) can skip them entirely.

All unit and kernel tests, and at least some functional tests are passing. Let's see how far it gets.

Regardless, I've completed significant work on the branch, though I'm sure it still needs work. I've hacked in some dependencies via \Drupal, and docs are missing or need to be updated. As well, Derivative needs fresh eyes to break it down, and the events that are fired need review and likely tests.

The Drupal\image\Pipeline namespace contains different interfaces that represent various known features of image style rendering systems. For example, a system may only have the concept of derivative URLs (keeping source images opaque to clients), or have security tokens like Amazon S3's signed URLs or Drupal's itok parameter. The goal is that each plugin would implement some subset of these interfaces, with the current Derivative plugin implementing all of them (since it's very fully featured!).

As well, I completed the switch from using events as an API, to using them more directly as an extension mechanism.

Finally, in solving the issue of determining what pipeline to use, I decided that making it a configurable option for image styles made the most sense. It currently exposes the available plugins when configuring the image styles, but I think we'd hide it if there's only one. This would allow existing sites to use one or more image providers, which could be especially helpful during migrations between them. Which reminds me, that totally needs a config dependency on any modules that expose pipeline plugins.

And the current hierarchy of the interfaces:

I have two more days available to work on this, currently set for December 9 and 10th.

When such kind of configuration for plugins discussed (#62) wanna point very old issues, I bet styles plugins could get benefit from that

- #2011038: Use Context system for actions
- #2347023: Move processing plugin context to data processors

Summary: Unless I hear otherwise, I will likely work tomorrow on refactoring pipelines to be bound to an image toolkit, instead of an image style.

For context, my own goal is for core to support what isn't viable today: Using Drupal's image style system to render images with a remote API like Cloudinary or Fastly Image Optimizer. If that is possible, for many sites it reduces or eliminates the need for fast shared disk storage, simplifying cloud and containerized deployments. While there is a cloudinary D8 module, in practice it doesn't work and couldn't be made to work without core hacks. We ran into similar challenges years ago with S3 support for images.

This is something I'd like to question. To me, it should be the other way round - the pipeline run should have configuration where the image style is one of the variables

That would mean a separate config form for pipeline instances? It feels like an additional level of abstraction that's not got a clear benefit to me.

To go through all of the Image concepts we have as of the current patch:

1. Image toolkits: Typically GD or ImageMagick. These are bound to the source image in the Image class. Part of what we're addressing is decoupling the "toolkit" used for the source image and any derivative images.
2. Image effects: These just manipulate data. It's the toolkit that "renders" effect data into a derivative. These are great in that they aren't tied to toolkits, but difficult in that they can't know if a given image's toolkit can successfully apply an effect.
3. Image styles: A collection of image effects.
4. Image pipelines: A plugin and a set of interfaces that define the types of features supported for making derivative images.

In core as it exists today, I think the largest limitation is that image toolkits have no way to communicate to image styles what effect operations they support via the apply method. We have FALSE returns, but those can also happen because of a runtime error. Conceptually, image toolkits also expect to be literally "saving" a file, when most remote APIs just want a URL prepared.

When I look at what has the most coupling, it supports your suggestion that styles are the wrong place to integrate with pipelines.

Instead, what if it was at the toolkit level? Pipelines become invisible to the administrator, and are a way for common toolkits like GD and ImageMagick to share code. I'm imagining a "Local file derivative" (shipped with core) and one or more "remote URL derivatives" (written in contrib).

From an admin perspective:

1. The site default toolkit is used for image styles like today.
2. Image styles can override the toolkit used, if more than one toolkit is available.
3. Someone building image styles can be warned if a given toolkit's derivative plugin doesn't support an effect (or we could hide those effects entirely).

To think through your use cases:

1. upload original JPEG photos, full size, after resolving potential orientation aspects and editing the metadata to add a copyright exif tag and ensuring GPS coordinates of the photo taken exist, to a private storage on AWS so that they cannot be accessed publicly;

This essentially implies "derivatives of derivatives". Or, perhaps a simpler method would be "apply this image style to the field when uploading". Is there any limitation in Drupal's core APIs to implement this in contrib today? I think it could be done by decorating or swapping out the ImageItem plugin, updating the field settings form and calling the image style in \Drupal\image\Plugin\Field\FieldType\ImageItem::preSave(). If this is possible today, then I think this issue should be limited to ensuring we don't break this extension point for contrib.

2. show a PNG thumbnail in the image upload widget to users allowed to upload the photos, stored in private schema;

I'm not clear how this is different from what can be done today. If an image field is created, we can set the image style in the Preview Image Style option.

3. generate publicly visible derivatives, downscaled and watermarked with copyright text taken from exif metadata, in three formats AVIF, WEBP, PNG so that browsers can decide the best format to retrieve, from the public schema.

I don't think this can be done today, in that the scheme kept the same for image styles. If an image is in the private stream wrapper, the derivatives stay there too. Perhaps this would be a new use case for a type of derivative plugin in contrib that extends the proposed "local derivative" plugin that ships in core, but exposes config to change the destination scheme. The multiple formats aspect seems to be handled given the contrib webp module, even if it's not an optimal implementation.

1. in the field settings of the image field, with an 'upload' pipeline configured appropriately

So, could this just be an image style? The uploading aspect would be handled by the selected stream wrapper.

2. in the item FieldWidget, configured to use a 'derivative' pipeline that uses a 'thumbnail' image style, and possibly defines the output format in the pipeline configration instead of a convert effect in the image style

This exposes the coupling to the image toolkit, in that it's toolkits that determine what formats are supported via \Drupal\Core\ImageToolkit\ImageToolkitInterface::getSupportedExtensions. If image formats need to be extracted from image styles, perhaps that's in the future? I think keeping formats as a part of image styles will help constrain scope here.

3. in the FieldFormatter, configured to use a 'multiderivative' pipeline that uses an image style for scaling and watermarking, and defines the output formats in the configuration in such a way that image toolkits would directly save the derivative in the three different formats after the image has been manipulated instead of running three separate 'derivative' pipelines.

This implies a push-based generation of derivatives instead of the pull-based workflow we have today. I think it's important to consider, as there are likely systems out there that benefit from pregenerating derivatives (or actually require it).

my own goal is for core to support what isn't viable today: Using Drupal's image style system to render images with a remote API like Cloudinary or Fastly Image Optimizer.

I'm not sure if I'm chiming in at a very late date or this is orthogonal to the changes described here, but I have done some work in contrib-land to support what I've called external image styles.

https://www.drupal.org/project/external_image_styles

Also perhaps related, some prior art on supporting splitting the origin image and derivative image storage:

https://www.drupal.org/project/cloud_native_image_styles

I'll have to take a read of the full thread to get a flavor for how this might affect this pattern, but A) you're not the only one interested in this pattern, even if it isn't typical and B) I hope some of this helps at least validate the pattern or perhaps inform an approach that suits different use cases.

Thanks for sharing these! I see External Image Styles:

• Alters the image style entity class using hook_entity_type_alter.
• Has the same approach as the current MR, where the image style has a reference to the provider or pipeline. One difference is that this MR makes that editable for existing image styles, while external image styles (at least at the UI level) locks it in to create only. I like that, because it sidesteps issues around flushing "stale" images when changing providers.
• Overrides \Drupal\external_image_styles\Entity\ImageStyle::buildUri and \Drupal\external_image_styles\Entity\ImageStyle::createDerivative to throw a runtime exception if used.

I was curious how likely those methods are to be called in real sites. I see:

1. Token module calls buildUri to expose the image URI token.
2. Webform has a WebformImageFile element calling buildUri.
3. webp module does too (but, perhaps that wouldn't be used with external image styles anyways)
4. All three modules also call createDerivative.

That leads me to believe that external image styles cannot be used without careful consideration, and for smaller or junior teams they could run into situations where images "break" without a clear reason why.

Cloud Native Image Styles is great! I see how it allows the default filesystem to be flysystem or S3, and derivatives to be stored in container instance storage (instead of shared storage). There'd be a higher temporary disk use since each container or web server would generate it's own cache of images, but I bet that's still way better then the slower performance and cost of something like EFS. This seems like a good solution for sites with containerized deployments, but still wanting to use core's image styles.

So... next steps? I think External Image Styles validates the approach of "the rendering pipeline is tied to the image style", given two of us came up with similar solutions independently. I'm going to work on cleaning up the current MR instead of the refactor I suggested above.

And, as an additional goal for this issue: When merged, a module like External Image Styles should no longer need to override and throw runtime exceptions for methods like buildUri().

Since my last push and post, I set up https://github.com/cshum/imagor as an easy to run image service to test an implementation against. I got it working! Meaning, I was able to write a contrib module that used that service for image generation, and show the results in the image style edit form.

However, I found that nearly all of the logic was in the image toolkit implementation, and it basically ignored all the work on image pipelines. Making it work without a custom image toolkit wasn't possible. It makes me think what we may want is an even simpler version of this patch that _just_ makes it so buildUri and friends can work with alternative storage systems, and pipelines become their own later addition. I need to think on that.

I was getting ready to push up the work I did changing ImageStyles to select the toolkit instead of the pipeline... but my laptop disk died. If I have time, I may see about pulling out the commits from backups. If someone was planning on working on this over the next two weeks, let me know and I'll prioritize that.

This would be really useful for the Imgix module, we have had to override the image_style entity class in order to make it work.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

cspell error

/var/www/html/core/modules/user/tests/src/Functional/UserPictureTest.php:123:34 - Unknown word (getfile)

Seems like something that could use a change record.

For any contrib modules that add an image style will they have to do anything or will this work automatically.

Did a first pass of review and wondered do we really need so many events fired while generation happening.

Very probably it affects performance as each event is dispatched (as I got).

Maybe instead it could use less events as some events can carry state of process

For #83

Wasn't trying to shoot down the idea. Not something I can speak heavily on though.

I really don't think this should be closed.

I have been a bit booked by other tasks so I haven't been able to give it a test run, which I really wanted to do before commenting to hopefully better comment, but given there is a move to close it I feel a need to comment without having done the testing yet.

@andypost in #83 you assert there is a 'probable' performance impact but did not provide a profile report with your post? Can you please attach the generated performance report file for the rest of us to review?

While I've not traced every single event yet in this MR, the majority of the ones I have are only needed when an image derivative doesn't exist, the majority of times images are served from the file on disk. The most common to trigger event I would expect to be the buildUrl() events which need to be fired to generate links to ImageStyles Derivatives, though the flexibility of an event is necessary to allow all the changes that have been requested over the years.

Could some events be combined? Without looking at every line, Maybe? But would it make sense? Most of these events are in response to a specific method being called, if no-one ever calls that method, why invoke the expense of gathering data that no-one will use?

IMHO it would be a shame for this work to be lost especially since #83 didn't provide any objective data to justify moving to needs-work.

Closed the outdated MR

Looking at the patch next

Left some comments on the MR

Keeping at NR and retaining the 'Needs architectural review' tag

Added #3375453: [policy, no patch] Decide on policy regarding use of Events class with string constants for event names or class names instead for discussion

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

@mondrake, deviantintegral, @bradjones1 thanks a lot for your work!

Just I think after 5 years being open, there's no community interest at carrying this on. So I won't spend time on rerolls.

I'm working on a way of bringing UI to specify image styles beeing used for remote images for IntelligenceBank DAM and I think this issue is a good base for it. Typically DAMs like IB or PicturePark provides a way to generate image derivatives on their side, but there is a lack for selecting right image style in content context (like ckeditor, specific node, block). In Drupal we have many ways to map content item with right display settings, but it fails if image is remote and we don't need generate derivative ourselfs.

I will check this issue more deeply!