Redirects from aliased paths aren't triggered

Thanks, I think there's an existing issue about this already as well.

Not sure about having that logic in preSave(), but lets see.

This will need tests, though.

#2821158: Document that URL redirects work from node/{nid} but not alias is the related issue.

For #4.

I ran into this issue as well, and here is an alternative patch where the path check is done in the generateHash function. I'm not sure if this has a performance impact or not, but it seems better to handle it there.

#9 worked for me, thank you!

It also applied cleanly for me... Wonder why the testbot couldn't do it.

I don't think that is the right place. That means we still store the alias, and while the hash might match, the actual stored source path will not, which in my opinion is confusing. We should do this on the user input and store the de-aliased source path.

for #2 i tried to use Dpendency Injection for Path Alias Manager Service but somehow it is throwing following error

Fatal error: Declaration of Drupal\redirect\Entity\Redirect::create() must be compatible with Drupal\Core\Entity\EntityInterface::create(array $values = Array) in /var/www/html/drupal/modules/redirect/src/Entity/Redirect.php on line 0

Can someone please let me know what could be the cause of this ?

Entity classes don't support dependency injection. Best you can do is add a method that wraps the Drupal::service() call.

Using #9 fixed the issue but if the URL path changes in a content type (i.e. changing a title with path auto enabled) it is not reflected back to the redirect and presents as a 503 error "Service unavailable" for the redirect to old URL and using the new path URL.

If you go into the redirect of that item and save it, the redirect and the new path work again.

Lots of side effects here..

If the user enters the alias source path and then we store the unaliased path, we have a dependency to the alias.
Deleting the alias (without awareness about the redirect) will break the redirect.
This is confusing with the mindset that obsolete aliases convert to redirect. People will expect that they can delete the alias if a redirect exists. We would need better UI integration like warning a user when editing / deleting an alias that a redirect depends to it.

Storing the alias source path and matching on the hash of the unaliased path could be OK as long as invalidation works. If the alias is deleted, we need to update the matching to the source path.

Currently a redirect is deleted if an alias with the same path is created. IMHO this should only happen if the target is the same. If such a redirect would still exist when an alias is created, we need to check if a redirect needs update. If the target is different i would consider the source path as reserved.

On the opposite - the user should be warned if trying to create a redirect for an URL with an alias. In most cases this is an accidental clash.

(More variations could happen with varying language settings - some for all languages - some only for a specific language.)

Hi @ameymudras - #2 patch is not able to apply with the redirect module version 1.3

Regarding redirect not working for URL alias, please find the patch for redirect module with version 8.x-1.3 and do the review.

Hi,
in our current project we faced the same problem.
Patch #18 could solve the problem for us

Thank you

Patch #18 is reworked, reformatted and rerolled.

Language parameter is added to path alias processing.

Hi maximpodorov,

Just a heads up as I tried patch #23 today and it didn't work. The redirect module still ignored any redirects created when using an alias.

@g.weston are you aware that this needs a resave of each related alias for the patch to take effect?

I find that the patch does not work when there are multiple redirects from a path, because the patch creates them the same hash from any alias for a node.

Example:

my-node and my-node-with-a-new-alias will have the same hash. The redirect from the old to the new alias is created automatically by Redirect.
If I visit my-node or my-node-with-a-new-alias I get a redirect loop because they both have the same hash
If I try to update the alias again there's an integrity constraint violation because the hash is already in the database.

Updated the #23 patch to support und langcode. Path alias manager was returning false results when langcode und was passed to it.

Hi! Patch #29 works for me after I re-generated all URL aliases.

It also worked if I instead of regenerating URL aliases, I re-saved the specific redirect after applying the patch.

This needs work for tests and coding standards (spaces, double quotes), it also shouldn't hardcode 'und' but use the constant for it from LanguageInterface.

I've added also the case when broken URL alias already exists, but leads to infinite redirect lopop

This is just a re-roll of 33 to clean up the coding standards, will take a look at why the tests are failing today.

Adding this to get it tested, should have resolved one of the failing tests, expecting some failures.

The major change here is to get RedirectRepository::findMatchingRedirect(); to check if the path is an alias and if it is, drop in another hash to look for, one that matches the alias's source (multiple hashes are already searched for, so this seems like a good place to put it).

At that point, we can switch things like searching for duplicates in RedirectForm::validateForm(); to use the repository rather than using `Drupal::entityManager` and having to check for an alias on the fly there.

Although I haven't looked at it yet, I'm guessing we'll still need a way to re-save all redirects to update hashes. On some sites, this is going to be a fairly large process (current site I'm working on has 4,000+). Not sure if we can handle this in an update hook or not, something I still need to look at.

@Feng-Shui well, we have batch API for that, node.module and taxonomy.module have performed schema updates that way.

Updated patch to resolve the last failing test, also includes an update hook to re-save all redirects to update their hashes.

Previous patch had an issue with the update hook.

Add test to cover off the ability create a redirect from a path's alias and then find the redirect using the repository by searching for the path's source. Removing the Needs tests tag for now.

Going to leave this here now until it gets a review.

How about checking the relative url besides just the node/id? I had an instance of redirect not working because there was an unpublished node with the same url, and it was grabbing that node id instead of the actual url.

I think some input from @Berdir as to the preferred approach would be good, comments in #32 suggested the patch in #40 was the direction to move in, but the above would work this in that specific instance. Displaying the patch in #40 again, it covers off the comments in #32.

I think the patch in #40 looks good, 2 code style comments though:

