Problem/Motivation

Originally, some work was started in #2430335: Browser language detection is not cache aware to resolve a caching issue. A language redirect was introduced, but the implementation was more general than just adding the language prefix. The target redirect URL was constructed from current route. This caused some test fails (especially, in case if this redirect is enabled always even if the language module is not installed, see 2430335#89). And this issue was created.

Current implementation

There is a new route_normalizer_request_subscriber service that perform redirects like the Global Redirect module. On a GET request, it construct a URL from the current route and the current route/query parameters, and redirects to that URL if it does not match the incoming URL. This causes:
- language redirect, for example from "/" to "/en", this resolves #2430335: Browser language detection is not cache aware
- path alias redirect, for example from "node/1" to "content/my-node"
- front page redirect, for example from "/node" to "/"
There can be more cases, but they are unknown at the moment.

Also, the patch contains some fixes for bugs caught during development, and some documentation changes.

Arguments for this feature:
- globalredirect has a lot of installations, people might need this feature in the core
- better SEO
- less cache entries
- it allows to catch bugs at early development stage (check the fixes provided in the patch)

Remaining tasks

- fix remaining test fails
+ write tests
- try to use CacheableRedirectResponse

User interface changes

The default Drupal installation will behave like the one having the Global Redirect module enabled.

API changes

The "disable_route_normalizer" request attribute can be set to disable the route normalization:

$request->attributes->set('disable_route_normalizer', TRUE);

Data model changes

none

Files: 

Comments

Leksat created an issue. See original summary.

Leksat’s picture

Status: Active » Needs review
FileSize
12.44 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2641118-2.patch, failed testing.

Leksat’s picture

Status: Needs work » Needs review
FileSize
12.21 KB

Moved back to system module.

Status: Needs review » Needs work

The last submitted patch, 4: 2641118-4.patch, failed testing.

Leksat’s picture

Status: Needs work » Needs review
FileSize
211 bytes

Just wanna be sure that Drupal installer tests fail because of the patch.

Leksat’s picture

Only consider clean URLs.

Status: Needs review » Needs work

The last submitted patch, 7: 2641118-7.patch, failed testing.

Leksat’s picture

Status: Needs work » Needs review
FileSize
12.43 KB

Restored http_build_query() in WebTestBase.

Status: Needs review » Needs work

The last submitted patch, 9: 2641118-9.patch, failed testing.

Leksat’s picture

Status: Needs work » Needs review
FileSize
12.43 KB

Try to always use <current> route.

Status: Needs review » Needs work

The last submitted patch, 11: 2641118-11.patch, failed testing.

Leksat’s picture

Status: Needs work » Needs review
FileSize
13.5 KB

Fix SimpleTestBrowserTest.

Status: Needs review » Needs work

The last submitted patch, 13: 2641118-13.patch, failed testing.

Leksat’s picture

Status: Needs work » Needs review
FileSize
14.12 KB

Fix BrowserTest.

Status: Needs review » Needs work

The last submitted patch, 15: 2641118-15.patch, failed testing.

Leksat’s picture

Status: Needs work » Needs review
FileSize
14.47 KB
606 bytes

Updated UrlGenerator to fix MenuRouterTest. Let's see if this will make other tests fail.

Status: Needs review » Needs work

The last submitted patch, 17: 2641118-17.patch, failed testing.

Leksat’s picture

Status: Needs work » Needs review
FileSize
14.89 KB
798 bytes

Okay, UrlGenerator worked absolutely correct. The real issue is that route parameters are double-decoded by the route-match code.

Status: Needs review » Needs work

The last submitted patch, 19: 2641118-19.patch, failed testing.

Leksat’s picture

Assigned: Leksat » Unassigned
Issue summary: View changes
Leksat’s picture

Issue summary: View changes
Leksat’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
16.17 KB

- fixed URL outbound processing in \Drupal\Core\Routing\UrlGenerator::processPath()
- disabled \Drupal\path\Tests\PathAliasTest::testPathCache()

Status: Needs review » Needs work

The last submitted patch, 23: 2641118-23.patch, failed testing.

Leksat’s picture

Status: Needs work » Needs review
FileSize
4.02 KB
18.92 KB

