Duplicate entry errors on menu item update

D'oh. I wonder why I never saw this error?

Any simple steps to replicate?

Wait. This is wrong. Under what conditions do you have two menu items with the same MLID?

In fact, if menu_node_exists() returns TRUE, we shouldn't run drupal_write_record() at all, AFAIK.

Yes, but look at the function:

  $new = menu_node_exists($nid, $mlid);
...
function menu_node_exists($nid, $mlid) {
  return db_result(db_query("SELECT COUNT(nid) FROM {menu_node} WHERE nid = %d AND mlid = %d", $nid, $mlid));
}

If this returns a value > 0, then a record already exists with that NID/MLID pair, and there is no reason to update that table row, since those are both foreign keys.

So the question, really, is how did you get this to happen (which I think doesn't matter) and how to optimize the code (which is probably to remove the else, which does nothing.

Yes. That's what I am suggesting.

And here is the patch. For the record, I could never duplicate this error, and I wonder why.

Did I misunderstand anything?

You suggested to eliminate the else part, but actually you didn't.

drupal_write_record does not change the primary key. Since your record consists of the primary key only, the function will call the database engine to change something, which maybe not exist.

Example:

Lets have a node, which has node id 5 and mlid 6. Suppose there is some editing, which changes the mlid to 27. Then drupal_write_record will make the following query:

UPDATE {menu_node} SET nid=5,mlid=27 WHERE nid = 5 and mlid = 27

This is completely useless. In fact, updating is not possible in this way. You will leave orphaned pairs in the database. The correct way would be to delete the old value and to insert a new record. But how do you know, which record to delete?

People can do everything with their modules. So this module should integrate with both modules, the node and the menu module.

One reason, that you did not realize the error meight be, that the menu_node has no ability to hook into menu_link_save. So it does not recognize all changes to the menu tree. So you might not have realized, that there are plenty of uses of the menu system.

I got the error, because the book trees are stored as hidden menus. Since I installed the module after these books have been created, they have been imported into the menu_node module.

No, the patch eliminates the else, so the drupal_write_record() should only run if no entry exists (see menu_node_exists()).

I see the point about book.module. That's a nice catch. And I have a question about the best fix:

-- book_update_bid() is called by _book_update_outline(). The second function is called twice, once in book_nodeapi() -- which we should already cover -- and the other in the book submit function. menu_node_nodeapi() may account for the first case, and we could use a form_alter() to account for the second.

But it looks like the better case is to watch the menu_link_save() function with hook_menu_link_alter(). In the case of new items added to the {menu_links} table, we would have to track the inbound link and ensure that it gets added. I'm not too concerned about being able to do that. We will need to test whether the book outline form allows the deletion of nodes from a menu.

For reference: 13 functions call menu_link_save --> http://api.drupal.org/api/function/menu_link_save/6 and we forgot the book functions.

6 more call menu_link_delete --> http://api.drupal.org/api/function/menu_link_delete/6. It looks like we will have to watch the book_remove_form explicitly.

I'll run some book tests and see if we can roll a patch.

One other clarification:

Is it the case that a book node can get two entries in the {menu_links} table? If so, that's really ugly, and a good argument for overhauling the system in D7.

A book node gets at least two entries if it is availlable in any other menu (e.g. navigation).

The book navigation is kept as hidden menu structure. That is not as ugly as it looks like. So modules like menu_node_edit can easily act on book chapters too. That would be no bad idea and fits into the everything-is-the-same-philosophy of drupal (e.g. everything is a node).

At the moment I would think the Idea to use hook_menu_link_alter() is better than hacking into all forms separately. It is much nearer on the resulting database entry.

Some new information: menu_nodes implements the same feature, which is availlable already as the {drupal_book} table. Thats ugly. The book id is just a numeric representation of the menu name. Maybe it is possible to combine both.

That would mean, rename {drupal_book} into {drupal_menu_node} and drop the uniqueness condition from the nid column. I can imagine cases where duplicate mlids occur, too, (involving panels and views). Maybe there should be introduced some definition of menu kind (I'd like to leave it open to the module programmers to find new uses of the menu API).

Though it doesn't solve our problems here, this could help to make a better design at all.

Actually, I took a look at how Book module interacts with the menu system, and the answer, I believe is this:

-- We want to ignore Book module. Deliberately.

Book module does not expose its data to the Menu UI, so it is useless for "menu-based nodes" purposes. In fact, if you move a parent book item into a custom menu, only the parent moves over (not the whole tree), which suggests a total separation of the two systems. (The book form on the node page also does not trigger {menu_node} insertion).

Book menus are not stored in the {menu_custom} table, which controls the ability to add items to a menu. (Note the the Admin Menu module doesn't either.)

I rewrote the patch to expressly avoid adding (on install) any node to the {menu_node} table whose parent menu is _not_ in {menu_custom}. I think this is the proper approach.

Book is just an odd fish. pwolanin actually suggested that this approach (in D7) could kill Book module entirely.

I do think we need to document this weirdness, and I am leaving this open to debate. But for modules like Menu Node Edit, we have no mechanism for converting a Book into a 'section', since Books are not exposed to the Menu Overview form.

I suppose we also need an update function to clear out bad entries in {menu_node}

And here is the patch with that added.

Committed to HEAD and released as 6.x.1.2