Flexible scheme and URI for image derivatives

This would be super useful for anyone using remote stream wrappers. It would make it easy to create local derivatives for non-writable stream wrappers, and it would also allow core image styles to be automatically generated on stream wrappers that don't use file-path-like naming. image_style_create_derivative() will already create derivatives on non-local stream wrappers (by creating them in a remote location and copying them over).

I believe this comment may be useful here as well.
In short, style generation problem seems to rise from the hook_menu() implementation in image.module. If url path is not identical with path used by "public:" wrapper and is not "private", Drupal won't invoke image_style_deliver() callback.

I have implemented hook_menu() in my module and syles work after that. Of course, I tested it with very simple module, but I believe that if a wrapper is implemented correctly (see File API and PHP streams) it won't have problems with styles creation/deletion.

From the other hand, implementing special-puropse method like getImageStyleUri(), which serves very specific task in otherwise generic file system is arguable.

I've created a related patch where I took a different approach to figuring out where to store image derivatives: #1821166: Support image style derivatives using a different file scheme than the original image

@benshell, your approach is too particular and assume that all derivatives will use the same scheme.

Rerolled #5. If passes, we need to add an alteration point so that modules can decide to use different schemes for different image styles. Coming soon.

Looking back to #9 it doesn't seems to me a good approach to let the stream wrapper define a policy for derivative location.

There's another approach that allows extending of \Drupal\image\Plugin\Core\Entity\ImageStyle in order to provide alteration of derivatives stream wrapper, path and even access: #2027423: Make image style system flexible.

Marking this as duplicate of #2027423: Make image style system flexible.

