Support from Acquia helps fund testing for Drupal Acquia logo

Comments

anrikun’s picture

Title: When editing an existing node with a link, the link itself is listed in "Parent item". » When editing an existing node with a link, the link itself is listed in "Parent item": menu_parent_options needs some rewriting
Assigned: Unassigned » anrikun
Priority: Normal » Major
Status: Active » Needs review
FileSize
3.23 KB

The attached should fix 2 issues:

1. When editing an existing node with a menu link, parent item options should not list the current link itself nor its descendants.
menu_parent_options function currently only returns options for a given menu item or a given node type but not both.
This is needed, that is why a 3rd optional parameter type has been added to this function.
This way, parent item options do not list the current link itself nor its descendants.

2. 'navigation:0' should not be returned as the default parent when we do not even know if it is present in options!
Instead, the returned default parent is now the first available option.

Status: Needs review » Needs work

The last submitted patch, menu.module.patch, failed testing.

anrikun’s picture

Status: Needs work » Needs review
FileSize
3.23 KB

Ooops, there was a stupid bug, sorry.

Status: Needs review » Needs work

The last submitted patch, menu.module.patch, failed testing.

anrikun’s picture

Status: Needs work » Needs review
FileSize
3.32 KB

This time I uploaded the wrong file. I should have slept a little more last night!

anrikun’s picture

Priority: Major » Critical

I'm changing this to critical as selecting the link itself as its parent may break the menu system.

int’s picture

Are you sure that is #550254: Menu links are sometimes not properly re-parented don't fix you issue?

anrikun’s picture

I haven't tried the patch at #550254 yet but it is related to a different issue:
My patch fixes a bug in the Menu module.
The patch at #550254 fixes a bug in includes/menu.inc

catch’s picture

Priority: Critical » Major

While this is a bit of a wtf, it doesn't cause any obvious issues as far as I can tell, and hence not critical. iirc Drupal 6 lets you do the same. Downgrading to 'major' since with the new slimmed down menu options, it's a lot more obvious that you can do this than it used to be.

anrikun’s picture

No this is not possible in Drupal 6. Look at modules/menu.module, line 399:

<?php
    // Generate a list of possible parents (not including this item or descendants).
    $options = menu_parent_options(menu_get_menus(), $item);
?>

Do you really think it is not critical that a user may set a descendant as a parent?
I do!

catch’s picture

No, critical bugs are those that render the system unusable, are a security risk, or cause data loss. This is just an (albeit annoying) bug, see http://drupal.org/node/45111 for the guidelines on issue priority. If you can demonstrate that it will actually break a site or cause irreversible corruption to {menu_links} I'd agree on the critical status, but not before then.

Kars-T’s picture

Hi,

I can reproduce the bug and the patch works.

But I have some questions about the code and don't understand it completly. Here a quick question:

