It seems as though menu.inc needs to clear the page and block cache when a menu item is saved or deleted, or the menu router is rebuilt? Since the menus may be displayed in cached blocks and/or on cached pages this seems pretty important.
of course, this means that sometimes it would get cleared twice (e.g on node_save for a node with a menu_orf menu item or in a book).
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | menu_less_cache_clears-170514-17.patch | 3.56 KB | pwolanin |
| #15 | menu_less_cache_clears-170514-15.patch | 3.48 KB | pwolanin |
| #12 | menu_less_cache_clears-170514-12.patch | 2.27 KB | pwolanin |
| #11 | menu_less_cache_clears-170514-11.patch | 2.29 KB | pwolanin |
| #7 | menu_less_cache_clears-170514-7.patch | 2.07 KB | pwolanin |
Comments
Comment #1
pwolanin commentedpatch still applies with offset
Comment #2
pwolanin commentedthis is pretty trivial - any reason not to proceed with this?
Comment #3
chx commentedYes -- though the menu blocks themselves are not cacheable it's still a lot safer to blow the cache.
Comment #4
gábor hojtsyThanks, committed.
Comment #5
chx commentedhttp://drupal.org/node/181758 warns us that menu_link_save will issue one cache_clear_all for each link which is very unnecessary.
Comment #6
pwolanin commentedhuh? That doesn't patch doesn't seem to make sense. No matter where or why menu_link_save is called the cache should be cleared once
Maybe a static var in menu_link_save()?
Comment #7
pwolanin commentedhow about something more like this - also clears out a now unused param for the link deletion. We may also be able to remove the cache_caler_all() in function menu_rebuild() but I'm not sure.
Comment #8
chx commentedI love this approach.
Comment #9
gábor hojtsyErm, if we clear the cache on the end of the page request in a shutdown function, the cache will not be rebuilt through the page request handling. For example the deleted menu item will still be in the page generated, and will only disappear on the next page reload. Let's discuss this.
Comment #10
pwolanin commented@Gabor - I don't think that's likely happen.
If the page load is saving or deleting a link, then you have almost certainly just submitted a form. The saving/deleting happens during the page load that handles the form submission, then you'll usually get a redirect. I guess you might get stale data shown if you have a form that does not redirect after submission.
The alternatives are to either clear the cache many times (potentially) or to only clear it after the first link is saved or deleted. I was trying to figure out whether this latter approach could also lead to problems - for example another user start a page load after the cache is cleared but before additional links are saved or deleted.
Comment #11
pwolanin commentedHere's a patch that should avoid all possible race conditions by clearing the cache up to two times. Is this too complicated?
I just whipped this up, so it needs some testing.
Comment #12
pwolanin commentedfixes indenting
Comment #15
pwolanin commentedMoshe pointed out in an e-mail that there are still an absurd number of cache clears upon menu rebuild - the come from other calls in menu_link_save() the specifically clears part of the cache_menu table, and also from calls to variable set.
Attached patch deals now with all 3 cases and greatly reduces the number of calls (also suggests that devel module needs a little reworking to make sure its shutdown functions are last)
Comment #16
pwolanin commentednote - when using devel module's "empty cache" function, this patch reduces for me the number of queries from 1626 to 901.
related patch for devel module too: http://drupal.org/node/182836
Comment #17
pwolanin commentedslight tweaking - we may want to check expanded menus twice also to minimize the possibility of having incorrect menus displayed.
Comment #18
pwolanin commentedpatch still applies with offset
Comment #19
pwolanin commentedThe easiest way to evaluate this patch is to have the Devel module installed - look at the number of queries - for example on admin/build/modules. I see this patch reduce the number of queries on that page from 1773 to 1009.
The logic is also designed to protect against race conditions on busy sites, but it's a little hard to test that directly.
Comment #20
pwolanin commentednote - the patch also removes a bit of code cruft - the now unused and unneeded 2nd parameter to function _menu_delete_item()
Comment #21
chx commentedI have checked the patch and it's ready to go.
Comment #22
gábor hojtsyCommitted, thanks.
Comment #23
(not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.