Refactor ImageStyleDownloadController so derivatives can be generated by contrib modules

1. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
   @@ -131,23 +115,97 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
   +      $image_uri = $this->validateSource($image_uri);
   

   Break the original method into several methods makes the code much easier to read!

2. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
   @@ -131,23 +115,97 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
   +      $this->logger->notice('Source image at %source_image_path not found while trying to generate derivative image at %derivative_path.', array('%source_image_path' => $image_uri, '%derivative_path' => $derivative_uri));
   

   Does this long line ok with phpcs rules?

3. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
   @@ -131,23 +115,97 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
   +      return new Response($this->t('Error generating image.'), 500);
   

   Feel like the response better include the same msg logged above, rather than just saying "Error generating image".

Looks good to me, contrib will be able to extend the class and change only some of the methods instead of rewriting the entire deliver() method.

#4.3 changing the existing error string would be out of scope here.

Second thinking :)

Actually since you mention #2359443: Allow creating image derivatives from an Image object, in order for this to work better with it later, I would suggest

1. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
   @@ -131,23 +115,97 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
   +  protected function send($scheme, $derivative_uri, $headers = array()) {
   +    $image = $this->imageFactory->get($derivative_uri);
   +    $uri = $image->getSource();
   

   pass $image as a parameter to send() instead of $derivative_uri

2. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
   @@ -131,23 +115,97 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
   +   * @return bool
   +   *   TRUE if the image exists or was generated, FALSE otherwise.
   +   */
   +  protected function generate(ImageStyleInterface $image_style, $image_uri, $derivative_uri) {
        // Don't start generating the image if the derivative already exists or if
   

   return an Image object instead of a bool - the image object of the generated derivative image, that can then be passed to send() - or FALSE/NULL in case of failure.

Looks good to me. It's purely refactoring within the ImageStyleDownloadController class - with introduction of some protected methods. Not sure whether it would need additional tests for that - let's bring to the right eyes and see feedback :)

RTBC

+++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -157,27 +214,54 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
+      throw new \RuntimeException(sprintf('%s was unable to be generated', $derivative_uri));

Let's add a specific exception for this and we need an @throws for this.

Nice refactoring in general!

+++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -125,23 +111,100 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
+    catch (\RuntimeException $e) {
+      $this->logger->notice('Unable to generate the derived image located at %path.', array('%path' => $derivative_uri));
+      return new Response($this->t('Error generating image.'), 500);
+    }

Is there a reason we catch any kind of exception and not just the more specific one like in the snippet above?

Because I forgot to change it?

Entirely wrong patch. Time for bed.

#10 has been addressed. RTBC again.

Nice cleanup!

Few nits:

1. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
   @@ -125,23 +111,100 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
   +      $this->logger->notice('Source image at %source_image_path not found while trying to generate derivative image at %derivative_path.', array('%source_image_path' => $image_uri, '%derivative_path' => $derivative_uri));
   ...
   +      $this->logger->notice('Unable to generate the derived image located at %path.', array('%path' => $derivative_uri));
   ...
   +  protected function send($scheme, ImageInterface $image, $headers = array()) {
   ...
   +    $headers += array(
   ...
   +    );
   

   Lets' use modern square brackets syntax for arrays.

2. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
   @@ -157,27 +220,54 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
   +      throw new ImageGenerationFailedException(sprintf('%s was unable to be generated', $derivative_uri));
   

   In such cases, by using double quotes, you can simply inject $derivative_uri into string and avoid any function call like spintf().

3. +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
   @@ -157,27 +220,54 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
   +    if (!$this->config('image.settings')
   

   I would prefer to move $this->config('image.settings')->get('allow_insecure_derivatives') in variable just for if (...) readability.

This is nice, but not help when more than one module needs to override the controller, e.g. stage_file_proxy and webp and avif. I'm wondering if it would be possible to have a service that can be decorated by multiple modules instead? See https://symfony.com/doc/current/service_container/service_decoration.html for details on service decoration in symfony. Then the ::deliver method can just call a method on the service and each module can decorate it with it's own behaviour.

Some update here and a "redo" of the patch for Drupal 11

[replace patch by a cleaner one]

Can you please put that code on a merge request? Tests don't run against patches anymore.

Merge Request available

Thanks for converting to an MR.

Noticed that the summary appears incomplete, would recommend using the standard issue template.

Also will need test coverage for the new functionality

Thanks

For test coverage, I don't what it need to be tested, it just code management, no new functionnality.

Thanks for MR.
Do you think we can add interface for these new methods?

In the issue summary it states

- with S3...

What does that mean? With (in the case of) the S3 contributed module? Or with the S3 stream wrapper?

Then

..we want to first serve an image locally..

Who is 'we'? The reporter's company? Drupal.org?

How does 'we' 'want to first serve an image locally'? By using or creating a contributed module? Or a custom module?

I dont know the answers to these questions. If someone would like to update the issue summary or I will if I can get answers to these questions.

@nikolay shapovalov :
"Do you think we can add interface for these new methods?"

I don't know ...
The purpose of the RM is simply to separate the processing/generation of the derivatives from the various validation/authorisation operations linked to this generation.

@oily : I don't know either what @deviantintegral want to say, when it create the issue.
I just done my best when @smustgrave ask me to update summary to the "good template"
I have my use case "with mediaContextualCrop, we want to create a new image delivering methodebased on 80% of the ImageStyleDownloadController" for the others I don't know their motivations.

The test coverage could be following the steps to reproduce.

Will slightly agree with @nikolay shapovalov feels like an interface now.

I just add the interface, but I have a doubt about making all methods public ... if some one can check

I've faced issues related to the namespace and the missing return type of the isSchemePublic method.
The .patch file is attached for the people who don't want to add thousands of commits to their patch section inside composer.json.

@drdam:
"I just add the interface, but I have a doubt about making all methods public ... if some one can check"

I don't think that we need to put into the interface methods that aren't planned for external usage. So, I believe we should describe only the method "deliver" as the one that could be executed externally(I might miss something because I spent a little time checking the code.)

Still appears to be missing test coverage. May need a CR for the new interface too, I should of mentioned before that's on me.

Having reviewed the module’s unit tests, I get the impression that none of the existing tests cover the module’s other public methods (sourceImageExists and getUriWithoutConvertedExtension).

Could you explain exactly why the refactoring of the code in this issue requires specific tests, whereas other refactorings do not?

Please understand that my point is not that "I don’t want to" write these tests, just that I don’t understand "what to test".

I was asked to weigh in on the architecture here. What I like about this is it makes things a little easier to extend/replace. But I'm not satisfied. Ideally, we would have a way for more than one extension to inject itself here. Either through decoration or maybe through event subscribers.

My long term plan (hope?) is that for responsive images we do things differently. I know this is specifically intended for image.module and its image_styles. And my hopes are a long term goal. See #3372932: [Meta] High-performance images (nearly) out of the box. Said goal would need a new dedicated route for responsive images. Anything we can do here and elsewhere that takes baby steps in that direction are good.

This only picks up half the equation. We're only handling images, not all general files. The controller here extends the file controller. For some of the alternate storage options mentioned in the IS, that more properly exists in the upstream controller. Not here.

Before we keep moving on this, a good IS update on _why_ we are doing what we are doing here and how the proposed solution would solve that problem would help convince me the approach here is good/bad. Examples are there already but if we could clarify which of them make sense to support images vs all files, that would be good.

Thanks @heddn for taking a look

Yesterday's security patch broke this MR.

Rerolled for Drupal 10.6.13 after SA-CORE-2026-010.

The only code adjustment is in the extracted isSchemePublic() helper. The previous patch preserved the older token-based public/private decision. Drupal 10.6.13 changed ImageStyleDownloadController::deliver() to determine public access from the derivative scheme plus file_additional_public_schemes, so this reroll keeps that new logic inside the helper.

Patch and interdiff attached.

Assisted by Codex.

This patch was AI-generated, but I really had to push it to only make bare-minimum changes to #50.

Patch for 11.4.4, made with claude, from the MR.

I'm working on fixing this issue: https://www.drupal.org/project/media_contextual_crop/issues/3608131