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

Command icon 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

pstewart created an issue. See original summary.

pstewart’s picture

Status: Active » Needs review
StatusFileSize
new4.34 KB

Here'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.

dave reid’s picture

Would the patch in #3068898: Image styles setting extension cause access denied also maybe solve the this problem?

pstewart’s picture

In 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 new WebpRemoteImageStyleDownloadController class 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 parent deliver method. 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.

socialnicheguru’s picture

Should we check if modules like http://drupal.org/project/imageapi_optimize_webp are installed?

pstewart’s picture

It 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.

pstewart’s picture

StatusFileSize
new7.7 KB

I'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.

pstewart’s picture

Version: 8.x-1.x-dev » 2.x-dev

Bumping issue to 2.x-dev as new patch is written for this branch

pstewart’s picture

StatusFileSize
new7.61 KB

Small fix to #7 to ensure the patch correctly identifies the new files

agoradesign’s picture

Status: Needs review » Needs work

The 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 :)

pstewart’s picture

Ah, 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.

agoradesign’s picture

just 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

pstewart’s picture

That'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.

kovalski.1298’s picture

Fixed 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.

ransomweaver made their first commit to this issue’s fork.

yorchperaza’s picture

StatusFileSize
new2.72 KB

This 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

  1. Added WebpRemoteImageStyleDownloadController to support WebP image styles.
  2. Fixed method signature mismatch in CheckManagedFileControllerTrait::deliver() that could cause fatal errors.
  3. Initialized the missing streamWrapperManager dependency.
  4. Removed duplicate isManagedFile() method.
  5. Allowed webp and gif extensions in managed file checks.
  6. Updated routing to automatically use the WebP controller when available.
nagy.balint’s picture

With the core webp support, and the latest dev version of the module, it might work without any additional code needed.