+++ b/src/Entity/Redirect.php
@@ -85,9 +86,17 @@ class Redirect extends ContentEntityBase {
    */
...
+    $path = ltrim(\Drupal::service('path.alias_manager')->getPathByAlias('/' . $path, $redirect_language), '/');

would it make sense to store path.alias_manager in a (static?) class variable?

+++ b/src/RedirectRepository.php
@@ -69,6 +70,15 @@ class RedirectRepository {
+    $alias_source = ltrim(\Drupal::service('path.alias_manager')->getPathByAlias('/' . $source_path, $alias_language), '/');

I suggest to inject the service here, instead of using \Drupal call

@valthebald thanks for taking a look at the patch.

In both cases I've used \Drupal (and helper classes such as Language:: over injection as that seems to be inline with the code style of the rest of the module. Happy to swap these out for service injection though if the maintainer would prefer.

I've got a colleague taking a look at this too tomorrow, we're going to run the update function on our site with the 3k+ redirects and make sure that works as expected.

Also just going to hide a few of the previous patches here.

Fixed two issues:

1: redirect count in install hook was off by one which meant that if there was only one redirect the update hook wouldn't run
2: when checking for an alias in the preSave hook, check for an alias using the redirect's language and "not specified" (Language::LANGCODE_NOT_SPECIFIED) - this was preventing some redirects from getting updated hashes.

Following some additional testing by a colleague, an additional alias language check needs to be done int he preSave function. Need to check for aliases using the redirect's language, language not specified and then the path systems default language value - this isn't overly elegant, but there's no way to determine the correct language to use to try and find aliases.

1. +++ b/redirect.install
   @@ -222,3 +222,46 @@ function redirect_update_8108() {
   +
   +/**
   + * Updates redirect hashes.
   + */
   +function redirect_update_8109(&$sandbox) {
   +  // Configure the batch on first pass. First check that we actually have
   +  // redirects in the database, if we don't stop right here.
   +  if (!isset($sandbox['progress'])) {
   +    $redirect_count = Database::getConnection()
   +      ->query('SELECT COUNT(rid) FROM {redirect}')
   +      ->fetchField();
   +
   +    if (empty($redirect_count)) {
   +      return t('There were no redirects to update.');
   +    }
   +
   +    $sandbox['progress'] = 0;
   +    $sandbox['current_rid'] = 0;
   +    $sandbox['max'] = $redirect_count;
   +  }
   +
   +  // Get redirects in groups of 20 to keeps this process within resource limits.
   +  $redirect_ids = Database::getConnection()
   +    ->select('redirect', 'r')
   +    ->fields('r', ['rid'])
   +    ->condition('rid', $sandbox['current_rid'], '>')
   +    ->range(0, 20)
   +    ->orderBy('rid', 'ASC')
   +    ->execute()
   +    ->fetchCol();
   +
   +  // Save each redirect to regenerate its hash.
   +  foreach (Redirect::loadMultiple($redirect_ids) as $redirect) {
   +    $redirect->save();
   +    $sandbox['progress']++;
   +    $sandbox['current_rid'] = $redirect->id();
   +  }
   

   I'm concerned a bit about this update function.

   I know there are sites out there with tens/hundreds of thousands of redirects and probably more. I'm testing it on a site with 34k, the update takes 3m30s , which isn't that long, but I'

   There are some things we should do. One, this should be a post update function, because we're using the entity api. Then, instead of saving all redirects, we could check if the new resave logic actually makes a difference, because for 99% of the existing redirects, it shouldn't. Last, instead of the hardcoded 20 limit, we can use Settings::get('entity_update_batch_size', 50), especially considering we won't resave most of the redirects. Large sites with a lot of resources can set that higher to process updates more quickly.

   The alternative would be to not include a forced update at all, I would be OK with that I think. But I think with those changes, it should be acceptable also on very large sites.

2. +++ b/tests/src/Kernel/RedirectAPITest.php
   @@ -159,6 +159,21 @@ class RedirectAPITest extends KernelTestBase {
   +    $redirect->save();
   +    $found = $repository->findMatchingRedirect('/node');
   +    if (!empty($found)) {
   +      $this->assertEqual($redirect->getHash(), $found->getHash());
   +    }
   +    else {
   +      $this->fail('Failed to find a redirect to a path alias by searching for the path source.');
   +    }
   

   As a phpunit test, it will stop on the first failed assertion, so I'd just write this as two assertions, first assertInstanceOf, and then that it is the same redirect.

Changes as requested.

Patch #49 did not fix the issue in my case.

I don't think the post_update applied correctly, as the hash stored in the redirects table did not change, and is still generated using the alias originally input.

Looking at the preSave, I think there is a mistake in the conditional regarding language - if ($path === $this->redirect_source->path) and elseif ($path === $this->redirect_source->path). Is this supposed to be a separate if statement?
---
In my case, $redirect_language is 'und', however getPathByAlias returns the input value when using either $redirect_language or Language::LANGCODE_NOT_SPECIFIED. It correctly returns the node path when no language parameter is provided, therefore the final assignation would have worked out.

$path = ltrim($path_alias_manager->getPathByAlias('/' . $path, $redirect_language), '/');
if ($path === $this->redirect_source->path) {
  $path = ltrim($path_alias_manager->getPathByAlias('/' . $path, Language::LANGCODE_NOT_SPECIFIED), '/');
}
elseif ($path === $this->redirect_source->path) {
  $path = ltrim($path_alias_manager->getPathByAlias('/' . $path), '/');
}

Thanks

Thanks for testing.

The conditional is correct. The issue here is that there's no way to determine what language may resolve the alias to a source. So while it's a bit ugly, we check for the three options we have. Firstly using the redirect's language, and if that doesn't change the path (ie it didn't resolve the alias to a source) we check with LANGCODE_NOT_SPECIFIED and if that doesn't change the path, we check with no language (which uses the path system's default language which is NULL).

Based on what you've said, if the first two returned the original path passed to getPathByAlias() unmodified then the elseif should have been satisfied and you should have had the alias resolved to a source on the third go. Do you know why that didn't happen, sounds like it should have?

Hm, both conditionals check for the exact same thing $path === $this->redirect_source->path, so the else if statement will never be hit, even on multiple iterations.

Ahhh, my bad you're saying that the elseif should be an if. Yes, that's correct and that's prob why your hashes didn't get updated, I must have made a typo when I created the above patch, see if this one works.

Several of my colleagues and I have been very confused by this behavior, especially because the Drupal 7 version of the module does not behave like this. In Drupal 7, when you add a redirect with a source of an existing path alias, the redirect module automatically converts it to the system path on save. In Drupal 8, that does not happen (thus the reason for this issue). I think some of the points brought up in this issue are valid, but I wanted to express this is a big WTF at the moment :(

Looks like an additional cache clear is needed in the failing test.

Please review the patch.

I don't have the time to keep working on this issue right at the moment, we're using a workaround of removing aliases that we want to redirect. I think this patch is pretty close (not sure why the latest one didn't apply, it did locally). Would be good if someone else can pick this up for a while, I think it's pretty close.

#56 patch worked for me. post_update is running correctly in that. Thanks for the help.

The last patch in #57 contains issues compared to #56.

1. +++ b/redirect.post_update.php
   @@ -0,0 +1,63 @@
   +    $sandbox['progress'];
   

   Should be $sandbox['progress']++;

2. +++ b/redirect.post_update.php
   @@ -0,0 +1,63 @@
   \ No newline at end of file
   

   Need newline here.

But what concerns me much more is that having the logic in the presave seems to cause infinite redirections as the tests shows.
I recently inherited a project with patch #53 and I'm having such infinite redirects when updating the title of nodes.

Here's the scenario:
1. Make sure you have "Automatically create redirects when URL aliases are changed." enabled at admin/config/search/redirect/settings
1. Edit an existing node and change its title
2. A redirect gets created but the hash is now generated from the system path (e.g: node/123) and its language, while the redirect source path is the old alias
> problem 1: the hash is not in sync anymore with the source path stored in DB. Loosing consistency.
3. When viewing the node we now get an infinite redirect because a redirect is found in \Drupal\redirect\EventSubscriber\RedirectRequestSubscriber::onKernelRequestCheckRedirect.
This is because it will query the redirect table with a hash generated from the current system path (e.g: node/123) and the current language.

One way I can think of to solve this issue is to convert the given path alias to its system path on form submit when creating a redirect manually. But even then, I believe it could break existing sites if some of their redirects get re-saved.
I'm thinking of the following scenario:
Status before patch:
- A redirect exists from "test" to "internal:/node/123"
- At this point, no aliases exist for "test" path itself, without the redirect it leads to a 404
- At some point, the "test" path now matches an alias of a new node: node/789
Status after patch:
- If we re-save the old redirect, node/789 redirects to node/123 > not good

I have not dug in to figure out why but patch #57 causes an infinite loop during database updates on a D8.7.9 site I'm working on. If I remove the patch, the updb goes fine. Apply the patch and run 'drush updb' and I get "[notice] All redirects have been re-saved to update their hashes." written to the console infinitely. "

Perhaps we could use another approach:
Change the form for the source path and use an entity_autocomplete form element (or link widget) allowing users to select nodes.
Then we would store a node path in the same format as link fields, e.g: entity:node/42
We would need to change slightly the the detection in \Drupal\redirect\EventSubscriber\RedirectRequestSubscriber::onKernelRequestCheckRedirect to add "entity:" if we're on a node or entity path.
This wouldn't require any risky or long updates.

Was able to troubleshoot the infinite loop in patch #57. Looks like it was just a ++ left off of a counter.

Patch #64 seems to have resolved the issue for me. We were able to create new redirects with path aliases.

The approach here is still problematic IMHO as I explained in #61.
Please have a look at my comment.

The approach I suggested in #63 is much safer I believe but we'd need more feedback from the maintainers and you all before anyone starts working on it.

Review of #64 patch

I am seeing in the last (#64) and some preceding patches:

+    if ($path === $this->redirect_source->path) {
+      $path = ltrim($path_alias_manager->getPathByAlias('/' . $path, Language::LANGCODE_NOT_SPECIFIED), '/');
+    }
+    if ($path === $this->redirect_source->path) {
+      $path = ltrim($path_alias_manager->getPathByAlias('/' . $path), '/');
+    }

Using the same conditional twice (($path === $this->redirect_source->path)) results in always the last being used. Both defining the same variable $path will result in always the second one being used.

In #50 the same was already mentioned as well but there the second conditional was an elseif. In that case, the first conditional is always used, so it seems incorrect in both cases.

Can someone clarify?

Furthermore, the patch does not apply:

martin@martin-XPS-13-9370 ~/redirect (8.x-1.x=) $ git apply -v redirect-does_not_work_for_path_alias-2879648-64.patch
Checking patch redirect.post_update.php...
Checking patch src/Entity/Redirect.php...
Checking patch src/EventSubscriber/RedirectRequestSubscriber.php...
Hunk #1 succeeded at 163 (offset 1 line).
Checking patch src/Form/RedirectForm.php...
error: while searching for:
      // Do nothing, we want to only compare the resulting URLs.
    }

    $parsed_url = UrlHelper::parse(trim($source['path']));
    $path = isset($parsed_url['path']) ? $parsed_url['path'] : NULL;
    $query = isset($parsed_url['query']) ? $parsed_url['query'] : NULL;
    $hash = Redirect::generateHash($path, $query, $form_state->getValue('language')[0]['value']);

    // Search for duplicate.
    $redirects = \Drupal::entityManager()
      ->getStorage('redirect')
      ->loadByProperties(['hash' => $hash]);

    if (!empty($redirects)) {
      $redirect = array_shift($redirects);
      if ($this->entity->isNew() || $redirect->id() != $this->entity->id()) {
        $form_state->setErrorByName('redirect_source', $this->t('The source path %source is already being redirected. Do you want to <a href="@edit-page">edit the existing redirect</a>?',
          [

error: patch failed: src/Form/RedirectForm.php:120
error: src/Form/RedirectForm.php: patch does not apply
Checking patch src/RedirectRepository.php...
error: while searching for:
use Drupal\Core\Database\Connection;
use Drupal\Core\Entity\EntityManagerInterface;
use Drupal\Core\Language\Language;
use Drupal\redirect\Entity\Redirect;
use Drupal\redirect\Exception\RedirectLoopException;


error: patch failed: src/RedirectRepository.php:6
error: src/RedirectRepository.php: patch does not apply
Checking patch tests/src/Functional/RedirectUITest.php...
Hunk #1 succeeded at 91 (offset 5 lines).
Checking patch tests/src/Kernel/RedirectAPITest.php...
Hunk #1 succeeded at 160 (offset 1 line).

Reroll needed.

Reroll of #64 patch that applies cleanly to the 1.5 version and the current dev.

martin@martin-XPS-13-9370 ~/redirect $ git apply -v redirect-does_not_work_for_path_alias-2879648-70.patch
Checking patch src/Entity/Redirect.php...
Checking patch src/EventSubscriber/RedirectRequestSubscriber.php...
Checking patch src/Form/RedirectForm.php...
Checking patch src/RedirectRepository.php...
Checking patch tests/src/Functional/RedirectUITest.php...
Checking patch tests/src/Kernel/RedirectAPITest.php...
Applied patch src/Entity/Redirect.php cleanly.
Applied patch src/EventSubscriber/RedirectRequestSubscriber.php cleanly.
Applied patch src/Form/RedirectForm.php cleanly.
Applied patch src/RedirectRepository.php cleanly.
Applied patch tests/src/Functional/RedirectUITest.php cleanly.
Applied patch tests/src/Kernel/RedirectAPITest.php cleanly.

Be aware, as stated in #69, that is is still using the same conditional twice (($path === $this->redirect_source->path)) resulting in always the last conditional being used. We need to find the correct logic to define the variable $path.

Furthermore, note that you need to resave each related alias redirect for the patch to take effect.

Question what is the purpose of

    if ($path === $this->redirect_source->path) {
      $path = ltrim($path_alias_manager->getPathByAlias('/' . $path, Language::LANGCODE_NOT_SPECIFIED), '/');
    }
    if ($path === $this->redirect_source->path) {
      $path = ltrim($path_alias_manager->getPathByAlias('/' . $path), '/');
    }

Why wouldn't we use the language of the redirect itself?

Thanks for your work so far!

I applied #70 to a D8.8.1 site, on both 1.5 and 1.x-dev versions of the module. Patches applied without an error, but I still receive a page not found error, when trying to open any of the redirects (with path aliases). I tried resaving and recreating redirects, with certain languages and language neutral, and did a cache rebuild but without no luck.

Is there any further step required to get the existing redirects working again with patch from #70?

As a temporary and very lame workaround, I added all redirects to the .htaccess file or the apache config for the vhost.

Thanks a lot!

Trying to replicate your issue (#72) opening a sandbox, with the patch applied at https://simplytest.me/project/redirect/8.x-1.x?patch[]=https://www.drupa...

Steps

1. Login with admin / admin
2. /admin/modules : enable Language and Content Translation core modules
3. /admin/config/regional/language : Add a language, e.g. German
4. /admin/structure/types/manage/page : Enable translation for a content typeand Show language selector on create and edit pages
5. /node/add/page : Add a German page with the title Source and the alias /source
6. /node/add/page : Add a German page with the title Target
7. admin/config/search/redirect/add : Create a permanent redirect from source to Target with the redirect language German
8. /de/source : Redirect works for me, going to /node/2

Going one step further:

1. /de/node/2/edit : Add an alias /target
2. /de/source : Redirect works for me, going to /de/target

@paper boy

Can you give the steps you did on your project, to try and replicate the issue you have on the sandbox?
Thanks.

I also had intermittent issues that i tracked down to this code (noted in #71):

  if ($path === $this->redirect_source->path) {
      $path = ltrim($path_alias_manager->getPathByAlias('/' . $path, Language::LANGCODE_NOT_SPECIFIED), '/');
    }
    if ($path === $this->redirect_source->path) {
      $path = ltrim($path_alias_manager->getPathByAlias('/' . $path), '/');
    }

The scenario is a redirect with no language (und) that targets a particular alias, but the alias only exists for true language(s). In my setup, AliasManager returns the path of the front page. That path is then used to generate the hash:

$this->set('hash', Redirect::generateHash($path, (array) $this->redirect_source->query, $redirect_language));

Then when the redirect lookup occurs in the RequestSubscriber, the hash is wrong. In my testing, the fix was to use the redirect language for the lookup but I'm also curious what the reasoning behind testing other languages besides the redirect language.

I could way off with my idea here. But should we only check the language if the language module is enabled? For one of our sites it is not even an option so we would expect the default site language to be used. Again I could be way off.

I tested the last patch and got some weird results, i did this:

• Make sure you have "Automatically create redirects when URL aliases are changed." enabled at admin/config/search/redirect/settings
• Create and publish a page
• Create and publish a page with title Obsolete 1 (alias obsolete-1)
• Create a redirection from obsolete-1 to the first page
• Clean cache
• As an anonymous load obsolete-1, i'm redirected
• Change the title of Obsolote 1 to Obsolete 1 updated (alias obsolete-1-updated)
• As an anonymous load obsolete-1-updated, i'm redirected
• The list of redirections is not updated in the URL Redirects tab
• If we create a new node with the same alias obsolete-1 it is not redirected, even if it is included in the list of redirections from the nodes created before. This is confusing because you see a redirection that it's not applied

I updated the test to fix some problems with path alias activation.

The previous patch didn't apply, retest with a new version.

I updated last patch to fix some testing errors related to the use of language on the operations.

I updated some test to use the current language, in line alias language

First off, thanks for all the work on this - great timing for me as I'm running into the same issue. Tested patch from #80, unfortunately failed with the following log from a dry-run:

Hunk #2 FAILED at 91.
1 out of 5 hunks FAILED
checking file tests/src/FunctionalJavascript/RedirectJavascriptTest.php
checking file tests/src/Kernel/Migrate/d6/PathRedirectTest.php
Hunk #1 FAILED at 16.
1 out of 2 hunks FAILED
checking file tests/src/Kernel/Migrate/d7/PathRedirectTest.php
Hunk #1 FAILED at 21.
1 out of 2 hunks FAILED
checking file tests/src/Kernel/RedirectAPITest.php
Hunk #1 FAILED at 27.
1 out of 3 hunks FAILED

Not sure it's incredibly helpful logs, but thought I'd pass along in case others see the same.

Pulled the latest version of the dev version and applied the patch from #80 with no issue.

Ran these scenarios too.
1. With the language module disabled I created 2 nodes. /page1 and /page2
2. Created a redirect with the alias /page1 to go to my second node
3. This worked.

1. With the language module enabled. Translated the first node and gave it the same alias. /es/page1
2. Created a redirect for spanish for /page1 to the second node.
3 I was still able to get to my english /page1 node
4. When I go to the spanish page I'm successfully redirected to my second node
5. This worked as expected.

The fact that the hash gets generated always from the source path (e.g: node/2) in \Drupal\redirect\Entity\Redirect::preSave while the redirect_source__path in the database contains the alias is not ok IMO (as I mentioned in #61) as it makes the hash generation inconsistent.
Also, as a result, the redirection works if you keep the node but if you delete it it doesn't work anymore.

Instead of changing the presave, I believe we should change the detection that happens in \Drupal\redirect\EventSubscriber\RedirectRequestSubscriber::onKernelRequestCheckRedirect to include the current source path AND alias.
This will keep the hash generation and what's in db consistent, and it will work even if you delete the node.
But this requires almost a complete rewrite of the patch.

Here is a draft of what I have in mind.
Though I'm not sure the test will pass.. let's see.
In any case, tests need to be provided to ensure the correct behavior of this added functionality.

Seems like tests are passing.
Feedback on this approach are most welcome and appreciated.

We had the same issue, i.e. new redirects with sources using alias paths were not working. The 83.patch seems to have redirects functional again.

One question: since the old redirects seem to work, do we need to run any db updates, as mentioned earlier in this thread?

thx,
Mohsen

I tested patch #83 and existing alias will continue to work after the update.
If we have a node alias with a redirection, for example redirect-test --> home and
we change the alias of the node, for example to redirect-test-2, the alias redirect-test will
redirect and redirect-test-2 will not.

@mgam737 which patch did you use?
If you used one of the patches before #70, you'll have to "undo" the update, which I did on the site I'm working on.
Here's what I did: https://gist.github.com/vever001/4f71e8b8c38797f73d68db2f5410bc92

Did some testing with the patch from #83
Ran these scenarios.
1. With the language module disabled I created 2 nodes. /page1 and /page2
2. Created a redirect with the alias /page1 to go to my second node
3. This worked.

1. With the language module enabled. Translated the first node and gave it the same alias. /es/page1
2. Created a redirect for spanish for /page1 to the second node.
3 I was still able to get to my english /page1 node
4. When I go to the spanish page I'm successfully redirected to my second node
5. This worked as expected.

I updated patch #83 to fix some coding standards and keep redirections when changing the alias of the of an entity.

Edit: I removed the patch because it contained errors.

I updated patch #83 to keep redirections when changing the alias of the of an entity.

I updated patch #83 to keep redirections when changing the alias of the of an entity.

Use the method createDuplicate to create the new redirection when changing an alias.

@enriquelacoma, please always provide an interdiff so we can see what has changed.
The update of redirect_path_alias_update() looks good to me but some other changes are not.

diff --git a/modules/redirect_404/tests/src/Functional/Fix404RedirectUILanguageTest.php b/modules/redirect_404/tests/src/Functional/Fix404RedirectUILanguageTest.php
new mode 100755

No need to change file permissions

+++ b/modules/redirect_404/tests/src/Functional/Fix404RedirectUILanguageTest.php
@@ -7,6 +7,7 @@ use Drupal\Core\Language\LanguageInterface;
+use Drupal\redirect\PathProcessor\RedirectPathProcessorManager;

+++ b/modules/redirect_404/tests/src/Functional/Fix404RedirectUITest.php
@@ -4,6 +4,7 @@ namespace Drupal\Tests\redirect_404\Functional;
+use Drupal\redirect\PathProcessor\RedirectPathProcessorManager;

+++ b/modules/redirect_404/tests/src/Functional/Redirect404LogSuppressorTest.php
@@ -3,6 +3,7 @@
+use Drupal\redirect\PathProcessor\RedirectPathProcessorManager;

+++ b/modules/redirect_404/tests/src/Functional/Redirect404TestBase.php
@@ -4,6 +4,7 @@ namespace Drupal\Tests\redirect_404\Functional;
+use Drupal\redirect\PathProcessor\RedirectPathProcessorManager;

+++ b/modules/redirect_domain/tests/src/FunctionalJavascript/RedirectDomainUITest.php
@@ -3,6 +3,7 @@
+use Drupal\redirect\PathProcessor\RedirectPathProcessorManager;

Those are not needed I believe.

I'm recreating a new patch with an interdiff from #83 so it's more clear which contains:
1. Update redirect_path_alias_update() to maintain redirections when updating the alias of of an entity (approach from #91/93)
2. Fix some coding standards
3. Update RedirectRequestSubscriberTest::callOnKernelRequestCheckRedirect to better match the current code in 1.x

Tests still need to be provided to ensure the correct behavior of this added functionality, so leaving as needs work.

I needed the patch to apply cleanly to 1.5 version, so i modified the patch in #70

I had to remove some lines in Redirect's preSave() in order to make the hash calculation from alias, not from source

I'm sure that i'm missing something... there must be a good reason to calculate hashes from source (i.e: node/xxx) but my way, calculating them from path_alias, I can have multiple path alias redirects for the same destination node, and it makes sense to me searching the redirect by the path alias and not by.... i don't know exactly what. I don't understand the purpose of passing the alias to alias_manager in Redirect's preSave()

I have run unit and kernel tests and everything seems to be ok.

PS. - I don't show the file because i don't want to interfere in dev's workline...

@vidorado

Looking at your patch #95, I don't understand how you can have an alias at this point in:

+++ b/src/RedirectRepository.php
@@ -69,6 +70,15 @@ public function findMatchingRedirect($source_path, array $query = [], $language
+    // The source path might be an alias, so add the source for the alias if it
+    // is. The alias manager will return the source path as is if it's not an
+    // alias, which will lead to a duplicate it in the hashes array, deal with
+    // that too.
+    $alias_language = ($language == Language::LANGCODE_NOT_SPECIFIED) ? NULL : $language;
+    $alias_source = ltrim(\Drupal::service('path.alias_manager')->getPathByAlias('/' . $source_path, $alias_language), '/');
+    $hashes[] = Redirect::generateHash($alias_source, $query, Language::LANGCODE_NOT_SPECIFIED);
+    $hashes = array_unique($hashes);
+

The $source_path we get as argument is passed through the inbound processor in \Drupal\redirect\EventSubscriber\RedirectRequestSubscriber::onKernelRequestCheckRedirect so it is a source path, it can't be an alias:
$path = $this->pathProcessor->processInbound($request->getPathInfo(), $request);

Unless the path doesn't have a source (e.g: node was deleted). Is that your scenario?
But then this means that you can't create a redirect for an existing node.

Patch #94 doesn't have this issue.

@herved

I supposed that $path = $this->pathProcessor->processInbound($request->getPathInfo(), $request); simply removed language prefix and things like that, and what what we finally have in $path is basically the url after the host and port.

When we try to find a matching redirect in onKernelRequestCheckRedirect() with $redirect = $this->redirectRepository->findMatchingRedirect($path, $request_query, $this->languageManager->getCurrentLanguage()->getId()); We are passing this path through path.alias_manager service, you are right...

alias_source = ltrim(\Drupal::service('path.alias_manager')->getPathByAlias('/' . $source_path, $alias_language), '/');
$hashes[] = Redirect::generateHash($alias_source, $query, Language::LANGCODE_NOT_SPECIFIED);
$hashes = array_unique($hashes);

I should remove this piece of code too if I want to honor my approach, and hash by alias, not by path. As you correctly say, this is working because there is not an existing alias for the redirect sources. If it would exist, my code won't not work. ¿I am right?

I attach a subset of the last rows in my redirect table, for illustration purposes.

I confirm that with patch in #95 redirections weren't being made for existing aliases.

I have had to correct the path inbound processing in RedirectRequestSubscriber in some.... "quick and dirty" way using Reflection to skip the PathProcessorAlias. The problem with reflection (apart from dirtiness) is performance, but since this is only used once per request, i think is negligible.

Now it works for multiple redirect source aliases redirecting to the same node, and even for existing path aliases

Patch #94 keeps redirections on nodes when changing alias. Also, when the node is deleted, the redirections of the aliases are kept. I added UI and UILanguage test for both cases.

Yes @enriquelacoma, that's what i guessed,

The normal approach from Redirect module is to make redirections **between nodes** whatever alias they have. But now that we are allowing to redirect from alias to node... that looses some sense, I think.

However, we are talking about different patches. Mine applies to version 1.5 and is based on #70. There could have been several improvements since #70, so don't bother too much. Sure that patch in #94 and yours in #99 are a good approach and better than #98, but i needed to patch against 1.5 version! :)

There was an error in the test of patch #99, i updated the test in this patch

Thanks for this @enriquelacoma.

Though I find these tests are a bit hard to understand and read.
If you agree, here's a more simple approach, based on your work.

I know this is a already a long thread but allow me to take a step back and re-assess here.

Problem/Motivation

The main issue here is to allow redirections for path aliases.
The code ATM in redirect only checks for the source/system paths (and not aliases) when trying to find redirects.
See https://git.drupalcode.org/project/redirect/-/blob/8.x-1.x/src/EventSubs...

So:
- if we have a node with ID 123 and alias "test-node",
- and we create a redirect from "test-node" to "/destination"
-> http://site.com/node/123 will redirect but not http://site.com/test-node

Proposed resolution

There are 3 proposed solutions so far:

1. #1 to #80: Always generate the hash from the source path in \Drupal\redirect\Entity\Redirect::preSave

Cons:
- it alters the hash generation, hashes are not always in sync anymore with the redirect_source__path in DB, so we loose consistency.
- on node/path update it can create infinite redirections as I explained in #61.
A workaround? for those infinite redirections has been added to patch #70.

+++ b/src/EventSubscriber/RedirectRequestSubscriber.php
@@ -163,10 +163,21 @@ class RedirectRequestSubscriber implements EventSubscriberInterface {
+      // Redirects to path aliases are stored and searched for using the alias's
+      // source (/node/123 vs /foo-bar-path). This can cause redirect loops
+      // because the path processor above may have converted a path alias to a
+      // path source (/foo-bar-path => /node/123) which will trigger a redirect
+      // on the path's valid alias.
+      $redirect_url = $url->setAbsolute()->toString();
+      if ($redirect_url === ($request->getSchemeAndHttpHost() . $request->getRequestUri())) {
+        return;
+      }

- validations are skipped (like check for duplicates), see \Drupal\redirect\Form\RedirectForm::validateForm
- on node/path delete: redirections won't work anymore

  Scenario:
  - we have a node with ID 123 and alias "test-node"
  - and a redirect from alias "test-node" to "/destination"
  - we delete the node
  - "test-node", which is the canonical path of the node, doesn't redirect anymore since the hashes were generated from the source path, node/123.
  

2. #83 to #102: Change the detection in \Drupal\redirect\EventSubscriber\RedirectRequestSubscriber::onKernelRequestCheckRedirect to include the current source path AND alias.

Pros:
- no change to the hash generation, keeping it consistent in db
- validation still runs in the form

Cons:
- big changes to the detection needed to match multiple paths (current source and alias)
- which also impacts performance
- can clash if we have 2 redirections with same source paths but different destinations:
- 1 with redirect_source__path "node/123"
- 1 with redirect_source__path "test-node"
- on node/path delete and update: see scenarios below

	Background:
	- we have a node with ID 123 and alias "test-node"
	- we want to redirect the node ("node/123" and "test-node") to "/destination"
	- we create a redirect from "node/123" to "/destination"

		- Scenario: on node/path update:
		  - we update the alias "test-node" to "test-node-updated"
		  - redirect_path_alias_update() finds no redirects for "test-node" and creates one, from "test-node" to "test-node-updated"
		  - "node/123", "test-node" and "test-node-updated" will all redirect to "/destination" (OK)

		- Scenario: on node/path delete:
		  - we delete the node
		  - redirect_entity_delete() will not delete the redirect
		  - /!\ "test-node" won't redirect anymore to "/destination", which will confuse users (NOT OK)

	Background:
	- we have a node with ID 123 and alias "test-node"
	- we want to redirect the node ("node/123" and "test-node") to "/destination"
	- we create a redirect from "test-node" to "/destination"

		- Scenario: on node/path update:
		  - we update the alias "test-node" to "test-node-updated"
		  - redirect_path_alias_update() finds a redirect for "test-node" and duplicates it, from "test-node-updated" to "/destination"
		  - "test-node" and "test-node-updated" will all redirect to "/destination"
		  - /!\ "node/123" doesn't redirect to "/destination", which may confuse users (NOT OK)

		- Scenario: on node/path delete:
		  - we delete the node
		  - redirect_entity_delete() will not delete the redirect
		  - "test-node" will still redirect to "/destination" (OK)
	

3. (NEW) Convert aliases to source paths in the redirect creation form, during the ajax callback. And act on path alias deletions.

This is a 3rd possible solution.
We convert aliases to system paths in the redirect creation form, during the ajax callback.

For the node deletion problem:
- when a path alias is deleted,
- and a redirect for the source of the path alias exists
- if the existing redirect language matches the path alias language being deleted, we update the redirect source to the path alias url
- else if the existing redirect language == "und", we duplicate the redirect and set the redirect source and language according to the path alias being deleted.

Pros:
- Relatively simple to implement
- no change to the hash generation, keeping it consistent in db
- no change to the redirect detection
- validation still runs in the form

Cons:
- ?

To summarize I'd go with option 3 in the end.
I'll post a patch soon, please let me know if I'm missing anything or if some of my analysis is flawed.
Thank you

Here is the 3rd approach, without tests.

Tested the page in #104

Created /page1 and setup the redirect to go from that page to /page2 on any language. Worked as expected.

Updated the redirect to only be English. Made a translation of /page1 (es/page1) and it displayed the translation page while redirecting /page1. Worked as expected.

Couldn't think of any other scenario but if there I don't mind helping test.

Thanks everyone for working on this, that is a huge issue. It would help a lot to have an updated issue summary that .. summarizes all those comments and explains what is exactly being done now.

Also, I fear we need test coverage for the use cases this fixes. For example with a new method on \Drupal\Tests\redirect\Kernel\RedirectAPITest that creates redirects on aliases and ensures they work as expected.

Hello @Berdir,

I summarized the different approaches in #103, and in the end decided to go for the 3rd approach which is to "convert aliases to source paths in the redirect creation form, during the ajax callback. And act on path alias deletions".
And I'd like yours and others opinion on the chosen approach.

A variant of this 3rd approach could be to make the source path behave like a link field, where we would start typing the title and then select the node from the auto-completion, but there are some caveats as this field is already quite specific.

Hi @herved

I appreciate your work on this because I'm dealing with this issue as I'm migrating tens of thousands of redirects from a D7 site to a D8 site.

I'm curious if your 3rd approach using the Ajax callback in #103 will work for this use case?

I am using this module https://www.drupal.org/project/path_redirect_import to import the redirects from a CSV file. Since the system "node/123" paths do not match between the old D7 and new D8 site, I can only import the the CSV in "pretty/old-url" -> "pretty/new-url" URL alias format.

However this was when I discovered this bug because none of these imported URL alias redirects actually work because they were not imported using their system "node/123" path.

In fact they cannot be imported using their system "node/123" path because a lot of these URL aliases no longer have corresponding nodes on the the D8 site. They are either a URL alias that existed once upon a time on D7 -or- a dynamic programmatically generated node displayed upon a specific URL alias request that isn't tied to an actual content node.

In D7 this module would let me create redirects for URL aliases that weren't tied to actual nodes and the redirects would still work. Is your Ajax callback method going to enforce that the source URL alias provided in the form must have a corresponding "node/123" that actually exists? Or can the the URL alias redirect stand on its own without trying to be converted to a "node/123" system path during the Ajax callback?

Thanks!

I applied #104 on a 8.8.6 site with redirect:1.6.0 and redirect:1.x-dev. The patch applies successfully but new or existing redirects still end up on the 404 page.

I upgraded from redirect:1.5.0 using composer and applied the patch from #104. Are there any further steps required?

Thank you!

I am not quite sure if I understood the issue right. But what I tried is:
- Create a node with a path alias. (For example: '/some-alias')
- Create a second node without a path alias.
- Create a redirect from the path alias to the URL from the second node. (From '/some-alias' to '/node/2')

When I create this redirect and go on '/some-alias' the redirect to '/node/2' does not work for me with patch #104.
I'm not sure if I missed something. Anyways I created a simple test to cover this scenario, which fails.
The test might need some more work but I would appreciate any feedback.

BR

Patch #104 works well. For newcomers to the issue, please keep in mind the language from which you are adding the redirect from. For example, if you want to add a redirect for a 'de' node, the language in the add redirect form URL must also be 'de'.

That is what getPathByAlias is looking at. Otherwise, the source_path to system_path conversion will not trigger.

I've also added a change to the order of the warnings. Since we always convert to the system path, the URL alias setup warning always triggers as well, which can confuse users as they have already set up the URL alias.

One small caveat remaining would be, I believe - in the future, when one wants to search for a past redirect, it won't be possible to find it based on the URL alias anymore but only by the system path. Regardless, it's nothing major I think.

I would appreciate any feedback on this change.

I'm having issues with #104 for multilingual aliases.

Example:

"und" language alias, `my-page` is converted to `node/123`. The redirect does work.
"es-es" language alias, `es-es/my-spanish-page` is NOT converted to `node/456`. The redirect seems to be displayed correctly, but gives a 404.

I was working on some similar functionality and I think the parsing of aliases needs more work here. However, should we try to select the language for the user if they use a language alias? If not, the alias will not work unless the user manually selects the language. For now, we could provide a notice when this needs to occur.

Here is a service I created to parse aliases that we might be able to use parts from. In particular, the `getNodeIdFromAlias` logic should probably be generalized, but it shows how language specific and "und" languages are handled by the Alias Manager.

namespace Drupal\my_module;

use Drupal\Core\Language\LanguageManagerInterface;
use Drupal\path_alias\AliasManagerInterface;

/**
 * Class AliasParser.
 */
class AliasParser {

  /**
   * The Alias Manager.
   *
   * @var \Drupal\path_alias\AliasManagerInterface
   */
  protected $aliasManager;

  /**
   * The Language Manager.
   *
   * @var \Drupal\Core\Language\LanguageManagerInterface
   */
  protected $languageManager;

  /**
   * AliasParser constructor.
   *
   * @param \Drupal\path_alias\AliasManagerInterface $alias_manager
   *   The alias manager.
   * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
   *   The language manager.
   */
  public function __construct(AliasManagerInterface $alias_manager, LanguageManagerInterface $language_manager) {
    $this->aliasManager = $alias_manager;
    $this->languageManager = $language_manager;
  }

  /**
   * Get a Node ID from an alias.
   *
   * @param string $path
   *   The alias.
   *
   * @return int|null
   *   The node ID if found or null.
   */
  public function getNodeIdFromAlias(string $path) {
    $node_id = NULL;

    $path_without_language_prefix = $this->getPathWithoutLanguagePrefix($path);

    $language = $this->getLanguageFromPath($path);

    if ($language !== 'und') {
      $potential_node_path = $this->aliasManager->getPathByAlias($path_without_language_prefix, $language);
    }

    if ($language === 'und') {
      $potential_node_path = $this->aliasManager->getPathByAlias($path_without_language_prefix);
    }

    // The alias manager returns the same path if there is no alias found.
    if (($potential_node_path !== $path_without_language_prefix) && (substr($potential_node_path, 0, 6) === '/node/')) {
      $node_id = (int) substr($potential_node_path, 6);
    }

    return $node_id;
  }

  /**
   * Get the path without the language prefix.
   *
   * @param string $path
   *   The redirect path that starts with `/`.
   *
   * @return string
   *   The path with the language removed if present.
   */
  public function getPathWithoutLanguagePrefix(string $path): string {
    $path_without_language_prefix = $path;

    $language = $this->getLanguageFromPath($path);

    if ($language !== 'und') {
      $path_without_language_prefix = substr($path, strlen($language) + 1);
    }

    return $path_without_language_prefix;
  }

  /**
   * Get the language from a path.
   *
   * @param string $path
   *   The path.
   */
  public function getLanguageFromPath(string $path): string {
    $language = 'und';

    $available_languages = array_keys($this->languageManager->getLanguages());

    // Example: fr-fr.
    $path_language = explode('/', $path)[1];

    // Try to make a match.
    // We aren't using in_array, because of capitalization issues.
    foreach ($available_languages as $available_language) {
      if (strtolower($available_language) === strtolower($path_language)) {
        $language = $available_language;

        break;
      }
    }

    return $language;
  }

}

Here is my attempt (without tests) for language support. Based on #104.

If a language is detected in an alias, like `es-es/my-page`, then the language portion is removed so the path can be converted into an alias. A notice is then displayed telling the user to select the language.

I couldn't figure out how to update the language field from this widget since changing the form state doesn't seem to do anything.

Sorry for the spam, I remember there was a request for tests, so I'm moving this back to "Needs work". Still hopeful this helps move things forward. :)

Hey, I will also add my input here. I was having problems with redirect loop after node is removed from the book. We have alias pattern for each book's node and when node was removed from the booking, the old url redirect (that contained book id) wasn't working. First of all I tried commenting out the part of RedirectRepository.php :
throw new RedirectLoopException('/' . $source_path, $rid);
and returning NULL instead. It worked.

But then I found this topic and solution from #9 worked in this specific bug case (at least it looks like it). So big thanks! This is some weird and really hard to debug issue.

This is an update to #115 which was based on #104.

I was making too many assumptions about language paths. Now, we get the configuration for language prefixes to use those to detect languages and trim the paths.

Sorry, I put the wrong comment number in the patch.

A small fix for foreach() errors if there are no languages (e.g. language module isn't enabled).

Hi All,

The applied patch will work only when you will add the redirection from the admin panel, coz it's converting alias into system path.

But Take a note: When I am migrating 100's of redirection from other sources, the alias is not converting into it's system path.

The only case where alias based redirects don't work for us is when the source node is unpublished, and this is confusing behavior. Visitors get a 403 error and admin users are shown the source page and neither are redirected. There are several workarounds for our particular problem, but I disagree that this is the right way to handle situations where a visitor isn't authorized to access a page. Access should be blocked if the destination page is 403 to the visitor, but not the source. It doesn't matter whether or not the source page is accessible.

For quick fix I have created patch and added following code on line number 150 in redirect\src\EventSubscriber\RedirectRequestSubscriber.php

      if(empty($redirect)) {
        $real_path = $this->aliasManager->getAliasByPath('/' . $path, $this->languageManager->getCurrentLanguage()->getId());
        $redirect = $this->redirectRepository->findMatchingRedirect($real_path, $request_query, $this->languageManager->getCurrentLanguage()->getId());
      }

I've rolled a patch with code change mentioned in #123 comment for 8.x-1.4

Patch https://www.drupal.org/files/issues/2021-03-31/redirect-path-alias-28796... (#124) works for me with Redirect 8.x-1.6

Our use case for needing this is that we got a spreadsheet of 700 URLs of redirects from our SEO team which only has the alias for the From URL. Using the redirect import module it's easy to create a CSV and import the URLs without having to write a script or manually removing aliases for unpublished content. This also let's us keep the unpublished content.

Patch redirect-path-alias-2879648-123.patch works for me with Redirect 8.x-1.6.

I've rerolled the patch for version 8.x-1.6.

A few issues with #120. As @bapi_22 mentioned in #121 this will only work for redirects added using the UI. Also as @malte.koelle experienced in #111 it doesn't work if the source path is prefixed with a slash.

It might be worth exploring if a link widget could be used for the source path as per #108. This would mean a redirect for an existing entity would be stored as entity:node/123, and a redirect for an arbitrary path would be stored as internal:/foobar.

I've also tested the patches in #102 and #124. Both of these are working for the most basic use cases I tested, but have their shortfalls as described in #103. I'm not sure the performance overhead is significant, but I did experience the collision when there are separate redirects for the system path and alias path. That is not a new issue though and can be reproduced without any patches. I would prefer to see redirects from the alias path take precedence over the system path though.

My suggestion is to carry on with the approach in #102 and earlier and ensure it's as robust and performant as possible.

This patch is the same as #102 with some extra tests for language-specific redirects and creating an alias for a path that's already redirected.

Please feel free to debate, but I'm thinking that this may need to come with a major version update to the module, be shipped with an integration sub-module, or be a configurable option that is off by default. This particular issue has come up multiple times in our interactions with our content manager so I'm fairly sure it's a wide-spread misconception. That being said, it is likely only to be discovered after a redirect has been created whose From value is an alias.

Upon finding that it doesn't work, how many folks would follow through and delete the redirect? I don't think we can be reasonably sure that such redirects are cleaned up. As a result, if suddenly these redirects start working, it could have strange and hard to debug side-effects (from the perspective from a site owner or content editor).

Imagine the following scenario...

1. User creates a node (/node/1) with the alias /some-node.
2. User creates a node (/node/2) with the alias /some-newer-node.
3. User creates a manual redirect from /some-node to /some-newer-node.
4. User discovers this does not work, contacts their technical support, and eventually creates a redirect from /node/1 to /some-newer-node that does work.
5. 3 years pass, the old content manager works at another business, and an entirely new staff has rotated in.
6. A new editor deletes /node/1 (it's been redirecting for years now and the page is going to be retired permanently).
7. A new editor creates a node (/node/10000) with the alias /some-node.
8. A developer upgrades the redirect module.
9. The new /some-node content is now broken by a 301 redirect.

I may be way off base here, but why have an existing node that cannot be accessed? This issue is caused because an alias exists for a node in Drupal for the path one is attempting to redirect. If the alias wasn't there, then the redirect would work. Am I missing something? Seems like deleting the node, or unpublishing and changing the alias would solve the problem. That way the redirect is using a non-existing path as the source which would result in a redirect to the destination.

#131 makes a strong point. Why have accessible content that can't be accessed.
The initial problem seems graciously solved by simply removing/unpublishing the content that's behind the faulty redirect.

Are there actual use case here that we are not seeing? There must be with so much comments and so many followers of this issue. Can someone please come up with another use case than the one presented in the original post? I can't think of a good one.
I just don't see why the module should be changed. This issue should be solved by just removing what's there and that shouldn't be there.

All I can think of is trying to hide something (like /user/[user_id] pages for example, or https://www.drupal.org/project/drupal/issues/2828724 ). But it seems like bad practice to want to try to obfusctate something by "protecting" it with a redirect. Content should be first rendered inaccessible through traditionnal Drupal means like unpublishing or access control otherwise it would probably be easy to find a workaround to access whatever content you are trying to "protect". I guess there must be uses cases where this just can't be done and hence people default to a redirect.

Sorry for the silence here. To be clear, I have no intentions of committing a patch that will automatically work on aliases. There's just too much that could go wrong with redirects that suddenly work and content no longer being accessible then.

I think this should go back to earlier approaches that do the lookup during redirect creation and for those who want to support their existing redirects, there could be a script or so for them to run, not an enforced update function, which would have the same result.

I agree that there are valid use cases for this, for example on unpublished content, you currently either have to delete or manually change the alias, which depending on your setup, might not be possible for your users.

I think #131 and #132 are missing the point a bit. The current behavior is inconsistent. Why should it work when using the system path as a redirect source but not when using the alias? I think either both should or should not work. The split behavior is confusing.

But #131 and #132 do bring valid criticism to the purpose of adding a redirect for an existing path to begin with. I can offer one explanation that I've encountered many times. A content type where you want to show a teaser of the content, but have the detail page redirect to some other page or website. For example, a Person content type that you want to show up in your site's search and "people" view, but redirect their detail page to that person's personal website.

However, I the use case I present above is probably better solved with some explicit checkbox on the content type itself to control this redirect behavior, and not involving the Redirect module at all. That's the approach we've recently taken, thought it does involve some custom code.

Yes, examples like that is exactly what https://www.drupal.org/project/rabbit_hole is designed to do.

Hi,

I added a small patch to quick-fix the problem with From aliases when we can't redirect from these aliases and added check on the existing path when we create redirect from aliases

But now From path has priority on node/{nid} and after that on path alias on redirect

Possibly related to #3295763: does not work on non-node Webforms

We are still having issues with the URL redirect not being triggered when the redirect is created using URL alias. Is there a resolution to this issue? System is Drupal 9.3.xx with PHP 8

I have listing page "/node/1" with alias "/listing".
I can have path like "/listing/test" (facets pretty path enabled), it comes like "/node/1/test" to RedirectRequestSubscriber.
So redirect from "/listing/test" doesn't work, even with patch #124.
I've added logic to check request URI for redirects before trying to get alias for current path.

A re-role the the #129 patch to work with Redirect 1.8

Patch #142 is not usable, it modifying a behavior, now instead of create a url redirect with the previous alias, it's creating a redirect with the new alias.

Besides patch #140 failing on tests, it's working as expected.

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.

I started working on my idea in #2821158: Document that URL redirects work from node/{nid} but not alias, very early UI only proof of concept, feedback welcome.

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