Support from Acquia helps fund testing for Drupal Acquia logo

Comments

christefano’s picture

Priority: Normal » Critical

This behavior is cruel and unexpected. When landing on a page that doesn't exist, site visitors should be able to use the search box to find their way out.

gpk’s picture

lyricnz’s picture

Please fix this. The problem seems to be caused by the fact that the search form (at the top-right of the page) has ACTION=[currentpage], which (when it is a 404) isn't handled properly.

greggles’s picture

Status: Active » Closed (duplicate)

@lyricnz - could you file a patch for that in the search issue queue?

#302726: search returns the same page when initiated from a search box on a 404

christefano’s picture

Why was this issue marked as a duplicate? It is almost a month older than #302726.

greggles’s picture

That one is in the right queue. That's why I marked this a dup.

If you want to move this to the search module queue and mark the other a dup of this, you certainly could.

christefano’s picture

Project: Drupal.org infrastructure » Drupal core
Version: » 5.x-dev
Component: Drupal.org theme » search.module
Status: Closed (duplicate) » Active

#302726 is a duplicate of this issue.

lyricnz’s picture

Version: 7.x-dev » 5.x-dev
Status: Needs work » Needs review
FileSize
367 bytes

The reason this is happening is that:
- the search form is submitted to the current page
- the submit handler redirects this to search/node/[text]
- the submit handler is NOT called when the menu-entry cannot be found (the bug we're observing) [EDIT: incorrect, see below]
- this is not normally a problem because the 404 page doesn't display this form (but d.o does)

We can fix this by defining #action for the search, so the form is posted to /search rather than the current page. However this might effect people who are doing log-analysis, so it's perhaps a little disruptive. Attached is patch for Drupal 5 - if people think this is the right thing to do, I can produce a patch for D6 + D7.

moshe weitzman’s picture

Status: Active » Needs review

Looks like the right fix to me. Do D7 and D need this patch?

greggles’s picture

D6 definitely does - D7 might.

I don't understand the log implications to this...if anything it seems like an improvement for logs but that is based on a limited understanding.

swentel’s picture

D7 and D6 could use the patch also, same thing happens in those versions.
Note: 'drupal-5.10' should be stripped out of the path in the current patch.

lyricnz’s picture

Sure, attached.

lyricnz’s picture

FileSize
343 bytes

Updated D5 patch without "drupal-5.10". Attached.

The change in the logging refers to the fact that the apache access log used to look like:

"GET /drupal5/node-40-story HTTP/1.1" 200 17971
"POST /drupal5/node-40-story HTTP/1.1" 302 -
"GET /drupal5/search/node/search+text HTTP/1.1" 200 17568

and now looks like

"GET /drupal5/node-40-story HTTP/1.1" 200 17950
"POST /drupal5/search HTTP/1.1" 302 -
"GET /drupal5/search/node/search+text HTTP/1.1" 200 17517

I don't think this is probably a big deal, but thought I should point it out.

Damien Tournoud’s picture

I don't buy this. If the form is display, the form handler should fire, whether or not we are in a 404 page: the form itself is generated with drupal_get_form(), which is also the form handler.

Looks like a d.o specific error to me: probably bluebeach doing something wrong (like outputting a form without calling drupal_get_form or something).

lyricnz’s picture

It's pretty easy to reproduce: in your theme page.tpl.php, copy the line that says something like " print $search_box " up to the top of the page, where it is unaffected by blocks/nodes etc. The problem occurs on all my D5/6/7 clean installs.

From what I can tell (and I'm no menu expert) the form handlers simply aren't invoked if no matching menu entry is found - menu_execute_active_handler() doesn't know where to go.

Damien Tournoud’s picture

Version: 5.x-dev » 7.x-dev
Status: Needs review » Needs work

The form submit handler is correctly fired. The issue is that drupal_not_found() sets $_REQUEST['destination']:

  // Keep old path for reference.
  if (!isset($_REQUEST['destination'])) {
    $_REQUEST['destination'] = $_GET['q'];
  }

... while drupal_goto() (which is called by drupal_redirect_form()) respects that $_REQUEST['destination']:

  if (isset($_REQUEST['destination'])) {
    extract(parse_url(urldecode($_REQUEST['destination'])));
  }

This means that no form can work today correctly on a 404 page. Weird. We should probably change drupal_not_found() somehow. Bumping to 7.x to get more eyes on that.

lyricnz’s picture

Hmm, good spotting (I really should get my debugger going, methinks). I guess this also applies to drupal_access_denied() ?

Is there a better fix for D5/6 than that suggested above?

gpk’s picture

Note this also appears to affect http://api.drupal.org/api/function/drupal_access_denied/7 (I've had a similar problem with the login block on an access denied page in 5.x), as well as http://api.drupal.org/api/function/drupal_not_found/7. All versions of Drupal since 4.7 would appear to be affected.

lyricnz’s picture

FWIW, the same problem was reported, and "fixed" basically the same way in 2006:
#67648: search field on 404 page not functional

Damien Tournoud’s picture

FWIW, the same problem was reported, and "fixed" basically the same way in 2006.

Well, we should get it right this time.

robertDouglass’s picture

Interesting problem.

Jody Lynn’s picture

Good call lyricnz. I tracked it down in the CVS commits and indeed this same issue was fixed in #67648: search field on 404 page not functional in 2006 in the same way planned here. It remained fixed until 2007 when #104662: Search blocks breaks if the block is not visible on the search page. was committed which undid the fix to solve a different problem. This undoing was backported from 6 to 5, which is why we have the original problem back in all versions.

See commit: http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/search/search.modu...

We need to put the original fix back into place while also re-solving the problem in #67648: search field on 404 page not functional

Damien Tournoud’s picture

John Morahan’s picture

FileSize
2.02 KB

Here's a patch that simply adds a test for each of these conditions (search from 404, and no block on the search page). I've made no attempt to actually fix it.

loyukfai’s picture

Seems the latest 6.x-release still has this problem...?

gpk’s picture

@26: correct, it should be fixed first in 7.x and then backported to 6.x and 5.x.

webchick’s picture

The problem w/ #13 is it's the exact opposite of the fix for #104662: Search blocks breaks if the block is not visible on the search page.. We need a fix for both problems. In other words, I can't commit any patches unless the tests in #25 pass. (Thanks, John Morahan!)

JohnAlbin’s picture

Version: 5.x-dev » 7.x-dev
Status: Needs review » Needs work

Fixing this with a form_alter() is annoying. subscribing

webernet’s picture

kleinmp’s picture

FileSize
4 KB

Here's a patch of drupal_redirect_form() that fixes both issues mentioned above (for me). It passes the search tests, though I wouldn't be surprised if it brakes something else.

I also included the search tests from #25 in the patch.

greggles’s picture

Status: Needs work » Needs review

Right?

dylan_thomas’s picture

#12 patch worked for me on 6.8

lyricnz’s picture

dylan_thomas: as mentioned by webchick in #28, my patches addressed the issue in a way that would cause a regression in another part of the system. Someone with better understanding of form processing than I needs to address this.

Jody Lynn’s picture

Status: Needs review » Reviewed & tested by the community

Kleinmp's patch in #31 looks good to me and indeed fixes both problems and passes all tests. Setting to RTBC until someone finds an issue with it. Needs to be backported after commit.

Dave Reid’s picture

Status: Reviewed & tested by the community » Needs work

Sorry to mark this as CNW, but it adds extra unwanted whitespace:

@@ -1105,7 +1105,7 @@ function search_box(&$form_state, $form_
   );
   $form['submit'] = array('#type' => 'submit', '#value' => t('Search'));
   $form['#submit'][] = 'search_box_form_submit';
-
+  
   return $form;
 }
 

Also, this block from the test should be moved to setUp():

+    // Disable the theme form to avoid confusion.
+    $settings = theme_get_settings();
+    $settings['toggle_search'] = FALSE;
+    variable_set('theme_settings', $settings);
wretched sinner - saved by grace’s picture

kleinmp’s picture

Status: Needs work » Needs review
FileSize
2.65 KB

I rerolled the patch after making the changes from comment #36.

Status: Needs review » Needs work

The last submitted patch failed testing.

John Morahan’s picture

Status: Needs work » Needs review
FileSize
2.84 KB

The addition of 'search content' to the $admin_user permissions got lost in the last patch.

webchick’s picture

Status: Needs review » Needs work

YAY!!! It works!! :D Testing bot is happy too!

Could we please add a comment above these lines to explain what's going on:

+        if (isset($_REQUEST['destination']) && $_REQUEST['destination'] == $_GET['q']) {
+          unset($_REQUEST['destination']);
+        }

So someone doesn't try and "fix" this again in the future? :P

Once that's done it will be with great pleasure that I commit this. :) Let's get a backport ready for Drupal 6 too so d.o stops doing this.

wretched sinner - saved by grace’s picture

+        if (isset($_REQUEST['destination']) && $_REQUEST['destination'] == $_GET['q']) {

I thought that there was a general move away from using $_GET and friends. I think it was a post on crell's blog that drew my attention to it. If so, would it be better to refactor this to not use $_GET if possible?

If I am mistaken, or it is a major mission, then ignore me :)

John Morahan’s picture

@wretched sinner: I do not think that applies in this situation.

However... while this fixes the search form, shouldn't the fix be above the if (is_array($goto)), not just inside the else? That way, it should work for all forms, including those that redirect to GET parameters like views_filterblock for example.

John Morahan’s picture

Well, that's a bad example because it sets #action but you get the idea.

John Morahan’s picture

Status: Needs work » Needs review
FileSize
3.06 KB
Jody Lynn’s picture

Status: Needs review » Reviewed & tested by the community

This looks good.

webchick’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

YAY!! :D Committed. :D

Could someone please please please back-port this to D6?

akalsey’s picture

I have a d6 backport. Do I create a new issue for it, or attach it here?

webchick’s picture

Attach it here, please.

akalsey’s picture

FileSize
540 bytes

D6 Backport of patch in #45

webchick’s picture

Status: Patch (to be ported) » Needs review

Marking needs review.

Status: Needs review » Needs work

The last submitted patch failed testing.

John Morahan’s picture

Version: 7.x-dev » 6.x-dev
Status: Needs work » Needs review
christefano’s picture

Status: Needs review » Fixed

It looks like this was fixed in #180416.

John Morahan’s picture

Status: Fixed » Needs review

No. That was for D7. This is for D6.

loyukfai’s picture

Any chance the fix can be in place for the next D6 release...?

pivica’s picture

FileSize
844 bytes

Here is a patch against Drupal-6-dev. I have tested on clean Drupal installation and its work fine for me.

Jody Lynn’s picture

Status: Needs review » Reviewed & tested by the community

Marking RTBC because the #57 patch is a tested duplicate of #50.

estim8d’s picture

I have this bug too... D6

loyukfai’s picture

I applied the patch from #57 on a 6.10 install and now searches initiated from 404 pages seem to be just fine.

Bartezz’s picture

Hmmz I have problems with the login block on access denied pages as well. (D5)
I never understood why, when I login on an access denied page, I never get logged in it just redirects me to a page like domain.com/?destination=admin which does as much as bringing me to the homepage....

Is this related?

Cheers

Jody Lynn’s picture

@Bartezz: yes, it's the same issue and this patch should fix that as well.

Can't wait to see this committed to D6 as this problem plagues me on d.o. constantly

Bartezz’s picture

Thanx for the reply Jody! Will this be committed to D5 core? I just updated all sites to 5.16 and I don't think it was committed to that yet?

Cheers

PS. which patch is for D5?

Damien Tournoud’s picture

Patch in #57 applies cleanly to 6.x, and is a direct backport of the fix that went in D7. Acked from me.

Jody Lynn’s picture

@Bartezz Bugs are fixed first in HEAD (D7), then backported to D6, then backported to D5. Once this is committed to D6 we will backport for D5.

Bartezz’s picture

Alright, thanx will have to wait ey :)

