On all general admin pages, panels_mini_menu() calls panels_mini_load() with an invalid argument.
admin/panels/panel-mini for example triggers the query: SELECT * FROM panels_mini WHERE name = ''.
This is both unnecessary and wrong. panels_mini_menu() needs to check if arg(3) exists.

The enclosed patch fixes this by requiring arg(3) and then checking against already registered paths:

if (strpos($_GET['q'], 'admin/panels/panel-mini/') == 0 && !isset($GLOBALS['_menu']['path index'][$_GET['q']])) {
  ...
}

If you don't like the first part of the check, the following would be an equal alternative:

if (arg(0) == 'admin' && arg(1) == 'panels' && arg(2) == 'panel-mini' && arg(3))
&& !isset($GLOBALS['_menu']['path index'][$_GET['q']])) {
  ...
}
CommentFileSizeAuthor
#3 panels-DRUPAL-5--2.menu_.patch5.62 KBsun
panels_mini_menu_check_existing.patch1.25 KBpancho

Comments

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented

panels_page_menu() is not much better. I created a separate issue for that: #267028: Performance: panels_page_admin_menu_items.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Component: Mini panels » Miscellaneous

Attached patch fixes hook_menu in all contrib modules.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new panels-DRUPAL-5--2.menu_.patch5.62 KB

I can't help. d.o doesn't let me upload my patch :(

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented

Sorry, but: Nope, patch #3 doesn't really deliver what it promises. :(
Neither does it properly fix panels_mini_menu() as the original patch does, nor panels_page_menu() as suggested in the other patch referenced above. && !isset($GLOBALS['_menu']['path index'][$_GET['q']]) is the trick we need.
The rest of your patch are mostly codestyle fixes. Those are good, so they shouldn't be forgotten, but I had good reasons why I didn't roll a patch fixing everything.

This menu stuff is not just copy and paste, so I had my reasons to do it one by one. If we're all fine with taking this way for panels_mini_menu(), we can go on to panels_page_menu(), which is another case.
So for now I'd still need a review on the general method of doing this, which eans a review on the original patch.

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
Component: Miscellaneous » Mini panels
sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Neither does it properly fix panels_mini_menu() as the original patch does, nor panels_page_menu() as suggested in the other patch referenced above. && !isset($GLOBALS['_menu']['path index'][$_GET['q']]) is the trick we need.

...could you please elaborate on this a bit further?

#3 adds && arg(3) to the if condition, so the conditional code will only be executed if a mini-panel name is contained in the URL (arg(#) returns an empty string for non-existing path arguments).

Sure, it does not check whether such a menu item actually exists, but I've not seen such a check in any other module yet. Accessing $GLOBALS['menu'] directly is definitely not recommended. The menu system will automatically fall-back to admin/panels/panel-mini if arg(3) is not present, and the conditional code already checks whether a panel with the name of arg(3) can be loaded in front of adding the mini panels menu items.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work

I got your point now. However, strpos() also returns FALSE (0) if there was no match, so without a === this code would try to load panels mini menu items on any page. Aside from that, directly accessing the global $_menu array is a bit dirty and won't work for D6. Since we know which arg(3)s are have already been cached, can't we simply do an !in_array() ?

esmerel’s picture

esmerel commented
Status: Needs work » Closed (won't fix)

No fixes are going to get committed to the 2.x line