I'm working on the Skeleton Outlines module, which lets users define templates of entire books. It works by storing node data and then calling drupal_execute() on that data to construct the book outline.

The problem is that book_get_books() keeps a static variable of all books. So the first time it's called, it works as expected, but then subsequent calls in the same request can fail because the newly-created book doesn't exist in the cache. This patch adds a reset parameter to book_get_books().

I will be working on a test for this soon, but if there is something obvious I've missed then please let me know :)

I've attached patches for D7 and D6, and I imagine it would make sense to backport to D5 as well.

CommentFileSizeAuthor
#26 360377-reset_book_cache-26.patch987 bytespwolanin
#17 360377-17-reset_book_cache.patch1.08 KBjanusman
#11 360377_reset_book_cache_6.x-11.patch1.71 KBjanusman
#4 360377_reset_book_cache_6.x_2.patch1.45 KBdeviantintegral
#2 reset_book_cache_7.x_2.patch3.43 KBdeviantintegral
#1 360377_reset_book_cache_7.x_1.patch3.37 KBdeviantintegral
reset_book_cache_7.x.patch801 bytesdeviantintegral
reset_book_cache_6.x.patch822 bytesdeviantintegral
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral commented
Assigned: deviantintegral » Unassigned
FileSize
360377_reset_book_cache_7.x_1.patch3.37 KB

Here is the start of a test. It's still broken in that somehow the nodes are being created in the actual DB, not in the simpletest DB. I'll work on this again in the next few days.

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral commented
Status: Needs work » Needs review
FileSize
reset_book_cache_7.x_2.patch3.43 KB

Here is a patch with a working test. The test is dependent on #297972: drupal_execute is incompatible with batch API.

In book.module:

  1. The $reset parameter is added as above.
  2. After inserting or deleting a book, book_get_books(TRUE) is called to reset the cache.

And in book.test:

  1. A new method, createBookDrupalExecute() is created, as we can't use drupalPost() to expose the bug.
  2. It then checks to see if the book outline differs when the new book is created, and if it is back to the original state after deleting the new book.

Review / testing / comments would all be appreciated!

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Yet another use case for a centralized static variable cache.

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral commented
FileSize
360377_reset_book_cache_6.x_2.patch1.45 KB

Here is a patch for Drupal 6 as well. It is pretty much the same except that it excludes the test.

A centralized cache does make sense, especially if it had some way of easily exposing to developers that something is cached per request. It's not obvious until you look inside book_get_books() that a static comes into play, leading to much frustration.

Status: Needs review » Needs work

The last submitted patch failed testing.

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral commented
Status: Needs work » Needs review

Setting to needs review as the bot tried to test the 6.x patch I posted.

Wies Hubbers’s picture

Wies Hubbers CreditAttribution: Wies Hubbers commented
Version: 7.x-dev » 6.x-dev

I have the same issue here! I add book dynamically and I use book_get_books to get to them.

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral commented

The Drupal 7 issue was addressed with the implementation of drupal_static over at #480412: Update book module to use drupal_static().

@wies, can you review the patch against Drupal 6?

idflood’s picture

idflood CreditAttribution: idflood commented

I applied the patch to Drupal 6 head without any error. ( no further testing yet ). How can I help?

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral commented
Issue tags: +Newbie

Thanks idflood; if you could provide feedback about the code itself, so others reading this issue have more info about what you reviewed. Some good guidelines are here: http://drupal.org/patch/review

janusman’s picture

janusman CreditAttribution: janusman commented
FileSize
360377_reset_book_cache_6.x-11.patch1.71 KB

I did some testing and I think the earlier patch missed one reset case. New patch for current DRUPAL-6 checkout.

janusman’s picture

janusman CreditAttribution: janusman commented

Re: #8 ... I don't think the D7 issue was addressed *fully*; while it's true that drupal_static is used, and that means tests now reset that static variable, a delete/update/insert action in nodeapi does not clear the $all_items in book_get_books; thus Batch API runs, or other modifications to the book tree will not be able to get updated data from book_get_books().

So IMO this means the patch in #11 should be ported to D7 too. Thoughts?

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

subscribe

IrishGringo’s picture

IrishGringo CreditAttribution: IrishGringo commented

I am thinking of using skeleton, but I do not know if the latest version of DRUPAL still requires the patch. can I get an update on if the patch is required please

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral commented

@janusman, that makes sense, though someone will have to try it to confirm.

@IrishGringo, Drupal 6 certainly requires the patch for Skeleton. Please try the one uploaded in #11. And if you get a chance, try syncronizing changes where the book root page is deleted, as I think that might be what will trigger the case mentioned in #11.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

If it should be in D7, let's fix it there first?

Looks simple enough patch for D6, though I might use a pattern like:

if ($reset) {
  $all_books = NULL;
}
janusman’s picture

janusman CreditAttribution: janusman commented
Version: 6.x-dev » 7.x-dev
FileSize
360377-17-reset_book_cache.patch1.08 KB

Ok, agreed this should get fixed in D7 first. Here's a patch.

Status: Needs review » Needs work
Issue tags: -Newbie

The last submitted patch, 360377-17-reset_book_cache.patch, failed testing.

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral commented
Status: Needs work » Needs review
Issue tags: +Newbie

#17: 360377-17-reset_book_cache.patch queued for re-testing.

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral commented

I've requested a retest as it apparently failed in contact.test, but the failing line is function testSiteWideContact() {

janusman’s picture

janusman CreditAttribution: janusman commented

Well... tests pass. Hopefully someone else will weigh in and mark this RTBC.

janusman’s picture

janusman CreditAttribution: janusman commented
Version: 7.x-dev » 8.x-dev

Moving to D8.

janusman’s picture

janusman CreditAttribution: janusman commented
Status: Needs review » Needs work

Also, needs work since patch won't apply anymore.

btopro’s picture

btopro CreditAttribution: btopro commented
Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review

bump. any thoughts on implementing this? Tried it in D6 and last patch worked for me. D7 patch passed validation, seems like a worthy improvement for both branches and relatively simple

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

bugs have to be fixed in HEAD first

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
360377-reset_book_cache-26.patch987 bytes

@janusman- the patch seems to apply fine (with offset). Here's a re-roll against 8.x.

eileen’s picture

eileen CreditAttribution: eileen commented

#4: 360377_reset_book_cache_6.x_2.patch queued for re-testing.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Reviewed & tested by the community

I think this is pretty trivial - marking rtbc since it's still passing tests.

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral commented

Agreed on the RTBC.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: +Needs backport to D7

tagging for backport.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

I can't think of a reasonable way to automatically test this, so committed and pushed to 8.x and 7.x. Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -Newbie, -Needs backport to D7

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

Issue tags: +Newbie, +Needs backport to D7

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.