Gábor Hojtsy’s picture

Version: 6.x-dev » 5.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks for the extensive analysis and testing. Committed to Drupal 6!

Moving to Drupal 5 for review.

grendzy’s picture

Status: Patch (to be ported) » Needs review
FileSize
840 bytes
bwynants’s picture

this patch (http://drupal.org/comment/reply/292565/1416460#comment-1365792) seems to break redirecting after login on my latest 6.x cvs version. To be clear I have not applied the patch, just synced in CVS...

bwynants’s picture

Version: 5.x-dev » 6.x-dev
Status: Needs review » Needs work

Put back at 6.x

lyricnz’s picture

How are you redirecting after login?

bwynants’s picture

When using the login-block from the user module redirecting is done automatically by drupal to the active node/location by adding '?destination=xxxxx' to the url....

This patch strips that and it does not work anymore....

mrfelton’s picture

Patch at #57 does not work for me on D6. I assume I'm running into the same issue here. I get a 404, and then try to search using the search box that is embedded into the page by the theme (not a block). Both with and without the patch, the search box simply returns a 404.

EDIT: note, I am using the customerror module too.

grendzy’s picture

Issue tags: +Release blocker

Yeah I'm seeing the same thing. This should probably block a hypothetical 6.11 release because it breaks existing functionality.

Gábor Hojtsy’s picture

Issue tags: -Release blocker

Since this was a bug from DRUPAL-6-0-BETA-2 onwards, I'd hardly call it a release blocker for any future Drupal release, given that it survived 1.5 years in Drupal 6 already. It does not make it any less critical though, and I definitely agree that we should make search work on error pages.

grendzy’s picture

Gábor – what I meant is that this breaks existing redirect behavior, that has worked at least since 5.0.

I agree the search issue itself wouldn't qualify, for the reasons you state. But the patch that was committed in #57 introduced a new bug.

To reproduce:

  • log out
  • visit /admin
  • log in via login block
  • observe that you are no longer redirected back to /admin, instead taken to the user page.
grendzy’s picture

Status: Needs work » Needs review
FileSize
844 bytes

Here's a patch that would roll back #57, at least until a bug-free implementation can be found (note I'm not saying that search is less important that login redirects, only that we shouldn't trade one bug for another in a stable branch).

webchick’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs review » Active

Bleh. Let's fix this right, and add a test so we don't break it again. Thanks for the report.

Is there some way we can tell contextually that we're on a 403/404 page (as opposed to the home page or somewhere else you'd want the login block to work)? If so, we could add that to the condition perhaps. Hrm.. not sure.

