Removing need for patching core

forngren - March 20, 2007 - 21:16
Project:CustomError
Version:6.x-1.1
Component:Code
Category:bug report
Priority:critical
Assigned:Unassigned
Status:needs work
Description

I think I have come up with a solution to remove the need for patch core. I haven't tested the code yet, but I wanted to share the idea with you ASAP.

<?php
 
switch($error_code) {
    case
403:
     
$_SESSION['destination'] = $_REQUEST['destination'];
    case
404:
?>

AttachmentSize
customerror-patch.patch732 bytes

#1

forngren - March 26, 2007 - 17:42

#2

kbahey - March 26, 2007 - 19:33

The way the patch to core works is to set the $_SESSION['destination'] to $_GET['q'], which is the intended path the user wanted to go it.

The patch you provided sets it to $_REQUEST['destination'], which may not be set at all.

So, what I did was to set the SESSION destination to $_GET['q'] instead of $_REQUEST['destination'].

It seems to work for me.

If someone verifies it, then I can update the documentation and remove the patching instructions, and patches.

#3

kbahey - March 26, 2007 - 19:34
Status:needs review» fixed

Setting to fixed.

#4

forngren - March 26, 2007 - 19:57
Status:fixed» needs work

I could be wrong on this, but the reason to why I constructed the patch the way I did was because

<?php
  menu_set_active_item
($path);
?>

overrides $_GET['q'];

I suppose we could do

<?php
 
switch($error_code) {
    case
403:
      if (isset(
$_GET['q'])) {
       
$_SESSION['destination'] = $_GET['q'];
      }
?>

(not tested, just braindump, sorry for not providing in diff format)

http://api.drupal.org/api/5/function/drupal_access_denied
http://api.drupal.org/api/5/function/menu_set_active_item

#5

kbahey - March 26, 2007 - 21:04
Status:needs work» fixed

I tried it with your suggestion, and it seems to work fine for me.

I ported it to 4.7 as well, and updated the documentation. The patch files are gone too.

#6

Anonymous - April 9, 2007 - 21:16
Status:fixed» closed

#7

TheRec - March 8, 2009 - 00:51
Version:5.x-1.x-dev» 6.x-1.0
Status:closed» needs work

Hello,

Sorry to bring up an old issue back to life, but I really feel it is exactly matching with the bug I encountered. Please note that I am talking about the 6.x branch, previous branches might not be able to work like this, thus I updated the "Version" for this issue.

I've been scratching my head few hours now and I suddenly realized my problem came from Customerror. Using this method of storing the destination in a session variable as an unpleasant effect, whenever an user encounters an error generated through Customerror, this session variable is set (in the customerror_page(), using hook_page()) and no matter where he goes after this, as long as his session is alive (I am not talking about a Drupal session of the logged user... we are in the case of an anonymous user, so I am talking about the current PHP session, usually maintained with simple cookies or through session ID) and then when the users decides to login, he's brought back to this destination (stored in the session variable previously), because of customerror_user() using hook_user(), which is hooked right after login. This does not make sense to me at all for 404 errors (what's the point of storing destination at this moment ?) and for 403 errors it sounds logical to login right after an error happened, but it is not logical to keep this session variable if the user goes "away" from this error.

I attached a patch solving the issue for 404 pages. And I think we should explore a better solution to store and/or destroy this session variable for 403 pages. I do not see how you could empty this variable if the user does not login straight away (unless maybe store another session variable with a timestamp and not use the destination in the session variable if it is too old, but I don't like this idea). So my real proposition is to remove completely this session variable, and let users handle this in the code of their custom 403 error... they can easily insert the login form in it or as you advise on your module homepage use drupal_get_destination() to provide a link to the login form. So this is the second patch attached, which removes completely the session variable. Of course, I may have overlooked a crucial point and the real reason of using this session variable, if so I would really like to know what is its purpose and if removing it has other side effects I didn't experience yet.

AttachmentSize
customerror_129585.patch 752 bytes
customerror_no_destination_129585.patch 1.19 KB

#8

brad bulger - November 12, 2009 - 05:59
Version:6.x-1.0» 6.x-1.1

this is a problem in the 6.x release because it is overriding any explicit "destination=bla/bla" set in a login link. as you say, it particularly doesn't seem to make sense for 404 errors, as logging in is not going to make the missing page appear.

 
 

Drupal is a registered trademark of Dries Buytaert.