This is not widely used, but may be relevant for custom 403/404 pages- in D5 functions drupal_not_found() and drupal_access_denied() called menu_set_active_item(), but in D6, just call function menu_get_item(). However, functions called later will still get the original path from arg(), rather than the 403/404 path because we do not set it.

Attached patch fixes this by setting $_GET['q'] as menu_set_active_item() did in D5, as well as by removing the now-empty menu_set_active_item() function.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dries’s picture

I don't understand what this means:

+ *   A path, or NULL for the current path.  If path is not NULL, the current
+ *   path will be set to the value of $path.

Also, could you document in the PHPdoc why this is useful? Maybe add a small use case?

pwolanin’s picture

FileSize
1.94 KB

added more comments.

This is rarely used (see drupal_not_found()), but should be tested by someone other than me before it's RTBC.

pwolanin’s picture

Status: Needs review » Needs work

I'm not sure about this approach- after thinking about it more menu_get_item() should be available to validate paths.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.37 KB

Maybe something simple like this instead

pwolanin’s picture

FileSize
1.67 KB

Actually, better yet to go back to something closer to the Drupal 5.x code. This may be useful in case the theme code or other code is trying to generate output based on arg().

pwolanin’s picture

FileSize
1.62 KB

re-rolled - previous patch had lots of fuzz/offset

bennybobw’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.62 KB

Rerolled against HEAD. I don't know exactly how this would be used, but I tested it by printing $_GET['q'] in a custom 403 and a custom 404 page. Setting to RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Please document (in the code) _why_ we need to clear out the invalid path.

Gábor Hojtsy’s picture

Pwolanin asked me to look at this, but it seems like Dries has a pretty clear open suggestion about the code.

pwolanin’s picture

FileSize
781 bytes

hmm, after talking this over with Jeff Eaton, I think an even more minimal patch like the attached may be the right approach (unless later testing shows that setting the path is really needed on 403/404 pages).

pwolanin’s picture

Status: Needs work » Needs review
RobRoy’s picture

I remember Rasmus saying a while back that he didn't like how we modified $_GET['q'], just thought I'd point that out but I know we do this in other places too.

Was Dries' question answered about why we need this? I'd like to know too, even though I've used this function in the past to do some menu hackery thought maybe D6 would not need this.

pwolanin’s picture

D5 used the function a couple places in core in addition to drupal_not_found()/access_denied().

D6 does not use it anywhere at this point. It's possible it would be helpful to use it for 403/404 pages as in the earlier patch - but it's not apparent to me that it's needed.

So, the last patch is just a really minimal effort to keep this function as part of the API in the event we find (for example) during the beta testing that it's useful and needed somewhere.

pwolanin’s picture

FileSize
2.31 KB

discussing this more with chx, I realize the earlier patch was better. We should set the active item for 403/404 pages since the custom 403, for example, might be a page like /search which has tabs. Re-rolled that patch with more code comments.

webernet’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine, and doesn't appear to break anything.

pwolanin’s picture

FileSize
2.31 KB

minor fix - the empty string is not a valid path, and so setting the path to this was causing a side effect of making the sidebars disappear on 403 pages for logged-in users.

pwolanin’s picture

ah - The empty string for $_GET[q] is not making the sidebar go away - it's making the menu blocks go away.

last patch is no good - makes random tabs appear. Re-evaluating.

pwolanin’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.84 KB

here's a simpler solution - only set the path if there is a custom 403 or 404 page.

webernet’s picture

Status: Needs review » Reviewed & tested by the community

Minor menu block bug fixed - looks fine.

pwolanin’s picture

FileSize
2.42 KB

small update to the patch re: http://drupal.org/node/172765

use the menu_set_active_item API function for clarity in user module as discussed in that issue.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

OK, it seems Dries' issues with the patch are solved, so I have seen no reason not to commit it. Thanks.

Anonymous’s picture

Status: Fixed » Closed (fixed)