webchick’s picture

Title: Search from 404 page does not work » Regression: login destination is broken (was: Search from 404 page does not work)

Changing title.

Jody Lynn’s picture

I was making a patch to do the login block redirect differently, but then I started wondering why does drupal_access_denied and drupal_not_found set a $_REQUEST['destination'] anyway? I couldn't figure out what it was being used for. Anyone know?

moshe weitzman’s picture

$_GET['q'] changes behind your back when you get 403 or 404. i guess this is a way to preserve the original $_GET['q'].

grendzy’s picture

Jody, Moshe:

The code for setting the "fake" destination was committed in #55869. Even after reviewing that issue, and the patch, I still can't understand it's purpose.

BTW removing the fake destination code from drupal_not_found() doesn't produce any ill effects that I can tell — though it must have had a purpose right? I just can't find any explanation in the issue or commit messages.

--- includes/common.inc	25 Feb 2009 23:16:45 -0000	1.756.2.48
+++ includes/common.inc	30 Apr 2009 02:25:29 -0000
@@ -343,11 +343,6 @@ function drupal_not_found() {
 
   watchdog('page not found', check_plain($_GET['q']), NULL, WATCHDOG_WARNING);
 
-  // Keep old path for reference.
-  if (!isset($_REQUEST['destination'])) {
-    $_REQUEST['destination'] = $_GET['q'];
-  }
-
   $path = drupal_get_normal_path(variable_get('site_404', ''));
   if ($path && $path != $_GET['q']) {
     // Set the active item in case there are tabs to display, or other
pwolanin’s picture

2 thoughts: for D7 we could actually set some non-empty path by default for 403 and 404.

For D6, we could call drupal_get_headers() and look for a 403 or 404 header. Ugly, but reliable, I think.

Jody Lynn’s picture

I'm seeing the same thing as grendzy, at least for D7. That patch in #55869: Login problem, after getting 403, user cannot get back to requested page after succesful login 3 years ago seems to now be unneeded. I'm thinking we could do some more testing on D7 to see if we can get rid of it there as a start.

wretched sinner - saved by grace’s picture

John Morahan’s picture

Status: Active » Needs work
FileSize
1.49 KB

first things first

Jody Lynn’s picture

Status: Needs work » Needs review
FileSize
2.29 KB
3.46 KB

Thanks for the test!

Removing the $_REQUEST['destination'] on the 404 and 403 pages and undoing our previous fix passes all the tests.

I also have an 'alternative patch' which was what I was first trying, just to fix the user login block redirection instead of removing code. I'd rather remove useless code but posting this one in case we change directions.

Status: Needs review » Needs work

The last submitted patch failed testing.

Jody Lynn’s picture

Status: Needs work » Needs review

Well I didn't do a good job with the alternative patch. At any rate, the real patch needs review.

pagaille’s picture

Thanks for the patch - I will test asap. Spent several hours over the weekend trying to figure out why the front page module suddenly stopped working. Whoops!

pagaille’s picture

Patch worked for me in 6.11 with and without the front page module enabled. Thanks again.

sagannotcarl’s picture

regression.patch in #87 worked for me in 6.11. Both front page module and Login Toboggan now work as expected.

Gábor Hojtsy’s picture

Jody, grendzy: this destination setting seems to be a hack on a hack :) So removing that seems like a good solution.

The original hack was added in #14591: User.module links for blocked/non-existant accounts + menu 403/404 issue, where Steven suggested adding a dummy menu_set_active_item() on 404/403 pages which resulted in this # menu item being invoked.

Note that if custom 403/404 pages are used, then no tabs will show up as menu_set_location() is called to move to the custom 403/404 page, which is located somewhere else on the menu tree.

This is further amplified by the fact that the access rules for the different tabs are inconsistent (some don't respect admin overrides, some don't respect blocked status).

We can fix this problem by calling menu_set_location() with some dummy location in drupal_not_found() and drupal_access_denied(), thus ensuring we're somewhere else in the menu tree. In that case, the tabs won't appear for any 403/404. If we make the access rules across the different tabs consistent, then what you see in the UI (no tabs) will match the access rules.

Since adding menu_set_active_item('#') also set the destination on the 403/404 pages, this was added on with another hack at #55869: Login problem, after getting 403, user cannot get back to requested page after succesful login to preserve the original path anyway for display in the HTML output. That was supposedly solving a login problem, so people would get redirected back to the same page when logging in on a 403/404 page.

The menu_set_active_item('#') hack was later modified to menu_set_active_item('') and eventually removed from core. Therefore it looks like its counter-hack can go as well.

We need to ensure that forms such as the login form still work on the 403/404 pages, not only the search form, which was the original starter of this issue.

Would be great to get it fixed in 6.x too as soon as possible.

Jody Lynn’s picture

Thanks Gabor, the patch in #87 includes a test for the login form on 403/404 as well as the search form test (thanks to John Morahan). After fixing in 7, we should also manually test for 6.

Eric_A’s picture

6174604’s picture

Is there any patch for D6.11? Patch #87 gave me error when I run it on my 6.11

webchick’s picture

Version: 7.x-dev » 6.x-dev
Status: Needs review » Patch (to be ported)

It seemed like most of the tests here were against 6.x, so I guess that's why this wasn't marked RTBC?

Anyway, I tested this locally on 7.x and it seems to work fine. I'm trying to think of what else might be affected by removing that $_REQUEST munging, but the fact that LoginToboggan works on 6.x is probably a good sign that this doesn't break anything else.

Slightly clarified the assert messages and committed to HEAD. Needs porting to 6.x.

Gábor Hojtsy’s picture

Version: 6.x-dev » 5.x-dev
FileSize
2.09 KB

Ported to Drupal 6 and committed this patch.

Matir’s picture

This patch breaks behavior that was working in versions < 6.12. We had set our sites to redirect to /user on 403, so they would get a login page. Previously, the $_REQUEST['destination'] was saved when this occured, and after logging in, the user was redirected to the original content. Now, they are redirected to their user page. This patch makes the behavior very unintuitive. Reverting the patch restores the original behavior.

John Morahan’s picture

Version: 5.x-dev » 7.x-dev
Status: Patch (to be ported) » Needs work
FileSize
1.68 KB

*sigh*

sagannotcarl’s picture

Why start over for that use case? Redirecting 403 to /user is kind of a hacky setup. Mostly because when a logged in user actually gets access denied then they go to their own user page. Now *that* is unintuitive.

If the previous commits solved the underlying problem I say leave the patch in and folks should use supporting modules to get the exact functionality they need for difference scenarios (Login Toboggan for example).

webchick’s picture

The patch is called "square-one" but what it is is a test that proves the current behaviour specified in #99 is broken. We know this bug is fixed when this test passes, along with the previous two tests that John Morahan wrote for the other bugs that have either been solved or introduced in this issue.

My. This issue is either extremely evil, or extremely awesome depending on your perspective. ;) Thanks for the test, John!

