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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Zen’s picture

FileSize
17.71 KB

Incremental patch with the fapi fixes attached.

-K

Zen’s picture

FileSize
34.15 KB

Attached: both the above patches rolled in one.

-K

killes@www.drop.org’s picture

I've applied the reformatting patch, can somebody please test the fapi part?

dww’s picture

FileSize
747 bytes

i 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

dww’s picture

FileSize
17.87 KB

here's the full patch with the minor edits from menu_fapi_inc-fixes.patch applied to zen's menu_fapi_inc.patch.

dww’s picture

FileSize
21.53 KB

here'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.

dww’s picture

one 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:

http://localhost/drupal-4-7/<a+href="http://example.com">example.com</a>

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

Zen’s picture

FileSize
21.65 KB

Thanks 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

Zen’s picture

Status: Needs review » Reviewed & tested by the community
killes@www.drop.org’s picture

Status: Reviewed & tested by the community » Fixed

applied

dww’s picture

Patch - Please always roll your patch relative to the site root.

that'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.

Please move any new fixes/amendments to a new issue :)

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!

Zen’s picture

Status: Fixed » Closed (fixed)

Patches - my bad. I'll look into the other issues shortly.

Thanks
-K

dww’s picture

re 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

dww’s picture

FYI: at the suggestion of sepeck on IRC i just added a comment to diffandpatch about this. now setting status to "closed and forgotten"... ;)