+++ modules/menu/menu.module	2010-11-02 09:41:19.921875000 +0100
@@ -333,18 +336,18 @@ function menu_parent_options($menus, $it
-    // If $item is a node type, get all available menus for this type and
-    // prepare a dummy menu item for _menu_parent_depth_limit().
+++ modules/menu/menu.module	2010-11-02 09:41:19.921875000 +0100
@@ -333,18 +336,18 @@ function menu_parent_options($menus, $it
-    // If $item is an array fill it with all menus given to this function.

Why remove the comments?

Powered by Dreditor.

anrikun’s picture

Status: Needs work » Needs review

This is because these comments no longer match what's done by the modified code below them.

Kars-T’s picture

Status: Needs review » Needs work
FileSize
3.59 KB

Okay sorry for that at a first gaze it seemed so similar.

The problem occurs because only the nodetype is given in original menu.module line 505 and you separate it to an optional parameter. I think this is great but maybe the algorithm could be simplified?

menu_parent_options() is currently used three times. Only the call from menu_form_node_form_alter() uses a string and not an array. So maybe we can make sure $item is always an array. By this there would never be a parameter duplication and the algorithm is easier to read.

Please take a look at my patch that builds on yours. Do you think this is easier?

Maybe even $item could be optional as it can default to $link = array('mlid' => 0);

+++ modules/menu/menu.module	2010-11-02 09:41:19.921875000 +0100
@@ -648,9 +651,9 @@ function menu_form_node_form_alter(&$for
+    $default = reset(array_keys($options));

This gives us the root item of the current menu and is a nice addition imo.

Powered by Dreditor.

anrikun’s picture

Status: Needs review » Needs work

@Kars-T: I'll review your patch ASAP :-)
Why don't you turn this issue into "needs review" again?
This way your patch could be tested.

Kars-T’s picture

Status: Needs work » Needs review

Hi,

because I wanted your opinion first. And because the calls for menu_parent_options() could be made similar. So there is still some work left :)

But sure it can't be a bad thing to let the testbot onto it. I am setting this to needs review.

Status: Needs review » Needs work

The last submitted patch, menu-module-955848.patch, failed testing.

Kars-T’s picture

Hmm a lot of failures. So my patch seems to be too much of an API change.

anrikun’s picture

I want to help but unfortunately too busy this week :-(
I will review your patch ASAP!

anrikun’s picture

Assigned: anrikun » Unassigned
Status: Needs work » Needs review
FileSize
3.44 KB

I think like you that as your patch changes how menu_parent_options() is used, it fails testing.
I had kept this in mind when writing my patch: it was made to add features to the original function without breaking the way it was originally used.
So I think it is better to keep my patch.
I have added extra comments to it based on your remarks.

@all: Please review and commit this patch.
I have finished working on this issue.

Kars-T’s picture

Status: Needs review » Reviewed & tested by the community

Okay I retested the patch and can confirm that this patch is a solution to the problem. imho it applies to the coding standards and is well commented. So I set this to RTBC.

Thanks for your work anrikun :)

anrikun’s picture

Thank you very much Kars-T :)
By the way, I add a reference to a different but still menu-related issue that needs review too:
#955700: menu/menu.admin.inc lets users add or move node menu links to unavailable menus.

webchick’s picture

Priority: Major » Normal

I'd like to get some reviews on this from one or more of the menu system maintainers before moving it back to RTBC. But I'm pretty sure coupling menu_parent_options() to the node system is not going to fly.

We also need tests for this to ensure we don't get the bug again.

Finally, I'm not sure why this is "major". It's just a bug.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests
anrikun’s picture

Priority: Normal » Major
Status: Needs work » Needs review

But I'm pretty sure coupling menu_parent_options() to the node system is not going to fly.

What do you mean? Even current menu_parent_options() makes references to node types.
Why do you revert this from RTBC to "Need works" by the way? Menu system maintainers won't review anything doing so. And adding tests is just an extra task.
I do think it is a major regression bug from D6.

Kars-T’s picture

Hi webchick.

I did set this to rtbc because I think it means "reviewed and tested by the community" which I did. As anrikun said I thought now the menu maintainers have to give an opinion as this doesn't mean "ready to be committed". So I'd say it is still rtbc :)

But I don't dare to reset it because mighty webchick said "oh noz" ;)

And about "major": I second that this is a big problem if you could attach a node to itself even if this bug was in D6.

anrikun’s picture

