Fix and prevent circular redirects

This is a Redirect module issue, and it's a duplicate of #1263832: Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions.

No, this is a separate issue. Debating removing the manual flood detection for redirect loops is different from "let's prevent circular redirects from happening in the first place".

Relevant comments from #1263832: Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions

#5
Posted by a.ross on September 19, 2012 at 2:51am
After the update to RC1, we're getting the message about infinite loop on every page. Could this expose another underlying bug? If not, I think it's time for this patch to be committed.

Edit: the error occurred only on pages with redirects, removing and re-adding the redirects fixes the notice.

#6
Posted by philbar on September 19, 2012 at 3:50pm
After the update to RC1, we're getting the message about infinite loop on every page. Could this expose another underlying bug? If not, I think it's time for this patch to be committed.

I am also receiving the infinite loop error. Reverting back to redirect 7.x-1.0-beta4

#7
Posted by dlumberg on September 21, 2012 at 7:17am
Also getting this error on lots of pages after update to rc-1

#9
Posted by hass on September 23, 2012 at 10:49am
There may be another underlying issue that has caused this issue. I guess it's the auto path that is added on updates.

For me it was a taxonomy term (taxonomy/term/52) with current alias tag/foo that also had a redirection from tag/foo to taxonomy/term/52 defined. The redirect module need to make sure that this is not possible and delete redirections aliases that point to the same url like the current alias.

BUG: This means redirection module redirects from tag/foo to tag/foo. A clear FAIL.

Workaround: Manual delete colliding aliases from redirects list at admin/config/search/redirect/list.

#11
Posted by hass on September 23, 2012 at 11:22am
For the next release we also need an update hook to delete all the overriding redirects that will cause endless loops and bring the site down.

#12
Posted by hass on September 23, 2012 at 11:25am
In http://api.drupal.org/api/drupal/includes%21path.inc/function/path_save/7 we can catch all alias changes with http://api.drupal.org/api/drupal/modules%21path%21path.api.php/function/... and delete the alias from the redirect table.

#13
Posted by hass on September 23, 2012 at 11:53am
I would also suggest to remove these logic as the input/duplicate need to be blocked somewhat earlier or at least change to class error:

    // Mark redirects that override existing paths with a warning in the table.
    if (drupal_valid_path($redirect->source)) {
      $row['class'][] = 'warning';
      $row['title'] = t('This redirect overrides an existing internal path.');
    }

#14
Posted by hass on September 23, 2012 at 1:06pm
Found a node that had the current alias also as redirect to itself defined. This node was NOT listed in the redirect list as warning.

#16
Posted by xixor on September 25, 2012 at 4:54pm
I'm getting this error after updating to RC1: "The webpage at webpage.com has resulted in too many redirects. Clearing your cookies for this site or allowing third-party cookies may fix the problem. If not, it is possibly a server configuration issue and not a problem with your computer."

This has happened due to the automatic redirect creation. On some pages, we changed the URL, and then changed it back. Therefore, it has a redirect that is pointing it to itself. Is there any way to check for this and delete/ignore the redirect? We have this on 100+ pages, so it's not workable to remove them manually.

#18
Posted by paulmckibben on September 28, 2012 at 2:53pm
Upgrading to rc1 has caused infinite loops for me as well. Is there a patch for this issue?

#19
Posted by paulmckibben on September 28, 2012 at 3:37pm
The following mysql command seemed to fix the infinite loop issues for me, without breaking any URL aliases.

DELETE r FROM redirect r INNER JOIN url_alias u ON r.source=u.alias AND r.redirect=u.source;

#20
Posted by hass on September 30, 2012 at 3:24pm
Marked #1799430: Too Many Redirects 310 Error when alias and redirect is same as duplicate.

#21
Posted by hass on September 30, 2012 at 3:38pm
Disabled Automatically create redirects when URL aliases are changed for now as several node revision updates cause loops. Editors don't understand this.

#22
Posted by eule on October 2, 2012 at 9:50am
hi,

i get the same problem..i use rc1 ...so if i try to visit my blog post under ./blogs/anyuser i get the msg

Oops, looks like this request tried to create an infinite loop. We do not allow such things here. We are a professional website!

i mean this is a standart drupal path and has nothing to do with any redirection

Nothing wrong with a last line of defense.

Something like this.

I just realized that the posted patch (#3) is missing URL language. I suppose there may be redirect cases where the target URL language is different from the default? Leaving at "Needs review" for now.

Also, I'd like to make a case for considering this a bug rather than a feature request. There's so much going on with pathauto, i18n and the like. Redirect should ultimately prevent circular redirects, no?

Cannot review and test now, but looks ok. In general I'm still asking me why we allow redirects to be saved if this will cause endless loops. Marking them as warning or error sounds plain wrong to me.

This one uses current_path() and adds the URL options.

Hmm, wait, current_path() can't be right here...

This one handles query parameters and absolute URL's.

Applying this patch solves the problem for me. I agree with @hass, those redirects should never be saved, if causing endless loops. For me however, it was caused by the upgrade to rc1. There was no redirect loop prior to that for me.

Same situation as @lsolesen. I'd say this is RTBC.

I do not think that last patch is correct. If my current path is foo and my node has also foo, I can add as many query params as I'd like and the loaded node is still the same. In such a case a url like foo?foo=bar will also trigger an endless loop. I have not tested it, but I think we need to ignore all URL query params.

@hass makes a good point, so that'd be similar to #6.

A redirect from foo?foo=bar to foo?whatever=somevalue is legit. And by itself this implies just one redirect, no looping. You'd only get in trouble when there's a second redirect in the system from foo?whatever=somevalue to foo?foo=bar, but that type of loop is not in the scope for the patch in #8. It does target redirects like foo?foo=bar to foo?foo=bar and foo/bar to foo/bar.

While the patch from #8 did prevent the error from occurring for me, we should really be fixing the problem, instead of detecting and working around it. If we did commit #8,it should be in addition to actually fixing the problem and old data.

I am able to cause new instances of the Oops, looks like this request tried to create an infinite loop. We do not allow such things here. We are a professional website! message by editing a node and setting its URL Path to be the same as an existing redirect to that same node.

It's possible to fix the old data using the query from #19 in #1263832: Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions.

To prevent it happening in future, redirect module must prevent nodes from being saved with an URL path that matches an existing redirect to that node.

I describe the cause of the problem, the fix, and prevention in more detail below:

Cause of the problem

Adding a redirect works properly. If you try and add a redirect to an existing URL alias, the redirect module correctly prevents you, with the message: "You are attempting to redirect the page to itself. This will result in an infinite loop."

However nothing prevents you from editing a node and setting its URL alias to be the same as an existing redirect. Since rc1 of redirect this causes the Oops error message when you go to one of the duplicate URLs.

Steps to reproduce

The following steps will reproduce the problem:

1. Create and save a test node and set its URL alias to "test" under URL Path Settings.. Note the nid of your new node.
2. Now add a redirect at Administration » Configuration » Search and metadata » URL redirects, and set the Source to "test2" and the Redirect to node/123 (or whatever your test node's nid is). At this point everything is fine, with /test and /test2 both reaching your /node/123
3. Next, edit your test node, and under URL Path Settings change its "URL Alias" to be "test2". The node save completes without any errors (bug here!)
4. Visit the new page at /test2 and you will receive the "Oops, looks like this request tried to create an infinite loop...." message. The redirect table now contains a redirect that's a duplicate of an URL alias, and going there produces the error.

Prevention

This could be prevented by having redirect implementing hook_node_validate(). Or adding a validator function to certain node edit forms via hook_form_alter(). Or quietly deleting the duplicate redirect row at node save time. I'm sure there are a number of other ways as well... Which would be best?

The Fix (for existing bad data and the Oops message)

For fixing existing bad redirects, the query in #19 from #1263832: Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions works great.

Show records to be deleted:

SELECT r.rid, r.language, r.source, r.redirect  FROM redirect r INNER JOIN url_alias u ON r.source = u.alias AND r.redirect = u.source AND r.language = u.language; 

Delete redirects shown in above query:

DELETE r FROM redirect r INNER JOIN url_alias u ON r.source = u.alias AND r.redirect = u.source AND r.language = u.language;

I think this query belongs in a redirect module DB update rolled out the same time as the fix the problem above. This will fix bad data and prevent the problem from happening in the future.

If we do that, we shouldn't need the patch in #9... but it might be nice to have anyway for situations where people get corrupt data somehow (via a buggy contrib module, failure to run the DB update, or who knows what else).

Thoughts? I could make a patch for this plan, if we can agree on which method is best.

EDIT: changed "active alias" to "primary alias".

@damien_vancouver, the clean-up query needs work. Would you be willing to open a separate issue? I'd be happy to help on getting that one done. There are at least two things to deal with when running such a database query:
1) Language.
2) The other is the fact that there may be multiple aliases in the database for one system path. The primary one is the one to check against, but it might be the case that (at times) Redirect selects a different one as the primary one than Drupal Core does.

