Menu link weight changes when editing a node

This sounds like a fairly critical bug to me.

I'm having the same issue and haven't found a way to resolve it.

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:

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

$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.

Why didn't you just flip these two lines?

    $query->fields('ml');
    $query->fields('m');

@sun - Aaah - /that's/ how you do it :-P

New patch, flips the lines.

+++ 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?

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.

Try the simple flip again with this one.

Patch seems to apply fine for me - maybe this is a naming issue? Re-attach patch from #10 with different name.

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.

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.

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.

Temporarily changing component, in the hope to get attention from the right folks.

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.

Alright, that's the kind of feedback / statement I wanted to hear from DB maintainer. Thanks! :)

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?

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.

Crell: thanks, I thought as much. Will upload a patch in a bit.

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.

+++ 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?

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.

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

Can we get tests for this?

OMG, my very first actual actual core test patch. Such a long learning road for such a small line.
eileenmcnaughton and I finally took the bull by the horns and overcame out fear of deep testing. Thanks heaps to the mentoring from Webchick here at DrupalSouth!
The error exists (even just trying to change the weight on an existing node through the edit form wouldn't work) , the above patch fixes it. This test proves it.

(is my comment too verbose?) Testbot, hit me!

I think we could skip these comments:

+    // when we go back to edit that node.
+    // http://drupal.org/node/605926
+    // Get node edit form.
+    $this->drupalGet('node/'. $node->nid. '/edit');
+    // Look at its selected weight option,.

Since the assertions are self explanatory, otherwise looks rtbc.

True!
They are largely what remains from notes to ourself as we workshopped it at a hackfest.
Figuring out tests was anything but self-explanatory for us at the time :-)

What we ended up with here is a distillation of 5 other false starts and long ways to get what we wanted using the available tools.
I WILL chop those comments out.
Then I WILL update the documentation with the half-dozen hidden functions from the "missing manual" that made me think I had to build an xpath parser myself ... But now I know better! It was a learning experience.
I'll be getting on to a few more (less verbose) tests now... :-B

trying again, quieter this time.
Thanks for your patience as I learn!

Last question - instead of specifying all the fields except one, we could maybe use drupal_schema_fields_sql() then unset weight similar to entity.inc loader - might be more resistant to schema changes. Rest all looks great :)

The actual patch - replacing one line with a long hard-coded array - does make me squirm.

Looks like a different in PDO vs. using a direct SQL query where we rely on the specified order (look at D6)

Specifying column names is considered a best practice for several reasons. It helps prevent collisions like this, for one thing. For another, it's better for performance to specify just the columns you want, instead of blindly grabbing everything in the table. So it might seem a bit iffy, but it's actually better style IMO.

The test tests the actual error, not the surrounding steps.
Following the long description exactly would have to include emulating drag & drop menu re-arrangement, not just form submissions.

However, tracing the state of the database while following the the steps described in the issues report, it boiled down to the issue described in the the title here : Menu link weights are not saved, and actually reset, when using the node edit form. This partially obliterates anything done to that menu link in the other screens and is the actual cause of the described problem.
You can add any number of irrelevant steps before and after the original issue report, but this is the actual issue being exposed.

Without the patch, this test fails - which is clearly an error. With patch, it doesn't.

Patch seems to be loading several router fields that are not needed - I would consider this a bug in D6 for this function.

compare to: http://api.drupal.org/api/function/menu_tree_all_data/7

where we load only needed router columns:

      $query = db_select('menu_links', 'ml', array('fetch' => PDO::FETCH_ASSOC));
      $query->addTag('translatable');
      $query->leftJoin('menu_router', 'm', 'm.path = ml.router_path');
      $query->fields('ml');
      $query->fields('m', array(
        'load_functions',
        'to_arg_functions',
        'access_callback',
        'access_arguments',
        'page_callback',
        'page_arguments',
        'delivery_callback',
        'title',
        'title_callback',
        'title_arguments',
        'theme_callback',
        'theme_arguments',
        'type',
        'description',
      ));

I would consider it a bug if the loaded columns are different in that function and menu_link_load. Ideally we could centralize this.