Any bright ideas on how to solve this edge case which, although hackish, I admit I've used on some of my sites as well?

yang_yi_cn’s picture

it's really a regression... subscribe

Ryan Palmer’s picture

Marked #500092: Redirect broken when using custom 403 error page as a duplicate of this issue. I attached a patch to the dupe issue before realizing the length at which this had been discussed over here.

I'm using d 6.12 with a custom error page. The login form takes me back to the 403 page instead of the node I didn't have access to as an anonymous user. Perhaps it's been fixed in d6 HEAD?

dharmatech’s picture

Firmly disagree with sagannotcarl in #101 when suggesting to use a supporting module.
Custom error pages is core as is the usability feature of forwarding a user to the requested URL after logging in (destination). Why should a user be expected to add dependency for another module simply because an upgrade from 6.11 to 6.12 breaks that integration?

A very common use case:
1. User tries to access their CiviCRM installation
http://www.example.com/civicrm.
2. User is not logged so they are presented with a custom error page telling them it's a "members only" feature.
3. They log in using the username/password block being displayed on the error page and they are happily forwarded to /civicrm.

John Morahan’s picture

Status: Needs work » Needs review
FileSize
3.78 KB

Maybe we've been approaching this from the wrong angle.

Most forms submit, do their thing, and redirect either back to themselves or to some other hopefully useful page, trusting that if a static destination is set in the request, it knows a better place to go. But the search form is different: it depends on the redirect for its functionality, and a static destination in the request can't ever know better.