I think it would be wise to keep this issue focused on just one problem space: the last line of defense for one looping case: redirect to self.

@Eric_A : I have opened a separate issue: #1817976: Updating an entity with an URL alias that matches an existing redirect causes bad data and circular redirect. Please have a look at comment #1, where I carry on the discussion from #14 and #15 here.

Can you please update the description and/or title of this issue so it better describes the last line of defense thing? Dave Reid described it in #2 as "let's prevent circular redirects from happening in the first place" but really it's now become "let's detect circular redirects are in the process of happening and stop after the first redirect".

Lastly, a +1 from me for the patch in #8, now that I understand that duplicate url_alias entries are allowed for a single source path. That might be bad data a user can't edit their way out of, so having a last line of defence like #8 would be the only way to stop the redirect from happening.

I was looking into test coverage of this problem and I found that the Redirect functional test is actually failing because of this issue, but the test isn't currently detecting the failure.

With 7.0-1.0-rc1, the testPathChangeRedirects() function creates a node with alias "first-alias", then resaves it as "second-alias", then again as "first-alias" (which fails) and finally back to "second-alias" (which also fails).

The failure happens in one of two ways, which appears to be a timing thing. I consistently get one of each kind of these failures when running the Redirect functional tests:

1. The Redirect detection catches the redirect and displays the "Oops, looks like this request tried to create an infinite loop..." message
2. The Redirect detection fails to catch the redirect, and instead the test gets an object with an empty $this->content, and with a whole bunch of repeated 302 Redirects in $this->header

This second one looks like this in the debugger - it appears that Simpletest just ends the request after 6 redirects in a row, and then since there are no assertions running it doesn't see that there was a problem:

headers	Array [83]	
		0	HTTP/1.1 302 Found\r\n	
		1	Date: Fri, 19 Oct 2012 19:52:51 GMT\r\n	
		2	Server: Apache/2.2.16 (Debian)\r\n	
		3	X-Powered-By: PHP/5.3.3-7+squeeze13\r\n	
		4	Expires: Sun, 19 Nov 1978 05:00:00 GMT\r\n	
		5	Last-Modified: Fri, 19 Oct 2012 19:52:51 +0000\r\n	
		6	Cache-Control: no-cache, must-revalidate, post-check=0, pre-check=0\r\n	
		7	ETag: "1350676371"\r\n	
		8	Location: http://drupal7/first-alias\r\n	
		9	Vary: Accept-Encoding\r\n	
		10	Content-Length: 0\r\n	
		11	Content-Type: text/html\r\n	
		12	
		13	HTTP/1.1 301 Moved Permanently\r\n	
		14	Date: Fri, 19 Oct 2012 19:52:51 GMT\r\n	
		15	Server: Apache/2.2.16 (Debian)\r\n	
		16	X-Powered-By: PHP/5.3.3-7+squeeze13\r\n	
		17	Expires: Sun, 19 Nov 1978 05:00:00 GMT\r\n	
		18	Last-Modified: Fri, 19 Oct 2012 19:52:51 +0000\r\n	
		19	Cache-Control: no-cache, must-revalidate, post-check=0, pre-check=0\r\n	
		20	ETag: "1350676371"\r\n	
		21	Location: http://drupal7/first-alias\r\n	
		22	X-Redirect-ID: 1\r\n	
		23	Vary: Accept-Encoding\r\n	
		24	Content-Length: 0\r\n	
		25	Content-Type: text/html\r\n	
		26	
  ... and so on (it repeats 6 times, giving $this->header 82 entries, before just stopping).

Adding two asserts to the second and third drupalPost in testPathChangeRedirects() catches this problem and fails the test without the fix from #8 applied. The saves are successful only if there is an HTTP 200 in the headers, and no Oops error message appears in the page text.

    // redirect 7.x-1.x-dev redirect.test, line 236:
	$this->drupalPost("node/{$node->nid}/edit", array('path[alias]' => 'first-alias'), 'Save');
    $this->assertResponse(200,'Changing node\'s alias back to \'first-alias\' does not break page load with a circular redirect.');
    $this->assertNoText('Oops, looks like this request tried to create an infinite loop. We do not allow such things here. We are a professional website!', 'Changing node\'s alias back to \'first-alias\' does not trigger Circular Redirect detection.');
    //$redirect = redirect_load_by_source('second-alias');
    //$this->assertRedirect($redirect);

    $this->drupalPost("node/{$node->nid}/edit", array('path[alias]' => 'second-alias'), 'Save');
    $this->assertResponse(200,'Changing node\'s alias back to \'second-alias\' does not break page load with a circular redirect.');
    $this->assertNoText('Oops, looks like this request tried to create an infinite loop. We do not allow such things here. We are a professional website!', 'Changing node\'s alias back to \'second-alias\' does not trigger Circular Redirect detection.');
    //$redirect = redirect_load_by_source('first-alias');
    //$this->assertRedirect($redirect);

I've attached a patch of the test only with it failing, and a re-rolled #8 with the test in there which passes.

I agree with Eric's comments in #13 with regard to the foo?foo=bar paths. Should the issue status be set back to Needs Review?