Okay, it seems that the previous approach is wrong. It looks like \Drupal\Core\PathProcessor\OutboundPathProcessorInterface::processOutbound() should process the URL-encoded path, but this is not stated in docs. At least \Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationContentEntity::processOutbound() sometimes adds a query string to the path, which is possible only on the already URL-encoded path.

Status: Needs review » Needs work

The last submitted patch, 25: 2641118-25.patch, failed testing.

Leksat’s picture

Status: Needs work » Needs review
FileSize
3.54 KB
18.98 KB

Introduced disable_route_normalizer request parameter and X-Drupal-Route-Normalizer response header.

Status: Needs review » Needs work

The last submitted patch, 27: 2641118-27.patch, failed testing.

Leksat’s picture

Some of these changes are nice-to-have but not required for this issue. Would be nice to get rid of them, but I don't remember which ones we need.

Leksat’s picture

Hmm, I thought all patches will go for testing...

Leksat’s picture

Leksat’s picture

What if I add another one right now... :)

Leksat’s picture

Leksat’s picture

Leksat’s picture

And the last one.

The last submitted patch, 31: 2641118-29-StatisticsReportsTest.patch, failed testing.

The last submitted patch, 32: 2641118-29-StatisticsTokenReplaceTest.patch, failed testing.

The last submitted patch, 33: 2641118-29-UrlGenerator-1.patch, failed testing.

The last submitted patch, 34: 2641118-29-UrlGenerator-2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 35: 2641118-29-UrlTest.patch, failed testing.

Leksat’s picture

Status: Needs review » Needs work

The last submitted patch, 41: 2641118-41.patch, failed testing.

Leksat’s picture

Status: Needs work » Needs review
FileSize
12.88 KB
15.88 KB

Moved the normalizer to the core. Updated docs/comments.

Status: Needs review » Needs work

The last submitted patch, 43: 2641118-43.patch, failed testing.

Leksat’s picture

Status: Needs work » Needs review
FileSize
847 bytes
16.71 KB

Fix book test.

Leksat’s picture

Issue summary: View changes
Status: Needs review » Needs work

Cool cool. Now we need tests.

Leksat’s picture

Assigned: Unassigned » Leksat

Already wrote half of test cases. Assigning to me for now.

Leksat’s picture

Assigned: Leksat » Unassigned
Status: Needs work » Needs review
FileSize
22.91 KB
4.18 KB
6.44 KB

Wrote tests. During the process I found some additional places in docs where it was not specified that path is URL-encoded.

The last submitted patch, 48: 2641118-48-test-only.patch, failed testing.

Leksat’s picture

Priority: Minor » Major
Issue summary: View changes

It works! Huh :)

Updated priority to major because this issue solves #2430335: Browser language detection is not cache aware

Leksat’s picture

Issue summary: View changes
almaudoh’s picture

Version: 8.0.x-dev » 8.1.x-dev

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

Leksat’s picture

Issue summary: View changes

An attempt to make the issue summary more readable :)

Leksat’s picture

Gábor Hojtsy’s picture

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?

Leksat’s picture

Gabor, thanks for the review!

1. These are not API changes. I just updated docs so it's clear that path is URL-encoded. That how these methods were working, I did not touched them.
I stated "URL-encoded path" because in other places "path" mostly means a decoded path.

2. Previous behavior of this piece of code: if $options['query'] is array, it is set to the empty array in the "else" condition, so the query is lost.

Leksat’s picture

Since this is a new feature, it should go to the new major version. I see the upgrade process like this:
- route normalizer can be enabled/disabled from the UI (maybe a new config page under admin/config/search ?)
- route normalizer is enabled by default on >= 8.1.x installations
- route normalizer is disabled by default on sites upgraded from 8.0.x

Any opinions?

Berdir’s picture

  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?

Leksat’s picture

FileSize
22.92 KB
808 bytes

@Berdir, thanks for the review!

1. I'm not sure whether we need such test in the current issue. Currently we have 4 redirect test cases: front page, path alias, language, and path special characters. The language-redirect part is already the biggest one. I would not make it even bigger.
But I tested https://github.com/md-systems/redirect/issues/77 case manually with different interface/content language negotiation configurations. It works well.

2. Fixed, thanks!

The last submitted patch, 48: 2641118-48-test-only.patch, failed testing.

Leksat’s picture

