Needs tests: Disable route normalizer for core routes.

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

Seems like you already have an option to opt out.

This is not tested, just an idea.

Why does this break image style generation in the first place?

@Berdir, I can confirm this breaks downloading private files as well. Extended @twistor's patch while awaiting for a more scalable solution.

Looks 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? :)

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

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

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

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

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

After 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

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

Would be good to tag a new release for this issue, caught me out for a little while.

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

Added redirect group tag.

+++ b/tests/src/Unit/RouteNormalizerRequestSubscriberTest.php
@@ -0,0 +1,225 @@
+    $kill_switch = $this->getMock('\Drupal\Core\PageCache\ResponsePolicy\KillSwitch');

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

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