"Redirect from non-canonical URLs to the canonical URLs" not working with language code in url

There's an open pull request on the GIthub repository, which was used to develop Redirect until a few days ago.

I've proposed a working patch there. I didn't get committed so far because some code refactoring in that class should be done as well.

However, I'll propose it again here, so that people can already use it, until a cleaner solution is developed and finally committed.

I'll change status to needs review to have the testbots apply the patch...

Patch #2 works great for me too.

Not working for me. I solved changing the 253 line to :

$this->setResponse($event, Url::fromRoute(' < front > '));

basically i'm forcing redicts to home for all /nodes urls....the 252 line give me a good url object but with no alias info and so it redirects to the non canonical url.
There a way to gey an url object with alias info?

Patch #2 works great for me aswell!

As commented on the pull request, this duplicates code that we already have for manual redirects. We should use the same approach.

That, or even better, replace all the global redirect features with #2641118: Route normalizer: Global Redirect in core. Any takers for trying to add that to this module?

For posterity, linking to original Github issue and PR:

https://github.com/md-systems/redirect/issues/53
https://github.com/md-systems/redirect/pull/87

So, here's a patch going with the idea of replacing global redirect features with what is being done in Core. Tests are expected to pass. I copied Core's RouteNormalizerRequestSubscriber class, with the following changes :

- I added a check in shouldRedirect to allow redirects for HEAD requests (for the tests) and to preserve the functionality of ignoring admin paths.
- Core uses the default 302 redirect response, I changed this back to 301.

Parameter $route_normalizer_enabled is unused.

I removed code related to those features in RedirectRequestSubscriber, removed the settings that are no longer necessary (nonclean_to_clean, frontpage_redirect, deslash, normalize_aliases and term_path_handler), removed those from the settings form and added an update function to remove them from storage. I removed parts of the tests that were testing proper behavior if these settings are disabled.

Most importantly, Dynamic Page Cache is interfering a lot, so we disabled it in GlobalRedirectTest so that it would pass. When locally, manually testing (very quickly tested), alias normalization and front page redirect seems to work, but not deslashing. Needs a bit more investigation and likely a proper fix.

Great work so far.

1. +++ b/redirect.install
   @@ -148,3 +148,27 @@ function redirect_update_8103() {
   +  }
   +  if ($config->get('normalize_aliases')) {
   +    $config->clear('normalize_aliases');
   +  }
   

   You always want to clear those settings. They are either true or false, so get('setting') returns FALSE, but you still want to remove it. You could do a !== NULL check but I think clear() doesn't fail if a key doesn't exist, so should be fine.

2. +++ b/src/Tests/GlobalRedirectTest.php
   @@ -114,6 +114,11 @@ class GlobalRedirectTest extends WebTestBase {
        $this->term = $term;
   +
   +    \Drupal::service('module_installer')->uninstall([
   +      'dynamic_page_cache',
   +    ]);
   

   Lets try to get our subscriber to run after routing (32) but before dynamic page cache (27). that should result in the same behavior as having it disabled.

Also, can you cross-post those findings into the core issue. You could also already update that patch with the changes that we identified here. Might also not hurt to expand the core tests based on hours.

Lets try to get our subscriber to run after routing (32) but before dynamic page cache (27). that should result in the same behavior as having it disabled.

Yes, a priority in-between (30) works just fine.

+++ b/src/EventSubscriber/RouteNormalizerRequestSubscriber.php
@@ -0,0 +1,123 @@
+      && !\Drupal::config('redirect.settings')->get('ignore_admin_path');

This doesn't really check whether we are on an admin path or not right ? Or am I missing something ?

I can't grok this from the patch, so I'll just ask: is the method in this patch to replace/supplant the core service? If so, we should be decorating the service instead. One reason is, if we're just copying RouteNormalizerRequestSubscriber then we have to track that code as it gets updated. Another is, if two modules try to replace a service, one of them will fail, so preferably all modules would be decorating to avoid this situation.

More information on decorating services can be found here:

http://symfony.com/doc/current/components/dependency_injection/advanced....

Decorating had been broken, per #2650812: Container::has() doesn't work for service aliases (consequently, decorating services doesn't work), but it looks like it should work now.

> is the method in this patch to replace/supplant the core service?

No. This is *based* on a not yet committed core patch, this does not yet exist in core.

The idea is to provide the same implementation as core does, which will a) Allow us to just remove it when it lands in core and b) allow us to test the core implementation, as shown above, we already found a bunch of bugs (or things that we consider to be bugs).