gpk’s picture

I like :-)

Would it be worth adding an extra line of comment to mention that dealing with 404 and 403 situations is what may tinker with $_REQUEST['destination'], e.g.

  // The search form relies on control of the redirect destination for its
  // functionality, so we override any static destination set in the request,
  // for example by drupal_access_denied() or drupal_not_found()
  // (see http://drupal.org/node/292565).
dharmatech’s picture

Patch looks good John.

Maybe there's a way to make the code clear so this sort of thing doesn't happen again. The original patch removed this functionality because it wasn't clear why those four lines were needed.

John Morahan’s picture

Now with even more comments.

dandaman’s picture

I vote that this patch (or at least the part in the drupal_access_denied() function) should be committed to Drupal 6.x as well. That is, for reasons such as #105, where you login from an access denied page and then get the access denied page after logged in as well, not the page you were trying to access but didn't have privilege.

Status: Needs review » Needs work

The last submitted patch failed testing.

John Morahan’s picture

aufumy’s picture

Without applying the patch, I cannot replicate this behaviour from the most recent drupal7 cvs checkout.

I am not sure that the above patch is needed anymore.

John Morahan’s picture

I can still replicate it (and the test still fails without the rest of the patch). Remember, the problem that this patch fixes (by providing yet another alternative fix for the original issue) is that setting the 403 page to /user redirects you to your account page when you login from there, instead of bringing you back to the page you tried to access.

