Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.menu_get_names() contains an incorrect PDO syntax, passing a single field as a string to fields() rather than an array.
This causes a fatal error when the function is called. The function is not called anywhere in core now, so it's hard to find this. I came across it while working on DHTML Menu, which does call it.
This is a critical bug as far as my addon is concerned, but not from the perspective of core, so I leave it at normal priority. Nevertheless, I'd like this patch to get in quickly. :)
| Comment | File | Size | Author |
|---|---|---|---|
| #28 | menu_get_names-3.patch | 2.33 KB | c960657 |
| #16 | menu_get_names-2.patch | 4.24 KB | c960657 |
| #14 | menu_get_names-1.patch | 3.43 KB | c960657 |
| #5 | menu-menu_get_names-fields-473240-9.patch | 1.41 KB | cburschka |
| #2 | menu-menu_get_names-fields-473240-2.patch | 592 bytes | cburschka |
Comments
Comment #1
cburschkaWhoops, my differ didn't work. Here we go.
Comment #2
cburschkaExtra closing parenthesis. I can't do anything right today, can I? ;)
Comment #3
Dries CreditAttribution: Dries commentedShould we write tests for this? Given that it is not used in core, it sounds prone to bugs ...
Comment #4
cburschkaSure, good idea. It can be a simple function in the menu test, which lists menus, creates one, looks for it, then deletes it, lists menus and checks if it's gone.
Comment #5
cburschkaThis patch adds a check to modules/menu/menu.test. Strictly speaking, the function is in includes/menu.inc, but the menu module is the one that already creates a custom menu, so I figured it is easiest to put a line in there to check for this menu in the return value of menu_get_names().
Comment #7
cburschkaI can't get it to work either, even after resetting the static cache. No idea why the menu name isn't listed.
Comment #8
cburschkaEdit: The testing framework has confused the hell out of me yet again.
Comment #9
catchI like #2.
Comment #10
catchI think I posted to the wrong issue. Patch is failing though.
Comment #11
cburschkachx confirmed that it's impossible to test this without using a test module. menu_get_names() would have to be called in the context of the test site, requiring simpletest to install a hidden module and query a particular page that shows menu_get_names() return value. Even though we can reuse an existing test module (menu_test) it will still require an extra menu callback in that module. At that point we really have to ask if the one-line fix in the ten-line function is worth it. If we can do without a test, then #2 would be ready to go.
Setting back to NR for this reason.
Also, given that this actually crashes, bumping to critical lest we lose sight of it again.
Comment #12
Hugo Wetterberg CreditAttribution: Hugo Wetterberg commentedI second Arancaytar on this. #2 should be committed straight away, and the issue of providing testing coverage should be posted as a separate issue.
Comment #14
c960657 CreditAttribution: c960657 commentedAdded a test.
Comment #16
c960657 CreditAttribution: c960657 commentedSorry, forgot to include the actual change to menu.inc.
Comment #17
cburschkaHey, neat! Less code than I thought would be required.
Patch works, uses good names for the new callbacks and has clean code-style. This is probably ready to go.
Comment #18
Hugo Wetterberg CreditAttribution: Hugo Wetterberg commentedGreat work Arancayar! Hopefully this will get committed soon now it's covered by tests too. Four months delay for that though...
Comment #19
pwolanin CreditAttribution: pwolanin commentedDo we really not use this anywhere in core? We are essentially pulling this data up for some forms - so maybe this function is redundant to http://api.drupal.org/api/function/menu_get_menus/7. That DBTNG looks a little odd also.
Comment #20
Hugo Wetterberg CreditAttribution: Hugo Wetterberg commentedWell, I wouldn't call it redundant. To just have the menu_get_menus function would require a dependency on the menu module. Which doesn't make sense as the menu api / inspection methods really shouldn't be tied that closely to what is described as "Allows administrators to customize the site navigation menu.". Rather it's the menu_get_menus function that's redundant, or whose functionality should've been merged into (or replaced) the menu_get_names in menu.inc
Comment #21
webchickLooks like this needs more discussion.
Comment #22
pwolanin CreditAttribution: pwolanin commented@webchick - no I was confused - this function needs to be available in the absence of menu module, and the patch here is a very straightforward bugfix.
Comment #23
Hugo Wetterberg CreditAttribution: Hugo Wetterberg commentedThis issue made me think about the division of API and UI for the menu system. Doesn't functions like:
menu_load
menu_parent_options
_menu_parents_recurse
menu_reset_item
_menu_parent_depth_limit
menu_get_menus
Belong to the API, and therefore menu.inc, rather than the menu module?
Comment #24
webchickOk cool, thanks Peter.
For the test, that looks a bit over engineered to me. All we really want to do is test the input/output of menu_get_names(), no? If so, don't bother with a page callback that var_export()s the contents of it, just do a straight-up assertEquals() on the function return value and the array of expected values.
Comment #25
Hugo Wetterberg CreditAttribution: Hugo Wetterberg commentedOk, I've created a separate issue for movin the API-functionality from the menu module into menu.inc http://drupal.org/node/569164
Comment #26
c960657 CreditAttribution: c960657 commentedNew patch with simpler test. This ignores the advice in #11 - on second thought I don't understand it.
Comment #27
cburschka@webchick: That's what I thought too (see #5). However, when calling a database-querying function inside a test class, the function will query the base site rather than the simpletest-generated site. So none of the menus we generate in Simpletest will be in the return value of the function. We can't even assume that the base site won't have custom menus - so it's not easy to get an array of expected values from anywhere.
Comment #28
c960657 CreditAttribution: c960657 commentedArgh! I forgot to attach the patch in #26. Here it is.
Sure? $db_prefix is changed in setUp(), so both the testing side and the tested side have access to the same database. Note that the "original" menu (defined in menu_test.module) is returned by menu_get_names(), both in menu_get_names-2.patch and menu_get_names-3.patch.
Comment #29
pwolanin CreditAttribution: pwolanin commentedsimple fix, simple test
Comment #30
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.