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.
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.
Comment | File | Size | Author |
---|---|---|---|
#12 | 189532-11.patch | 7.39 KB | pwolanin |
#5 | 189532-5.patch | 6.98 KB | pwolanin |
#3 | 1189532-3.patch | 3.82 KB | pwolanin |
#1 | 1189532-1.patch | 2.99 KB | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedIt 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.
Comment #3
pwolanin CreditAttribution: pwolanin commentedMissed a use of the variable in the .inc file.
Comment #5
pwolanin CreditAttribution: pwolanin commentedneed to force a menu rebuild via the UI to get it to work properly for the tests.
Comment #6
pwolanin CreditAttribution: pwolanin commentedcommitted.
Comment #7
sunI don't really get this comment -- why should the function change?
Typo in "triggereing".
Since hook_footer() is invoked before hook_exit(), it looks like there's a chance for getting an empty menu?
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?
This change shouldn't be needed.
Comment #8
pwolanin CreditAttribution: pwolanin commented@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.
Comment #9
pwolanin CreditAttribution: pwolanin commentedmanual installation of the module works via the UI.
Testing drush - that fails. does drush skip hook_exit()?
Comment #10
sunIf that may be the case, then we may need to retain the old menu_router_build() as a backup?
Comment #11
pwolanin CreditAttribution: pwolanin commented@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.
Comment #12
pwolanin CreditAttribution: pwolanin commentedComment #13
pwolanin CreditAttribution: pwolanin commentedComment #15
xjmCrossposting #808526: admin_menu.inc calls menu_router_build() explicitly, causing Duplicate entry error INSERT INTO {menu_router} (race condition).