Setting issue status to Needs Review and re-attaching those patches so the Testbot sees them.

Can we please make this module UNAVAILABLE or CARRY A BIG WARNING IN RED at its download page?

Since my infinite loop occurred at my home page (mydomain.com) and I don't have shell access to run MYSQL commands, it looks like I have no other option but to nuke my entire Drupal installation and start from scratch recreating my pages - 100 hours of work, right down the crapper.

Or is there some other way I can fix this?

Downgrade should help either.

Downgrade should help either.

I downgraded the redirect module because of this error. I'm not sure if it's related, but around the same time my javascript aggregation has been broken. The files are being javascript files are being generated and I can view them in FTP. But my site gives 404 errors when anyone tries to access them.

I can't think of any other change that would cause this because I haven't been making many changes on my site. Just updating modules. I've searched for similar issues on Drupal.org and nobody else seems to have the same problem. That is why I believe it is related to downgrading this module, simply because many people haven't needed to downgrade it.

The fix in #14 works for me. I believe it should be added to the update script.

On a related note, redirect_path_update() appears to be creating a circular redirect when I create a redirect that mimics a url alias.

STR:
1. Create a node with a url alias, e.g. node/123 -> content/my-node
2. Create a redirect from node/123 -> content/my-node-1 (note the slightly different destination path)
3. Edit your node and change the url alias to content/my-node-1 (matches the destination path from step 2)

The redirect module appears to create a new redirect from content/my-node-1 -> node/123. This is done via redirect_path_update().

Is anybody else seeing this behavior?

I like #18 and it seems to work well for my case. However, I wonder if we should prevent it from ever saving the reciprocal redirect in the first place.

The redirect module appears to create a new redirect from content/my-node-1 -> node/123.

My impression was that it created a redirect from content/my-node to node/123.
But yes, that amounts to the same thing: a looping issue for as long as the active alias from node/123 happens to be the same as the path the system was redirecting from.
There are multiple ways to get such direct redirects to self. The patches in this issue simply provide a last line of defense: if the target alias right now happens to be same as our original from path then don't redirect. If the code from damien_vancouver and me works for you, then your changing the status to RTBC would be welcomed. Others here have mentioned that it worked, so the time has come.

Oops, I said it wrong. Kevins scenario is one of those that could happen.

I encountered this redirect loop issue on my site. I initially had some issues applying the patch in #18, it failed "git apply --check", until I realized that the repo was defaulting to the "master" branch rather than the "7.x-1.x" branch. Once I checked out the correct branch, the patch passed check, applied without issue and solved my redirect loop issue.

Though I don't want to presume that my testing definitively counts as "reviewed & tested by the community," it definitely worked for me. Thanks for the patch!

I will create a separate issue for getting the repo to default to the "7.x-1.x" branch.

After applying both this patch and the global redirect patch from #905914: Merge global redirect functions into Redirect module it becomes apparent that the patch here (#18) needs a little work. A possible fix that comes to mind is passing in 'alias' => TRUE when normalizing the original URL.

Suscribe

I have --dev + the patch in #18, and I can confirm that at least the error message goes away when repeating the node title change and then change back trick.

The only issue I foresee is that the redundant redirect still continues to persist in the database. I think there should be node_update() implemented to purge any redirects that now point away from the current $node URL.

Thoughts ?

The patch in #18 is working for me in fixing circular redirects. I support thinking through #1817976: Updating an entity with an URL alias that matches an existing redirect causes bad data and circular redirect to see if we can prevent the problem in the first place. It's a difficult problem to solve.

Either this issue or #1817976: Updating an entity with an URL alias that matches an existing redirect causes bad data and circular redirect needs to be closed as a duplicate.

I would recommend leaving this one open and updating the first post to have all the info from the other one, because this one has more discussion and patches.

@rooby: I agree that there's a lot of overlap between the two issues.

I have a suggestion:
Discussion in this issue seems to have coalesced around "Stop circular redirects from redirecting infinitely". That's the problem the patches here try to fix, at least. Let's re-title this issue appropriately let it carry on.

#1817976: Updating an entity with an URL alias that matches an existing redirect causes bad data and circular redirect has an unnecessarily specific title but is intended to be a broader discussion of how to stop circular redirects from being established in the first place. Personally I don't think that's feasible and have suggested a report-based solution but that's neither here nor there. I suggest re-titling that issue to something clearer. Perhaps "How to resolve conflicts and undesired interactions between url aliases and redirects".

Oh, I figured that this one was for fixing the underlying cause.

If it is about handing the infinite redirects that have been allow to be created, then this one should be closed as a duplicate of #1263832: Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions

[EDIT] Maybe. At any rate, there are too many issues open for what is actually one single underlying issue (infinite redirects should not be created in the first place).

I agree that there are a lot (perhaps too many) issues for one basic problem. But I'm not so sure #1263832: Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions is an exact duplicate either (personally I think the flood detection code discussed there should be stripped out in favor of the patch in this issue).

