Posted by sylv3st3r on September 8, 2010 at 8:06pm
85 followers
| Project: | Redirect |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | D7 stable release blocker, Needs issue summary update, Needs tests |
Issue Summary
The Drupal 6 Global Redirect module provides a lot of features like:
- Checks the current URL for an alias and does a 301 redirect to it if it is not being used.
- Checks if the current URL is the same as the site_frontpage and redirects to the frontpage if there is a match.
- Checks if the Clean URLs feature is enabled and then checks the current URL is being accessed using the clean method rather than the 'unclean' method.
- Checks access to the URL. If the user does not have access to the path, then no redirects are done. This helps avoid exposing private aliased node's.
- Make sure the case of the URL being accessed is the same as the one set by the author/administrator. For example, if you set the alias "articles/cake-making" to node/123, then the user can access the alias with any combination of case.
Checks the current URL for a trailing slash, removes it if present and repeats check 1 with the new request.
This patch aims to add those features (and the ability to configure them) into the Redirect module.
Comments
#1
Hrm, you can describe more about what's going wrong? I can't duplicate this currently.
#2
No response in over a month, so marking as fixed.
#3
Automatically closed -- issue fixed for 2 weeks with no activity.
#4
Hey, I got the same issue.
I enter URLs to redirect from and redirect to and press "Save".
I got moved immediately to correct redirect page (the one in Redirect To field) but when I go back to the URL Redirects page I can see no redirect entries at all. The list is empty.
#5
I still cannot duplicate this. Can you please post the details on the redirect you were trying to save?
#6
Redirect From = relative old path, pointing to a non-existent page, a remnant of an old site, for example:
texts/text.htmlRedirect To = relative path to actual page, for example:
articles/textWhen I press Save, I got redirected immediately to articles/text page (instead of staying in URL Redirects settings page).
If I move back to admin/config/search/redirect, I cannot see any entries. The redirect table in site DB is empty as well.
I do not have Global redirect or Path redirect modules installed.
#7
Ok, I got it figured out.
You cannot create URL Redirect if the Redirect To path is a synonym OR has a synonym defined.
You won't receive any warning about that, you just got moved silently to the target web page.
#8
Further info:
the problem is with
// Normalize the path.$value = drupal_get_normal_path($value, $form_state['values']['language']);
part of the code (redirect.admin.inc, line 363). When drupal_get_normal_path got invoked, it receives a "normalized", i.e. "non-synonym" path as a $value.
Then drupal_get_normal_path itself performs a hook_url_inbound_alter(), so Redirect module receives the priority again and redirects us to the "synonym" page asap.
It means the code after line 363 is never get executed IF the path has a synonym defined.
#9
sub
#10
Can someone confirm that this patch : http://drupal.org/node/944120 resolve this issue? I haven't tried it since redirect module breaks my cron.
#11
No, that patch does not fix this issue.
I have tested the last dev. version with that patch incorporated, but the reason of this erroneous behaviour I have already explained in the above posts.
Actually escalating this to Critical, because the main function of the module is broken - and I think like 90% of the sites use path aliases.
I will give it a good debugging again when time allows and MAYBE I will be able to come up with the solution. Still, want to hear from Dave if he was able to reproduce this issue.
#12
Yeah I confirmed this. I'll have to disable the global redirection functionality temporarily until I can figure out a solution.
#13
I am experiencing this issue as well.
#14
subscribing
#15
I just committed the temporary disabling of global redirection until I can work out a better solution. It may have to wait until after 7.0 release.
#16
Please, take a quick look at this part of code (redirect.admin.inc, line 356 onwards):
function redirect_element_validate_redirect($element, &$form_state) {
// $value = &$element['#value']; <== possibly disabling this
_redirect_extract_url_options($element, $form_state);
$value = &$form_state['values']['redirect'];
// Normalize the path.
// $value = drupal_get_normal_path($value, $form_state['values']['language']); <<== and this
if (!valid_url($value) && !valid_url($value, TRUE) && $value != '<front>' && $value != '') {
form_error($element, t('The redirect path %value is not valid.', array('%value' => $value)));
}
return $element;
}
I have disabled these two lines of code and everything seems to be working as designed.
The reason to disable the drupal_get_normal_path call is this: you have already called _redirect_extract_url_options (line 358), which performs drupal_get_normal_path all right (line 404), so the $value (line 359) contains normalized path. When you call drupal_get_normal_path for the second time, you got redirected immediately via hook call because you are not supposed to pass normalized paths to drupal_get_normal_path function!
So I would just roll back the last change (enabling global redirect again) and change just these two lines (commenting or deleting them) to fix this issue. Works perfectly for me. At least I can not see any possible flaws in this solution.
P.S. Plus one more minor fix: line 378
$options = & $form_state['values']["{$type}_options"];should not contain a space between "&" and "$" symbols.
#17
Changing scope and priority
#18
Here's a patch that
a) does what Azol describes in #16
b) renables the global redirect functionality. I also had to change the $redirect variable to a stdClass instead of an array because that gave a lot of notices
Global redirect seems to be working fine now (so node/1 to 'a-nice-alias') and managing redirects in the admin too.
#19
Needs work, still have the problem of editing an existing alias, the value never gets saved, will look into it in a few moments.
#20
Ok temporarily a patch which makes editing redirects or aliases work. Removing the code as suggested in #16 is not a solution, so that code is back in. Path.admin.inc also calls drupal_get_normal_path() in its validate function so you get redirected when editing url aliases.
I've added (a very ugly) check in redirect_url_inbound_alter() checking on arg(2) and arg(3) for 'config' and 'redirect' or 'path'. This works for now so we can edit which is good, however, this can never be RTBC.
I think we should find another way than using redirect_url_inbound_alter() because as soon as someone calls drupal_get_normal_path() you might get redirected without knowing what's going on. @Dave Reid If you have suggestions, add some comments or ping me on IRC, I'd be happy to try and test other things out for this patch.
#21
What setup do you have? #16 worked for me so far...
#22
Azol, try editing an existing URL alias (at admin/config/search/path), you'll be redirected too and no save.
#23
sub
#24
Subscribing
#25
sub
#26
Patch from #20, corrected, simplified, and also avoids redirecting from cron.php, xmlrpc.php, update.php, etc.
EDIT: Also filed bug against core: #1217668: drupal_path_initialize() should not call drupal_get_normal_path() when run from cron.php.
#27
Additional fix to avoid php 5.3 errors from implicitly casting an array to an object.
#28
The last submitted patch, redirect_object.patch, failed testing.
#29
Trying again, with the correct patch file this time.
#30
Re-rolling patch from #30, removing extra tabs/spaces from two line endings.
#31
Revised patch also prevents the redirect from running under drush and other CLI applications.
#32
Improved patch re-enables the relevant admin settings and also leverages the existing redirect_can_redirect() function.
#33
I patched with #32, but got the following error when visiting /index.php:
Recoverable fatal error: Argument 1 passed to redirect_object_prepare() must be an instance of stdClass, array given, called in drupal/sites/all/modules/redirect/redirect.module on line 930 and defined in redirect_object_prepare() (line 649 of drupal/sites/all/modules/redirect/redirect.module).
#34
Updated patch.
#35
The last submitted patch, redirect-restore-global-redirect-905914-34.patch, failed testing.
#36
Fixed typo.
#37
The last submitted patch, redirect-restore-global-redirect-905914-36.patch, failed testing.
#38
Trying again...
#39
(sigh) I'll get it right eventually...
#40
I don't think it's quite working the way intended.
Does what I'm experiencing mean that the issue needs to go back to "Needs work", or that I need to submit a new issue?
#41
Reviewing the configuration I realized that this was a new feature, so I added a new issue: #1255998: Option to redirect to the site root directory if the path == site_frontpage
#42
DaveReid, what re-engineering were you intending before re-adding this functionality?
#43
Works for me. I'm not that experienced with this module, so I don't want to set this to RTBC. Anyone else?
#44
#45
#39: redirect-restore-global-redirect-905914-39.patch queued for re-testing.
#46
args() is being crushed by hook_url_inbound_alter which means any code that relies on it is falling down. My fix is just to totally remove redirect_url_inbound_alter. I'm going to roll with this until something better comes along.
#47
Please cite a reference or otherwise support your assertion. I see nothing in the code you remove that changes the output of the
arg()function.Furthermore, you are hijacking this issue with an unrelated patch which has nothing to do with the problem and fix addressed by this issue. Please open a separate issue, instead.
Re-uploading the working patch which is currently under review.
#48
#49
#47: redirect-restore-global-redirect-905914-39.patch queued for re-testing.
#50
Anyone else running into problems with canonical URL support clashing with the pager and throwing the browser into an infinite loop?
#51
@DamienMcKenna -- Please describe how to reproduce this error so we can write a test case and fix it.
#52
I'm not really sure what I am reviewing here, I was hoping this patch would add the functionality of global_redirect module into redirect, but after testing I think maybe that's not the case - so I'm leaving as needs review.
Results of testing: navigating to node/15 did not gently redirect me to the drupal_get_path_alias() version of the path. My page still loads and the URL says node/15. Similarly, navigating to index.php?q=node/15 also did not redirect to the clean-urls version of the path :(
For now, I'm still going to use both modules, but it would ne nice if we could get all the old D6 redirect modules into Redirect :)
#53
@pillarsdotnet: I'm using a Panels node view for the content type, on the node I have a view with a pager, clicking on any page in the pages list should go to e.g. /mypage/page/1/0 but instead is redirected back to /mypage. I haven't dug into it too much, but disabling the "redirect canonical URL" option fixes it.
#54
@jenlampton: Did you enable the "redirect canonical URL" option? What options did you enable in the Redirect settings page?
#55
@DamienMcKenna I am running the dev snapshot from Dec 2, and *thought* I had applied this patch. It appears 'git apply' didn't work at all for me, and after you asked me if I'd turned on canonical redirection, I realised that I'd never even seen that option! :) a patch -p1 did the trick for me this time, but bad news...
With the following settings, I get this error when visiting index.php?q=node/15:
Recoverable fatal error: Argument 1 passed to redirect_redirect() must be an instance of stdClass, array given, called in .../sites/all/modules/contrib/redirect/redirect.module on line 287 and defined in redirect_redirect() (line 916 of .../sites/all/modules/contrib/redirect/redirect.module).Redirect from paths like index.php and /node to the root directory. (Checked)
Redirect from non-clean URLs to clean URLs. (Checked)
Redirect from non-canonical URLs to the canonical URLs. (Checked)
Remove trailing slashes from paths. (Checked)
Allow redirections on admin paths. (Un-Checked)
#56
The last patch was not applying for me. Attached is a re-roll. I've yet to test the actual workings of the patch.
#57
Working on the patch from #56 (which applies cleanly) :)
- changing pathauto "patterns" and re-saving node with pathautp checkbox checked redirects.
- changing node title and re-saving node redirects.
- visiting the Drupal system path for the node (node/1) redirects.
- visting non-clean url (?q=node/1) redirects.
- visiting actual full url (index.php?q=node/1) also redirects
- visiting any one of the paths above with a trailing slash, redirects.
- visiting the Drupal system path with a query string (node/1?foo=bar)redirects, retaining the query string.
Anything else I should be testing? test bot?
#58
@jenlampton: did you try a pager? E.g. attach a view to a page and have sufficient records in the view that it makes a pager display, the URLs will be in the form /clean/url/page/0/1 but they all redirect back to /clean/url.
#59
A page-view pager seems to be in the format pager-test?page=1. A block-view pager also adds a query string articles/test-article?page=4.
... still not sure where the redirects come in for views.
But Test bot likes it :)
#60
#1255998: Option to redirect to the site root directory if the path == site_frontpage
Given that the issue above has been marked a duplicate of this issue, I imagine that after applying this patch, the path I have specified as my front page should be redirected to the to the sites base URL, but this is not the case.
Steps to reproduce:
#61
So in other words with this patch Redirect module should be replace for Global Redirect module ?
#62
I'm running into the same problem with front page, but all other redirections are working. Changing status.
@batigol, yes :)
#63
I have added global home page redirect for both clean and "dirty" URLs for AntoineSolutions and myself.
It's reworked patch from #56 and it should do all things from #57 and a bit more
* example.com/index.php?foo=bar&q=node/123 -> example.com/node/123?foo=bar
* example.com/index.php?q=node/123&foo=bar -> example.com/node/123?foo=bar
* example.com/index.ph?q=node -> example.com/ (if "node" is home page)
* example.com/node -> example.com/
#64
The last submitted patch, redirect-frontpage.patch, failed testing.
#65
patch fixed (i hope)
#66
I don't use front page on my site so I can't test it. But it seems to work fine on my friend page.
However do I need to install Global Redirect in addition to this module ? Does redirect works the same way or there are some differences ?
#67
@pwinlacki -- With the patch, the Redirect module should do everything that Global Redirect does, and more.
#68
Pillarsdotnet thank you for clearing this out. I can't wait till beta5/rc1 release !
#69
I would note one specific thing: You should not set alias (welcome) for front-page path (node/1). If you do this redirect will send you to front page's alias (http://example.com/welcome) if you type site URL (http://example.com)
Is this a bug to resolve?
#70
@fasdalf@fasdalf.ru -- You should not RTBC your own patch. Also,
+ $is_query = (strpos($request_uri, '?q=') !== FALSE) || (strpos($request_uri, '&q=') !== FALSE);Yuck! Isn't there anything in the Drupal API to accomplish this?
#71
New patch checks if requested page is front and skips redirect to alias.
#72
The last submitted patch, global.patch, failed testing.
#73
Patch rebuilt
#74
The last submitted patch, global.patch, failed testing.
#75
#76
The last submitted patch, global.patch, failed testing.
#77
Sorry. Can't figure out what it's don't like. Attach is new version of home/front page feature.
#78
After I figured out what you changed in the patch file, I applied the same changes to my own copy of redirect. I do not see the site redirecting from 'welcome' to the front page with no path (to use your example).
Trying a new approach.
#79
The last submitted patch, reenable_global_redirections-905914-78.patch, failed testing.
#80
Hm, I'm not sure why that test failed.
I disabled cleanURLs and un-checked the cleanURL redirect checkbox, and my site redirects from ?q=redirect to ?q=node just fine.
Rerolled with one other minor bug fixed, but if the test needs to be rewritten I may need some help with that.
#81
The last submitted patch, redirections_with_frontpage-905914-80.patch, failed testing.
#82
#83
The last submitted patch, redirect-global-redirection-905914-82.patch, failed testing.
#84
IMHO there is a problem with this code
+ // Redirect to front page.+ if (variable_get('redirect_global_home', TRUE) && drupal_is_front_page()) {
+ $redirect_global = TRUE;
+ $request_uri = '';
It will make endless redirect. I believe it should be like this:
+ // Redirect to front page.+ if (variable_get('redirect_global_home', TRUE) && drupal_is_front_page() && isset{$_GET['q']}) {
+ $redirect_global = TRUE;
+ $request_uri = '';
#85
@fasdalf: I believe you meant:
+ if (variable_get('redirect_global_home', TRUE) && drupal_is_front_page() && isset($_GET['q'])) {#86
Also see http://drupal.org/node/1183208
#87
This is slightly related. The stub for implementing global redirect functionality is causing problems with some modules: #1463010: Entity_load during url_inbound_alter is incompatible with text fields
#88
in response to #85,
isset($_GET['q']))doesn't prevent the infinite redirect on the home page either. Would love suggestions as to how to get that working.#89
Okay, here I tried to determine if we were on the front page already by comparing $_SERVER['REQUEST_URI'] to base_path().
Also trying a new approach, this time with the redirect firing in hook_url_inbound_alter instead of in hook_init.
#90
The last submitted patch, enable_global_redirections_with_frontpage-905914-88.patch, failed testing.
#91
and one more time sans dsm.
dunno why tests are failing, but maybe someone else can help with that?
#92
The last submitted patch, enable_global_redirections_with_frontpage-905914-91.patch, failed testing.
#93
Please note that calling entity_load() can break things if done before menu_get_custom_theme().
See #1370964: Redirect interferes with hook_custom_theme for details.
#94
This patch results in infinite redirections when Persistent URL (purl) module is installed. Investigating.
#95
Doing a redirect from hook_url_inbound_alter is not right; this hook is called from drupal_get_normal_path(), and callers of drupal_get_normal_path() certainly do not expect a redirect to be done.
IMHO all redirects should be done in hook_init().
#96
#91: enable_global_redirections_with_frontpage-905914-91.patch queued for re-testing.
#97
The last submitted patch, enable_global_redirections_with_frontpage-905914-91.patch, failed testing.
#98
Ok, so the errors are because the test expects the user to be redirected to the actual redirect url, but it might (should) redirect to the frontpage (no internal path) if the redirect url is actually the frontpage. So I guess we have to fix the test.
There's some logic already available at redirect_url_inbou'nd_alter checking for the redirect_global_home variable. And I don't think it should default to TRUE, since that wasn't the behaviour before. I'm just changing that and hoping it fixes the tests until we improve them.
#99
Marked #1793330: Global redirect feature is not working at all as a duplicate.
#100
Re-rolled against current 7.x-1.x checkout.
#101
The last submitted patch, 905914-redirect-reenable-global-redirections-100.patch, failed testing.
#102
Missed a closing brace; sorry.
#103
The last submitted patch, 905914-redirect-reenable-global-redirections-102.patch, failed testing.
#104
Not sure whether the static is supposed to store a single redirect or an array as returned from (e.g.) redirect_load_by_source().
Going with the latter for now...
#105
#106
Patch works great for me!
#107
Removed whitespace errors too.
#108
Can someone update the issue summary? The patch is RTBC and passing tests, but I don't remember what it's supposed to do. At a camp this weekend so might have time to commit this.
#109
Tagging.
#110
I tried to update the issue summary based on reading many of the comments here (and skimming some). I didn't review the code.
#111
reverting unintentional de-tagging...
#112
Update: Patch still applies cleanly, and still works :)
#113
Having got rid of the module globalredirect recently and added redirect, I did miss the functionality that globalredirect provided around sending 'node/1234' => path_alias.
I applied this patch to 7.x-1.x-dev, and it mostly seemed fine. The only major issue is that the homepage no longer is accessible on '/', as it redirects to 'home' (the path alias for node/1). This some how causes panels selection criteria to break (page is the site front page).
Obviously there is some work around the homepage that needs to be completed before this is ready to go.
I started altering the patch, and then worked out it would probably be easier if I merely implemented hook_init() myself and do the redirect for node/1234 => path_alias
#114
One of the reasons for implementing global redirect is to avoid having two urls point to the same content. If the
example.comfront page is set tohomewhich is an alias fornode/1then exactly one of the two rules should be enforced:http://example.com/node/1orhttp://example.com/... should redirect to
http://example.com/homehttp://example.com/node/1orhttp://example.com/home... should redirect to
http://example.com/Do your panels selection criteria only break when rule #1 is active, or do they also break when visiting
http://example.com/node/1orhttp://example.com/homedirectly? If the latter, then I'd say you have identified a bug in panels, not in the redirect module.#115
#116
When I had the globalredirect module enabled,
node/1would go to '/' which was perfect as it was the homepage.With this patch applied, this is what happened:
1) Visit '
/', redirected to '/home', panels selection criteria fail2) Visit '
/node/1'. redirected to '/home', panels selection criteria failI don't know if it makes any difference, but the site I am working on is multilingual as well (so there is indeed two site_frontpage)
#117
@pillarsdotnet i think the expected result is option #2: both node/1 and /home should redirect to /
I think that's what @wiifm says in #116 and that's what I'd expect as well. I think (from #98) that there's a variable (
redirect_global_home) and a test for that.#118
I'm putting this patch on yet another site, here is what I did:
1) enable redirect module (dev version, with patch applied)
2) go to redirect settings page to enable 'global redirect' functionalty
3) enable the setting "Redirect from paths like index.php and /node to the root directory."
And here are my tests:
Navigate to the home page '/' - expect no redirection. Works as expected.
Navigate to my panel for use on the front page '/frontpage' - expect redirect to '/' - Works as expected.
#119
changing status, I have a feeling you just forgot to check the box to turn on the home page redirect feature. I can't get it to misbehave. :)
#120
Reroll.
Minor change
<?php+ 'redirect_global_home' => FALSE,
+ 'redirect_global_home' => 0,
?>
and
<?php+ '#default_value' => variable_get('redirect_global_home', FALSE),
+ '#default_value' => variable_get('redirect_global_home', 0),
?>
#121
Another style error :(
However, this doesn't completely work as expected for pages with aliases.
Enable Path & Redirect
Create a node and set alias to home
Check redirect_global_home
Set Site Information Default front page to home
node/1 redirects to home
Set Site Information Default front page to node/1
node/1 redirects to home
A quick look shows that on /home, $_GET['q'] is set to the alias when drupal_is_front_page() is called.
No time to investigate further, but the quickest workaround I saw was to remove the alias, and to use Page manager / Panels to embed the node on a custom page with the URL home and setting this as the default front page in the site Information settings.
#122
I notice that this patch includes new functionality, but doesn't include any tests.
The discussion in #117/119 makes it clear that some tests could be helpful to be more confident that the functionality works as expected.
#123
Changing status as per Greg's comment and issue related to path aliases noted in #116 & #121.
#124
The functionality I'm looking for is the "Deslash" setting from Global Redirect that 301's a request for about-us/ to about-us
I've applied #121 to latest dev of Redirect and it looks like the setting redirect_global_deslash (Remove trailing slashes from paths) doesn't Deslash anything and the setting isn't being referenced anywhere.
#125
The work being done here is great, and is moving in the right direction. Although it does what it is supposed to, I've discovered a conflict that it produces. I am using the Video Embed Field module to display youtube videos in a colorbox on my site. With the latest version of the patch applied, a $variables array created by Video Embed Field is not populated correctly when using a colorbox to load the video. As a result, the video does not display. I originally opened an issue over there at #1908058: $variables array not being built correctly when using colorbox..
Not sure where the conflict is exactly, but I figured I should point it out. Let me know if you need more clarification.
#126
Only thing holding me back from making the jump to this module is the entity id url -> alias redirection. IMO that is one of the best features of globalredirect, but I need my clients to have an admin interface for redirects. Any ideas of when this feature will come in?
#127
After reading this post as well as several others in the queues about the differences between Global Redirect and Redirect, I too am confused as to a few things. First, Obviously we want the functionality of cleaning up duplicate content that Global Redirect provided.
Also, in Pathauto config screen under settings recommends using the "Redirect" module when "leaving old aliases intact when generating new aliases." Which, is of course what I want as I am about to do a massive url structure rewrite and want redirects from the old aliases.
So, Can I use both Global Redirect and Redirect despite the effort of merging the former into the latter? Are their conflicts?
Secondly, as of the patch and few comments above, this still needs testing and people are finding minor bugs.
So then, what is the stable solution? Do / Can I use both Global Redirect and Redirect together? Or is the merging functionality stable enough that it has made GR obsolete, and I can happily get rid of it and use just Redirect as the complete solution?
Thanks!
#128
I'm currently running an older dev release of Redirect that contains the alias redirecting from the system paths which I needed. I disabled globalredirect and enabled that and it works a treat. I don't think it'd be a good idea to run the 2 side by side though.
#129
Here's where I'm at working on #121:
* Got Deslash (Remove trailing slashes from paths) working.
New features:
* redirect_global_index setting to remove 'index.php' from non-front paths.
* redirect_global_canonical_front setting to indicate if you want the front page to redirect to its canonical URL. I found that with the existing redirect_global_canonical setting, a request for the root "/" would redirect to "/welcome-to-my-site" because that is the front page canonical URL. That's undesired behavior in most cases.
I've tested this with pathauto and subpathauto with success. Obviously it still lacks test cases.
The big structure question I have is should this global behavior reside in redirect_url_inbound_alter() or redirect_init()? I basically left it as it was - calls to redirect_redirect() split between the two. I'm guessing redirect_url_inbound_alter() should only contain calls to redirect_set_current_redirect() and no calls to redirect_redirect() so that other modules can still execute their hook_url_inbound_alter(). Then redirect_init() can use redirect_get_current_redirect() and do the redirect.
#130
Here is #129 + Add Slash functionality for use with the Trailing Slash module to enforce trailing slashes.
#131
I used #130 for a week, works for me so far. Thanks.