Redirects from aliased paths aren't triggered

Patch #142 can trigger the following error if $paths ends up as an empty array.

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near ') ORDER BY LENGTH(redirect_source__query) DESC' at line 1: SELECT rid FROM "redirect" WHERE hash IN () ORDER BY LENGTH(redirect_source__query) DESC; Array ( ) in Drupal\redirect\RedirectRepository->findRedirectByHashes() (line 140 of modules/contrib/redirect/src/RedirectRepository.php). 

This doesn't happen without this patch because $path is just an empty string, which generates hashes and everything works.

I can trigger this locally by hitting a URL like 127.0.0.1/%2f

This popped up in our logs from malicious users trying to do weird stuff on our site. The fix is easy (check for empty $paths in the event subscriber) but writing a test is proving to be more difficult than expected.

Yeah it doesn't seem possible to test this in a functional test because of UnroutedUrlAssembler::buildLocalUrl which ends up doing a rawurlencode

Also tidied up the other new tests, replacing the deprecated functions.

1+ year after comments #131-135, I partially changed my opinion, but only partially.

As said back then, some of the use cases people have seem to overlap with rabbit_hole, which is a better solution for having content that shouldn't be accessible on its own path.

But there are valid use cases for this. We get support requests quite frequently about this, when people archive old content for example and want to keep it but still redirect it to another page. There are basically two options to handle that right now, without this patch.

a) Remove the alias of the archived content, then create the redirect.
b) Set up the redirect on the system path of the archived content. Problem is that it's then tied to that, if you delete that in the future, the redirect is broken.

Neither option is perfect. For most cases that *I* see, a) is better, but its hard to understand and requires multiple steps.

First, some relevant feedback on this patch:

