Route normalizer: Global Redirect in core

Created on 27 December 2015, over 9 years ago
Updated 27 October 2023, over 1 year ago

Problem/Motivation

Originally, some work was started in πŸ› Browser language detection is not cache aware Needs work 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 πŸ› Browser language detection is not cache aware Needs work
- 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

✨ Feature request
Status

Needs work

Version

11.0 πŸ”₯

Component
Request processingΒ  β†’

Last updated 3 days ago

No maintainer
Created by

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡©πŸ‡°Denmark ressa Copenhagen

    Getting Redirect β†’ and ✨ Pathauto in Core Needs work would be awesome, as well as Token β†’ (is there an issue for this?), since these three modules are probably installed in most Drupal sites.

  • πŸ‡³πŸ‡±Netherlands spokje
  • πŸ‡³πŸ‡±Netherlands spokje
  • First commit to issue fork.
  • Merge request !10285Add two redirect subscribers. β†’ (Open) created by amateescu
  • πŸ‡·πŸ‡΄Romania amateescu

    Re #170.1:

    I think we need to check whether this causes a regression for those requests (which should be the majority of requests on most sites).

    Yes, this definitely causes a regression, see this issue in the Redirect module: πŸ› Feature or Bug? - The path-alias preloading is not used if the "Enforce clean and canonical URLs" option is enabled Active

    I've started a MR with a fresh approach based on the suggestions from #171: let πŸ› Browser language detection is not cache aware Needs work deal with language detection, and only handle front page and path alias redirects in this issue.

    Setting to NR for architectural review, please don't change it to NW because of missing test coverage :)

  • Pipeline finished with Failed
    5 months ago
    Total: 4648s
    #345676
  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    > That would leave this issue dealing with the front page and aliases only

    Might not be quite as simple. \Drupal\redirect\EventSubscriber\RouteNormalizerRequestSubscriber::onKernelRequestRedirect() specifically also deals with index.php in the URL and other unclean URLs such as upper/lowercase and trailing /.

    I know @amateescu explicitly requested to not set it to needs work on test coverage, but I'd suggest going through the test coverage that redirect module has on this and decide which of those things core wants to support (\Drupal\Tests\redirect\Functional\GlobalRedirectTest)

    Also, it has a number additional checks that it runs in RequestChecker::canRedirect(), such as it being a request that's safe to redirect (not a POST request, for example as well as some other things like destination and maintenance mode, although I'm not sure how necessary they really are.

    Should also double check that this doesn't run in case of URL's result in an access denied. There's test coverage for that too in redirect, although we don't actually checking anything about that, as the commented out test around that proves. The setting is still useful, but it's only used for access on the target page for regular redirects.

  • Status changed to Needs review 2 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Should turn to a trait be added to remaining task.

  • Pipeline finished with Failed
    14 days ago
    Total: 542s
    #461651
  • πŸ‡¬πŸ‡§United Kingdom catch

    The more limited approach looks good to me, however there are performance test failures - do we need to add a static cache to the path alias lookup? Or if the path alias has already been resolved shouldn't we able to determine that somehow without looking it up again?

Production build 0.71.5 2024