1. +++ b/modules/image/image.module
   @@ -1082,7 +1082,9 @@ function image_style_path($style_name, $uri) {
   +  drupal_alter('image_style_path', $result, $style_name, $uri);
   

   Here, the alterable is $result. The others ($style_name and $uri) are context variables. That said, why not passing as context everything that was previously processed in image_style_path()? I would go with this:

   $context = array(
     'style_name' => $style_name,
     'uri' => $uri,
     'scheme' => $scheme,
     'path' => $path,
   );
   drupal_alter('image_style_path', $result, $context);
   

   This will assure that alter implementations will not have to recompute again $scheme and $path.

   Also there's need for a new entry in docs: image.api.php with the new hook_image_style_path_alter().

2. +++ b/modules/image/image.module
   @@ -1082,7 +1082,9 @@ function image_style_path($style_name, $uri) {
   +  return $result;
   

   Let's add an empty line before return.

I ended up using the patch at #14 to support image styles in the amazons3 module. It's working pretty well, though it is a (mild) annoyance to be completely throwing out $result to recreate it in an alter hook.

I found this useful not just for managing the stream wrapper, but also for changing the entire URL. For S3, we have to ensure that there is always a bucket in the URL and don't have bare paths like s3://image.jpg. With this, I was able to do nearly all of the heavy lifting of image generation in contrib code without having to patch image module at all.

Anyone following this might be interested in #2479523: Add a hook_file_stream_wrapper_uri_normalize_alter() hook too.

Hi all,

I found error message in Watchdog : Amazon S3 was unable to create an image style derivative. Check the temporary directory configuration and permissions.

But some of my images could be generated correctly, some not.

What is wrong ?

Thanks

Added:

• The alter hook dispatcher.
• Documentation for the new hook_image_style_uri_alter().
• Test for hook_image_style_uri_alter().

• Fixed the phpunit test.
• More meaningful example in docs and test.
• Added issue summary.
• Added beta evaluation.

Green. Reassigning.

@claudiu.cristea, what about ImageStyle::flush()? If I am not mistaken, with the patch in #26 the altered URI can be anything, and therefore there is no guarantee that derivative images will be flushed. I think that at least the initial part of the altered URI should be enforced to be '{scheme}://styles/{style-name}' so that ::flush() can do its job.

@mondrake, that we cannot do because we'll lose exactly the main point of this feature request. Modules should take care of their own flushing policy by implementing: hook_image_style_flush().

Ah yeah forgot about that... then maybe we need a comment in the hook docs to make clear that implementations should take care of flushing the custom directories. Also some test for altering URI to a totally arbitrary directory structure and that flushing actually removes it.

@mondrake, Just take a look at ImageStyle::flush(). He handles very well the flushing of derivatives inside Drupal. That's because he uses also ::buildUri() to detect the location of file that needs to be flushed. And ::buildUri() returns something already altered ;)

But, it's true that we need to add some docs for the cases when developers are altering the URIs to make them point outside Drupal (to Instagram, etc.)

uses also ::buildUri() to detect the location of file that needs to be flushed

True, but only if a path to a image is passed (i.e. a single derivative needs to be flushed).

But if you are flushing (or updating) the ImageStyle entity, all the derivative images need to be flushed. This occurs by deleting the entire directory and all its files. And here ::buildUri() is not used, only the pattern {scheme}://styles/{style-name} for each writable scheme is deleted.

If, say, your unaltered URI is
public://styles/mystyle/public/field/image/bingo.png
and your altered URI is
public://mymodule/mystyle/bingo.png

then ImageStyle::flush itself will not remove the /mystyle directory from public://mymodule, with the consequence that the derivative images will be out of sync. So, that's where hook_image_style_flush() should also care about.

@mondrake, yes, that makes sense. But implementing a whole testing scenario seems to much. I think, now, the docs are enough.

Fine with me, thanks. Leaving at NR for @Dave Reid.

Nitpicks:

1. +++ b/core/modules/image/src/Entity/ImageStyle.php
   @@ -185,7 +208,16 @@ public function buildUri($uri) {
   +    // Assure a new extension, if case.
   

   "Append a different extension to the image file name, if the image style specifies so."

2. +++ b/core/modules/image/src/Entity/ImageStyle.php
   @@ -185,7 +208,16 @@ public function buildUri($uri) {
   +    // Allow modules to alter the uri before is returned.
   

   ... before it is returned.

Thank you for these findings.

Ouch! Sorry, I didn't wanted to unassign.

@claudiu, I have been still thinking about #32.

Now we are building the URI and then pass it to the alter hook with the context.

What about if, instead, we pass, by reference, the components, like e.g. based on the example:

• scheme: public
• basepath: styles/mystyle
• filepath: public/field/image/bingo.png

then, let the hook alter the components, and build the URI only after returning from the alters.

This way we could also add the same alter hook in ImageStyle::flush, and pass only scheme + basepath, for example something like

    // Delete the style directory in each registered wrapper.
    $wrappers = \Drupal::service('stream_wrapper_manager')->getWrappers(StreamWrapperInterface::WRITE_VISIBLE);
    foreach ($wrappers as $wrapper => $wrapper_data) {
+    $context = ['scheme' => $wrapper, 'basepath' => 'styles/' . $this->id(), 'image_style' => $this];
+    $this->moduleHandler->alter('image_style_uri', $context);
-      if (file_exists($directory = $wrapper . '://styles/' . $this->id())) {
+      if (file_exists($directory = $context['scheme'] . '://' . $context['basepath'])) {
        file_unmanaged_delete_recursive($directory);
      }
    }

so that the flushing of the image style is covered the same way of the flushing of a single derivative?

Re-rolled against 8.3.x

I was searching for a way to make image styles of a private image public. What you are doing here makes a lot of sence to me so please push this into the next release so I can create a module (my first d8 module) adding to the image style form a radio element "Storage as" with "same as original" (default), "public file" and "private file". What can I do to make this work go into core?

I believe this issue addresses what was described #1903190: Allow image style derivatives of private images to be stored on the public file system.

Using Drupal 8.3.5 I was able to apply this patch and use the following hook to allow the watermark image style of an image field using private storage to have those watermarked images be available publicly:

/**
 * Implements hook_image_style_uri_alter().
 */
function mymodule_image_style_uri_alter(&$uri, array $context) {
  $image_style_id = $context['image_style']->id();

  // Watermarked images are available publicly.
  if ('my_watermark' == $image_style_id) { 
    $source_scheme = $context['scheme'];
    $path = $context['path'];
    $uri = "public://styles/{$image_style_id}/{$source_scheme}/{$path}";
  }
}

So, the patch works!

I'm setting this to RTBC given that it has tests, works as described/meets requirements, and in my expedited review of the code I didn't find any obvious issues.

Bunch of nits, plus missing CR.

1. +++ b/core/modules/image/image.api.php
   @@ -39,5 +39,38 @@ function hook_image_style_flush($style) {
   + * This hook allows modules to alter the uri of the derivative. By implementing
   ...
   + * builds the uri of an image derivative.
   ...
   + * Note: If you use a special uri alteration that stores the image derivatives
   
   +++ b/core/modules/image/src/Entity/ImageStyle.php
   @@ -183,7 +206,17 @@ public function buildUri($uri) {
   +    // Allow modules to alter the uri before it is returned.
   

   s/uri/URI/

2. +++ b/core/modules/image/image.api.php
   @@ -39,5 +39,38 @@ function hook_image_style_flush($style) {
   + * be available on a different schema. This hook is called when the image module
   

   s/schema/scheme/

3. +++ b/core/modules/image/image.api.php
   @@ -39,5 +39,38 @@ function hook_image_style_flush($style) {
   +  if (($name = $context['image_style']->id()) == 'watermarked_low_resolution') {
   

   ===

fixed the nit picks and added the CR

https://www.drupal.org/node/2917669

#50 worked for me, hope to see this patch applied to the core soon.

+++ b/core/modules/image/image.api.php
@@ -39,5 +39,38 @@ function hook_image_style_flush($style) {
+ * Alters the derivative uri.
...
+ * This hook allows modules to alter the uri of the derivative. By implementing
...
+ *   The image style uri to be altered.

These replacements were left out: s/uri/URI

fixed nit picks #55

This issue has been RTBC'd in #50. Since #50 there were only nits fixes. I'm RTBCing back (only for the nits part #50 => #56).

I've also improved the change record.

Thank you for the work here.

Sorry, once more:

1. +++ b/core/modules/image/image.api.php
   @@ -39,5 +39,38 @@ function hook_image_style_flush($style) {
   + * be available on a different scheme. This hook is called when the image module
   

   Nit: "on a different scheme" sounds strange? In a different stream wrapper?

2. +++ b/core/modules/image/image.api.php
   @@ -39,5 +39,38 @@ function hook_image_style_flush($style) {
   +function hook_image_style_uri_alter(&$uri, array $context) {
   +  if (($name = $context['image_style']->id()) === 'watermarked_low_resolution') {
   +    $path = $context['path'];
   +    // Low resolution, watermarked images are available publicly, in the
   +    // public:// stream wrapper.
   +    $uri = "public://styles/$name/public/$path";
   +  }
   

   Doesn't this mean only writeable stream wrappers can be used?

   See \Drupal\Core\StreamWrapper\StreamWrapperInterface::WRITE.

3. +++ b/core/modules/image/src/Entity/ImageStyle.php
   @@ -185,7 +208,17 @@ public function buildUri($uri) {
   +    // Allow modules to alter the uri before it is returned.
   

   s/uri/URI/

4. +++ b/core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php
   @@ -265,4 +265,20 @@ public function doImageStyleUrlAndPathTests($scheme, $clean_url = TRUE, $extra_s
   +    // The 'watermarked_low_resolution' derivatives are available publicly, in
   +    // the public:// stream wrapper.
   
   +++ b/core/modules/image/tests/modules/image_module_test/image_module_test.module
   @@ -32,3 +32,15 @@ function image_module_test_image_effect_info_alter(&$effects) {
   +    // Low resolution, watermarked images are available publicly, in the
   +    // public:// stream wrapper.
   

   "in the public:// stream wrapper" seems kind of pointless to mention?

5. +++ b/core/modules/image/tests/modules/image_module_test/image_module_test.module
   @@ -32,3 +32,15 @@ function image_module_test_image_effect_info_alter(&$effects) {
   +  if (($name = $context['image_style']->id()) == 'watermarked_low_resolution') {
   

   ===

6. +++ b/core/modules/image/tests/src/Unit/ImageStyleTest.php
   @@ -58,10 +58,14 @@ protected function getImageStyleMock($image_effect_id, $image_effect, $stubs = [
   +    $module_handler = $this->getMockBuilder('\Drupal\Core\Extension\ModuleHandlerInterface')
   

   Let's use ModuleHandlerInterface::class, this is easier to refactor in the future.

Worked as per the nit picks

1. +++ b/core/modules/image/image.api.php
   @@ -65,8 +65,8 @@
   -    // Low resolution, watermarked images are available publicly, in the
   -    // public:// stream wrapper.
   

   I don't understand the "writable" thing. Probably we want to say: "Low resolution, watermarked images are available in the public readable stream wrapper."

2. +++ b/core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php
   @@ -277,7 +277,7 @@
   -    // the public:// stream wrapper.
   +    // the stream wrapper.
   

   The correct phrase would be: "The 'watermarked_low_resolution' derivatives are available publicly."

3. +++ b/core/modules/image/tests/src/Unit/ImageStyleTest.php
   @@ -59,7 +59,7 @@
   -    $module_handler = $this->getMockBuilder('\Drupal\Core\Extension\ModuleHandlerInterface')
   +    $module_handler = $this->getMockBuilder('\Drupal\Core\Extension\ModuleHandlerInterface::class')
   

   This should be:

   
   ...
   use Drupal\Core\Extension\ModuleHandlerInterface;
   ...
      $module_handler = $this->getMockBuilder(ModuleHandlerInterface::class)
   

fixed as per #60

Thank you. RTBC for changes since #50.

Hi

I discussed this with @catch and we're not hugely keen on having config entities fire alter hooks.
To try and understand why this is still needed, can we get an issue summary update here?
I see in #12 this was marked as a duplicate and in #13 it was moved to Drupal 7 only.

But then in #18 it was moved back to Drupal 8, but without mentioning why, in particular what the need is that can't be solved by #2027423: Make image style system flexible. I note it was tagged `Media initiative` at the same time, so can we get some information on how the two are related?

Can someone provide some examples of why this is needed - thanks.

Lee

Related, #2986669: Split ImageStyle into the config entity and a separate event-based image processing service

Here's an attempt at a re-roll for 8.5.5 (and hopefully 8.6.x). I would be interested in the traction that the approach proposed by @mondrake gets, though...

I've applied #67 against 8.5.6 and the paths are updated but the image is a 404. I've run drush if --all but they're still 404'ing. Did I miss a step?

Re-rolling patch from dpagini for 8.7.x

re-rolling the 8.8.x patch

Re-roll patch created for 9.1.x.

Modified patch for 9.2.x.

Fixed the fail tests, Please have a look.

Interdiff added.

Re-rolling patch for 9.3.x.

+++ b/core/modules/image/src/Entity/ImageStyle.php	(date 1634652867285)
@@ -94,6 +95,28 @@
+  public function __construct(array $values, $entity_type, ModuleHandlerInterface $module_handler = NULL) {

Adding a constructor to an entity class with DI seems a bit weird to me. Entity objects don't do DI, and I seem to remember the reasons involve something like serialisation.

That might not affect config entities, but still, there's no support for this in the Entity system yet.

Updated patch in #83 with the changes suggested in #84, i.e., removed constructor from an entity class as it doesn't support DI.

Thanks!

Cleanup patch from #85 to not inject module handler service to ImageStyle in tests.

The tests are broken, because now the container is needed to be set in unit test because of \Drupal::moduleHandler() call.

Also, I've noticed that when accessing the url of the image derivative that was made public with a hook, but the original image is in private filesystem, the derivative itself is not automatically created, like it is done for images that are in public folder. Maybe there was something wrong with my NGINX settings, I will check more.

Fix unit tests

I can confirm that in web/core/modules/image/src/Controller/ImageStyleDownloadController.php on line 160 the source of the image can't be found and controller throws an exception 'Error generating image, missing source file.' This happens when the private image has a public derivative url. So it means that the derivative is not created automatically like for "normal" image styles. I suggest to add here the alter hook. Or the concept of changing the url should be changed and it should be as a setting on ImageStyle entity to select the scheme of the derivative.

This change includes automatic image derivative creation. Be aware that it also requires another alter hook, so that the original file uri can be restored to point for example to different scheme.

Re-uploading the patch with fixed code standards

After implementing the new alter hook, patch #96 seems to do the trick for us!

Rerolled patch against #96 for 9.4.3.

Rerolled patch against #96 for 9.5.0.

Tagging for CR update as a new api function hook_image_style_source_alter doesn't seem to be referenced

Also was previously tagged for IS update which may still need to happen

Did not test patch.

I hadn't seen this yet, it looks like the same use-case as #3354207: Allow image style derivatives to use a separate stream wrapper except that would just allow the stream wrapper to be configured. #3027639: Make css/js optimized assets path configurable shows what it looks like.

Oh, dear me, a major showstopper and nothing has changed for years. :-( That hook would be desperately needed, up to the latest 10.1 and more... What could we do to make it happen ASAP?

For those who happen to be in my shoes: create a route subscriber and alter the route image.style_private to your own controller. Copy Drupal\image\Controller\ImageStyleDownloadController::deliver() to it and modify it to either do something entirely different, or probably just handle your own case and call parent::deliver() to get the original behavior.

In my case, I mostly copied it in its entirety, leaving out where it calls the `hook_file_download_alter()` hook and doing by own permission based decision instead. YMMV.

@djg_tram - I can managed to get the derived image written to the public filesystem, but how do I tell Drupal to use the public URL for this image, rather than the private /system/files/... one?

@fonant, if you use the patch and the hook, there is no need to do anything else. The uri of the image is changed for image style, so if it is public then the url will be for public scheme.

@a.dmitriiev thanks, I was wondering how @djg_tram had managed with his method.

I now have this working with the patch and a custom module with a hook. Needs a bit more work and some tidying before I post it here.

Now that #2786735: Image derivative generation does not work if effect "Convert" in use and file stored in private filesystem patch in #101 no longer applies

Rerolled 101 to apply to Drupal 10.2.x with the commit, #2786735: Image derivative generation does not work if effect "Convert" in use and file stored in private filesystem which causes the conflict.

I am testing it.
the uri has changed.
but i can't seem to get the image_style with the new public uri to be generated.
Should I add code to the hook to generate the image style

I used the example from the patch.

But I am unclear how can the image_style be generated once I rewrite the uri with the hook.

drush cr does not seem to be doing it

I have the patch and this hook in a custom module.

/**
 * Implements hook_image_style_uri_alter().
 */
function MY_MODULE_image_style_uri_alter(&$uri, array $context) {
  $image_style_name = $context['image_style']->id();
  if (in_array($image_style_name, ['watermarked', 'watermarked_other'])) {
    $path = $context['path'];
    // Low resolution, watermarked images are available in the public readable
    // stream wrapper.
    $uri = "public://styles/$image_style_name/public/$path";
  }
}