Currently in the menu and book modules, it's possible that if the current link/page has child links, a parent that is presented in the select for new parent may be invalid because saving the link with that parent would result in a net depth greater than MENU_MAX_DEPTH.

This should be fixed by only displaying parents that are valid choices.

note - this bug noticed by webernet

CommentFileSizeAuthor
#20 2nd_cleanup_plimit_2.patch5.86 KBpwolanin
#18 2nd_cleanup_plimit_1.patch5.35 KBpwolanin
#17 cleanup_plimit_1.patch4.88 KBpwolanin
#14 parent_select_limit_9.patch13.11 KBpwolanin
#12 parent_select_limit_8.patch13.41 KBpwolanin
#11 parent_select_limit_7.patch6.93 KBpwolanin
#10 parent_select_limit.patch6.04 KBwebernet
#9 parent_select_limit_6.patch5.94 KBpwolanin
#8 parent_select_limit_5.patch5.8 KBpwolanin
#7 parent_select_limit_4.patch5.52 KBpwolanin
#3 parent_select_limit_3.patch4.56 KBpwolanin
#2 parent_select_limit_2.patch2.52 KBpwolanin
#1 parent_select_limit_1.patch2.53 KBpwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Active » Needs work
FileSize
parent_select_limit_1.patch2.53 KB

starting patch - only the book module. Works as desired, I thnk.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Assigned: Unassigned » pwolanin
Status: Needs work » Needs review
FileSize
parent_select_limit_2.patch2.52 KB

a patch for both book and menu modules.

Actually is pretty simple.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
parent_select_limit_3.patch4.56 KB

oops ran diff only on book. *this* patch has both book and menu modules.

webernet’s picture

webernet CreditAttribution: webernet commented

Patch looks good and works as expected - parent items that would result in the menu/book becoming too deep are not shown.

The patch needs to update the user facing text so that it explains why not all of the (theoretically) possible parents are listed.

There is also an additional case that should probably be handled for book module: the UI currently makes it possible to attempt to move the top level page of a book (that is maximum depth) to another book, even though it would result in the target book becoming too deep and thus fails with an error message.

Dries’s picture

Dries CreditAttribution: Dries commented

Looks good. Not committing this yet, because -- as per webernet's suggestion -- we might want to document this in the UI.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@Dries - webernet tells me that this patch doesn't apply cleanly on top of the one from http://drupal.org/node/154470, so I'm going to wait until that's in to re-roll with the updated description text in the UI.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
parent_select_limit_4.patch5.52 KB

ok, re-did the patch after that commit and added text to the form element descriptions.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
parent_select_limit_5.patch5.8 KB

Improved help text with suggestions from webernet, Addi, & Heine.

Also, slightly altered the recursive function in book module so that we check the depth on every call of the function. This means the parent select may be empty if the current page is a book with children at depth == 9.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
parent_select_limit_6.patch5.94 KB

better? Checking only in the intial if{} means that we might iterate through an extra round of recursion/level of links where none are valid. Thus, we should probably check again before the call as well.

webernet’s picture

webernet CreditAttribution: webernet commented
FileSize
parent_select_limit.patch6.04 KB

Attached patch attempts to deal with the case where the book is max depth and you should not be able to move the top level page.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
parent_select_limit_7.patch6.93 KB

hmmm, webernet has thrown down the gauntlet I see after my wussy reluctance on IRC to figure out how to truncate the book select if a top level node has children out to MENU_MAX_DEPTH.

Ok, so here's a rework of his new code, plus an alternate approach to checking the depth in the other loops that may be a better tradeoff in term of simplicity.

Note in the reworked part I check if the node is, in fact, a top level node ($node->nid == $node->book['bid']) so that we avoid running the extra queries on pages not at the top level.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
parent_select_limit_8.patch13.41 KB

This patch fixes some comment typos noticed by webernet.

Also, while discussing the presentation (as well as the content) with webernet and chx of these select boxes I realized the CSS classes and IDs were lack and/or inconsistent. So, this patch also makes the menu module use a fieldset for the single item editing form (as opposed to where it's part of the node form) so that the CSS IDs are consistent (book module already does this). In addition, a couple CSS class attributes are added.

webernet’s picture

webernet CreditAttribution: webernet commented
Status: Needs review » Needs work

The latest patch causes some undesirable behaviour - (disabled) menu links for disabled modules are appearing the menu module parent select list.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
parent_select_limit_9.patch13.11 KB

oops - I accidentally removed the check that prevented menu callbacks from being displayed in the select.

That's fixed in this patch, as well I moved the submit buttons outside the fieldset on the book outline form and the menu item editing form for consistency with other core usage.

webernet’s picture

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

Tested OK, and looks good.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Reviewed the code, and it looked OK, especially with the informative text for users. Committed!

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Fixed » Needs review
FileSize
cleanup_plimit_1.patch4.88 KB

oops - noticed something today - this code causes some problems with after a node preview or similar operations. After this the link doesn't have the right data to run the check on the depth of the children and warnings come as:

# notice: Undefined index: p1 in /Users/Shared/www/drupal6/includes/menu.inc on line 1640.
# notice: Undefined index: depth in /Users/Shared/www/drupal6/includes/menu.inc on line 1648.

Attached additional patch fixes this by saving the original link data for use with this check so that the check for the depth of the item's children uses the original values for the menu_name, etc (in other words the menu where it still resides in the DB) rather than what's been chosen in the select.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
2nd_cleanup_plimit_1.patch5.35 KB

here's a somewhat different approach to solving the same problem.

This solution has the advantage that the query to find the depth limit is only run once, even if the node is previewed. This is also a slight disadvantage, since the depth limit is not updated upon preview (e.g. in the unlikely case another user added deeper child links).

Crell’s picture

Crell CreditAttribution: Crell commented

Applies cleanly. Tested it according to pwolanin's instructions and everything worked correctly without errors anywhere.

He said not to RTBC yet, though, so I'm not. :-)

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
2nd_cleanup_plimit_2.patch5.86 KB

previous patch didn't apply for some reason - maybe whitespace changes in block caching patch?

new patch - also combines ugly repeated code into a private function in each module.

webernet’s picture

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

Looks and tested OK.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Reviewed & tested by the community » Fixed

Thanks, committed.

Anonymous’s picture

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