I have pathauto 2.0 running with the path redirect module. On pathauto settings, under "Update action", I have "Create a new alias. Redirect from old alias." which is AWESOME, but I just discovered a bug that I think is caused by this. I think it is caused by changing the title of the article and then later reverting to the original title (or one that would result in the same URL)
The error messages I get back from the browsers are:
-----------------------
Firefox:
The page isn't redirecting properly
Firefox has detected that the server is redirecting the request for this address in a way that will never complete.
* This problem can sometimes be caused by disabling or refusing to accept cookies.
Safari:
Safari can’t open the page.
Too many redirects occurred trying to open “http://www.example.org/news/charity-skydive-breath-life-sponsorship”. This might occur if you open a page that is redirected to open another page which then is redirected to open the original page.
-------------------
The Safari message was more useful in this case, and when I went to the URL redirects admin page, I noticed that there were several aliases pointing to node/471 - apparently the url was changed several times. One of the redirects was the same as the current URL being requested, so I guess it sends the redirecting into a loop and causes this problem.
I deleted the redirect that had the same URL as the current page alias and the problem was fixed. :)
I guess what needs to happen is that before changing the alias to the page, we need to check if there is already a redirect with the same alias and delete the alias if it exists, since the page no longer needs to redirect from there.
Now that I've written this out, I suspect that this is actually an issue with the pathauto module, but since I'm not sure I'm still submitting it here as well. Hope that's not a problem.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | path_redirect-218277-9-prevent_endless_redirects.patch | 1.34 KB | greggles |
| #10 | path_redirect-5.x-1.x_loopy_10.patch | 2.25 KB | HorsePunchKid |
| #9 | path_redirect-5.x-1.x_loopy_9.patch | 2.31 KB | HorsePunchKid |
| #3 | prevent_endless_redirects_218277.patch | 985 bytes | greggles |
Comments
Comment #1
OpenChimp commentedI created a duplicate of this issue on pathauto, which I think is the more appropriate place for it.
http://drupal.org/node/218279
Comment #2
HorsePunchKid commentedThis is probably a case that path_redirect should attempt to guard against. Detecting cycles in the redirect graph is not hard; I'm just not quite sure where or when it should be done. Maybe it would make sense to do it in the cron hook rather than trying to do it on every call to
path_redirect_save(which is where pathauto would probably need it done).I'm not sure this qualifies as critical, given that you can simply delete any of the redirects in the cycle to fix it.
Comment #3
gregglesYes, this is a bug in both Pathauto and path_redirect. If it is detected on cron then users will be qui
To repeat in path_redirect:
1. Create a node with the alias "some-alias" that is node 10
2. Create a redirect from "some-alias" to "node/10"
3. Trying to access "some-alias" will fail
That workflow is easy enough to protect against in path_redirect. A more complex workflow is:
1. With pathauto set up to use path redirect for the update action and to use [title-raw] as the pattern
2. Create a node with the title "some" - an entry is made in url_alias
3. Edit that node to be "some-other" - url_alias is updated, an entry is made in path_redirect
4. Edit that node back to "some" - url_alias is updated, an entry is made in path_redirect
The entry from step 3 is what causes the infinite redirects, but it doesn't cause them until step 4, so it's kind of hard to detect I think.
Another solution to this problem would be - "if the page that path_redirect is redirecting to is the same as the page that was requested, don't do anything"
I did a quick hack on that idea and it's attached. I think it should solve this problem. Note that the patch is against the latest DRUPAL-5 code which is quite different than the DRUPAL-5--1-1 so this won't apply to the 1.1 release of the module.
Comment #4
gregglesMy second sentence should have been something like "then users will be quite surprised when they submit a node and are endlessly redirected to an error page".
If you also want to check on cron that would help long term efficiency and be reasonable as well, probably. My patch just solves the "right now" part of the problem.
Comment #5
gregglesmore specific title.
Comment #6
greggles@horsepunchkid - any thoughts on committing this patch? Do you prefer a different approach?
Comment #7
HorsePunchKid commentedExcellent, no, I just missed the fact that there was a patch ready to go here! This won't catch all cycles, but it's a simple check that will probably handle the most common case. I'll test it out tonight and commit it if all is well. Thanks for the bump.
Comment #8
HorsePunchKid commentedTested, and it seems to work as advertised. What do you think about recording a warning in the watchdog when this happens?
Comment #9
HorsePunchKid commentedHere's a version with a warning. Absent feedback, I'll commit this tonight.
Comment #10
HorsePunchKid commentedSlightly better version.
Comment #11
gregglesSounds good. Here's a patch that does that.
It will need to be changed for 6.x based on the new watchdog params - I think it's just removing the t() function. http://drupal.org/node/114774#watchdog_parameters
Comment #12
gregglesYour #10 looks far better to me than my own. Thanks!
Comment #13
HorsePunchKid commentedOops, sorry for the crossed wires there. I committed my own version, but thank you for following up on the suggestion! I'll see about whipping up a 6.x version...
Comment #14
HorsePunchKid commentedPorted to 6.x; minor change to check that
$ris defined, plus the watchdog API change.Comment #15
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.