book_get_books() cache becomes stale when batch-inserting book pages
deviantintegral - January 17, 2009 - 22:57
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | book.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | Newbie |
Description
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| reset_book_cache_6.x.patch | 822 bytes | Idle | Failed: Failed to apply patch. | View details | Re-test |
| reset_book_cache_7.x.patch | 801 bytes | Idle | Failed: Failed to apply patch. | View details | Re-test |
#1
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.
#2
Here is a patch with a working test. The test is dependent on #297972: drupal_execute is incompatible with batch API.
In book.module:
book_get_books(TRUE)is called to reset the cache.And in book.test:
createBookDrupalExecute()is created, as we can't use drupalPost() to expose the bug.Review / testing / comments would all be appreciated!
#3
Yet another use case for a centralized static variable cache.
#4
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.
#5
The last submitted patch failed testing.
#6
Setting to needs review as the bot tried to test the 6.x patch I posted.
#7
I have the same issue here! I add book dynamically and I use book_get_books to get to them.
#8
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?
#9
I applied the patch to Drupal 6 head without any error. ( no further testing yet ). How can I help?
#10
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