Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.I'm writing tests for aggregator.module and noticed that menu_link_maintain() dies with a fatal error when called with $op update.
The reason is that $item is now an object, it was an array before and has not been converted in #320510: [DBTNG] revamp menu.inc with DBTNG syntax.
A test is not yet included, you can reproduce the issue by enabling the aggregator.module, create a category and then update it. it will die with an fatal error.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | 423690-menu-link-maintain.patch | 2.15 KB | agentrickard |
| #9 | drupal.menu_link_maintain_bug_4.patch | 4.31 KB | berdir |
| #7 | drupal.menu_link_maintain_bug_3.patch | 3.48 KB | berdir |
| #4 | drupal.menu_link_maintain_bug_2.patch | 747 bytes | berdir |
| drupal.menu_link_maintain_bug.patch | 594 bytes | berdir |
Comments
Comment #1
mr.baileysConfirmed bug, reviewed patch, applied patch, all good...
Comment #2
pwolanin commentedNo, I disagree - we made a choice to handle all menu links as associative arrays at all stages for consistency - hence the query should be altered to fetch associative arrays.
This is a bit ugly, but apparently is the way to do it for DBTNG:
Comment #3
pwolanin commentedOr Crell suggest we can use:
before the foreach.
Comment #4
berdirUpdated patch, it does now use fetchCol().
Comment #5
berdirSetting to needs review again.
Comment #6
pwolanin commentedHmm, looks ok - though not sure we've used that method much - I had to look at the doxygen to see what it does - I was confused with fetchField()
Can you double-check that this is correct if there are multiple rows in the result set?
Comment #7
berdirUpdated:
- Added some tests for menu_link_maintain(). Including adding, updating multiple links in different menus and delete
- Added a groupby to the query which loads the updated menus, we don't need to clear the cache of a menu multiple times.
Comment #9
berdirForgot to add the change in menu_test.module...
Comment #10
pwolanin commentedsure - guess there is no reason not to group the results.
Comment #11
pwolanin commentednew tests pass, looks good.
Comment #12
dries commentedCommitted to CVS HEAD. Thanks.
Comment #14
agentrickardThis is actually still broken, IMO. Since the update $op doesn't use menu_link_save(), we can't get expected results for patches like #457450: Adds hooks to track changes to {menu_links}.
Shouldn't we just be calling menu_link_save()?
Comment #15
agentrickardOK. Here is a revised version of the patch which standardizes the 'update' op to use menu_link_save().
I think if we don't use menu_link_save() in both cases, then we have written an exception handler, not an API function.
Comment #16
agentrickardComment #17
pwolanin commentedI'm still not clear why we'd need to track the change if all that's changing is the title - are you going to update the node title based on this?
Comment #18
agentrickardPossibly. Who knows what someone might want to do with the information. And, technically, not just nodes might use this function.
Comment #19
pwolanin commentedProbably premature optimization, but it would be nice to then tell menu_link_save that there are only title/options changes and that all the hierarchy info can be assumed to be correct as-is. Otherwise, we are doing a lot of work to make this change to one field. Still, I understand from a consistency standpoint why it's preferable.
Comment #20
agentrickardI'm going to set this issue back to fixed and address this change over in #457450: Adds hooks to track changes to {menu_links}.