+++ b/redirect.module
@@ -99,9 +99,10 @@ function redirect_path_alias_update(PathAliasInterface $path_alias) {
-  // Create redirect from the old path alias to the new one.
   if ($original_path_alias->getAlias() != $path_alias->getAlias()) {
-    if (!redirect_repository()->findMatchingRedirect($original_path_alias->getAlias(), [], $original_path_alias->language()->getId())) {
+    $existing_redirect = redirect_repository()->findMatchingRedirect($original_path_alias->getAlias(), [], $original_path_alias->language()->getId());
+    if (!$existing_redirect) {
+      // Create redirect from the old path alias to the new one.
       $redirect = Redirect::create();
       $redirect->setSource($original_path_alias->getAlias());
       $redirect->setRedirect($path_alias->getPath());
@@ -109,6 +110,13 @@ function redirect_path_alias_update(PathAliasInterface $path_alias) {

@@ -109,6 +110,13 @@ function redirect_path_alias_update(PathAliasInterface $path_alias) {
       $redirect->setStatusCode($config->get('default_status_code'));
       $redirect->save();
     }
+    else {
+      // Duplicate the existing redirect with:
+      // source = [new alias], dest = [existing redirect].
+      $redirect = $existing_redirect->createDuplicate();
+      $redirect->setSource($path_alias->getAlias());
+      $redirect->save();
+    }
   }
 }

+++ b/tests/src/Functional/RedirectUITest.php
@@ -275,4 +274,63 @@ class RedirectUITest extends BrowserTestBase {
+
+  /**
+   * Test adding a node alias when a redirect already exists.
+   */
+  public function testNodeAliasOnExistingRedirect(): void {
+    $this->drupalLogin($this->adminUser);
+
+    $this->drupalGet('admin/config/search/redirect/add');
+    $this->submitForm([
+      'redirect_source[0][path]' => 'some-url',
+      'redirect_redirect[0][uri]' => '/node',
+    ], 'Save');
+
+    $this->assertRedirect('some-url', '/node', 301);
+
+    $this->drupalCreateNode([
+      'type' => 'article',
+      'title' => 'Test node redirect',
+      'path' => ['alias' => '/some-url'],
+    ]);
+
+    $this->assertRedirect('some-url', '/node', 301);
+  }

I don't think this behavior is desired and it's inconsistent. Above, we still have the line to delete redirects and that is important, when changing an alias through pathauto or manually, you do _not_ expect that an existing redirect will override it and it will mean your content is inaccessible. Redirects get set up automatically, and if there's real content again on a certain path, that has to show up again.

If anything, we should restrict this logic here to not remove redirects if updating an alias and the alias does not change and only then.
The use case that this is trying to solve also already "just works" technically if you set the request path to the system path and not the alias, which is very similar to how link fields on a path/alias vs. selecting an entity via autocomplete (aka an entity:TYPE/ID URI). But unlike the autocomplete behavior, it's hard to understand for users.

The test coverage here needs to be adjusted to cover that scenario then.

To summarize, there are 3 possible options on how to deal with a redirect on an existing alias, a and b above, plus what this patch is adding, to actually support that aka c. I still have serious concerns about the current patch and its effect on existing sites. Site might have a considerable amount of redirects on aliases that they do not know about and that are "invisible" right now. By adding this they would suddenly start to work and content might no longer be accessible. I'm not sure how to deal with that in a backwards-compatible way. A setting maybe?

I still think I would prefer to have a patch that improves the usability of those two existing options a and b, for example by giving users a choice to do either directly from the add redirect form. Additionally to the warning message we have right now, we would display a radios element where users can choose between automatically deleting that existing alias on save, and a description that re-creating said alias will delete the redirect again. Or they let the system change the from path to the system path, tying it to the entity behind it then, no matter which alias it currently has. That could be done in a separate issue, and this could then tie into that with a third option, if enabled, that allows them to save the redirect as-is for the alias. But in my opinion, with the first two existing options (and existing modules like rabbit_hole), we can already cover 90% of the use cases. But I'm interested in hearing what use case still needs explicit support of redirecting on aliases.

I know that this is awkward for people who rely on this patch and have data that builds on it. That is in the end a risk you always have when relying on patches that change data structures or affect how things work on stored data. But if we introduce this with an optional setting, you can just enable that, or migrate to option a or b, someone could write an update script/batch for that.

Reroll of #146

#149 isnt working for me

TypeError: Drupal\redirect\EventSubscriber\RedirectRequestSubscriber::__construct(): Argument #9 ($redirect_path_processor) must be of type Drupal\redirect\PathProcessor\RedirectPathProcessorManagerInterface, Drupal\Core\PathProcessor\PathProcessorManager given, called in /app/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 259 in Drupal\redirect\EventSubscriber\RedirectRequestSubscriber->__construct() (line 95 of modules/contrib/redirect/src/EventSubscriber/RedirectRequestSubscriber.php).

D9.5.11, php 8.1

FWIW #149 resolved some problems I had with path aliases on D10.0 not being created when user entities were modified. While I appreciate Berdir wants a more solid solution for the module's long term stability, #149 seems like a reasonable temporary solution for sites running into this problem.

FYI I should note that patch #149 did hit a minor issue:

• Have a user with an alias.
• Run this in terminal:
  • drush php
  • use \Drupal\user\Entity\User;
  • $u = User::load(USERID);
  • $u->set('field_stuff', 'something different');
  • $u->save();
• Load this admin page: admin/config/search/redirect
• Confirm that a redirect was created for the user entity.
• In the same terminal, still logged into "drush php", run the following:
  • $u->set('field_stuff', 'something else');
  • $u->save();
• Reload the redirect admin page.
• The new redirect will no be visible.
• Clear the caches.
• The new redirect will be visible.

The redirect admin page uses core's tag-baed caching. This suggests that the tags are not being invalidated correctly somewhere.

We came across this issue using Drupal 10.1.5 and Redirect 1.9.0.
I created a new redirect from /old-page to /new-page but when navigating to /old-page I still can view the page.
The only change on the page is the breadcrumb pointing to the new page.
I did clear the cache and even ran cron but the redirect is not triggered.
Applying the patch from #149 fixed the redirect but the URL redirect is not added to either node's edit form.

Hi everyone,

With:

• Drupal: 10.0.11
• Redirect: 8.x-1.9

The patch #149 works for me and the URL redirect is added to either node's edit form.

Hi all,

Sorry if this is cross pollinating with another issue but we had exactly this issue after migrating to D9 and updating to the new version of the Redirects module. It caused a lot of confusion with clients trying to manage their content as in D7 the alias redirects worked but not in D9 after the upgrade.

One way we solved this was to integrate not only showing the redirects to a node but the redirect away as well. This makes it easier for a user when editing the node to see it is being redirected to another URL. And this simplifies the creation of a redirect from an old node to a new one.

There is a patch here that some people may find useful https://www.drupal.org/project/redirect/issues/3386107 ✨ Simplify how to redirect from one node to another node or URL Active

Was going through this same problem, the module wasn't working with path aliases for the redirection. The patch #142 → works for me.
Thanks @Rajab Natshah

The patch #142 works for me also. Thanks

#142 is working for me also. Thanks

#142 🐛 Redirects from aliased paths aren't triggered Needs review and #149 🐛 Redirects from aliased paths aren't triggered Needs review could introduce a redirect loop if:

1. An alias is updated (ex: from /hi to /hello)
2. The alias is rollback to the last alias (ex: from /hello to /hi

Attaching patch without feature of // source = [new alias], dest = [existing redirect].

Re-attaching #146 patch without feature of // source = [new alias], dest = [existing redirect]. - correcting my own patch.

Let's get this work into the MR? Patches are dead now.

robert-arias → changed the visibility of the branch 2879648-redirect-aliased-paths to hidden.

✅ - opened MR with changes from #161.

rajab natshah → changed the visibility of the branch 2879648-redirects-from-aliased to hidden.

rajab natshah → changed the visibility of the branch 2879648-redirects-from-aliased to active.

Updated the MR
After Redirect 8.x-1.10 → was released

Attached a static redirect--2024-08-11--2879648--mr-109.patch file
to be used with composer patches

There are two test failures:

    Testing Drupal\Tests\redirect\Functional\RedirectUILanguageTest
    ........F.                                                        10 / 10
    (100%)
    
    Time: 01:03.892, Memory: 4.00 MB
    
    There was 1 failure:
    
    1)
    Drupal\Tests\redirect\Functional\RedirectUILanguageTest::testNodeAliasRedirects
    Failed asserting that 200 matches expected 301.
    
    /builds/issue/redirect-2879648/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:95
    /builds/issue/redirect-2879648/tests/src/Functional/AssertRedirectTrait.php:43
    /builds/issue/redirect-2879648/tests/src/Functional/RedirectUITest.php:329
    /builds/issue/redirect-2879648/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
    
    FAILURES!
    Tests: 10, Assertions: 169, Failures: 1.

and:

    Testing Drupal\Tests\redirect\Functional\RedirectUITest
    ....F.                                                              6 / 6
    (100%)
    
    Time: 00:37.410, Memory: 4.00 MB
    
    There was 1 failure:
    
    1) Drupal\Tests\redirect\Functional\RedirectUITest::testNodeAliasRedirects
    Failed asserting that 200 matches expected 301.
    
    /builds/issue/redirect-2879648/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:95
    /builds/issue/redirect-2879648/tests/src/Functional/AssertRedirectTrait.php:43
    /builds/issue/redirect-2879648/tests/src/Functional/RedirectUITest.php:329
    /builds/issue/redirect-2879648/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
    
    FAILURES!
    Tests: 6, Assertions: 98, Failures: 1.

Anyone want to try fixing these?

The 2 failures were from the 2 lines, essentially testing some functionality which I can not see implemented anywhere.

The tests were testing that the existing alias based redirect was updated when the node had its alias changed. I personally can't see any code that would add that functionality, and IMO is not necessary. The whole idea of redirecting from a node alias would mostly around redirecting away from stale content easily (by using the alias). If we want to add automatic updates to existing alias redirects then we could do that in a follow up, especially seeing how long this issue has taken to get resolved.

Therefore, I've removed the assertions.

Hi, I have applied patch #169 🐛 Redirects from aliased paths aren't triggered Needs review o Drupal 10.3.2 and Redirect 8.x-1.10, the redirection is working properly. However, I am getting an error after logout Drupal. Is anyone aware of this how this needs to be resolved?

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near ') ORDER BY LENGTH(redirect_source__query) DESC' at line 1: SELECT rid FROM "redirect" WHERE hash IN () ORDER BY LENGTH(redirect_source__query) DESC; Array ( ) in Drupal\redirect\RedirectRepository->findRedirectByHashes() (line 142 of /code/web/modules/contrib/redirect/src/RedirectRepository.php).

I've tested the patch #169 🐛 Redirects from aliased paths aren't triggered Needs review with Drupal 10.3.3 and Redirect 8.x-1.10, I see all redirections are working properly, even the existing ones that had no issues before applying the patch. Also I took a look to the dblog after applying the patch and tested several redirects. No issues found or error messages logged.

Hi, is patch #169 🐛 Redirects from aliased paths aren't triggered Needs review good to go? Greetings,

Hi, Patch #169 working good. greetings,

I've added the patch #169 with Drupal 10.3.10 and Redirect 8.x-1.10 ,
Getting below error. Anyone have any solution for this?
Issue : Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near ') ORDER BY LENGTH(redirect_source__query) DESC' at line 1: SELECT rid FROM "redirect" WHERE hash IN () ORDER BY LENGTH(redirect_source__query) DESC; Array ( ) in Drupal\redirect\RedirectRepository->findRedirectByHashes() (line 142 of /code/web/modules/contrib/redirect/src/RedirectRepository.php).

I've added the patch #169 and i have this error :

TypeError: array_diff_assoc(): Argument #2 must be of type array, null given in array_diff_assoc() (line 154 of modules/contrib/redirect/src/RedirectRepository.php).

$query does not exist in findRedirectByHashes function.

refreshing status considering the last 2 comments...

1. Noting that the error in #178 only occurs in conjunction with the in-progress enhancement 🐛 Redirects from aliased paths aren't triggered Needs review , it seems that accommodating that should be done as part of that enhancement, rather than this scope.
2. I'm unable to reproduce the problem described in #177. More information (steps to reproduce) would be needed.
3. The latest comment on the MR is addressed.

Based on the above, my own functional testing, and the passing automated tests, I'm setting this back to RTBC.

I have not yet reviewed the changes since I provided feedback in #147/#148 but my opinion on this has not changed. I understand that many people are interested in this and use this patch, but for everyone who does use the patch, there are literally thousands of sites out there who don't. Those sites might have inactive redirects for some reason that they do _not_ expect to work and with this, their content might suddenly become inaccessible.

At this point, I have no plans to merge this change.

I proposed an alternative solution in #148 that guides users through creating a valid redirect when creating them. I haven't managed to find time to get back to that, but I'd be much more open to merging that instead. Sites who rely on this patch will then need an update path to convert aliases to the system path.

At this point, I have no plans to merge this change.

Thanks for the clarification on the thinking, @berdir. In that case, I think it might be sensible to more clearly label this issue as something that will not be merged, and leave it going for people who want to patch the module, and direct folks to 📌 Document that URL redirects work from node/{nid} but not alias Needs work to complete that work. Also, based on the progress on that issue, it could probably be renamed to something like "Check for path aliases and guide users to use internal path"?