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.

CommentFileSizeAuthor
#20 msai_5.patch2.42 KBpwolanin
#18 msai_4.patch1.84 KBpwolanin
#16 msai_3.patch2.31 KBpwolanin
#14 msai_2.patch2.31 KBpwolanin
#10 msai_1.patch781 bytespwolanin
#7 menu_6_0.patch1.62 KBbennybobw
#6 menu_set_5.patch1.62 KBpwolanin
#5 menu_set_4.patch1.67 KBpwolanin
#4 menu_set_3.patch1.37 KBpwolanin
#2 menu_set_2.patch1.94 KBpwolanin
set_path_1.patch1.53 KBpwolanin

Comments

dries’s picture

dries commented

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

pwolanin commented
StatusFileSize
new menu_set_2.patch1.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

pwolanin commented
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

pwolanin commented
Status: Needs work » Needs review
StatusFileSize
new menu_set_3.patch1.37 KB

Maybe something simple like this instead

pwolanin’s picture

pwolanin commented
StatusFileSize
new menu_set_4.patch1.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

pwolanin commented
StatusFileSize
new menu_set_5.patch1.62 KB

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

bennybobw’s picture

bennybobw commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new menu_6_0.patch1.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

dries commented
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

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

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

pwolanin’s picture

pwolanin commented
StatusFileSize
new msai_1.patch781 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

pwolanin commented
Status: Needs work » Needs review
RobRoy’s picture

RobRoy commented

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

pwolanin commented

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

pwolanin commented
StatusFileSize
new msai_2.patch2.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

webernet commented
Status: Needs review » Reviewed & tested by the community

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

pwolanin’s picture

pwolanin commented
StatusFileSize
new msai_3.patch2.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

pwolanin commented

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

pwolanin commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new msai_4.patch1.84 KB

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

webernet’s picture

webernet commented
Status: Needs review » Reviewed & tested by the community

Minor menu block bug fixed - looks fine.

pwolanin’s picture

pwolanin commented
StatusFileSize
new msai_5.patch2.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

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
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

(not verified) commented
Status: Fixed » Closed (fixed)