Route normalizer: Global Redirect in core

should be in 8.1.x since it's a feature request.

Leksat asked me to review on IRC. Just reviewing for API changes for now:

1. +++ b/core/lib/Drupal/Core/PathProcessor/OutboundPathProcessorInterface.php
   @@ -19,7 +19,7 @@
   -   *   The path to process, with a leading slash.
   +   *   The URL-encoded path to process, with a leading slash.
       * @param array $options
       *   (optional) An associative array of additional options, with the following
       *   elements:
   @@ -48,7 +48,7 @@
   
   @@ -48,7 +48,7 @@
       *   (optional) Object to collect path processors' bubbleable metadata.
       *
       * @return string
   -   *   The processed path.
   +   *   The processed URL-encoded path.
   
   +++ b/core/lib/Drupal/Core/Url.php
   @@ -772,7 +772,7 @@ public function toRenderArray() {
   -   *   The internal path for this route.
   +   *   The internal URL-encoded path for this route.
   

   Why are these API changes needed.

2. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
   @@ -123,10 +123,12 @@ public function processOutbound($path, &$options = [], Request $request = NULL,
   -      if (isset($options['query']) && is_string($options['query'])) {
   -        $query = [];
   -        parse_str($options['query'], $query);
   -        $options['query'] = $query;
   +      if (isset($options['query'])) {
   +        if (is_string($options['query'])) {
   +          $query = [];
   +          parse_str($options['query'], $query);
   +          $options['query'] = $query;
   +        }
          }
   

   Why was this change necessary?

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RouteNormalizerRequestSubscriber.php
   @@ -0,0 +1,106 @@
   +      $redirect_uri = $this->urlGenerator->generateFromRoute($route_name, [], $options);
   +      $original_uri = $request->getSchemeAndHttpHost() . $request->getRequestUri();
   +      if ($redirect_uri != $original_uri) {
   +        $response = new RedirectResponse($redirect_uri);
   

   This is an interesting approach. Wondering if we should also change the code in redirect.module to something like this if that covers all the cases there.

   Would make it easier to remove the functionality there in case this gets into core.

   One thing is that there are a lot of settings in redirect/globalredirect. Not sure if they're really needed if things work reliably.

   There's a bug report open there right now about a special configuration with content language negotation being different that results in a loop. Might make sense to test that here as well.

2. +++ b/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php
   @@ -114,6 +114,9 @@ protected function buildLocalUrl($uri, array $options = [], $collect_bubbleable_
   +    $uri = UrlHelper::encodePath($uri);
   
   +++ b/core/modules/book/src/Tests/BookTest.php
   @@ -435,6 +435,10 @@ function testBookNavigationBlock() {
   +    // It may happen that user is redirected to the front page during the
   +    // logout, so the front page may be already cached and we did no action to
   +    // clear the cache so far. Do it now.
   +    drupal_flush_all_caches();
   

   should probably only do an invalidate of the rendered cache tag?

1. +++ b/core/lib/Drupal/Core/EventSubscriber/RouteNormalizerRequestSubscriber.php
   @@ -0,0 +1,120 @@
   +   * - A route that set as the front page is requested: redirect to the front
   

   that's set, not that set.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/RouteNormalizerRequestSubscriber.php
   @@ -0,0 +1,120 @@
   +    if (!$this->config->get('core.routing')->get('route_normalizer.enabled')) {
   +      return;
   +    }
   

   If the killswitch is a config object, then IMO a UI to enable/disable it is a must.

   If there's no UI, then it should NOT use the config system but simply be a container parameter, set via the site's services.yml.

   I can see a good argument for either approach, personally, but it should be one or the other.

3. +++ b/core/lib/Drupal/Core/EventSubscriber/RouteNormalizerRequestSubscriber.php
   @@ -0,0 +1,120 @@
   +    $can_redirect = $event->isMasterRequest()
   +      && $request->isMethod('GET')
   +      && !$request->query->has('destination')
   +      && RequestHelper::isCleanUrl($request)
   +      && !$request->attributes->get('disable_route_normalizer');
   +    if ($can_redirect) {
   

   IMO, this should be factored out to a protected shouldRedirect() method on this class, for cleanliness.

4. +++ b/core/lib/Drupal/Core/EventSubscriber/RouteNormalizerRequestSubscriber.php
   @@ -0,0 +1,120 @@
   +      $redirect_uri = $this->urlGenerator->generateFromRoute($route_name, [], $options);
   +      $original_uri = $request->getSchemeAndHttpHost() . $request->getRequestUri();
   +      if ($redirect_uri != $original_uri) {
   +        $response = new RedirectResponse($redirect_uri);
   +        $response->headers->set('X-Drupal-Route-Normalizer', 1);
   +        $event->setResponse($response);
   +      }
   

   I believe direct use of the generator is discouraged at this point, as it bypasses caching metadata. Which of course begs the question of whether this should e a cacheable redirect or not. I think I'm leaning yes, as there's no case in which a subsequent request would not want the same redirect. Right?

5. +++ b/core/lib/Drupal/Core/EventSubscriber/RouteNormalizerRequestSubscriber.php
   @@ -0,0 +1,120 @@
   +    // Execute after routes are initialized in
   +    // \Drupal\Core\Routing\RoutePreloader::onRequest().
   +    $events[KernelEvents::REQUEST][] = array('onKernelRequestRedirect', -1);
   

   -1 would be after routing has happened too.

I am overall +1 on the concept, although the implementation has me a bit concerned about performance. This adds another route generation call and event listenener to every request. It's also very aggressive; if anything might possibly have changed the URL, redirect. Should it be opt-in, rather than opt-out?

Also, even if we keep the kill switch on the request attributes it should be _-prefixed.

Berdir: I still think that redirect module would be better served by creating routes that all map to a single redirection controller rather than using a subscriber, so I don't know that the approach here would translate at all.

We have lots of options in configuration without a UI and for example let contrib provide a UI for those that need it, so I don't agree with that argument. However, performance might be an argument. Loading a config is quite a bit of overhead, a container parameter is practically free.

However, not sure what to do about the upgrade path. The patch currently disables that option for existing sites, I don't see how we could do that with a container parameter?

About redirect module. Redirect 8.x-1.x merged in all the features from globalredirect, which isn't about specific redirects but about things like redirecting to the canonical path, removing trailing slashes and a few others. Which is exactly what this is doing.

For the manual redirects, using the router table is an interesting idea that's worth trying out, however, there are quite a few things to consider:
* There might be (tens of) thousands of redirects. Every cache clear or redirect change would have to write them all. I'm fairly sure that's a show-stopper.
* Redirects have features like being language-specific, can vary by query arguments, override actual routes. I guess we would have to use route filtering for that.

I tried my best to optimize it as much possible. We're doing a single, simple db query against just a hash + language. The only thing is that we have to run inbound processing too for language stuff. I'd love reviews on improving that and maybe finding a way to not run that multiple times. See https://github.com/md-systems/redirect/blob/8.x-1.x/src/EventSubscriber/.... Note that we have to run before routing, as the routing could e.g. abort with an access denied exception.

The site-level services.yml file already has a lot non-environmental stuff in it, for better or worse. Drupal has never really had a clear environment switch, as we couldn't agree on what buttons and dials should change when. While that may be a convention in some companies it's not how Drupal is built currently.

If this is something only advanced users will be messing with, then moving it to a container parameter still makes the most sense to me.

Well, then let me ask this: We're assuming so far that the upgrade behavior MUST be that this service is not enabled. But... is that really necessary? This is an SEO improvement (presumably), and fixes a few bugs, and is low-level enough that most people likely won't know/care, so... What is wrong with having it enabled by default, even on upgrades? People are expecting new functionality in 8.1 (or 8.2, as it looks like this probably won't make the 8.1 cutoff), so this is new functionality.

That also then allows the kill-switch to be a container parameter without issue.

Leksat: #70 sounds like a bug to me, for reasons like... well, this issue. :-) Can you file a bug issue for that? I hope it's an easy fix...

Just some super quick feedback.

@Leksat
Please use xhprof for actual benchmarking. Its way easier to see the actual difference with it. AB/apache adds quite some noise to the equation.

Meh.. I wanted to use \Drupal\Core\CoreServiceProvider to dynamically register route_normalizer_request_subscriber service depending on the container killswitch parameter. But then I found that both core and module service providers have no access to site-specific yaml files: http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/DrupalKernel...
So, I'll have to leave the service provider registration in the core/core.services.yml file.

Well, at the moment this is kinda by design, because the idea is that site specific yaml files can override at the final stage ... anway not part of the issue

1. +++ b/core/config/install/core.routing.yml
   @@ -0,0 +1,2 @@
   +route_normalizer:
   +  enabled: true
   

   Do we really want to enable it by default?

2. +++ b/core/config/schema/core.routing.schema.yml
   @@ -0,0 +1,11 @@
   +core.routing:
   +  type: config_object
   +  label: 'Routing system settings'
   +  mapping:
   +    route_normalizer:
   +      type: mapping
   +      label: 'Route normalizer settings'
   +      mapping:
   +        enabled:
   +          type: boolean
   +          label: 'Enabled'
   

   We don't expose this as UI option at the moment, so I'm wondering whether a container parameter is actually the better option.

3. +++ b/core/lib/Drupal/Core/EventSubscriber/RouteNormalizerRequestSubscriber.php
   @@ -0,0 +1,130 @@
   +    return $event->isMasterRequest()
   +      && ($request = $event->getRequest())
   +      && $request->isMethod('GET')
   +      && !$request->query->has('destination')
   +      && RequestHelper::isCleanUrl($request)
   +      && !$request->attributes->get('_disable_route_normalizer');
   

   This needs a bit of elaboration why this is covering all this cases.

Two small points, otherwise the code seems fine to me pending benchmarks:

1. +++ b/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php
   @@ -114,6 +114,9 @@ protected function buildLocalUrl($uri, array $options = [], $collect_bubbleable_
   +    // The path should be URl-encoded before possible path processing.
   +    $uri = UrlHelper::encodePath($uri);
   

   Hm. This is a subtle but rather important change, and maybe a small API break if someone has code that messes with the URI. Why is this needed?

2. +++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php
   @@ -123,10 +123,12 @@ public function processOutbound($path, &$options = [], Request $request = NULL,
   -      if (isset($options['query']) && is_string($options['query'])) {
   -        $query = [];
   -        parse_str($options['query'], $query);
   -        $options['query'] = $query;
   +      if (isset($options['query'])) {
   +        if (is_string($options['query'])) {
   +          $query = [];
   +          parse_str($options['query'], $query);
   +          $options['query'] = $query;
   +        }
   

   I do not understand this change. Needs a comment, maybe?

3. +++ b/core/modules/system/src/Tests/Routing/RouteNormalizerTest.php
   @@ -0,0 +1,104 @@
   +class RouteNormalizerTest extends WebTestBase {
   

   BrowserTestBase works now. Why aren't we using it? For new tests, if it works, let's use it. (Or does it not work in this case for some reason?)

Why did the BigPipe test fail?

Thanks for the clarification, I had forgotten that that is what this issue is about. Clarified issue title.

Could we leverage the CacheableRedirectResponse here to apply this redirect much faster?

It should yes.

I still think, given that this didn't make it into 8.1.0, that it would be useful to refactor the implementation redirect.module to use this approach. a) It will allow us to test this much more easily in the wild. Redirect is just an alpha, so having something break there is acceptable, much more so than core. In fact, there are currently known bugs with multilingual sites that this could help address.

It would also be much easier to just drop the implementation in redirect module once this lands in core.

Attached is the same patch as in #78 but against 8.1.1

We are working in contributed Redirect module to integrate RouteNormalizerRequestSubscriber (#2704213: "Redirect from non-canonical URLs to the canonical URLs" not working with language code in url). I have re-rolled this patch 2 days ago. Uploading it here to see if it still applies and to see what tests still fail. Will try to work on that. We also noted that the default RedirectResponse's 302 response code is used, although 301 would be more appropriate, so we changed that.

Attached is the patch from comment #91 above, but with a 302 rather than 301 redirect, as I agree, yes it makes more sense given that this isn't a permanent redirect based on the browser's language settings.

I haven't made any other changes here.

A bug has been discovered against the redirect.module route normalizer which also affects this patch. See #2852680: Redirect loop when serving cached pages to anonymous users

The redirect loop is caused by the fact that the URI string returned by "Request::getRequestUri()" can include characters that don't have any effect on the HTTP request but they are present in the original string and this string is compared to an URI generated from a route object.

+      if ($redirect_uri != $original_uri) {
+        $response = new RedirectResponse($redirect_uri);
+        $response->headers->set('X-Drupal-Route-Normalizer', 1);
+        $event->setResponse($response);
+      }

For example these URIs are different but the HTTP requests are equal:

• https://www.example.com/test? vs https://www.example.com/test
• https://www.example.com/test?&amp=true vs https://www.example.com/test?amp=true

The RouteNormalizerRequestSubscriber uses \Symfony\Component\HttpFoundation\Request::getRequestUri() and then \Symfony\Component\HttpFoundation\Request::prepareRequestUri() which will return different results depending on the server setup.

In the patch I've submitted for the redirect.module, I've used parse_url() and GuzzleHttp\Psr7\Uri::fromParts() to process the URLs before comparing them but I'm not sure if that is the best approach.

Patch reroll against latest 8.2.x in case anyone needs it.

Broken patch, rerolling

Gah, that was for 8.2.x, sorry!

Updated & re-rolled the patch to apply on 8.4.x stream.

Re-work the re-roll.

Drupal\file\Tests\DownloadTest is probably also all about the fact that path aliases don't support query parameters. The test ContentNegotiationRoutingTest at least was.

Once this lands, I'll update the changes made to the CDN module in #2924916: Compatibility with contrib Redirect module: disable language redirects for CDN's "farfuture" route, because they'll no longer be necessary for "just a contrib module", but for Drupal core itself.

Here is just a reroll, I couldn't figure out though how to fix the failures.

It is, but that was always meant to be temporary until added to core. There are unresolved caching issues in core without this, core can't just say it only works correctly with a contrib module :)

Retesting #113, it still applies cleanly. On its original November 29 test run, it had 21 failures. Let's see if that number has changed.

@Leksat:

Reason: the result of Drupal\Core\Path\PathMatcher::isFrontPage() is statically cached (so it contains the result of the primary request to /graphql).

Oh! Nice catch :) This definitely is an independent bug that we can reproduce and fix. It's easy enough to write test coverage: create a controller that does subrequests to two paths A and B, with A being the frontpage and B being anything else. Allow the parameter to the controller to determine the subrequest order: either A,B or B,A. In case of A,B it should result in B erroneously being considered the frontpage, and in case of B,A it should result in A erroneously NOT being considered the frontpage. That failing test should allow you to remove the static caching of PathMatcher::isFrontPage(). Care to open an issue? :)

drive by review, i see some changes around /user/login in the patch and #81 mentions

Because it used /user/login path for testing. This path is set as front page in the minimal installation, so route normalizer performed a redirect. BigPipe test checked response headers, that's why it failed.

BrowserTestBase also uses /user/login to login and the first failure I saw was triggered in BrowserTestBase::drupalLogin() so I'd wager something is up there.

Patching #113 on 8.6 produce few hunk with offset

patching file core/core.services.yml
Hunk #2 succeeded at 860 (offset 37 lines).
patching file core/lib/Drupal/Core/EventSubscriber/RouteNormalizerRequestSubscriber.php
patching file core/lib/Drupal/Core/PathProcessor/OutboundPathProcessorInterface.php
patching file core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php
patching file core/lib/Drupal/Core/Routing/UrlGenerator.php
patching file core/lib/Drupal/Core/Routing/UrlGeneratorInterface.php
patching file core/lib/Drupal/Core/Routing/UrlMatcher.php
patching file core/lib/Drupal/Core/Url.php
patching file core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php
Hunk #1 succeeded at 115 (offset 6 lines).
Hunk #2 succeeded at 159 (offset 6 lines).
patching file core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
patching file core/modules/book/tests/src/Functional/BookTest.php
patching file core/modules/image/src/PathProcessor/PathProcessorImageStyles.php
Hunk #1 succeeded at 68 (offset 3 lines).
patching file core/modules/language/tests/src/Functional/LanguageListTest.php
patching file core/modules/path/tests/src/Functional/PathAliasTest.php
Hunk #1 succeeded at 32 (offset -1 lines).
patching file core/modules/simpletest/src/Tests/BrowserTest.php
patching file core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
patching file core/tests/Drupal/FunctionalTests/Routing/RouteNormalizerTest.php
patching file core/tests/Drupal/KernelTests/Core/Routing/ContentNegotiationRoutingTest.php

ReRoll I guess... no changes because its just offset warnings.

refactored for 8.6.2

Changing the status of this issue to "needs review". This will set of the testbot to do it's work.

Patch #113 reworked for Drupal Core 8.5.8

Forgot to add the newly created files, retry.

#126 needs reroll for 8.7

patching file core/lib/Drupal/Core/EventSubscriber/RouteNormalizerRequestSubscriber.php
patching file core/lib/Drupal/Core/PathProcessor/OutboundPathProcessorInterface.php
patching file core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php
patching file core/lib/Drupal/Core/Routing/UrlGenerator.php
patching file core/lib/Drupal/Core/Routing/UrlGeneratorInterface.php
patching file core/lib/Drupal/Core/Routing/UrlMatcher.php
patching file core/lib/Drupal/Core/Url.php
patching file core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php
Hunk #1 succeeded at 114 (offset -1 lines).
Hunk #2 succeeded at 158 (offset -1 lines).
patching file core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
patching file core/modules/book/tests/src/Functional/BookTest.php
patching file core/modules/image/src/PathProcessor/PathProcessorImageStyles.php
patching file core/modules/language/tests/src/Functional/LanguageListTest.php
Hunk #1 FAILED at 149.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/language/tests/src/Functional/LanguageListTest.php.rej
patching file core/modules/path/tests/src/Functional/PathAliasTest.php
patching file core/modules/simpletest/src/Tests/BrowserTest.php
Hunk #1 succeeded at 40 (offset 2 lines).
patching file core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
Hunk #1 succeeded at 56 with fuzz 1.
patching file core/tests/Drupal/FunctionalTests/Routing/RouteNormalizerTest.php
patching file core/tests/Drupal/KernelTests/Core/Routing/ContentNegotiationRoutingTest.php
patching file sites/default/default.services.yml

#130 needs also a reroll for 8.7

patching file core/core.services.yml
Hunk #2 succeeded at 865 (offset 39 lines).
patching file core/lib/Drupal/Core/EventSubscriber/RouteNormalizerRequestSubscriber.php
patching file core/lib/Drupal/Core/PathProcessor/OutboundPathProcessorInterface.php
patching file core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php
patching file core/lib/Drupal/Core/Routing/UrlGenerator.php
patching file core/lib/Drupal/Core/Routing/UrlGeneratorInterface.php
patching file core/lib/Drupal/Core/Routing/UrlMatcher.php
patching file core/lib/Drupal/Core/Url.php
patching file core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php
Hunk #1 succeeded at 114 (offset -1 lines).
Hunk #2 succeeded at 158 (offset -1 lines).
patching file core/modules/big_pipe/tests/src/Functional/BigPipeTest.php
patching file core/modules/book/tests/src/Functional/BookTest.php
patching file core/modules/image/src/PathProcessor/PathProcessorImageStyles.php
patching file core/modules/language/tests/src/Functional/LanguageListTest.php
Hunk #1 FAILED at 149.
1 out of 1 hunk FAILED -- saving rejects to file core/modules/language/tests/src/Functional/LanguageListTest.php.rej
patching file core/modules/path/tests/src/Functional/PathAliasTest.php
Hunk #1 succeeded at 33 (offset -1 lines).
patching file core/modules/simpletest/src/Tests/BrowserTest.php
Hunk #1 succeeded at 40 (offset 2 lines).
patching file core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
Hunk #1 succeeded at 56 with fuzz 1.
patching file core/tests/Drupal/FunctionalTests/Routing/RouteNormalizerTest.php
patching file core/tests/Drupal/KernelTests/Core/Routing/ContentNegotiationRoutingTest.php
patching file sites/default/default.services.yml

Patch rerolled

I've noticed on the Frontpage View, the Pager links having ../?destination=/&_exception_statuscode=404&page=1 in the URL where as normally they will be ../?page=1

Clearing the cache they revert back to normal. Does this patch address this and if so, any idea when it will be pushed to Core?

Rerolled for 8.9.x

Let's see what the testbot thinks.

Does this address the issue the Frontpage View, the Pager links having ../?destination=/&_exception_statuscode=404&page=1 in the URL where as normally they will be ../?page=1

Clearing the cache will revert URLs back to normal. Has anyone noticed this or are the issues unrelated?

Reroll of #139 against D8.8.5 (core/core.services.yml has an offset of 6 lines)

Reroll of #139 against D8.8.5 (core/core.services.yml has an offset of 6 lines)

Previous patch was incomplete.

#144 doesn't apply on 9.1.0

patching file core/core.services.yml
Hunk #1 succeeded at 38 (offset 5 lines).
Hunk #2 FAILED at 879.
1 out of 2 hunks FAILED -- saving rejects to file core/core.services.yml.rej
patching file core/lib/Drupal/Core/EventSubscriber/RouteNormalizerRequestSubscriber.php
patching file core/lib/Drupal/Core/PathProcessor/OutboundPathProcessorInterface.php
can't find file to patch at input line 186
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php b/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php
|index bae2028f3..af4fa4162 100644
|--- a/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php
|+++ b/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php
--------------------------
File to patch: 

Rerolls