menu_link_maintain(), $op update critically broken.
Berdir - April 4, 2009 - 15:17
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | menu system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Berdir |
| Status: | closed |
| Issue tags: | Quick fix |
Description
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal.menu_link_maintain_bug.patch | 594 bytes | Idle | Failed: Failed to apply patch. | View details | Re-test |
#1
Confirmed bug, reviewed patch, applied patch, all good...
#2
No, 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:
$result = db_select('menu_links', NULL, array('fetch' => PDO::FETCH_ASSOC))#3
Or Crell suggest we can use:
$result->setFetchMode(PDO::FETCH_ASSOC);before the foreach.
#4
Updated patch, it does now use fetchCol().
#5
Setting to needs review again.
#6
Hmm, 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?
#7
Updated:
- 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.
#8
The last submitted patch failed testing.
#9
Forgot to add the change in menu_test.module...
#10
sure - guess there is no reason not to group the results.
#11
new tests pass, looks good.
#12
Committed to CVS HEAD. Thanks.
#13
Automatically closed -- issue fixed for 2 weeks with no activity.
#14
This 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()?
#15
OK. 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.
#16
#17
I'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?
#18
Possibly. Who knows what someone might want to do with the information. And, technically, not just nodes might use this function.
#19
Probably 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.
#20
I'm going to set this issue back to fixed and address this change over in #457450: Adds hooks to track changes to {menu_links}.
#21
Automatically closed -- issue fixed for 2 weeks with no activity.