AFAIK this bug was not in D6 (see #10), it is a regression bug.

Kars-T’s picture

Ah okay sorry. Was my missunderstanding.

jastraat’s picture

subscribe

zilverdistel’s picture

subscribe

anrikun’s picture

@jastraat, @zilverdistel:
Did you review the patch?

zilverdistel’s picture

@anrikun: Apparently not, since I didn't mention it. I'm working on some other issues now.

jastraat’s picture

@anrikun
I just reviewed the patch against the latest -dev and did not get any errors. (And it fixed the problem)

anrikun’s picture

@jastraat: Thanks for your review. I suggest you mark this issue as RTBC.

bfroehle’s picture

Status: Needs review » Needs work
+++ modules/menu/menu.module	Sat Jan 22 18:34:41 2011
@@ -648,9 +655,10 @@
+  // If the current parent menu item is not present in options, use the first available option as default value.
+  // @todo User should not be allowed to access menu link settings in such a case.

Not to pick nits, but comments should break at 80 characters.

Also:

We also need tests for this to ensure we don't get the bug again.

This means writing a test which verifies

When editing an existing node with a menu link, parent item options should not list the current link itself nor its descendants.

So write a test which creates two nodes 'A' and 'B', assigns menu items to each, with 'B' being the child of 'A' and then verify that you are not allowed to set the parent of 'A' to 'A' or 'B'.

Powered by Dreditor.

anrikun’s picture

Version: 7.x-dev » 8.x-dev
Priority: Critical » Major
FileSize
5.77 KB

Here is an updated patch. Changes are:
- Applies to 8.x
- Comments break at col 80
- Added tests (added tests fail without the patch, as expected)

anrikun’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
5.77 KB

The same patch should apply to 7.x too:

anrikun’s picture

Priority: Major » Critical

Changing this to "critical":
If the user by mistake sets the node itself as its parent, then reverts this, the menu item is still displayed as having children ({menu_links}.has_children = 1 when it should be 0).
So data integrity is broken.
The only workaround to clean data back is to attach a new menu item as a child of the broken one, then to remove this new item.

catch’s picture

Priority: Major » Critical
Status: Needs review » Needs work
Issue tags: +Needs backport to D7

@webchick, that function already takes $item as either an menu item or a node type string, so this patch doesn't couple the menu system/module to node any more than it already is.

This patch really, really needs more docs about why we're adding a $type parameter to menu_get_options() when that overloading of the arguments is already in place though. I can't see why exactly it's needed to fix this particular bug at the moment (is it standing in for $is_a_node?), nor when it's appropriate to to pass one vs. the other. This might be a case where we want a _menu_get_options() with the extra param so it hides the API addition from everyone on 7.x (and the bizarre almost-double parameter from 8.x for that matter).

Also this needs to go into 8.x first and be backported, changing version back again and adding tag.

anrikun’s picture

Status: Needs work » Needs review

@catch: this is explained at #1:

menu_parent_options function currently only returns options for a given menu item or a given node type but not both.

Also at #20:

[...] patch: it was made to add features to the original function without breaking the way it was originally used.

To summarize:
1. when $item is a node type, the function returns the menu options for this node type (original behaviour; $type is ignored).
2. when $item is a menu item (and $type is left empty), the function returns the menu options for this menu item (original behaviour).
3. when $item is a menu item and $type is given, the functions returns the menu options for these menu item and node type (behaviour added by the patch).

The way the original function works is absolutely unchanged. That's why the patch successfully passes the tests. Only an extra and optional parameter is added.
So we are sure committing it won't break current 7.x API.

catch’s picture

Uploading just the full tests and the full patch again to ensure the tests catch the bug, looks like they will but it's nice to see the red and green together ;)

Assuming that runs OK I'll mark this back to RTBC.

anrikun’s picture

Wow, I didn't know it was possible to run 2 tests at once like that :-0
Thanks catch!

catch’s picture

Priority: Critical » Major
Status: Needs review » Reviewed & tested by the community
Issue tags: +Usability

I read through this again and it looks like the minimal possible change to get this fixed.

webchick had two concerns when she marked this back to CNW:

1. That it ties menu_get_options() to node module, it's already tied to node module so there's no added coupling.

2. No tests - the new tests look good to me and catch the bug.

However I'm marking this back down from critical again, there is data inconsistency here but it is extremely minor and depends on the edge case of the user actually choosing the node as a parent link for itself. This is a bizarre usability wtf though so adding that tag.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

You forgot 3) sign-off from a menu system maintainer. :) This code looks like it's introducing a bunch of ugliness to work around a relatively minor edge case, and I want to make sure it gets sanity-checked by someone who's expected to maintain this code going forward.

catch’s picture

Webchick, when you make RTBC status reliant on sign-off from specific subsystem maintainers, you reduce the pool of reviewers down to almost zero (in this case 2, one of whom is sick this week).

Should I stop bothering to mark major bug reports RTBC for subsystems that I don't maintain then? That would make my life easier.

webchick’s picture

I try to only do this in cases where I'm dubious of it passing the muster. If it's a no-brainer fix I've no problem committing it. Experience has taught me though that subsystem maintainers get exceptionally grumpy when things that don't pass the sniff test are committed without their consultation.

So, no. You shouldn't stop marking bug reports RTBC. But neither should we ignore the contents of MAINTAINERS.txt, especially on "borderline" issues like this.

chx’s picture

Status: Needs review » Needs work

Good grief, we have The menu item or the node type for which to generate a list of parents. in HEAD? Did I write that? It's ugly enough for that...

Anyways. Patch looks solid indeed with a few minor nitpicks

+    // Use it as $type and prepare a dummy menu item.
-    // prepare a dummy menu item for _menu_parent_depth_limit().

You already had a comment which explains what dummy is for, why are you removing that?

+ $options = menu_parent_options(menu_get_menus(), ($link['mlid'] ? $link : $type), $type); This really needs a comment.

+ $default = reset(array_keys($options)); this is an E_STRICT error, reset needs a variable. Yes. I hate E_STRICT too.