aufumy’s picture

Ah, I understand now. Patch works for me. +1

Steps to test
* applied patch
* enable search block on /admin/structure/block
* set 403 page to user on /admin/settings/site-information
* logged out
* went to /admin, and logged in, it went to /admin
* went to /abc, showed page not found and used the search box, showed search results

webchick’s picture

Title: Regression: login destination is broken (was: Search from 404 page does not work) » Regression: Can't set /user to 403 page (was: login destination is broken)
Version: 7.x-dev » 6.x-dev
Status: Needs review » Patch (to be ported)

Great! Let's find out what other interesting edge-case bugs this fix uncovers. ;) Thanks John Morahan for your great work doing test-driven bug fixing, and aufumy, dharmatec, and others for testing the patch. :)

Committed to HEAD. Moving down to 6.x. Also fixing title.

John Morahan’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.38 KB
2.38 KB

Backport.

dharmatech’s picture

Great work John. Thanks for being persistent with this patch.

msjones design’s picture

Pardon - but I'm unclear on how to resolve this for d5 - my users can not search from 404 - is there a patch that works?

John Morahan’s picture

No, but I will make one if someone reviews my D6 patch ;)

dandaman’s picture

Status: Needs review » Reviewed & tested by the community

It works, although my Search block will not show up on the "page not found" error page no matter how much I try. (I've noticed that Drupal 6 doesn't want to load blocks when on the "not found" page.) It does fix all the other problems with logging in on the access denied page and not getting redirected, and that, I think is also a very important part of having this fix.

