Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Image styles weren't generated with flysystem.
Route normalizer redirects flysystem 'flysystem.' . $scheme . '.style_public'
see
// Public image route.
$routes['flysystem.' . $scheme . '.style_public'] = new Route(
'/' . $settings['config']['root'] . '/styles/{image_style}/' . $scheme,
[
'_controller' => 'Drupal\image\Controller\ImageStyleDownloadController::deliver',
'scheme' => $scheme,
],
[
'_access' => 'TRUE',
]
);
Proposed resolution
exclude flysystem routes
Remaining tasks
-
User interface changes
-
API changes
-
Data model changes
-
Comment | File | Size | Author |
---|---|---|---|
#20 | redirect-normalize-opt-out-2854462-20.patch | 8.38 KB | amatzies |
| |||
#18 | redirect-normalize-opt-out-2854462-18.patch | 8.32 KB | amatzies |
#13 | redirect-normalize-opt-out_2854462-13.patch | 637 bytes | Boobaa |
#6 | redirect-normalize-opt-out_2854462_6.patch | 1.84 KB | bceyssens |
#4 | redirect-normalize-opt-out.patch | 1.72 KB | twistor |
Comments
Comment #2
BerdirWhat if there is a third module that happens to not follow that naming convention? I also didn't test private files yet, they might be affected too?
What if we add configuration to exclude urls from normalization, with wildcard, and defaults that can be customized per project? Or we check for something specific on the route or so, so other modules/routes can opt in?
Core plans to add this without UI configurable options.
Comment #3
slasher13I think this option (route nomalitzation) was designed for a global scope and other modules should have the possibility to opt out.
Comment #4
twistor CreditAttribution: twistor as a volunteer commentedSeems like you already have an option to opt out.
This is not tested, just an idea.
Comment #5
twistor CreditAttribution: twistor as a volunteer commentedWhy does this break image style generation in the first place?
Comment #6
bceyssens@Berdir, I can confirm this breaks downloading private files as well. Extended @twistor's patch while awaiting for a more scalable solution.
Comment #7
BerdirLooks like #2856279: Add possibility to disable redirect for a specific request basically implements the same with a new property, who wants to close that as duplicate? :)
Comment #8
twistor CreditAttribution: twistor as a volunteer commentedI would, but I'm not entirely sure what happens in RouteNormalizerRequestSubscriber vs RedirectChecker. My guess is the first handles trailing slashes and whatnot, whereas the other handles stored redirects. Although, #2856279 seems to state otherwise.
Comment #9
BerdirRedirectChecker is used for both. I don't think explicit redirects need to have such an opt-out, so using the existing one in this normalizer seems fine to me.
Comment #11
BerdirJust closed a number of duplicates, this is definitely a bug and a pretty critical one.
I committed this for now, we definitely need at least more tests. Maybe there's also a better and more generic way to identify those kind of file routes.
Comment #12
twistor CreditAttribution: twistor as a volunteer commentedI doubt there's a better way to identify them. Seems like opt in is the only reasonable solution.
I didn't check, but this should also be well documented.
Comment #13
BoobaaMarking it as major since redirect-8.x-1.x redirects the browser from perfectly valid image style derivatives of private images to an obviously invalid path.
Attached patch solves this by disabling the route normalizer for private images, too.
Comment #14
flocondetoileAfter upgrading to 8.x-1.x-alpha5, all the images set with a private file system were broken. Whereas the img src was valid, a redirect was performed to an invalid path.
For example (and for reference, it took me some hours to find this issue, because I didn't upgrade only redirect), my valid path was
/system/files/styles/logo_partner/private/partner/2017/04/my_image.png?itok=0nLrnRCW
and the redirect point to this form of path
/system/files/styles/logo_partner/private?itok=0nLrnRCW
Patch #13 fix this issue
Comment #16
BerdirIt's better in such cases to open a new issue, I didn't really see the patch here. It's still open but a patch was already committed.
Committed that patch now.
Comment #17
benjy CreditAttribution: benjy commentedWould be good to tag a new release for this issue, caught me out for a little while.
Comment #18
amatzies CreditAttribution: amatzies commentedI added a unit test for the route normalizer. It does not cover the new RouteSubscriber class added by bceyssens, though. To test this code, a functional test case seems to be more appropriate.
Comment #20
amatzies CreditAttribution: amatzies commentedAdded redirect group tag.
Comment #21
amatzies CreditAttribution: amatzies commentedComment #24
BerdirI prefer using KillSwitch::class, shorter and you don't have to worry about typos.
In my personal opinion, the test uses a bit too many helper methods with boolean arguments and indirection to set up the mocks, if you just look at a single test method it's quite hard to understand what's being tested. The comments help, and the downside of not doing that is usually a lot of code, which can also be hard to read.
But fine for a first step, we'll see how it goes when we change things.
Comment #26
progzy CreditAttribution: progzy commentedThe problem occurs with a custom stream wrapper as well. Implementing the same fix than #13 https://www.drupal.org/project/redirect/issues/2854462#comment-12047347 fixes it.