Menu link weight changes when editing a node
jhodgdon - October 15, 2009 - 22:25
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | menu system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | reviewed & tested by the community |
Description
I have several nodes added to the Main Menu in Drupal 7.
I have changed their order by visiting the menu configuration page, dragging them around, and saving.
Then I go to edit one of the pages. Before editing, it's the first page in the menu, at that level of the hierarchy. I change the node title and save the page. Now it's the last item in the menu, at that level of the hierarchy.
I didn't do anything in the menu section of the vertical tabs. All I did was change the node title. The menu item name didn't change, and the order in the menu should not have changed either.
#1
I think the reason this is happening is that in includes/menu.inc, in function menu_link_load, the query it runs is joining the menu_links table to the menu_router table, and taking all the fields of each.
$query = db_select('menu_links', 'ml');$query->leftJoin('menu_router', 'm', 'm.path = ml.router_path');
$query->fields('ml');
$query->fields('m');
$query->condition('ml.mlid', $mlid);
if ($item = $query->execute()->fetchAssoc()) {
This would be OK, except that both menu_links and menu_router have a 'weight' field, and in the result, at least on my machine, $item is ending up with weight=0 (from menu_router) rather than the weight of the item in my Main Menu (which in this case has been set to be -30 or something like that).
This information is eventually used by menu_form_alter() in modules/menu/menu.module to build the menu section of the node edit form. So then when you save the node, this weight of zero gets stuck into the menu_links table, and your menu is out of order.
So the solution is probably to make menu_link_load() take all the fields except weight from menu_router, because the primary thing it's doing here is loading the menu link item. I think.
#2
Just as a note, I tried reversing the order of the ->fields calls in the code above, but that didn't help. Weight still came out 0.
So I am not sure how to patch this -- is there a function that will list all the fields from the menu router table, so that we can just remove 'weight' from the list and pass the resulting array into the fields() call? The alternative would be to list all the fields out, but that is a bad idea (hard to maintain as the table spec changes).
#3
This sounds like a fairly critical bug to me.
#4
I'm having the same issue and haven't found a way to resolve it.
#5
Cracked it, I hope. Patch attached.
This was caused by translating a query from D6 into DBTNG-speak in D7. In D6, the query was executed as follows:
SELECT m.*, ml.*FROM
menu_links ml
LEFT JOIN menu_router m ON m.path = ml.router_path
WHERE ml.mlid = 185
Whereas in D7 it is executed like this:
SELECT ml.*, m.*FROM
menu_links ml
LEFT OUTER JOIN menu_router m ON m.path = ml.router_path
WHERE (ml.mlid = 185)
Spot the difference - apart from the parentheses in the last line and the more explicit LEFT OUTER JOIN? Yep, m.* and ml.* are switched in the first line. When a query like this is run in MySQL and the two tables being queried have row that share the same name, which ever one is called second is the one that is returned - this really confused me, as running the query through Navicat returns both headers, but I've been told this is not how raw MySQL works.
This meant that in the D6 query, weight was returned from menu_links, but in the D7 query it was returned from menu_router and was therefore always loaded into the form as "0" - hence menu items moving around when the form was submitted. However, because of DBTNG switching m.* and ml.* back isn't straightforward. The function that calls the query in menu.inc looks like this:
<?php$query = db_select('menu_links', 'ml');
$query->leftJoin('menu_router', 'm', 'm.path = ml.router_path');
$query->fields('ml');
$query->fields('m');
$query->condition('ml.mlid', $mlid);
?>
I couldn't quite see how I was going to switch m.* and ml.* in this, and I didn't want to change the order of the join, so I opted for switching it out for the corresponding RIGHT JOIN:
<?php$query = db_select('menu_router', 'm');
$query->rightJoin('menu_links', 'ml', 'm.path = ml.router_path');
$query->fields('ml');
$query->fields('m');
$query->condition('ml.mlid', $mlid);
?>
This may not be the right way to do things but I hope that with this information someone with more experience than me will be able to easily get this fixed in the right way.
#6
Why didn't you just flip these two lines?
$query->fields('ml');$query->fields('m');
#7
@sun - Aaah - /that's/ how you do it :-P
New patch, flips the lines.
#8
+++ includes/menu.inc 14 Nov 2009 16:00:58 -0000@@ -2158,10 +2158,11 @@ function menu_get_active_title() {
- $query = db_select('menu_links', 'ml');
- $query->leftJoin('menu_router', 'm', 'm.path = ml.router_path');
...
+ $query = db_select('menu_router', 'm');
+ $query->rightJoin('menu_links', 'ml', 'm.path = ml.router_path');
I don't think that you need to change FROM and JOIN.
I'm on crack. Are you, too?
#9
Whoops - didn't clear out the code like I thought I had.
But in any event I've done some further testing, and it seems that the order of those two lines doesn't dictate the order in which the tables are queried, so we're back to the original patch - reuploaded for clarity, along with a couple of screenshots showing what's going on here.
#10
Try the simple flip again with this one.
#11
The last submitted patch failed testing.
#12
jhodgdon requested that failed test be re-tested.
#13
The last submitted patch failed testing.
#14
Patch seems to apply fine for me - maybe this is a naming issue? Re-attach patch from #10 with different name.
#15
#16
The last submitted patch failed testing.
#17
For the record, the patch from #10 that I re-uploaded in #14 doesn't work. I completely agree with the rationale, but because of the way __toString() constructs queries the patch changes
SELECT ml.*, m.*FROM
menu_links ml
LEFT OUTER JOIN menu_router m ON m.path = ml.router_path
WHERE (ml.mlid = :db_condition_placeholder_0)
to
SELECT m.*, ml.*FROM
LEFT OUTER JOIN menu_router m ON m.path = ml.router_path
menu_links ml
WHERE (ml.mlid = :db_condition_placeholder_0)
i.e., it uses the weight to decide more than just the order of m.* and ml.* in the first line, which it should never do.
As stated, I do really like how this works and I think that it should be possible to decide the order that tables are queried. I'll have a further look at this tomorrow to see if I can get it to work.
#18
Has someone verified that changing the order of the m.*, ml.* in a query actually causes the query to get the right information into the object consistently, independent of whether it's MySQL, PostgreSQL, etc.? I am not at all certain it does, and I'm actually not sure it does even on MySQL, and if it is an undocumented "feature" of MySQL and/or other databases and/or the PHP functions that are picking out the information from the query result, should we be relying on it?
I still think the safest course of action would be to make sure the unwanted weight field is not selected in the query at all.
You don't want m.*, you want m.* except weight.
#19
I guess it would be in the PHP part and not in the DB, though, because the DB query returns both weight fields. It is when the db row is converted into a PHP object that it chooses one or the other.
#20
I think this is a PHP thing, yes - and it had been relied upon to work this way in Drupal 6, with whatever database backend you want to throw it at. Drupal 7 constructs the database query differently from Drupal 6, sure; but if you look a thte query itself the only thing that has changed is the order of m and ml.
#21
New patch attached. It's commented pretty thoroughly; it uses sun's idea for sorting the fields by weight, but it un-sorts them before the seirous query-building work is done. I think it's a little unelegant but I do think that tables should be queried in the order that they're selected, and I'm not sure quite how else to do this.
#22
Temporarily changing component, in the hope to get attention from the right folks.
#23
Silly question: Why don't we just list out the fields we want and get rid of the duplicate fields in the first place? Then the order won't matter, and the query is more readable.
Relying on a quirk of PHP that screws with the result set is a bad idea in the first place.
#24
Alright, that's the kind of feedback / statement I wanted to hear from DB maintainer. Thanks! :)
#25
Thanks Crell.
The patch needs work, in that case, and should address only the fact that the menu system shouldn't be asking for the weight field from the menu router table, to avoid this problem completely.
#26
Removing the duplicate fields is really straightforward, but do we want to force the query to be executed and interpreted in the order that it's built?
#27
jim0203: The query builder will do exactly as it's told in terms of the order stuff gets built. It's up to the developer to make sure that it tells the query builder something reasonable and rational. Over-using * is neither reasonable nor rational, and is in fact slower to execute as well as, in this case, buggy.
If your code depends on the order of fields in the first place, then you have a bug in your code.
#28
Crell: thanks, I thought as much. Will upload a patch in a bit.
#29
New patch as per above. Style taken from other uses of $query->fields() already in core: e.g., in menu_tree_all_data() on line 987 of menu.inc.
All of the columns except weight are now selected from menu_router - it may be possible to select fewer columns, I don't know - but I'm guessing that since this is such a general function it will need to select everything.
#30
The patch looks good to me. I applied it and tested, and it appears to have fixed the problem.
#31
+++ includes/menu.inc 27 Nov 2009 18:46:27 -0000@@ -2161,7 +2161,33 @@ function menu_link_load($mlid) {
+ // SELECT all the columns from {menu_router} except weight, which
+ // should be taken from {menu_links}.
We can shorten this and there's no reason to scream.
Select all columns from {menu_router} except 'weight'.
err. Did anyone think of renaming one of both columns? That would likely allow us to simplify a couple of queries...
I'm on crack. Are you, too?
#32
I think that renaming one of the weight columns would be a much bigger operation with many many side effects... and besides, weight is the name used for that type of column all over drupal's table structure, so probably it should stay.
sun: you didn't change the issue status. Did you mean to make it "needs work"?
#33
Actually capitalizing key SQL terms is a very common style. I believe DBTNG uses it all over the place. SELECT is not screaming, it's referencing a specific part of an SQL query string.
#34
Re #31, #32 #33:
I agree with @johodgson that changing the name of the weight column is problematic. It would almost certainly require quite a few code changes and a number of tables have a weight column - it's easier just to pick and choose which weight column is needed when building a query, which is what we've done here.
And yes, I was imitating the style of other SQL comments in core when I wrote SELECT in caps.
Assuming @sun meant to change this to "needs work", I would change it back to RTBC now :-)