in admin_menu_menu_alter - a variable_set() is being used to try to pass info within a page request - but that may have weird consequences under load, such as having another request trigger a full menu_rebuild

We need to switch to a static variable and find some way to insure admin_menu_footer() either uses locking, or never triggeres a menu rebuild itself.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Status: Active » Needs review
FileSize
2.99 KB

It also turn out that hook_footer() IS NOT CALLED on the same page request as the menu rebuild happens on form submissions, since the form submit function does the rebuild, and then there is a drupal_goto() and we never do output functions like hook_footer(). So the logic of this code was fundamentally broken.

Moving the logic to hook_exit() seems to be the right thing to do.

This also uses a static variable rather then imposing the performance penalty of variable_set/get.

Status: Needs review » Needs work

The last submitted patch, 1189532-1.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
3.82 KB

Missed a use of the variable in the .inc file.

Status: Needs review » Needs work

The last submitted patch, 1189532-3.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
6.98 KB

need to force a menu rebuild via the UI to get it to work properly for the tests.

pwolanin’s picture

Status: Needs review » Fixed

committed.

sun’s picture

Status: Fixed » Needs work
+++ b/admin_menu.inc
@@ -4,8 +4,19 @@
+  // Since it's possible this core function might change, check
+  // that it exists. We do this instead of calling menu_router_build()
+  // since that may trigger another menu rebuild that is not protected
+  // by the lock API.
+  if (function_exists('_menu_router_cache')) {

I don't really get this comment -- why should the function change?

+++ b/admin_menu.inc
@@ -4,8 +4,19 @@
+    // Something went wrong. Don't risk triggereing another menu rebuild.

Typo in "triggereing".

+++ b/admin_menu.module
@@ -125,23 +125,12 @@ function admin_menu_suppress($set = TRUE) {
 function admin_menu_footer($main = 0) {
...
-    _admin_menu_rebuild_links();
...
   $content .= admin_menu_tree_output(menu_tree_all_data('admin_menu'));

@@ -149,6 +138,17 @@ function admin_menu_footer($main = 0) {
+function admin_menu_exit() {

Since hook_footer() is invoked before hook_exit(), it looks like there's a chance for getting an empty menu?

+++ b/tests/admin_menu.test
@@ -8,11 +8,24 @@
+    // Forcing a menu rebuild via the UI seems to be required for
+    // the tests to work.
+    $this->drupalPost('admin/settings/performance', array(), t('Clear cached data'));
+    $this->drupalLogout();

I can only guess that this just simply means that the (re)building logic does not work upon module installation. Did you test the installation manually?

+++ b/tests/admin_menu.test
@@ -126,7 +123,7 @@ class AdminMenuLinksTestCase extends DrupalWebTestCase {
-    drupal_flush_all_caches();
+    $this->drupalPost('admin/settings/performance', array(), t('Clear cached data'));

This change shouldn't be needed.

pwolanin’s picture

@sun - The core function starts with "_" so in theory might be changed between core versions, or it may even be absent in old D6 versions (didn't look)

All the test changes were necessary to get it to pass. The trick is that hook_exit() has to happen in the same page request as the actual menu rebuild.

pwolanin’s picture

manual installation of the module works via the UI.

Testing drush - that fails. does drush skip hook_exit()?

sun’s picture

it may even be absent in old D6 versions

If that may be the case, then we may need to retain the old menu_router_build() as a backup?

pwolanin’s picture

Status: Needs work » Needs review

@sun - that function was added to core over 2 years ago

http://drupalcode.org/project/drupal.git/commit/9bd26d2ed0d5c13ac726e1db...

though I guess we can have the fallback just in case.

I had not pushed the commit, so backed it out and committing this. Note includes a drush .inc file to make this work with drush.

pwolanin’s picture

FileSize
7.39 KB
pwolanin’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

xjm’s picture