This doesn't really check whether we are on an admin path or not right ? Or am I missing something ?

You are absolutely right, nice catch, thanks. It changes / clutters up shouldRedirect quite a bit, unfortunately.

Hum... This should fix the failing tests.

1. +++ b/config/schema/redirect.schema.yml
   @@ -19,30 +19,15 @@ redirect.settings:
        access_check:
          type: boolean
          label: 'Menu Access Checking'
   

   is this still used? there are some tests but I don't see it in the changes, does it only affect manual redirects?

2. +++ b/redirect.install
   @@ -148,3 +148,17 @@ function redirect_update_8103() {
   +
   +/**
   + * Removes unnecessary settings from storage.
   + * @see https://www.drupal.org/node/2704213
   + */
   +function redirect_update_8104() {
   

   no need for the @see I think.

3. +++ b/src/EventSubscriber/RouteNormalizerRequestSubscriber.php
   @@ -0,0 +1,129 @@
   +        && !$request->query->has('destination')
   

   interesting, does this come from core? why would having a destination matter to redirecting or not?

4. +++ b/src/Tests/GlobalRedirectTest.php
   @@ -174,8 +157,14 @@ class GlobalRedirectTest extends WebTestBase {
    
   +    // Test alias normalization again with ignore_admin_path false.
   +    $this->assertRedirect('Test-node', 'test-node');
   +
        $this->config->set('ignore_admin_path', TRUE)->save();
        $this->assertRedirect('admin/config/system/site-information', NULL, 'HTTP/1.1 200 OK');
   +
   +    // Test alias normalization again with ignore_admin_path true.
   +    $this->assertRedirect('Test-node', 'test-node');
   

   considering that core does not and will not have this feature, I'd say lets drop it too.

Ah,and one more thing that we need here is test coverage for the specific bug that was reported here.

Actually, this module doesn't work AT ALL in a multilingual site with a subdomain (such as dev.mysite.com) where the default language also has a language code (such as "/fr")

All 301 redirections that I have properly created end up in a 404. No matter the parameters selected. No matter if a redirection language was specified or not. I tried both 8.x-1.x-dev and 8.x-1.0-alpha1.

Should I open a new thread of is this one sufficiently similar?

My use case is a D7 site with paths such as /blog/160813 that will soon be replaced by a D8 site with paths such as /blog/seo-friendly-url. Thanks.

Patch works for me.
I have a bilingual website, where I use /en as prefix for English and no prefix for Dutch (default language).

Dutch node:
/node/123 > /news/title-of-news-item WORKS

English node:
/en/node/456 > /en/news/title-of-news-item WORKS

English node:
/node/456 > /en/news/title-of-news-item DOES NOT WORK (but never worked with Global Redirect as well)

+1

I found out that image styles aren't generated anymore after applying the patch. This should definitely be fixed before this patch can be set to RTBC.

Also, it breaks the non-clean redirects (index.php/.../) so that part isn't covered by the patch it seems.

I can confirm, that patch #19 breaks image styles.

With the patch applied i get:

$ curl -I http://d8.local/sites/default/files/styles/medium/public/2016-09/generateImage_1DktpA.jpg?itok=MJNKrOg7
...
HTTP/1.1 301 Moved Permanently
...
Location: http://d8.local/sites/default/files/styles/medium/public?itok=MJNKrOg7&file=2016-09/generateImage_1DktpA.jpg

I worked on this a bit and addressed the following topics:

• Fix redirects for nonclean urls
• Write a test for the fix tests for nonclean urls
• Fix broken image style urls (Seems to work but this must be done better ...)
• Write a test for the original purpose of this issue

The failing test was caused due to the subfolder where drupal is checked out on the testbots. Thx @Berdir for the hint. Hope they will pass now. There is still work to do but i hope that maybe the test will help us to push this issue forward.

For a site that supports two languages, en & es, I've applied the patch from #31. My configuration for redirect activates all options, except for 'Allow redirections on admin paths.'. I'm running 1.0.0-alpha1. English is the default language.

es/node/7 => Does not redirect a.k.a. global redirect
node/7 => Does redirect

Patch #31 works fine for me. Redirections on admin path works fine for me.

I applied the patch #31 and it works fine on admin paths.

This is not fixed.

Patch #31 works fine for me.

1. +++ b/redirect.install
   @@ -148,3 +148,17 @@ function redirect_update_8103() {
   +/**
   + * Removes unnecessary settings from storage.
   + * @see https://www.drupal.org/node/2704213
   + */
   +function redirect_update_8104() {
   

   No need for a @see to the issue. This likely also look weird in drush/UI

2. +++ b/redirect.services.yml
   @@ -1,3 +1,5 @@
   +parameters:
   +  route_normalizer_enabled: true
   

   I'm not sure I understand why we need the parameter in the module.

   Core adds it for backwards compatibility and we have our settings, so if we do want to make it configurable, we should just add a config key.

3. +++ b/src/EventSubscriber/RouteNormalizerRequestSubscriber.php
   @@ -0,0 +1,140 @@
   +   * @return bool
   +   */
   +  protected function shouldRedirect(GetResponseEvent $event) {
   +    if ($request = $event->getRequest()) {
   

   missing return description.

   Can't we pass in the request object here?

   And can't we use the redirect.checker service for this?

4. +++ b/src/Tests/GlobalRedirectTest.php
   @@ -174,8 +177,49 @@ class GlobalRedirectTest extends WebTestBase {
    
   +    // Test alias normalization again with ignore_admin_path false.
   +    $this->assertRedirect('Test-node', 'test-node');
   +
        $this->config->set('ignore_admin_path', TRUE)->save();
        $this->assertRedirect('admin/config/system/site-information', NULL, 'HTTP/1.1 200 OK');
   +
   +    // Test alias normalization again with ignore_admin_path true.
   +    $this->assertRedirect('Test-node', 'test-node');
   +  }
   

   Don't get this, we are testing something with the setting on/off but do the same check in both cases?

   If it doesn't work anymore we either need to fix it or remove the setting if core won't do this either.

5. +++ b/src/Tests/GlobalRedirectTest.php
   @@ -174,8 +177,49 @@ class GlobalRedirectTest extends WebTestBase {
   +
   +    $this->drupalGet('es/node/' . $spanish_node->id() . '/edit');
   +
   

   what is this drupalGet() for?

@heddn: Can you check what's different with your scenario compared to what we have in the test?

re: #38

I cannot reproduce the problem I was seeing in #32. https://www.mtech-llc.com/es/node/35 now redirects correctly => https://www.mtech-llc.com/es/blog/charlotte-leon/migracion-de-datos-csv-... when the patch is applied.

Thank you for your great feedback @Berdir
I rerolled #31 to apply against latest HEAD, before starting working on your feedback.

Working on a single-language, but non-English site. I had the same problem: /node/1 to /my-first-page redirect didn't work (tried just about every checkbox on the admin screen). Applied the patch from #40 to 8.x-1.0-alpha1 (with some offsets), cleared the caches, then this redirect started to work as expected. One side note: I do NOT have any language codes in the URL (as it would be frustrating for a single-language site).

Rebased and resolved the conflict on top of the patch in #1559310: 404 pages should be language aware that will be committed this night.

This patch covers basically the feedback from #37 by @Berdir.

1. RouteNormalizerRequestSubscriber uses redirect.checker service now (plus small adaptions to that service)
2. @see is removed from install hook
3. route_normalizer_enabled was moved to configuration (plus configuration form checkbox)
4. Fixes to tests

we are testing something with the setting on/off but do the same check in both cases?

This makes sense to me. We check if the "normal route normalizer" is still working even if we switch ignore_admin_path on/off. The actual tests are there:

    // Test ignoring admin paths.
    $this->config->set('ignore_admin_path', FALSE)->save();
    $this->assertRedirect('admin/config/system/site-information', 'site-info');
    
    $this->config->set('ignore_admin_path', TRUE)->save();
    $this->assertRedirect('admin/config/system/site-information', NULL, 'HTTP/1.1 200 OK');

Here is an impression of the form:

1. +++ b/src/RedirectChecker.php
   @@ -84,6 +84,12 @@ class RedirectChecker {
   +    elseif ($request->get('_route') == 'image.style_public') {
   +      $can_redirect = FALSE;
   +    }
   

   At least manual redirects happen before routing AFAIK, so I guess this check won't work there, but that's probably fine, we don't want to prevent manual redirects there. We might even want to keep it in the normalizer to make that explicit?

2. +++ b/tests/src/Unit/RedirectCheckerTest.php
   @@ -138,6 +138,7 @@ class RedirectCheckerTest extends UnitTestCase {
          ->with($this->anything())
          ->will($this->returnValue($method == 'GET'));
   +    $request->query = new ParameterBag([]);
        $request->attributes = new ParameterBag($attributes);
   

   can we also add some asserts for the two new conditions in there?

Thank you @Berdir for the reviews, i really appreciate that. I tried to put all your feedback in this patch and focused on testing.

One more thing we might have to address is the checkbox we see in the attached screenshot in #44. The text "Enable the route normalizer" does not explain what that actually means. This checkbox enables/disables the whole "Global redirect" functionality and has an effect on all the following form elements in this box. Should we address that here, or create a follow up?

Thanks for working on this. It's a big and pretty complicated issue, but I do think it is one of the few things that we should do before a beta release and once we've seen sufficient testing of this, we can possibly a) release stable version soon-ish and try to re-activate the core issue this code is based on.

1. +++ b/config/schema/redirect.schema.yml
   @@ -19,33 +19,24 @@ redirect.settings:
        trailing_zero:
          type: integer
          label: 'Remove Trailing Zero Argument'
   ...
        term_path_handler:
          type: boolean
          label: 'Taxonomy Term Path Handler'
   

   Looks like those two don't actually exist anymore except in schema, so lets remove them.

2. +++ b/config/schema/redirect.schema.yml
   @@ -19,33 +19,24 @@ redirect.settings:
        content_location_header:
          type: boolean
          label: 'Set Content Location Header'
   

   looks like this setting doesn't do anything, it exits in schema, we move it around, we have a UI.. but there is no implementation for it.

   We should open a separate issue to either remove it or implement it.

3. +++ b/redirect.install
   @@ -178,3 +178,16 @@ function redirect_update_8104() {
   +function redirect_update_8106() {
   +  $config = \Drupal::configFactory()->getEditable('redirect.settings');
   +  $config->clear('term_path_handler');
   

   We need to explicitly set the new config in the update function, or it won't work for existing sites. I'm not sure if we should build in logic to disable it if "it" was disabled before.. maybe if all options were off? Or if normalize was off, which is the most common thing?

4. +++ b/src/EventSubscriber/RouteNormalizerRequestSubscriber.php
   @@ -0,0 +1,133 @@
   + * Not every but most of GET requests are processed. All conditions can be found
   + * in shouldRedirect() method.
   + *
   

   this needs to be updated or just removed, assuming we have documentation in RedirectChecker.

5. +++ b/src/Form/RedirectSettingsForm.php
   @@ -55,28 +55,16 @@ class RedirectSettingsForm extends ConfigFormBase {
   +      '#title' => $this->t('Enable the route normalizer.'),
   +      '#default_value' => $config->get('route_normalizer_enabled'),
   

   Yes, I think normalizer is too technical for the UI. Even though we actually used that before as well Maybe:

   Enforce clean and canonical URLs

   Description: Enabling this will automatically redirect to the canonical URL of any page. That includes redirecting to an alias if existing, removing trainling slashes, ensure the language prefix is set and similar clean-up.

Using the patch in #48 on a multilingual site. It mostly works as expected except for homepage URLs with an arbitrary query string.

For example, our client has legacy URLs like http://example.com?foo which trigger a popup/modal via JS.

With the 'Enable the route normalizer' setting on, http://example.com?foo redirects to http://example.com/?foo= ie. slash and equal sign added.

But these types of links on the Italian homepage e.g. http://example.com/it?foo lose the query string and redirect back to http://example.com/it.

#49.1 Removed.
#49.2 #2846931: Follow-up for #2704213 Implement or remove content_location_header.
#49.3 Done. I enabled the route_normalizer_enabled option when the option normalize_aliases is enabled.
#49.4 shouldRedirect method is no longer present since #44 so I removed it.
#49.5 Done.

Edit: There already was a follow-up issue #2841123: Remove or implement Content Location Header

And the interdiff.

The patch #48 seems working great, thanks!

I had a problem with utf8 chars in aliases. For example Chinese chars and apostrophe cause endless redirect for me.
/zh-hans/blog/中国消费品销售总额
/en/blog/rigidity-tajikistan’s-banking-system

I added urldecode() function to normalize request URI and redirect URI.

+++ b/src/EventSubscriber/RouteNormalizerRequestSubscriber.php
@@ -0,0 +1,132 @@
+        $response = new RedirectResponse($redirect_uri, 301);

I think this should take the redirect settings for the response code into account.

That setting is currently only used for the default value for new redirect entities, the existing "global redirect" features also all had 301 hardcoded.

I'm not sure if it makes sense to respect that default actually, maybe discuss that in a follow-up, this is complicated enough already.

This is a big and complicated change, is anyone still seeing any problems with this patch applied that weren't there before? with image styles or anything like that.

here is a patch and interdiff for #55

edit: sorry @Berdir, posted the patch without looking again.

Regarding #57:
We use the patch on 3 sites in production without problems.

But recently i discovered a problem with the following configuration:

- language negotiation Browser language before url
- Request to the front page without query string will be redirected correctly to the browser language with a 301 redirect.
- Varnish caches the 301 redirect.
- The site is on Acquia with no possibility to change the varnish config to not cache 301 redirects.

-> language negotiation on browser language is cached what you do not want.

- A 302 redirect would not have been cached on Acquia varnish.
- 301 is not really the best code if the redirect depends on the browser language because the redirect target changes. 302 would be more correct.

Interesting use case. Problem is that tons of other redirections would be better with 301, *especially* manually created redirects which will now all use 302 by default. (assuming we even correctly implement this, not sure right now).

Which is why I'm not convinced that it is a good thing to use the same configuration for this, we could have configuration but it should possibly be a different config key.

But fine, lets add this for now, then we can open a new issue to split it up.

#2430335: Browser language detection is not cache aware is also related, although it is mostly about when you *don't* use this module. It's also where the core issue originated from.

Thanks everyone. Lets get this in and see if anyone reports any issues.

Guys,

I have got a critical issue with version alpha5 of this module and when I am logged in as admin, redirect is not working and I can not switch between languages but when I do log out, it starts working. Does anyone know why it is happening!? My drupal version is 8.3.5 and redirect version is alpha5 latest one.

Regards,
MahPari

You probably enabled user language negotation, don't do that if you don't want that.

Redirect doesn't change your language, it just redirects you do the language that Drupal selected.

Thanks Berdir, your solution solved my problem.

Drupal 8.4.0
Redirect 8.x-1.0-beta1

Szenario:
Node A = DE & FR
Node B = DE

User navigate:

1. Current site language = DE
2. Result: Node A (DE) = de/node-a-de-title
3. change site language to FR
4. Result: Node A (FR) = fr/node-a-fr-title
5. click on Node B (menu link)
6. Node B (DE) = /fr/node/123

summary:
No redirect from /fr/node/123. But with /node/123 the redirect to /de/node-b-de-title is working.

Who can help me with this?

never mind. I am opening a separate issue for this.