Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#20 | msai_5.patch | 2.42 KB | pwolanin |
#18 | msai_4.patch | 1.84 KB | pwolanin |
#16 | msai_3.patch | 2.31 KB | pwolanin |
#14 | msai_2.patch | 2.31 KB | pwolanin |
#10 | msai_1.patch | 781 bytes | pwolanin |
Comments
Comment #1
Dries CreditAttribution: Dries commentedI don't understand what this means:
Also, could you document in the PHPdoc why this is useful? Maybe add a small use case?
Comment #2
pwolanin CreditAttribution: pwolanin commentedadded more comments.
This is rarely used (see drupal_not_found()), but should be tested by someone other than me before it's RTBC.
Comment #3
pwolanin CreditAttribution: pwolanin commentedI'm not sure about this approach- after thinking about it more menu_get_item() should be available to validate paths.
Comment #4
pwolanin CreditAttribution: pwolanin commentedMaybe something simple like this instead
Comment #5
pwolanin CreditAttribution: pwolanin commentedActually, 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().
Comment #6
pwolanin CreditAttribution: pwolanin commentedre-rolled - previous patch had lots of fuzz/offset
Comment #7
bennybobw CreditAttribution: bennybobw commentedRerolled 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.
Comment #8
Dries CreditAttribution: Dries commentedPlease document (in the code) _why_ we need to clear out the invalid path.
Comment #9
Gábor HojtsyPwolanin asked me to look at this, but it seems like Dries has a pretty clear open suggestion about the code.
Comment #10
pwolanin CreditAttribution: pwolanin commentedhmm, 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).
Comment #11
pwolanin CreditAttribution: pwolanin commentedComment #12
RobRoy CreditAttribution: RobRoy commentedI 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.
Comment #13
pwolanin CreditAttribution: pwolanin commentedD5 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.
Comment #14
pwolanin CreditAttribution: pwolanin commenteddiscussing 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.
Comment #15
webernet CreditAttribution: webernet commentedLooks fine, and doesn't appear to break anything.
Comment #16
pwolanin CreditAttribution: pwolanin commentedminor 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.
Comment #17
pwolanin CreditAttribution: pwolanin commentedah - 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.
Comment #18
pwolanin CreditAttribution: pwolanin commentedhere's a simpler solution - only set the path if there is a custom 403 or 404 page.
Comment #19
webernet CreditAttribution: webernet commentedMinor menu block bug fixed - looks fine.
Comment #20
pwolanin CreditAttribution: pwolanin commentedsmall 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.
Comment #21
Gábor HojtsyOK, it seems Dries' issues with the patch are solved, so I have seen no reason not to commit it. Thanks.
Comment #22
(not verified) CreditAttribution: commented