anrikun’s picture

This code looks like it's introducing a bunch of ugliness to work around a relatively minor edge case

@webchick: do you think that it is the correct way to treat community? Read http://drupal.org/dcoc
I am trying to help to fix a bug and spent time to provide everything that was asked: clean code, tests, etc.
But it seems that you keep ignoring all the explanations that I have posted in this issue so far. This bug is not minor: it was not in D6 and can lead to data inconsistency.

Well, if you think that non-maintainer people from the community should not take part to core bug fixes, then it should be stated somewhere: RTBC means "Reviewed and tested by the Community", not "the Maintainers".

(Sorry for my English)

pwolanin’s picture

This make me cry a bit:

+  if (!is_array($item)) {
+    // If $item is not an array then it is a node type.
+    // Use it as $type and prepare a dummy menu item.

Not the patch so much as how something this ugly got in core.

webchick’s picture

anrikun: I'm really sorry! That comment wasn't intended to be at all an insult to you. It was in reference to the solution that had to be engineered in order to work around this problem—having to create a dummy menu item, adding a node type as a parameter to a function that is general-purpose to menu module and not at all related to nodes, etc. I guess a better word would've been "complicated" or so.

I do understand the bug, and how it can be triggered, and how it will lead to data corruption when it does. I just wanted to make sure that this solution was supported by one of the folks who knows this code inside and out, since I don't, and it raised by "spidey sense" for things that might not quite be core-worthy, yet.

However, regarding RTBC status, that's a recommendation to the maintainers to commit something. It's not a carte-blanche "ok, this will now get in. now hurry up and press the button." It's in fact my job as core maintainer to say "Hm. But what about this?" or "Are we sure that's the best way to fix this?" So that's what I will continue to do.

But don't think that doesn't mean that I don't appreciate your contributions, and your willingness to dig in and fix this. I just want to make sure the fix stands the test of time, and the best way to do that is to consult with experts when in doubt.

anrikun’s picture

Status: Needs work » Needs review
FileSize
5.89 KB

@webchick: Thank you for your explanations.
@chx: about the removed comment, see #13.

Here is an updated patch. Changes:
- Changed

<?php
// Use it as $type and prepare a dummy menu item
?>

into

<?php
// Use it as $type and prepare a dummy menu item for _menu_get_options().
?>

- Changed

<?php
$options = menu_parent_options(menu_get_menus(), ($link['mlid'] ? $link : $type), $type);
?>

into some clearer code.
- Fixed the E_STRICT issue.

catch’s picture

@webchick

having to create a dummy menu item, adding a node type as a parameter to a function that is general-purpose to menu module and not at all related to nodes, etc.

This was not a correct statement in January, and it remains wrong now.

anrikun pointed this out in #25, I corrected it in #39, then again #43 so I'm not sure why you are repeating it again.

http://api.drupal.org/api/drupal/modules--menu--menu.module/function/men...

$item The menu item or the node type for which to generate a list of parents. If $item['mlid'] == 0 then the complete tree is returned.

The wtf is already there in Drupal 7, multiple people on this issue have pointed it out, can we stop blaming it on this patch?

For the record, I wanted to see where this horrible code was added, and git blame + git log showed it was in #351249: Finer control over the Parent Menu select box, committed October 2009.

No-one (including me who RTBC'ed one of the versions of the patches there, nor Dries who did in-depth reviews himself then committed straight from CNR) caught that particular bit of ugliness, I think at least in part because it had been identified as a critical UX issue, and the last flurry of work was close to the final week before the 'UX freeze' for Drupal 7 during code slush, or that is how I'm rationalizing it today (other than 2009 and 2011 catch having different review criteria).

I will fully take my fair share of the blame for letting the ugly slip through in 2009, if you will accept that we are not letting it slip through now in 2011 - the patch here only continues the process of cleaning up the mess that patch added.

Let's quote from that issue:

OK, this looks good enough. Committed to CVS HEAD. We can refine in follow-ups as necessary.

This is one followup, #761648: Menu D6->D7 upgrade doesn't maintain node-menu configuration (f.k.a.: Menu items disappear after upgrade or manual menu entry) was another (and only got committed this month). It is a shame they are happening two years too late and within the constraints of an already released core version, but that is why we need to clear through the backlog of broken and buggy crap before adding new stuff to D8 that we'll end up fixing in 2014 or 2015.

We can open a new issue for Drupal 8 only to roll back the whole thing maybe?

chx’s picture

Status: Needs review » Needs work

I am sorry for putting this back to CNW -- and for not explaining this one better -- but $options = menu_parent_options(menu_get_menus(), ($link['mlid'] ? $link : $type), $type); the issue is not with the ?: operator. I asked for a comment because the WTF here is ($link['mlid'] ? $link : $type). Questions immediately jumping at me: so $link['mlid'] is 0 in this case? But mlid is a serial, how can a serial be 0? Is this coming from the dummy? This is what needs explaining in a comment. You can keep this in a ?: again, that's not the problem.

anrikun’s picture

Status: Needs review » Needs work

@chx: does it really need any extra comment? I mean, in menu.module it seems to be assumed that new (not saved yet) menu items have 0 as 'mlid'.

See:

<?php
/**
 * Implements hook_node_prepare().
 */
function menu_node_prepare($node) {
  if (empty($node->menu)) {

    // [...]

    // Set default values.
    $node->menu = $item + array(
      'link_title' => '',
      'mlid' => 0, // <-- It's here!
      'plid' => 0,
      'menu_name' => $menu_name,
      'weight' => 0,
      'options' => array(),
      'module' => 'menu',
      'expanded' => 0,
      'hidden' => 0,
      'has_children' => 0,
      'customized' => 0,
    );
  }

  // [...]

}
?>

There is no comment about the fact that $item['mlid'] = 0 when it is a new item anywhere in the file (just look for the 'mlid' pattern occurrences in the file and you will see). So why add one especially for this case?

anrikun’s picture

Status: Needs work » Needs review
FileSize
5.81 KB

I have put $options = menu_parent_options(menu_get_menus(), ($link['mlid'] ? $link : $type), $type); back.

anrikun’s picture

Status: Needs work » Needs review

Drupal 7.7 is out, but...
Too bad that this is not committed yet :-(
(catch had RTBCed this patch on June 10)

xjm’s picture

Tagging issues not yet using summary template.

Bojhan’s picture

@anrikun Can you just add a comment? Sure the other place doesn't do it, but if it adds clarity we should pursue it. Clarity over consistency any day.

chx’s picture

FileSize
6.02 KB

I have written a comment and nuked a superflous argument. I still think calling such a goofy function (oh god, did I write that one??) with such weird arguments needed a comment.

catch’s picture

Status: Needs review » Reviewed & tested by the community
fenstrat’s picture

Seconded for RTBC. Tested patch in #59 against D7, it solves the original issue when using menu_admin_per_menu.

healycn’s picture

subscribe

catch’s picture

#59: 955848_58.patch queued for re-testing.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Looks good. Committed to 7.x and 8.x.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

casey’s picture

Status: Closed (fixed) » Needs review
FileSize
939 bytes

The "superflous" argument is required after all. If you edit an exiting node with a menu link, the third argument is used. If it is not passed the user may choose a parent link from all menus, not only the configured ones.

Status: Needs review » Needs work

The last submitted patch, missing-argument.patch, failed testing.

casey’s picture

Priority: Major » Critical
Status: Needs work » Needs review

Critical as it introduced a regession for the 7.10 release.

anrikun’s picture

An updated patch below: basically, it's what was supposed to be committed at #55 + comments added by chx at #59.

anrikun’s picture

Status: Needs review » Needs work

Hmm, reverting to "needs work".
Actually we need to add a new test so that case at #66 is properly tested too.

anrikun’s picture

I have updated the patch adding the missing test.
First patch below only contains the test and should fail nicely.
The second one is the final patch (fix+test) and it should pass the test.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, so I'm marking RTBC. Will commit in a few days if no objections.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 8.x.

Moving to 7.x for backport.

sun’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.36 KB

Re-rolled for D7.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Straight re-roll, back to RTBC.

anrikun’s picture

Please commit to D7 too so that I can submit a new patch that moves function assertNoOption() to simpletest drupal_web_test_case.php according to the todo.

Actually, should this new patch be submitted as a separate issue or as a follow-up of this one?
And should it be marked as a simpletest.module issue?
Don't really know what's the right way to do in such a "crossed" case...

catch’s picture

That should be a separate issue, either component is fine (or 'base system' if it really does go across everything).

webchick’s picture

Priority: Critical » Normal
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

anrikun’s picture

Status: Fixed » Closed (fixed)
Issue tags: -Usability, -Needs issue summary update, -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.