Are you suggesting an updated patch that skips some of those columns?
If so, here that is for testbot to chew on, but I don't know enough about the implications to suggest what else in that change would now need testing.
Missing out some data that used to be there could be a little dangerous, but I really dunno. It still passes all the existing tests.

Yep, side-effects.
With the above modifications, I'm now seeing

Notice: Undefined index: access in _menu_link_translate() (line 804 of /Library/WebServer/Documents/drupal7-test/includes/menu.inc).

because something, somewhere, was assuming that all the required fields would be loaded and available. Could be any number of those.

Benchmark

For the lulz, I ran a quick benchmark on the difference between a 'wildcard' lookup (before patch, no array) and the limited list of fields (as per #51 and the patch I rolled).

My benchmark may have any number of inconsistencies due to the effect of MySQL caching or things I don't know, but my gut was that SELECT * FROM.. would be optimized better than SELECT a,b,c,d,.... FROM ... so getting all and ignoring what we know we don't want could have benefits.

Results (3 runs over 1000 lookups):

Duration for detailed lookup took 0.84
Duration for wildcard lookup took 0.65

Duration for detailed lookup took 0.85
Duration for wildcard lookup took 0.69

Duration for detailed lookup took 1.22
Duration for wildcard lookup took 1.06

( the third time I put a consistent load on the server to see if it was affected)
I also reversed the order of operations and saw the same results.

Sooooo. Without getting too deep into arcane DB optimizations, It looks like we are 20% better off to SELECT * and unset($item['weight'])); than we are to be so specific about the columns we want or don't want.
Being economical about what we select is measurably inefficient. Best to get the lot and filter in code.

And that approach makes me feel less squirmy also. Selecting and discarding the problem is more robust, maintainable code than a hard-coded schema list. My gut (and this benchmark) agrees with catch in #42.

You want the code? I'd be educated to hear that my back-of-the-envelope approach was wrong.

The built-in benefits of the lazy SELECT * may be common knowledge among SQL gurus, it may also be a fallacy - which is why I benchmarked.

I'm saying I think the code would be better in the long run if we didn't get the DB to to our work for us.

Yeah, I'll admit I don't yet grok DBNG so if there is a join there that is actually getting the real weight we want as well - well I dunno then. Could it be the DB API needs the ability for us to name variables that isn't being utilized?
Would a second DB lookup for that be so bad if the JOIN is incapable of doing what we want?
I think I've reached the limit of my depth here.

What I've seen from looking at it is that selecting some but not all of the relevant fields is a minefield.

Ok, looks like since DBTNG does not have a way to say "get me all fields except ...", we need to list all fields except the weight field ourselves. Not listing all fields seemed to cause problems in testing above and it would be an API change at this point.

So let's list everything but the weight field to get weight from the menu link. Worked in manual testing. Test hunk still included for automated testing. I also run the node menu tests locally, and they passed. Did not run the whole test suite.

Agreed this is pretty critical.

dbtng doesn't, schema API does:

http://api.drupal.org/api/function/drupal_schema_fields_sql/7

See DrupalDefaultEntityController for an example.

This simplifies the code a lot indeed.

That's the one. Everything else looks great. Please wait for the test bot before commit.

Awesome. THAT's what clean code looks like!

Wicked. :) Thanks, folks!!

Committed to HEAD.

Hi,

Sorry to necromance this thread back to life. I'm getting the same issue with the current release version.

It looks like includes/menu.inc has changed substantially over the last couple of years, so I can't easily adapt the patch from #60 was written (for a start, all the variable names have changed).

This is a serious pain in the neck.

Can anybody help me or at least point me in the right direction?

Thanks

Hi,

Sorry to necromance this thread back to life. I'm getting the same issue with the current release version.

It looks like includes/menu.inc has changed substantially over the last couple of years, so I can't easily adapt the patch from #60 was written (for a start, all the variable names have changed).

This is a serious pain in the neck.

Can anybody help me or at least point me in the right direction?

Thanks

I'm seeing this issue now also, did you find a solution pav? I'm on 7.14 with all the latest modules.

have you got any solution of this.please help it still coming in recent version