Bartezz’s picture

That's by design I think. Try this for blocks on 404 pages; http://drupal.org/project/blocks404

John Morahan’s picture

Thanks for the review, and here's the D5 patch I promised. #117 is still the patch for D6.

John Morahan’s picture

correct d5 patch, sorry. #117 is still the patch for D6.

Gábor Hojtsy’s picture

Version: 6.x-dev » 5.x-dev
Status: Reviewed & tested by the community » Needs review

Committed to 6.x, moving to 5.x.

drumm’s picture

Status: Needs review » Fixed

Committed to 5.x.

phazer’s picture

This patch broke my site. I have a simple routine as follows

function redirect() {
$path = $_REQUEST['q'];
drupal_goto('go/'. $path);
}

I point the 404 error page to /redirect so that when someone visits /john_doe it goes to /go/john_doe

I had to comment out the following from common.inc to make my code continue to work.

// if (!isset($_REQUEST['destination'])) {
// $_REQUEST['destination'] = $_GET['q'];
// }

It runs in an infinate loop unless I make the above change to common.inc. Do you know of a way I can resolve this other than changing core? I was doing something similar with the custom_error module, but had the same result after upgrading to 6.14.

Is there a way I can do this w/o using the 404 redirect logic and not hacking index.php?
Thanks

John Morahan’s picture

function redirect() {
  $path = $_REQUEST['destination'];
  unset($_REQUEST['destination']);
  drupal_goto('go/'. $path);
}

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.