This patch adds the following behavior:
1. On new Drupal installations, the route normalizer is enabled by default.
2. If Drupal installation is upgraded (say from 8.0.x), the route normalizer is disabled (because route_normalizer.enabled config setting is NULL) to not change the existing routing behavior.
3. Route normalizer can be enabled/disabled via the config change (for example, using drush) or config override (for example, via settings.php).

I have a lot of questions about this:
1) Do we need this at all?
2) Is that okay that route_normalizer_request_subscriber service is always registered, and that route_normalizer.enabled config setting is checked in the onKernelRequestRedirect()? For example, language module uses service provider to register its services.
3) Is it required to have UI for route_normalizer.enabled config setting?

Crell’s picture

  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.

Berdir’s picture

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.

Leksat’s picture

@Crell, thanks for the review!

I'll go through all your points a bit later. For now, here is performance testing results.

Conditions: used ab -n500 -c1 at /user/password page, Drupal 80821af (8.x-1.x, 09.02.16) with standard profile, Redirect 34bee9a (8.x-1.x, 12.02.2016), dynamic_page_cache and page_cache are uninstalled.

No route normalizer (patch #61 applied, but the route normalizer service is commented out):
Requests per second: 2.32
Connection Times (ms) Total: 431 +/-sd 11.1

Route normalizer, no config (patch #59):
Requests per second: 2.31
Connection Times (ms) Total: 432 +/-sd 6.9
Difference: +0.23%

Route normalizer (patch #61):
Requests per second: 2.31
Connection Times (ms) Total: 432 +/-sd 11.7
Difference: +0.23%

No route normalizer, Redirect module (with default configuration):
Requests per second: 2.27
Connection Times (ms) Total: 441 +/-sd 5.8
Difference: +2.32%

Difference is calculated on Connection Times parameter.

(Would be great if someone can repeat testing because I did it for the first time)

Leksat’s picture

Reply to #62.

2.

I also considered using a container parameter. I see the following disadvantages of this:
- In my company we use service yml files only for storing environment related data (I think most developers do). While the global route normalizer killswitch should be something deployable in my opinion.
- We would have to add that parameter to sites/default/default.services.yml. I think it's not so important to go there.

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

My arguments for not having UI:
- I expect that people won't disable route normalizer usually.
- The global killswitch is only exists as a BC layer to not change behavior of already existing Drupal installations. If we don't need to keep the behavior, then we don't need the killswitch at all.
- I have no idea where to put this setting... All existing config pages seem unrelated. Adding a new page for a single option does not sound like a good idea.
- If we add UI, we'll add some description to the killswitch option. I can't think out a good one :)
- Only advanced users can have a need to disable the route normalizer.
- As it's pointed by @Berdir, a contrib module can provide a UI.

4.

I think I'm leaning yes, as there's no case in which a subsequent request would not want the same redirect. Right?

If the language negotiation is set to url+browser (I'd guess most of multilingual installations use that combination), and the incoming URL does not have the language prefix, this is the case.

5.

I checked this. It works even with "1". So I removed the priority and the comment to make the code simpler.

Crell’s picture

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.

Leksat’s picture

@Crell, when I considered the route_normalizer.enabled to be a container parameter, I thought that we can set it to FALSE in core/core.services.yml and set to TRUE in sites/default/default.services.yml so that existing installations won't have the route normalizer enabled. But somehow I missed this change: #2547447: Stop creating services.yml by default The sites/default/services.yml is not created by default on Drupal installation anymore. So container parameters cannot be used for upgrade-path purposes.

The only way to make it via files is to add something like $settings['route_normalizer_enabled'] = TRUE; to the sites/default/default.settings.php.
Did you mean this approach?
I see the following disadvantages:
- that would be a setting anyway
- default.settings.php should not be used for upgrade-path purposes IMO

Crell’s picture

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’s picture

Okay, got it :)
So, we implement the killswitch as a container parameter, enable the route normalizer for existing installations, publish a change record where we describe how do disable it, and that's it!
Will work on that as soon as I have time.

Leksat’s picture

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.

This is not related to the current issue, but maybe someone knows if the above is okay or not? In my opinion, service providers should get final container parameters (which include site-specific overrides). Tried to find an appropriate existing issue for this, but got nothing...

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Leksat’s picture

I currently work on a D8 project using language-domain negotiation. We need to test how RouteNormalizer behaves in this case because external redirects are forbidden by default.

Crell’s picture

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

dawehner’s picture

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.

Leksat’s picture

- replaced core.routing route_normalizer.enabled config parameter with the route_normalizer_enabled container parameter
- restored \Drupal\path\Tests\PathAliasTest::testPathCache()

Status: Needs review » Needs work

The last submitted patch, 75: 2641118-75.patch, failed testing.

Leksat’s picture

@dawehner,

Do we really want to enable it by default?

My opinion was "yes, but only for new installations". @Crell's opinion is "yes", see #68
Now I think that @Crell is right:

People are expecting new functionality in 8.1 (or 8.2), so this is new functionality.

Leksat’s picture

Status: Needs work » Needs review
FileSize
25.24 KB
1.25 KB

Fixed BigPipeTest.

Crell’s picture

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

Leksat’s picture

@Crell

  1. The \Drupal\Core\PathProcessor\OutboundPathProcessorInterface::processOutbound() is called in two places:
    - \Drupal\Core\Routing\UrlGenerator::processPath() there a URL-encoded path was passed
    - \Drupal\Core\Utility\UnroutedUrlAssembler::buildLocalUrl() there a decoded path was passed
    To fix test fails I had to make both methods to use the same approach. I chose URL-encoded way because I think it more fits with the Symfony style, where paths are mostly encoded.
  2. Here is the old code:
    if (isset($options['query']) && is_string($options['query'])) {
      $query = [];
      parse_str($options['query'], $query);
      $options['query'] = $query;
    }
    else {
      $options['query'] = [];
    }
    

    If $options['query'] is a non-empty array, it is emptied in the end.

  3. I'll try to migrate to BrowserTestBase as soon as I have time.

Now, profiling!

I used xhprof this time. The dump is attached in 2641118-78.xhprof.txt

I used the same conditions as in #64.
As we have only one new method, I looked at it only. First result showed 0.5% performance regression:

That was too much and I suspected that this is because generateFromRoute() and isFrontPage() are called for the first time in our onKernelRequestRedirect() and a lot of init stuff happens. So I added additional calls of generateFromRoute() and isFrontPage() to the the RouteNormalizerRequestSubscriber constructor, and here is what I got:

I think this is more real result.

If we assume that we have an additional service, I'd say that performance difference should be 0.1-0.2%, so nearly the same as in #64. The only expensive thing we do - we generate a URL from a route. (Is this really expensive? :) )

Wim Leers’s picture

Why did the BigPipe test fail?

Leksat’s picture

@Wim Leers
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.

Wim Leers’s picture

Title: Route normalizer » Route normalizer: Global Redirect in core

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

dawehner’s picture

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

Berdir’s picture

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.

dasjo’s picture

FileSize
25.32 KB

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

Leksat’s picture

Status: Needs review » Needs work

The last submitted patch, 87: 2641118-87-test-only.patch, failed testing.

The last submitted patch, 87: 2641118-87.patch, failed testing.

Leksat’s picture

Issue summary: View changes
Bambell’s picture

Status: Needs work » Needs review
FileSize
23.52 KB
895 bytes

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.

Status: Needs review » Needs work

The last submitted patch, 91: route_normalizer-2641118-91.patch, failed testing.

Leksat’s picture

@Bambell, I would not say that 301 (permanent) is more appropriate as 302 (temporary). For example, the language redirect may depend on user browser settings, so it is not permanent.

Leksat’s picture

Also, today I caught a circular redirect after upgrading Drupal 8.0 to 8.1. It was caused by route normalizer + page cache. Reported in #2761639: PageCache should not use $request->getUri()

Leksat’s picture

BTW, I just thought that Route Normalizer is not the best name. The service normalizes URLs, routes stay the same.
I would rename it to URL Normalizer.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

stella’s picture

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.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

SchnWalter’s picture

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.

stella’s picture

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

stella’s picture

Broken patch, rerolling

geerlingguy’s picture

Status: Needs work » Needs review
geerlingguy’s picture

Status: Needs review » Needs work

Gah, that was for 8.2.x, sorry!

piyuesh23’s picture

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

Status: Needs review » Needs work

The last submitted patch, 105: drupal-adding_route_normalizer-2641118-105-8.4.x.patch, failed testing.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
9.16 KB
23.11 KB

Re-work the re-roll.

Status: Needs review » Needs work

The last submitted patch, 107: 2641118-107.patch, failed testing.