Yet another static cache problem. This time, the menu tree can become stale when inserting multiple book pages in a single request. As the menu tree is stale, children of children can't be inserted:

Example:

Parent
-- Child
-- Child

works but

Parent
-- Child
--- Child

will fail on the last page.

Patches are forthcoming. This affects Drupal 6 as well.

CommentFileSizeAuthor
#9 menu_tree_all_data_cache_D6.patch1.62 KBmark.lindsey
#7 364529_menu_tree_all_data_cache_6.x_7.patch1.39 KBdeviantintegral
#5 364529_menu_tree_all_data_cache_6.x.patch1.33 KBidflood
#1 364529_menu_tree_all_data_cache_6.x.patch1.34 KBdeviantintegral
#1 364529_menu_tree_all_data_cache_7.x.patch1.32 KBdeviantintegral

Comments

deviantintegral’s picture

deviantintegral commented
Status: Active » Needs review
StatusFileSize
new 364529_menu_tree_all_data_cache_7.x.patch1.32 KB
new 364529_menu_tree_all_data_cache_6.x.patch1.34 KB

Here are patches for 6.x and 7.x. Any suggestions for appropriate test cases? I could just verify that the cache is accurate after adding menu items, but that seems a little simple.

Also, I currently reset the cache in menu_rebuild(), and then leave it up to the calling code to call menu_rebuild(). I'm guessing that book.module should call that somewhere (it doesn't), but I think that should be a seperate patch as it probably occurs in other modules as well.

svdoord’s picture

svdoord commented

I ran into this problem with my custom module in Drupal 6. The suggested patch fixes my problem, so I hereby vote to have it in the next Drupal 6 release.

Status: Needs review » Needs work

The last submitted patch failed testing.

deviantintegral’s picture

deviantintegral commented
Version: 7.x-dev » 6.x-dev
Status: Needs work » Needs review

This has been dealt with in HEAD over at #422368: convert menu.inc to use new static caching API.

Anyone feel like giving the patch against 6.x another review?

idflood’s picture

idflood commented
StatusFileSize
new 364529_menu_tree_all_data_cache_6.x.patch1.33 KB

I simply added manually the change to the head, the last patch won't add everything.

deviantintegral’s picture

deviantintegral commented
Issue tags: +Newbie

Re-roll looks good; don't want to RTBC it as I created it, but I'm marking as a newbie-friendly patch as it's a fairly simple and clear change.

deviantintegral’s picture

deviantintegral commented
StatusFileSize
new 364529_menu_tree_all_data_cache_6.x_7.patch1.39 KB

Here is a reroll to sync against DRUPAL-6.

Status: Needs review » Needs work

The last submitted patch, 364529_menu_tree_all_data_cache_6.x_7.patch, failed testing.

mark.lindsey’s picture

mark.lindsey commented
Status: Needs work » Needs review
StatusFileSize
new menu_tree_all_data_cache_D6.patch1.62 KB

I am also running into this static caching issue.

The attached patch adds a reset parameter to menu_tree_all_data() so you can refresh the cached storage of a particular menu. This is not an issue in D7 because menu_tree_all_data() uses the drupal_static API. However, this bug still affects D6.

mark.lindsey’s picture

mark.lindsey commented

Bump. Is this something that will be addressed?

Status: Needs review » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.