This infinite loop, url alias collision problem is a fairly complex one without a single clear solution. Even your statement "infinite redirects should not be created in the first place" which seems self-evident on the face of it is perhaps not really so self-evident (see #1817976: Updating an entity with an URL alias that matches an existing redirect causes bad data and circular redirect for some discussion of when we might need to preserve infinite redirects).

There probably needs to be a meta-issue established for the overall problem which can link to the various related issues and map an approach forward.

* #1263832: Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions was the original issue and discussed rolling back the "Oops! This website caused a circular redirect" message.

* (As explained in #2 here), this issue was requested to hold a patch for preventing circular redirects from occurring. This issue has a patch that has been marked RTBC for a while and I've personally had it on a few production sites that were experiencing the issue. It prevents anonymous users seeing that message.

* Then following that issue we started #1817976: Updating an entity with an URL alias that matches an existing redirect causes bad data and circular redirect, which was requested as a fork of this issue to deal with preventing the bad data that is the root cause of the problem. That issue's discussion has now become more focused on providing a report of bad links, which is really almost another approach again besides fixing the bad data.

Combining the discussion makes sense, but... this issue's patch fixes the error message, has test coverage of the bug, and has been RTBC for a while.

If it really is ready, it could be committed, and close(fix) it instead. That message showing up to anonymous users is a nasty bug that's very easy to trigger at best, or crippling to an existing site at worst.

Then #1817976: Updating an entity with an URL alias that matches an existing redirect causes bad data and circular redirect, the youngest issue of the three, could be marked as duplicate of #1263832: Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions (the oldest).

EDIT: i see it got marked Needs Work again... but we should still figure out how to get it committed... It seems close.

Thanks for the explanation.
I can see why there are some different issues.

The low hanging fruit that would be great is, like you say, making sure the end user doesn't see that message via the patch in this issue.

If there are other ways of triggering that message then another low hanging fruit would be related to #1263832: Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions: Either removing that whole message code, or making it an error message instead of a status message and also potentially getting rid of the watchdog logging or making that aspect an option to turn on & off.

Then the harder parts of what to do with the underlying problem could be addressed at leisure.

My 2 cents.

Anyway, back on topic.

Actually, the low-hanging fruit would be getting a patch from THIS issue committed. As far as I can tell, the change in #1263832: Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions is *mostly* pointless since making that change without also making the change from this issue means you could get truly infinite un-caught redirects and an error from your browser. But once the patch from this issue is committed the one from #1263832: Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions could follow.

That is, unless I'm missing something.

No I don't think you're missing anything, that's the same as what I said, although I tend to not write so clearly sometimes.

Oops, sorry rooby, I see now that I didn't read your post closely enough.

Bottom line: this patch needs to go in first. It's by far the most important first step for solving this problem.

OK then, I'll mark #1817976: Updating an entity with an URL alias that matches an existing redirect causes bad data and circular redirect as a duplicate of #1263832: Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions and post its relevant discussion there along with a summary of what we did.

To get this issue (the fix) back on track then, here's where we are at:

Posted by Eric_A on November 19, 2012 at 3:39pm
Status: needs review » needs work
After applying both this patch and the global redirect patch from #905914: Merge global redirect functions into Redirect module it becomes apparent that the patch here (#18) needs a little work. A possible fix that comes to mind is passing in 'alias' => TRUE when normalizing the original URL.

@Eric_A, are you around? Could you provide some guidance on what next or why it's apparent the patch needs work?

@damien_vancouver:

You can probably leave them both open.
After this one goes in #1263832: Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions still needs to either amend or remove that message, and the #1817976: Updating an entity with an URL alias that matches an existing redirect causes bad data and circular redirect issue is for trying to fix the cause of the infinite redirects. Or at least some of them. If possible.

I marked the patch here NW because the implementation does not allow for redirecting a node system path to its alias, i.e. it will not allow for node/1 to be redirected to its alias. It should allow this. (And the other way around, too, I'd say.)

I don't see why you would want to redirect node/1 to its alias? I can't think of a use case for that.

Use the globalredirect module if you don't want people hitting the node/1 page.

Although I could be missing something.

It's really a shame this issue is being held up for nearly 2 months because of a separate issue for globalredirect module integration. I thought it was already agreed that there are a number of ways that infinite loops can happen. Thus it follows that the solution should be progressive, fixing one problem at a time and getting working code into dev, so we can keep pushing forward and finding and nailing down the other issues. This seems like a big enough issue to simply get it in and THEN worry about globalredirect integration. no?

The one problem we try to fix here is the "redirect to self loop". The patch from #18 does this *and* as an unwanted bonus it kills a subset of perfectly legal redirects. The global redirect issue helped identify this, but it does not hold up this issue in any way. This one just needs work (and apparently a new test assertion is needed). I'll try to find some time this weekend to see if the idea I mentioned earlier is any good.

Eric_A: If there are remaining problems with the patch in #18 (aside from issues with Global Redirect) can you elucidate them here? It's not clear to me what those problems might be just from reading through the comments. As it stands, #18 makes sense to me and is working for my use-cases.

@azink, I've described the problem in #43
Attached patch should do the main job *and* not break a redirect from a system path to its alias. Would you be able to do some testing?

Here's an update script I wrote to purge redirects that are causing an infinite loop:

/**
 * Fix infinite redirect loops.
 */
function mymodule_update_700() {
  $results = db_query("SELECT
    r.rid, u.alias
    FROM {url_alias} u
      INNER JOIN {redirect} r
        WHERE u.alias = r.source");

  foreach ($results as $record) {
    db_delete('redirect')
      ->condition('rid', $record->rid)
      ->execute();
    drupal_set_message(t("Removed infinite redirect loop for the path @path", array('@path' => $record->alias)));
  }
}

I decided to do it as two separate steps rather than one large query so there can at least be some logging of the pages that were affected.

Patch in #48 seems to resolve the issue. I tested using system path example.com/node/123 and redirect example.com/foo. Redirects are stopped. If I rename the path alias to foo2, then the redirect logic doesn't get fired, which is also appropriate. Let's get this thing committed.

BTW, I like having #49 available but not included in redirect. Not so sure I want it in a hook_update_N that is provided by redirect because I want to be able to pick and choose which redirects should be deleted.

Hm, it also seems that removing redirects like that (#49) won't remove all possible loops. It just removes a redirect to itself.
On the other hand IMO some dedicated diagnostics interface just to find redirect loops is overkill - at least for now. It could always be added later if it turns out to be a popular request or something.

+1 for patch from #48 - it works great for me and prevents the Oops message. I've tested it on multiple produdction sites.

Since this issue is already marked RTBC, we need a maintainer for redirect module to do a final test and if it looks good, commit it.

Then 7.x-1.x-dev will at least contain the fix, for novice users who cannot apply a patch (ie. "most people").

re #49 and discussion in #50-52:

- The query should join on the "language" column as well or the join may be ambiguous on multilanguage sites.

- Fixing the bad data doesn't make the underlying problem go away. As soon as users are in there working and changing URL paths, then circular redirects start happening again in the database. It happens almost immediately in the Real World. But we already forked another issue to discuss that: #1817976: Updating an entity with an URL alias that matches an existing redirect causes bad data and circular redirect, specifically for that discussion so it would not hold up the fix in this issue. So let's discuss it further or talk about update hooks and fixing / preventing bad data there. I will un-dupe it again, re-title it and paste the comments from #49-52 there for further discussion.

Let's please keep this issue focused on testing the patch in #48 and getting that patch committed ASAP! Does it need anything else? If not hopefully a maintainer will review and commit it soon.

The only thing I would like to add is that the infinite loop error message shouldn't sound that immature... :)

@a.ross:

The only thing I would like to add is that the infinite loop error message shouldn't sound that immature... :)

That's actually discussed in the oldest of the three issues on this topic: #1263832: Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions.

This issue was forked from that issue originally, as requested by a maintainer.

If this patch gets committed, then #1263832: Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions could then become a cleanup of the message code, which won't get run any more and is no longer needed.

Meanwhile #1817976: Updating an entity with an URL alias that matches an existing redirect causes bad data and circular redirect could focus on preventing/fixing bad data in the database, and everything would end up surprisingly neat and tidy.

Thanks for everyone's work on this.

I'm curious why this happens in redirect_redirect() instead of redirect_goto() where the header is actually set?

Thanks in advance; I'm still trying to figure out the standards for this stuff.

#48 looks good and is working well for me. +1 RTBC

Is the patch in #48 commited to the dev version? I ask because I see that the last development version is from february 6th.

It doesn't look like this has been committed yet. +1 for RTBC since this seems to solve a critical problem for many people. Not sure if there are other considerations for how this is done, but it would be nice to get some solution for this.

Still needing tests I think, but this worked for us too.

We only noticed after Google AdWord links all generated the loops http://www.example.com/?utm=test

Can we get this committed? I've ran into this problem again.

Steps to reproduce:

1. Add URL Redirect to a node "redirect/example.html"
2. Change URL Alias to "redirect/example.html"

Expected Result:

• Alias change would remove the URL Redirect (Or possibly displaying a warning message asking for change).

Actual Result:

• Infinite Loop

In moderated D7 workflows there is an additional fish-hook because there may be an extra node_save() call to set the "current" revision - see #1945558: Redirects incorrectly created when saving as draft (using pathauto/redirect default settings).

The patch at #48 does remove the resulting warning message (using latest redirect dev) when a redirect-to-self is viewed, but does not prevent the actual creation of the two additional redirects created at the "draft" stage mentioned in that issue. These are redirects from the current revision pathauto alias and a draft revision pathauto alias.

Yet another +1 for RTBC.

This is a complex set of issues here, but the patch in #48 stops the bleeding with the warning message.

Hi,

I have created a bit more sophisticated solution. This also filters a longer redirect chain e.g.:
- foo -> bar
- bar -> baz
- baz -> foo

And this does not allow saving a redirect loop.

The patch #64 doesn't do anything about redirects that are created from the path alias side of things. #48 does account for that by not redirecting if someone creates a path alias that duplicates a redirect. I do like the idea behind this patch, but I think it should also incorporate more of what is in
#48 or redirects could still occur.

subscribe

Patch from #64 fixed that infinite loops for me, thanks!

I rerolled patch #64 to match with 1.0-rc1. (also included #48 in this)

I had the problem with infinite loops when in combination with automatic URL aliases. When i changed a node title and saved the node i was redirected twice on the same node and got the infinite loop message.

The patch in #68 fixed the problem for me.

I have tried #48 and #68 and but niether help with #62, which what I am facing too.

#68 works with a simple test case (no loops can be created) which is already a big win from an editors or end-user POV.

One minor annoyance is that a user enters his or hers content, gets the warning message and then goes of to remove any aliases which may cause the loop. This action will cause them to leave the node edit page and loose their content changes.

Tested #68 against 7.x-1.x-dev and it gets rid of the redirect loop message.

We have a few hundred sites and this always happens when users are creating content, and it is confusing for them. Would be great to get some fix for this committed. Thanks!

Dave asked for critical issues in the queue. Setting priority.

I never *asked* for critical issues hass, so do not put words in my mouth EVER. I agree this is a high priority to fix, but this is something that I still consider major. I have time today set aside to look at it.

If the module is broken we name this critical.

@hass: It doesn't happen on every install, therefore it isn't critical.

500.000 pathauto installations are out there. This is over 50% of all drupal installations compared to 53.000 redirect rc1 installations. No good chances that we have a countable number of sites not affected. This brings us nowhere. This is critical as I need to downgrade.

@hass: Nobody's stopping you from using the patch, and following smart development practices like Jen Lampton documented back in March you can easily track the patch. Dave said he was going to try looking at it today, please give him the time to do so.

My temp fix is the following: I used a Pathauto hook which is triggered whenever a new alias is created, updated or read by Pathauto. The hook checks whether the current node has redirects with the same alias as the current node's new alias. If so, it deletes these redirects from the database table.

/**
 * Implements hook_pathauto_alias_alter().
 *
 * Problem  The Redirect module stores redirects with the same alias as the node's current alias, this causes a redirect loop.
 * Fix      Remove redirects with the node's current alias.
 * Info     - Redirect bug report at drupal.org/node/1796596.
 *          - Pathauto API at drupalcontrib.org/api/drupal/contributions!pathauto!pathauto.api.php/7.
 */
function mymodule_pathauto_alias_alter(&$alias, array &$context) {
  // $alias is a reference and since pathauto_alias_uniquify() also references to $alias, we make a local copy of $alias.
  $_alias = $alias;

  // See if our new local alias is unique, optionally add an incremented suffix as in article/my-alias-0.
  pathauto_alias_uniquify($_alias, $context['source'], $context['language']);

  // Check operation, possible values are insert, update, return and bulkupdate.
  if ($context['op'] !== 'return') {
    // If the new alias equals an existing redirect, remove the redirect from the database table.
    $redirects_deleted = db_delete('redirect')
      ->condition('source', $_alias) // Path alias.
      ->condition('redirect', $context['source']) // Internal Drupal path.
      ->execute();

    // For debugging purposes: tell user how many redirects have been removed.
    if ($redirects_deleted) {
      drupal_set_message(t('%redirects_deleted redirect paths deleted.', array('%redirects_deleted' => $redirects_deleted)));
    }
  }
}

Patch from #68 still applies cleanly.

I refactored #80 and submitted a new issue to Pathauto. #68 created some issues for me (sorry I forgot what they were) and most importantly still kept a Redirect rule that was the same as the new alias which I don't think is very clean.

The patch for Pathauto is here https://drupal.org/node/2065091#comment-7754273

Keep an eye on https://drupal.org/node/1817976#comment-7753335. It seems to be more simple and patch works fine.
on hook_path_update or hook_path_insert, if a redirect with the new path already exists, it's deleted. That's all. No patch needed for pathauto nor heavy tests.

Thanks GoZ, the patch in #83 works in my tests and my patch for Pathauto (#82) got closed in favor of the work being done in Redirect anyways.

#83 seems to be working, and it works with moderation

#83 works well and stops circular redirects created by changing a node's alias back to one that previously existed should we close this as a duplicate, the patch at https://drupal.org/files/redirect-prevent_circular_redirects_in_hook_pat... is much lighter and a better approach IMO

Um, that issue assumes that the circular redirects are all due to the generated path aliases doesn't it?

I managed to trigger this via Google AdWord links with #905914: Merge global redirect functions into Redirect module. So from my (poorly researched) perspective:

#1817976: Updating an entity with an URL alias that matches an existing redirect causes bad data and circular redirect = Keep the entity path alias tables clean
This issue = Sanity check / fallback if the s** hits the fan

;)

Another 4 weeks passed away what is far longer than "today".

Let's get an update to the issue summary, see: https://drupal.org/issue-summaries

New patch in #68 is definitely an improvement. Updating to that stopped aliases being created that had redirects from them (very confusing)

Patch needs to be re-rolled against current 7.x-1.x.

Hrm, this applies cleanly here. Tested it manually: you don't get a redirect or a message of infinite loop.
However, in the patch I see a form error, but wasn't really able to trigger it, so not sure exactly where to go next.

swenteldoos-2:redirect swentel$ git pull
Already up-to-date.
swenteldoos-2:redirect swentel$ git apply redirect_loop_detection-1796596-68-reroll.patch 
swenteldoos-2:redirect swentel$ git status
# On branch 7.x-1.x
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	modified:   redirect.module
#	modified:   redirect.test
#
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#	redirect_loop_detection-1796596-68-reroll.patch
no changes added to commit (use "git add" and/or "git commit -a")

Update: could get the form error now too (I finally understand the flow), so this is ready to go for me.

I just applied #68 and it worked well for me and prevented getting into a redirect loop. RTBC++ Thanks!

The tests for #48 (and #68) were broken by http://drupalcode.org/project/redirect.git/commit/ad09f6c.

Also, I'm pretty sure that #68 is broken for multilingual sites and for any non-trivial URL (i.e. with URL options). The patch in #68 is sophisticated and thus needs sophisticated tests. Let's please create a separate issue for that and fix the simple (and probably most frequently occuring) scenario first.

Pasted Issue Summary template. Added related issues to the Issue Summary.

Can't do more right now. We could probably do with summarizing the two patches by describing pros and cons and who reviewed, tested and plussed one those.

#68: redirect_loop_detection-1796596-68-reroll.patch queued for re-testing.

Also, I'm pretty sure that #68 is broken for multilingual sites and for any non-trivial URL (i.e. with URL options).

Then we need more people testing instead of a gut feeling. Note, I've actually tested on a multilingual site and worked perfectly.

The patch in #68 passed after re-testing and that was expected. The point is that the patch in #68 (and this goes for #48 as well) needs an accompanying test only that will fail on the new assertions. (One of them is broken now and will pass instead of fail.)

As for the extra tests that are needed for the sophisticated part of #68. Indeed, it's not my initial analysis or gut feeling that matters. It's up to the patch makers to add automated tests for at least all normal use cases, including cases that start out with the state when there are URLs with the same path but different languages. The point is that it should not detect a loop when there is a target URL with the same path but a different language than the one we're redirecting too. If the logic in #68 really works it might be the perfect replacement for the detection that is being removed in #1263832: Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions.

Tried to apply the patch "redirect_loop_detection-1796596-68-reroll.patch" and it partially patched (Cannot apply hunk @@ 10 ) using netbeans

The only way after trying many patched was to open the redirect.module file and omit line #989

/** drupal_set_message('Oops, looks like this request tried to create an infinite loop. We do not allow such things here. We are a professional website!'); */

Just an observation. After doing a git biscect after reproducing the symptoms, It sent me to the following commit.

I wonder if anyone tried patch #1936820-7: Auto-redirect doesn't check for old aliases, it seems a pretty simple patch and it should fixed this issue, I pretty sure it's almost a duplicate of the other one and the patch seems nicer.

The patch in #103 won't fix aliases that are already broken, so something like #48 would still be needed.

Patch attached that combines the two, with updated assertions in the test that work against the current 7.x-1.x as per #91.

+++ b/redirect.module
@@ -1020,6 +1024,11 @@ function redirect_redirect($redirect = NULL) {
+

Why did you add that empty line?

+++ b/redirect.test
@@ -234,10 +234,14 @@ class RedirectFunctionalTest extends RedirectTestHelper {
+    $this->assertResponse(200,'Changing node\'s alias back to \'first-alias\' does not break page load with a circular redirect.');
...
+    $this->assertResponse(200,'Changing node\'s alias back to \'second-alias\' does not break page load with a circular redirect.');

Please add a space after the comma and put the string in double quotes so that you don't have to escape some parts.

Just tested and #106 works for me.

Updated issue summary.

This patch is working for me also, handling existing and preventing new "back-and-forth-rename" redirect loops. Code looks good, and tests pass so RTBC.

Also confirming the RTBC status of #106.

We've used the patch in #48 in production for quite a while, #106 improves on it by removing existing redirects on update if they match the new alias.

Would be great to see this committed.

Marked #1263832: Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions as a duplicate of this.

Pasted template. Added Related Issues. It's a start.

Updated issue summary.

Updated issue summary.

Adding a +1 to #106. This patch worked well for me.

Updated issue summary, adding related issue #1936820

Patch in #106 worked for me.

Reverting changes, I assume these somehow got reset on the d.o D7 upgrade.

The patch in #106 is still very much RTBC.

#106 looks good. Tried it but ran into some errors... tried both the 7.x-1.x and master branches.. not sure which of those corresponds to 7.x-1.x-dev.

In case it means anything to some of you, here's the error I got;

error: while searching for:
    //$this->assertRedirect($redirect);

    $this->drupalPost("node/{$node->nid}/edit", array('path[alias]' => 'first-alias'), 'Save');
    //$redirect = redirect_load_by_source('second-alias');
    //$this->assertRedirect($redirect);

    $this->drupalPost("node/{$node->nid}/edit", array('path[alias]' => 'second-alias'), 'Save');
    //$redirect = redirect_load_by_source('first-alias');
    //$this->assertRedirect($redirect);
  }

error: patch failed: redirect.test:234
error: redirect.test: patch does not apply

Perhaps Redirect could also (in addition to #106?) check for infinite loops when cron runs, and then delete them. I wrote a sandbox module that does this; https://drupal.org/sandbox/plethoradesign/2138793.

@plethoradesign The patch in #106 applies cleanly to redirect-7.x-1.x-dev for me.

As for cleaning up existing infinite loops, I agree it's a good idea. However given this issue has been going on for so long and we've finally got some consensus on the patch in #106 as the way to fix creating new infinite redirects (as well as removing them if they exist on path update). I think we should get this in first. Removing existing ones should probably be a follow up issue.

@fenstrat, I totally agree #106 needs to get in first. My sandbox module has been tiding me over in terms of dealing with existing ones. Will follow up once #106 is committed.

The above patch is combined with the patch from https://drupal.org/comment/7805833#comment-7805833 as they both covered removing duplicate redirects on entity updates. If it passes tests I'd like to close that issue as a duplicate.

For reference I will post an interdiff between #119 and my patch in #106.

The difference seems to be that #119 also runs the deletion check upon hook_path_insert instead of only on hook_path_update, and goes about the deletion differently. The deletion has seemingly the same result; unless multiple redirects with the same problematic source exist, in which case it will delete all of them instead of just a single one.

Looks good to me! :)

The changes in #119 look good. Thanks for the interdiff in #122 @stefan.r

+++ b/redirect.module
@@ -394,6 +394,8 @@ function redirect_path_update(array $path) {
+    // Delete all redirects that would redirect the alias.

This comment should be the same as it is below for consistency:
Delete all redirects having the same source as this alias.

With that done I can RTBC this.

Confirming that #124 is RTBC.

Just installed -dev, with patch 124 and it resolve the warnings with regards to #2105063: Oops, looks like this request tried to create an infinite loop. We do not allow such things here. We are a professional website!

Woohoo! Can we get this committed?

please commit!
*subscribing*

Bangin, #124 is wonderful with Workbench Moderation. I owe everyone a drink.

I 2nd (or 3rd or 4th) -- please commit! Thanks for the great work on this.

and 5th... - time to commit this to -dev.

Yup, tested #124 just now with one or several redirects and it works as advertised always.

Just want to warn you about MySQL collation. By default it's utf8_general_ci (I don't know from where this coming, from Drupal or from MySQL itself). And, in this case, MySQL make no difference between umlaut symbols and them "normal" analogs. This basically means that, for example, "é" = "e".

Because of this peculiarity I met redirect loops. After installing the transliteration module, I got a record in the redirect table with "personnalisées" source and "personnalisees" redirect. And this query (simplified)

SELECT * FROM redirect
WHERE source = "personnalisees"

returned that record.
To resolve this I had to change the collation of the source field to utf8_bin.

This issue may happen in redirect_delete_by_path() function, so correct redirect record may be deleted.

I can also confirm that 124 works

This patch is great, thanks.
I think a hook_update_N would be useful to remove bad data from the db.
What do you think about this attached patch ?

Cool, #135 solves the issue for me (no infinite loop after setting the path back to the original path)

I tested #135 and it works for me.

I have applied patch #135 & it is working fine for me.

I am changing the status back to "Needs review". At this point, it is not clear if we should be committing the patch in 119, 124 or 135.

#124 is just #119 with a correction of a comment as pointed out in #123

All #135 is, is #124 with an update hook (which seems OK to me).

So #135 seems the most complete and is probably the one that should be committed.

Thank you for the explanation.

Has anyone tested the update hook? We don't want to accidentally delete valid redirects.

I've not yet tested the hook_update_n() addition made in #135. However at first glance:

1. +++ b/redirect.install
   @@ -356,3 +356,29 @@ function _redirect_migrate_path_redirect_variables() {
   + * Remove existing redirect causing infinite loops
   

   Missing trailing period.

2. +++ b/redirect.install
   @@ -356,3 +356,29 @@ function _redirect_migrate_path_redirect_variables() {
   +function redirect_update_7100(&$sandbox) {
   

   Shouldn't this be redirect_update_7001()?

3. +++ b/redirect.install
   @@ -356,3 +356,29 @@ function _redirect_migrate_path_redirect_variables() {
   +  // Thanks to https://drupal.org/comment/6786640#comment-6786640
   

   No need for this comment.

4. +++ b/redirect.install
   @@ -356,3 +356,29 @@ function _redirect_migrate_path_redirect_variables() {
   +  $rid2delete = array();
   

   We don't usually concatenate variables in Drupal. $rids_to_delete would probably be better.

5. +++ b/redirect.install
   @@ -356,3 +356,29 @@ function _redirect_migrate_path_redirect_variables() {
   +    $deleted = db_delete('redirect')->condition('rid', $rid2delete, 'IN')->execute();
   

   Due to the possibility of many existing redirects I can understand why you've done and 'IN' query here. Not yet tested it for performance, but I think the theory is sound.

6. +++ b/redirect.install
   @@ -356,3 +356,29 @@ function _redirect_migrate_path_redirect_variables() {
   +    return format_plural($deleted, '1 redirect was deleted.', '@count redirects were deleted.');
   

   'redirect was deleted' => 'circular redirect causing infinite loop was deleted'.

7. +++ b/redirect.install
   @@ -356,3 +356,29 @@ function _redirect_migrate_path_redirect_variables() {
   +    return t('No redirect was deleted');
   

   Needs to mention 'circular redirect causing infinite loop' as above. Also missing trailing period.

Regarding the update script's number, per the hook_update_N documentation, 71xx updates are for update that match to v7.x-1.x of the module, 70xx updates are for the "initial porting of your module to a new Drupal core API" which does not apply here.

Baah, brain fart on my behalf. Thanks for clearing that up Damien.

The rest of my nit picks still apply from #143.

Thanks for the review, I think all points are fixed in attached patch.
Please test and review :-)

Great, thanks @SylvainM.

I've tested the update hook on a few sites and it works as expected.

The general approach here was RTBC back in #109. The addition of the update hook to remove existing circular redirects makes sense. So marking this as RTBC yet again. This is good to go.

Tested patch in #146 on a test site with several scenario's that used to cause an infinite loop. After that went well I am now running it on a production site for a few days. Had no trouble. So imho also RTBC.

#146 works for me too

#146 fixed a recurring issue we were running into on one of our projects. Thank you much!

Confirm that #146 fixes this issue for my sites too.

I also confirm #146. Thanks!

Brilliant - I can confirm this works!

Confirmed working for me as well.

Works as advertised +1

#146 tested working.

#146 fixed *several* infinite redirect loops I had inadvertently created on my site - many thanks for this! Pushing it over to my live environment now :)

#146 installed for some hours and it works. if i have problems, i will come back and report. thanks a lot to the people that fixes bugs :)

Hmm, apparently I have to uncheck the "hidden" ones too.

#146 works well for me.

Thanks SylvainM for the fix (#146). Just applied the patch and worked perfectly.

146: redirect.circular-loops.1796596-146.patch queued for re-testing.

This hit us on Drupal.org: #2274715: Changing name and then back again caused infinite loops to be created

#146 works for me too, thanks.

Seems like this has been accepted by the community, so let's get it committed.

Tested it with a couple of sites and it appears to have resolved the issues we were experiencing. RTBC for me, too.

I just created #2300419: Is this module still maintained?

works for me too. Please commit this.

It also works for me!

Same as #146 but with bump to hook_update function to 7101, as fix for #2051313: uid not stored when redirects added via Add Redirect page is now committed to dev and uses 7100

hi,
I see that changes are made to .install file as well. So is it advised to uninstall module first and then install patched module? Uninstalling would remove database entries and that I do not want.
So I patched all other files except .install file and it is working.
Should I be worried about .install file? Thank you.

No, just run update.php and it will run these functions.

After patching current dev with #146 and running upgrade I get error:
Fatal error: Cannot redeclare redirect_update_7100() (previously declared in .../sites/all/modules/redirect/redirect.install:271) in .../sites/all/modules/redirect/redirect.install on line 389

Patch #172 works against current dev version.

Just to make sure - so which patch against what version is advised?
Thank you.

@loparr
Patches should always be against the current -dev version.

Here is the difference between #146 (which has been RTBC for a long time) and #172:

13c13
< +function redirect_update_7100(&$sandbox) {
---
> +function redirect_update_7101(&$sandbox) {

This is a trivial yet essential update. So we can assume #172 is RTBC.

Hiding the old patch.

Issue was reported exactly 1 year and 11 months ago, as of yesterday. Can we beat the 2 year mark!?!

I'd totes commit this myself if I could.

Want to open a ticket requesting to be a comaintainer, thedavidmeister? :)

I hope it doesn't come to that, I wish there was a way to "escalate" a ticket without needing to apply for co-maintainer.

#172 works

confirm that #172 works. tested on large site with 10000+ content.

i think should it deploy into release version since a lot of site having the same problem.

The community have had a very strong voice here, something that I can only dream about in my own projects ;)

Push to dev, and start child issues for following up tasks? If unsure, tag a new release and then commit to dev? It would nice to see this resolved before Drupal 8 is released.

IMHO, this and #905914: Merge global redirect functions into Redirect module would be blockers issues to determine the base functionality that would be used as the milestone for doing the Drupal 8.x port.

If we were to read back through the history of this issue, a version of the patch has been RTBCed for over a year. With several, I mean several, confirmations. Can this just get committed?

This patch must be in about all the makefiles of the planet. It's time to push it to the dev release at least!
It has an upgrade path so it's safe and the code is well optimized so d.o would not suffer from this awesome fix.

Please! Please! Commit.
From France with Love.

Yes, please push it at least to -dev.

Please push it to some version. Everyone needs to keep applying this patch. #172

I have put my hand up to be a co-maintainer and apply some of the outstanding RTBC patches in the queue #2342117: Application for co-maintainer: thedavidmeister.

#172 appears to have applied whereas#146 had the WSOD error.

Here's the patching results from #172 when applied to 7.x-1.x-dev after running the "redirect-drupal-subdirectory-fix-1198028-13.patch" patch:

patch < redirect.circular-loops.1796596-172.patch
patching file redirect.install
Hunk #1 succeeded at 363 (offset 7 lines).
patching file redirect.module
Hunk #1 succeeded at 376 (offset -18 lines).
Hunk #2 succeeded at 392 (offset -18 lines).
Hunk #3 succeeded at 843 (offset -18 lines).
Hunk #4 succeeded at 1006 (offset -28 lines).
patching file redirect.test

1. +++ b/redirect.install
   @@ -356,3 +356,28 @@ function _redirect_migrate_path_redirect_variables() {
   +  $rid_to_delete = array();
   +  while ($record = $result->fetchAssoc()) {
   +    $rid_to_delete[] = $record['rid'];
   +  }
   

   Could just use $rid_to_delete = $query->execute()->fetchCol();

2. +++ b/redirect.install
   @@ -356,3 +356,28 @@ function _redirect_migrate_path_redirect_variables() {
   +    $deleted = db_delete('redirect')->condition('rid', $rid_to_delete, 'IN')->execute();
   

   Should this be using redirect_delete_multiple()?

3. +++ b/redirect.module
   @@ -408,6 +410,16 @@ function redirect_path_update(array $path) {
   +function redirect_path_insert(array $path) {
   +  if (!empty($path['alias'])) {
   +    // Delete all redirects having the same source as this alias.
   +    redirect_delete_by_path($path['alias'], $path['language'], FALSE);
   +  }
   +}
   +
   

   This is interesting. I wonder if it would be better *not* to delete existing data, and instead just make it not work and allow the alias to work. I don't feel great about deleting manual redirects/data.

4. +++ b/redirect.module
   @@ -1017,6 +1034,10 @@ function redirect_redirect($redirect = NULL) {
   +    // Prevent circular redirects.
   +    if ($GLOBALS['base_root'] . request_uri() == url($redirect->redirect, array('absolute' => TRUE) + $redirect->redirect_options)) {
   +      return FALSE;
   +    }
   

   This feels a little bit like the wrong location for this code. Should this be in path_redirect_alter() (as an implementation of hook_redirect_alter() for core's path.module)?

5. +++ b/redirect.test
   @@ -234,10 +234,14 @@ class RedirectFunctionalTest extends RedirectTestHelper {
   +    $this->assertResponse(200,"Changing node's alias back to 'first-alias' does not break page load with a circular redirect.");
   

   Missing spacing between the parameters here.

6. +++ b/redirect.test
   @@ -234,10 +234,14 @@ class RedirectFunctionalTest extends RedirectTestHelper {
   +    $this->assertResponse(200,"Changing node's alias back to 'second-alias' does not break page load with a circular redirect.");
   

   Same here.

Would it help if we could change redirects into some kind of 'disabled' state when this happens? I'd really rather not delete.

@Dave Reid: we see this issue happen only as described in #1, and the redirects created are never needed and need to be manually deleted to fix the bug. What's the use case for keeping them around, but disabled?

I agree with Dave Reid about keeping the redirects around. All it takes is a content editor unwittingly setting an alias that collides with some important redirects and the redirects are silently killed, never to be seen again.

I'd rather see logic implemented that prefers to serve the page with the alias if there's a collision, but doesn't delete the redirects.

re: #192.2
Can we do what commerce did recently? Way back in #51 I voiced caution about deleting redirects.

I really don't want us to bikeshed on this, as some version of a patch is in about every install profile that exists. What is the best path forward? I'd say we remove the hook_update_N and punt that to a follow-up.

Opened the follow-up @ #2343755: Follow-up: Fix existing bad redirect data to address #191.2

I'm with heddn in #196, but I'd say we need to not only remove the hook_update_N but also the calls to redirect_delete_by_path() in redirect_path_update() and redirect_path_insert().

Correct, those calls I'm also concerned about. Just moving the update hook doesn't solve it.

We haven't had any problems, at least that I know about, on Drupal.org, after running this on Drupal.org for a few weeks. That said, having the disabled redirects kept around would be a big help, if there were any problems. I think adding a disabled state for redirects would be a separate issue, which needs to be resolved before this.

If there's a collision, why doesn't the new redirect add -0, -1, etc.?

We really can't know which side of the collision to disable, the original or the new.

Don't understand why you don't want delete redirects which are not needed anymore.

Redirect is made for SEO and to be sure old path can be hit and return to right page (so new path).
If an another page build a path which is same as a redirect, redirect is no needed anymore and should be removed. Next time this path will be updated, a new redirect will be created to redirect to this new one.

Keeping old redirects disabled instead of deleted can be only useful for history, and to have a redirect table bigger (and so lower).

I agree with GoZ.
My advice:

• Commit patch #174 it is RTBC: it solves circular redirects.
• Make a new issue about the deletion/disable control issue (for all redirects).

I disagree with #202 and #203. Circular redirects are solved without the deletion of colliding redirects. You can remove that bit *and still have circular redirects solved*.

Especially on large sites with lots of editors I think it's bad policy to silently delete content (redirects) when people save nodes with colliding path aliases. I should not have to be nervous that creating some test page with any old path I might choose (or might have chosen for me by the pathauto settings) might, without my knowing it, deal hard-to-recover-from damage to someone else's carefully crafted redirects.

I understand that some (many?) people want the Redirects cleaned up. But I do not think that should be the default behavior. Redirect does not currently try to clean up collisions and adding that goes beyond the scope of what's needed to fix the actual problem at hand.

Redirect aren't real content but 301 to not loose users and seo.

Right now, creating node with title which will make same path as existing redirect will generate this path but never be reachable. Redirect take hand so we will never can access this node.

If we take consideration of your example: you create some test page with same path as existing redirect.
1\ Right now, you're page will never be available on your path, redirect will redirect to old page.
2\ If redirect is deleted, you're page will be available. If you delete your page, redirect will be lost (that you don't want).
3\ If redirect is disabled, you're page will be available. If you delete your page, redirect can be enable, but only if there is one redirect. What's happened if there is other disabled redirect with this path ? Which one do you want to enable ?

For 2 and 3, you must also take care of unpublish content. If you create page with same path as redirect and this one is deleted or disabled, until content is published, users will see an 'access denied' on this page.

Another solution will be:
4\ Never allow to create same path as existing redirect. If redirect exists, set -n+1 to path like pathauto already does with path collisions.

I'd prefer your option #4 to ensure that you can't create aliases or redirects on the same path as an existing redirect, but I don't think that's really tenable. There are many possible ways to create aliases and this would require API changes.

You are considering too narrow a use-case. On some sites redirects *should* be considered as content. Many sites use redirects to map multiple paths to a single node for SEO, marketing, or other content strategy purposes. Some sites may even use redirects in this way to maintain url mappings from old systems (pre-Drupal) to new content locations.