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.