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.
Steps to reproduce the bug
1. Create a new node and provide a menu link
2. Save
3. Edit the node again
4. The node link itself is listed as a possible Parent item.
Comment | File | Size | Author |
---|---|---|---|
#74 | drupal.menu-link-self-in-parents.73.patch | 1.36 KB | sun |
#71 | menu_link_itself_in_parents-955848-70-testonly.patch | 660 bytes | anrikun |
#71 | menu_link_itself_in_parents-955848-70.patch | 1.4 KB | anrikun |
#69 | menu_link_itself_in_parents-955848-69.patch | 773 bytes | anrikun |
#66 | missing-argument.patch | 939 bytes | casey |
Comments
Comment #1
anrikun CreditAttribution: anrikun commentedThe 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.
Comment #3
anrikun CreditAttribution: anrikun commentedOoops, there was a stupid bug, sorry.
Comment #5
anrikun CreditAttribution: anrikun commentedThis time I uploaded the wrong file. I should have slept a little more last night!
Comment #6
anrikun CreditAttribution: anrikun commentedI'm changing this to critical as selecting the link itself as its parent may break the menu system.
Comment #7
int CreditAttribution: int commentedAre you sure that is #550254: Menu links are sometimes not properly re-parented don't fix you issue?
Comment #8
anrikun CreditAttribution: anrikun commentedI 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
Comment #9
catchWhile 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.
Comment #10
anrikun CreditAttribution: anrikun commentedNo this is not possible in Drupal 6. Look at modules/menu.module, line 399:
Do you really think it is not critical that a user may set a descendant as a parent?
I do!
Comment #11
catchNo, 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.
Comment #12
Kars-T CreditAttribution: Kars-T commentedHi,
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:
Why remove the comments?
Powered by Dreditor.
Comment #13
anrikun CreditAttribution: anrikun commentedThis is because these comments no longer match what's done by the modified code below them.
Comment #14
Kars-T CreditAttribution: Kars-T commentedOkay 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);
This gives us the root item of the current menu and is a nice addition imo.
Powered by Dreditor.
Comment #15
anrikun CreditAttribution: anrikun commented@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.
Comment #16
Kars-T CreditAttribution: Kars-T commentedHi,
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.
Comment #18
Kars-T CreditAttribution: Kars-T commentedHmm a lot of failures. So my patch seems to be too much of an API change.
Comment #19
anrikun CreditAttribution: anrikun commentedI want to help but unfortunately too busy this week :-(
I will review your patch ASAP!
Comment #20
anrikun CreditAttribution: anrikun commentedI 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.
Comment #21
Kars-T CreditAttribution: Kars-T commentedOkay 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 :)
Comment #22
anrikun CreditAttribution: anrikun commentedThank 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.
Comment #23
webchickI'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.
Comment #24
webchickComment #25
anrikun CreditAttribution: anrikun commentedWhat 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.
Comment #26
Kars-T CreditAttribution: Kars-T commentedHi 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.
Comment #27
anrikun CreditAttribution: anrikun commentedAFAIK this bug was not in D6 (see #10), it is a regression bug.
Comment #28
Kars-T CreditAttribution: Kars-T commentedAh okay sorry. Was my missunderstanding.
Comment #29
jastraat CreditAttribution: jastraat commentedsubscribe
Comment #30
zilverdistel CreditAttribution: zilverdistel commentedsubscribe
Comment #31
anrikun CreditAttribution: anrikun commented@jastraat, @zilverdistel:
Did you review the patch?
Comment #32
zilverdistel CreditAttribution: zilverdistel commented@anrikun: Apparently not, since I didn't mention it. I'm working on some other issues now.
Comment #33
jastraat CreditAttribution: jastraat commented@anrikun
I just reviewed the patch against the latest -dev and did not get any errors. (And it fixed the problem)
Comment #34
anrikun CreditAttribution: anrikun commented@jastraat: Thanks for your review. I suggest you mark this issue as RTBC.
Comment #35
bfroehle CreditAttribution: bfroehle commentedNot to pick nits, but comments should break at 80 characters.
Also:
This means writing a test which verifies
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.
Comment #36
anrikun CreditAttribution: anrikun commentedHere 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)
Comment #37
anrikun CreditAttribution: anrikun commentedThe same patch should apply to 7.x too:
Comment #38
anrikun CreditAttribution: anrikun commentedChanging 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.
Comment #39
catch@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.
Comment #40
anrikun CreditAttribution: anrikun commented@catch: this is explained at #1:
Also at #20:
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.
Comment #41
catchUploading 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.
Comment #42
anrikun CreditAttribution: anrikun commentedWow, I didn't know it was possible to run 2 tests at once like that :-0
Thanks catch!
Comment #43
catchI 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.
Comment #44
webchickYou 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.
Comment #45
catchWebchick, 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.
Comment #46
webchickI 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.
Comment #47
chx CreditAttribution: chx commentedGood 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
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.Comment #48
anrikun CreditAttribution: anrikun commented@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)
Comment #49
pwolanin CreditAttribution: pwolanin commentedThis make me cry a bit:
Not the patch so much as how something this ugly got in core.
Comment #50
webchickanrikun: 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.
Comment #51
anrikun CreditAttribution: anrikun commented@webchick: Thank you for your explanations.
@chx: about the removed comment, see #13.
Here is an updated patch. Changes:
- Changed
into
- Changed
into some clearer code.
- Fixed the E_STRICT issue.
Comment #52
catch@webchick
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:
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?
Comment #53
chx CreditAttribution: chx commentedI 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.Comment #54
anrikun CreditAttribution: anrikun commented@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:
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?Comment #55
anrikun CreditAttribution: anrikun commentedI have put
$options = menu_parent_options(menu_get_menus(), ($link['mlid'] ? $link : $type), $type);
back.Comment #56
anrikun CreditAttribution: anrikun commentedDrupal 7.7 is out, but...
Too bad that this is not committed yet :-(
(catch had RTBCed this patch on June 10)
Comment #57
xjmTagging issues not yet using summary template.
Comment #58
Bojhan CreditAttribution: Bojhan commented@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.
Comment #59
chx CreditAttribution: chx commentedI 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.
Comment #60
catchComment #61
fenstratSeconded for RTBC. Tested patch in #59 against D7, it solves the original issue when using menu_admin_per_menu.
Comment #62
healycn CreditAttribution: healycn commentedsubscribe
Comment #63
catch#59: 955848_58.patch queued for re-testing.
Comment #64
Dries CreditAttribution: Dries commentedLooks good. Committed to 7.x and 8.x.
Comment #66
casey CreditAttribution: casey commentedThe "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.
Comment #68
casey CreditAttribution: casey commentedCritical as it introduced a regession for the 7.10 release.
Comment #69
anrikun CreditAttribution: anrikun commentedAn updated patch below: basically, it's what was supposed to be committed at #55 + comments added by chx at #59.
Comment #70
anrikun CreditAttribution: anrikun commentedHmm, reverting to "needs work".
Actually we need to add a new test so that case at #66 is properly tested too.
Comment #71
anrikun CreditAttribution: anrikun commentedI 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.
Comment #72
catchLooks good, so I'm marking RTBC. Will commit in a few days if no objections.
Comment #73
catchCommitted/pushed to 8.x.
Moving to 7.x for backport.
Comment #74
sunRe-rolled for D7.
Comment #75
catchStraight re-roll, back to RTBC.
Comment #76
anrikun CreditAttribution: anrikun commentedPlease commit to D7 too so that I can submit a new patch that moves function
assertNoOption()
to simpletestdrupal_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...
Comment #77
catchThat should be a separate issue, either component is fine (or 'base system' if it really does go across everything).
Comment #78
webchickCommitted and pushed to 7.x. Thanks!
Comment #79
anrikun CreditAttribution: anrikun commented@catch:
I have just posted the new separate issue #1413888: Move function assertNoOption() from menu.test to simpletest drupal_web_test_case.php