Problem/Motivation
The remote_stream_wrapper module handles image style downloads by creating new routes for the supported schemes to delivery files with RemoteImageStyleDownloadController::deliver, which is a thin wrapper around the coreImageStyleDownloadController::deliver function to ensure the target is a managed file.
Meanwhile the webp module implements it own ImageStyleDownloadController to allow webp derivatives images to be generated from non-webp sources (for example if .../some-image.webp is requested it will be able to generate it if a source .../some-image.jpg exists), and replaces the core implementation with it for the public and private schemes.
It also modifies the responsive image formatter to augment picture source sets with webp versions of the same images to allow browsers to use the webp derivatives if it supports them, which works fine for public / private schemes, but leads us to a problem for remote stream wrapper schemes as RemoteImageStyleDownloadController::deliver is only working with the core implementation.
Proposed resolution
Creation of a WebpRemoteImageStyleDownloadController class that subclasses the webp implementation and performs the managed file validation check for any source extension webp supports. The routing can then check to see if the webp module exists and if it does use the new Webp class, and if not then fall back to the existing implementation
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | patch.diff | 2.72 KB | yorchperaza |
| #14 | drupal-remote_stream_wrapper-webp_compatibility-3210163-14.patch | 9.03 KB | kovalski.1298 |
| #9 | drupal-remote_stream_wrapper-webp_compatibility-3210163-9.patch | 7.61 KB | pstewart |
Issue fork remote_stream_wrapper-3210163
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
pstewart commentedHere's an attempt to implement this. This fixes the problem for me, my remote stream wrapped webp image derivatives now generate nicely with this patch, but would definitely benefit from some more extensive testing.
Comment #3
dave reidWould the patch in #3068898: Image styles setting extension cause access denied also maybe solve the this problem?
Comment #4
pstewart commentedIn this case we have to do a bit more work than #3068898 as both this module and the webp module subclass
ImageStyleDownloadController, so #2 addresses this by detecting if the webp module exists, and if so uses the newWebpRemoteImageStyleDownloadControllerclass instead, which subclasses the webp controller class in order to be aware of webp specifics and allow the webp implementation to do its work when calling into the parentdelivermethod. I think the patch will need a re-roll to align itself with the changes being made in #3068898 though.For reference I've been using the current patch in production for the past year without issue (first with D8.9 and currently with D9.3 and remote_stream_wrapper 1.5), so I think the approach is sound.
Comment #5
socialnicheguru commentedShould we check if modules like http://drupal.org/project/imageapi_optimize_webp are installed?
Comment #6
pstewart commentedIt looks like it's taking the same approach in overriding the controller, so I suspect 'yes' if we want to be compatible with that module too.
It looks like there's an open issue there for remote_stream_wrapper compatability in 2850373.I didn't spot that's an issue on the parent project rather than the webp processor specifically, so that's probably a different compatability concern.Comment #7
pstewart commentedI've written a new patch to accomplish the following goals:
* Add compatability with the wpf module (I'm currently switching over from webp to wpf on my site)
* Incorporate the fix from #3068898
* Move the managed file check and deliver logic into a trait to avoid duplication and make it simple to subclass the controllers from both wpf and wepb (and probably also imageapi_optimize_webp, but I've not implemented for that one in this patch).
As this patch essentially does the same work as https://www.drupal.org/project/remote_stream_wrapper/issues/3068898#comm... but implements it in a new trait, this patch also fixes #3068898 as an alternative to that patch.
Comment #8
pstewart commentedBumping issue to 2.x-dev as new patch is written for this branch
Comment #9
pstewart commentedSmall fix to #7 to ensure the patch correctly identifies the new files
Comment #10
agoradesign commentedThe patch from #9 does not work with webp module for me. I've tried both webp beta6 and beta5, but I'm always running into 404 errors. I've tracked that down that the new trait checks the wrong fallback path. It is assumed that an image named abc.jpg will have a webp derivated called "abc.jpg.webp", but webp module does not append the suffix, but rather replace it ("abc.webp"). This behaviour is different to imageapi_optimize_webp module, which does append the .webp suffix.
Patch #2 works for me :)
Comment #11
pstewart commentedAh, it looks like I was a bit over-eager in simplifiying the logic in the trait. However there is also an open webp issue 3281606 for changing behaviour there to the append approach, as the replace approach does seem to be more problematic. It looks like the patch there needs more work, but if that issue progresses then I think patch #9 wouldn't need any changes.
Comment #12
agoradesign commentedjust a vague idea - but what about switching to a plugin based architecture? the route callback then would not define the Controller hardcoded, but rather resolving the Controller to use, allowing other modules to integrate with (see the PriceResolverInterface in commerce). or maybe better swith to a single Controller and there we resolve the plugin to deliver the image. We could then easily factor out common code parts to leave them in the Controller and do the necessary work in the plugins
Comment #13
pstewart commentedThat's a nice idea - it would need some upstream effort in the core image module I think, but could provide a better way for modules to do this kind of custom work without having to override the core ImageStyleDownloadController.
Comment #14
kovalski.1298 commentedFixed the issue that prevented the previous patch from being applied.
Fixed the WebP file search in the database and ensured correct usage of the WebP module.
Comment #17
yorchperaza commentedThis patch is generate for the last code in remote_stream_wrapper-3210163 branch.
The patch adds WebP image style support to the Remote Stream Wrapper module and fixes several issues affecting remote image derivatives.
When the webp module is enabled, the module now correctly generates and serves WebP derivatives for remote images while preserving managed file security checks.
Changes
Comment #18
nagy.balint commentedWith the core webp support, and the latest dev version of the module, it might work without any additional code needed.