- π©π°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.
- First commit to issue fork.
- π·π΄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 :)
- π¨π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 11:24pm 3 February 2025 - πΊπΈUnited States smustgrave
Should turn to a trait be added to remaining task.
- π¬π§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?