Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part 1: Formatting patch attached. This also fixes some description text on the admin/settings/menu screen. The existing text was below par IMO.
On the same screen, one of the select elements has a description:
'If you select the same menu as primary links then secondary links will display the appropriate second level of your navigation hierarchy.'
I have no idea what that means, but I've left it alone.
-K
Comment | File | Size | Author |
---|---|---|---|
#8 | menu_fapi_inc_2_0.patch | 21.65 KB | Zen |
#6 | menu_fapi_inc_2.patch | 21.53 KB | dww |
#5 | menu_fapi_inc_1.patch | 17.87 KB | dww |
#4 | menu_fapi_inc-fixes.patch | 747 bytes | dww |
#2 | menu_fapi.patch | 34.15 KB | Zen |
Comments
Comment #1
Zen CreditAttribution: Zen commentedIncremental patch with the fapi fixes attached.
-K
Comment #2
Zen CreditAttribution: Zen commentedAttached: both the above patches rolled in one.
-K
Comment #3
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedI've applied the reformatting patch, can somebody please test the fapi part?
Comment #4
dwwi tested zen's patch as heavily as i could, and reviewed the code. i found another bug, but it's neither caused, fixed, nor made worse by this patch, so i submitted that as a completely separate issue (problems with resetting menu items).
other than that, everything seemed to work perfectly, with the exception of a few small typos in some of the titles in menu_menu(). attached is a small diff against zen's patch to fix these (menu_fapi_inc-fixes.patch). i'll upload a new full patch in a separate follow-up that includes all of zen's patch, re-rolled with these minor edits.
i'd change the status to RTBC, but i figured someone (e.g. zen) should look at my diff(s). ;)
thanks,
-derek
Comment #5
dwwhere's the full patch with the minor edits from menu_fapi_inc-fixes.patch applied to zen's menu_fapi_inc.patch.
Comment #6
dwwhere's a new version that adds some help text about primary/secondary links, since that seems to be an area of confusion (i know i got tripped up on it for a little while until i remembered the admin/settings/menu page). i'm just going to include the full, re-rolled patch (overwhelmingly zen's code, plus my minor fixes mentioned in #4, and the help text) as "menu_fapi_inc_2.patch". if anyone wants just the help text as an isolated patch, just let me know.
Comment #7
dwwone more problem i discovered (which is true in both FAPI and non-FAPI) is that if the thing you enter in the "path" field is an actual URL with
<a href="http://example.com">example.com</a>
, all hell breaks loose. the resulting link generated by the menu system becomes something like:i'm not sure if it'd be easier to verify this using FAPI or not, and it's way too late for me to try to figure it out right now. ;) if we're going to be using FAPI, it seems like we should take advantage of the validation stuff more thoroughly to notice if users attempt to create menu items with paths that we can't handle. i don't think this is necessarily worth holding up this patch for, but i just wanted to mention it here... (this one didn't seem worthy of a separate issue, especially since it's potentially directly related to FAPI).
thanks,
-derek
Comment #8
Zen CreditAttribution: Zen commentedThanks Derek! The review and the fix is much appreciated :)
Patch - Please always roll your patch relative to the site root. i.e. your cvs diff should be something along the lines of:
cvs diff -up modules/menu.module > menu_fapi.patch
This makes it easier to patch stuff.
Help text - I removed the 'administer blocks' line as it was being mentioned twice in three lines :) Also removed an extra space. Cleared the ambiguity noted in dww's #7 comment with an example.
Patch re-rolled. Please give it the once over and set to RTBC if everything is A-OK. Please move any new fixes/amendments to a new issue :)
Thanks again!
-K
Comment #9
Zen CreditAttribution: Zen commentedComment #10
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedapplied
Comment #11
dwwthat's what i did (e.g. for menu_fapi_inc_1.patch and menu_fapi_inc_2.patch), i just thought it'd be handy to post a small diff showing what i actually changed, in addition to the full (huge) patch... i guess someone could download both patches and run diff themselves, but i was just trying to save people the additional effort.
like the validate thing? ;) ok. node/55578 created.
thanks,
-derek
p.s. now that this has been committed, i think node/53870 should be committed (in particular, the patch in #7). zen, can you review that and set it to RTBC if you agree? thanks!
Comment #12
Zen CreditAttribution: Zen commentedPatches - my bad. I'll look into the other issues shortly.
Thanks
-K
Comment #13
dwwre patches: oh sorry, i missed your point. you meant the root of the core directory tree, not from inside the modules directory. that'd be my bad, not yours. ;) you are correct, my previous patches were from running cvs diff while sitting in the modules directory. i'll try to remember this for future reference.
however, there's no mention of this point at all in diffandpatch. if this is in fact the desired behavior, it should be documented as such...
thanks,
-derek
Comment #14
dwwFYI: at the suggestion of sepeck on IRC i just added a comment to diffandpatch about this. now setting status to "closed and forgotten"... ;)