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.
book_node_load()
ignores the 'book_allowed_types' and trigger excessive SQL queries when a lot of nodes are arbitrarily loaded on the page.
Proposed patch: do not do the SQL query that load book contents on nodes for which the book type is not allowed.
Potential risk: if a node type is a book then is not anymore, some data may stall and remain silent in the book
database table, although there is very poor chances that happens on a production site.
Comments
Comment #1
pounardComment #2
pounardTagging.
Comment #3
pounardSo I guess this needs review after all, because book module is weird. I don't really understand why is there two variables, allowed types and child type?
But aside of that, patch really is working and saving us a looooooot of SQL queries.
Comment #4
pounard1: 2070807-1-book-node_load_excessive_sql.patch queued for re-testing.
Comment #5
pounardPlease someone review this, this seem to do its job quite well and would be a nice performance improvement.
Comment #6
amoebanath CreditAttribution: amoebanath at ComputerMinds commentedWorking nicely as needed
Comment #7
pounardWow, that's fast ! Thank you.
Comment #8
mcdruidI could be missing something but it looks like this applies to D9/10 too.
book_node_load() calls \Drupal\book\BookManager::loadBookLinks() which calls \Drupal\book\BookOutlineStorage::loadMultiple() which looks for matching nids in the book table without checking book_allowed_types.
I've not dug into that in any more depth, but it seems like a fairly similar situation.
Having said that, this seems like a big performance win for D7 and I'm inclined to say we should commit it and file a followup for D9/10 rather than work the other way around and potentially never get this improvement in to D7 (@pounard has already been very patient!).
Comment #10
mcdruidFWIW in D9/10 I think book_node_load would check each node's type with book_type_is_allowed($type) before looking up its nid.
In terms of the backport policy, committing this to D7 will not introduce a regression in functionality for any sites migrating from D7 to D9/10, so I'm going to commit it and we can file that followup "forward-port" for consideration in D9/10.
Finally, LOL #7 :)
Thank you!
Comment #11
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedActually I have found the D9/10 issue, but it seems that the approach there will be a bit different, because it uses the
hook_ENTITY_TYPE_load
, so the only possibility is to loop thru all nodes and filter them by types.https://www.drupal.org/project/drupal/issues/3096844
This D7 patch used the recommended approach mentioned also in the
hook_node_load
documentation example (the$types
variable).Comment #12
alrh CreditAttribution: alrh commentedThis patch broke some of the books on our sites.
It assumes that a book only contains node types from 'book_allowed_types' but the description for this setting mentions: Users with the "Administer book outlines" permission can add all content types.
Comment #13
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedThanks for reporting this, we will take a look at that.
If possible, I think there could be a (temporary) workaround for sites experiencing this, if they allow also other content types in book outlines. This should "fix" the problem for the content types currently not allowed, but still used in book outlines.
Comment #15
mcdruidFor now we've reverted this in preparation for #3326836: hotfix release of Drupal 7 (7.94?).
Perhaps for a future release we could consider putting the check behind a flag (defaulting to off) so that sites that use the book module but only with book_allowed_types could opt-in to the optimisation.
Comment #16
mcdruidBack to NW for the above.
Comment #17
joseph.olstad@mcdruid,
I think rather than an opt-in, we should just improve the patch.
All that would have to be done would be to invoke the original logic for those with the following permission:
"Administer book outlines"
For everyone else, the "ahem, clearing 9 years of flem in the throat" *new* optimised logic would be invoked making relevant db queries correctly targetted.
I applaud the fact that this is included in D7.93 and the quick feedback we got as an opportunity to improve it quickly for D7.95.
The D7.93 was a very ambitious release and I'm putting it into production tomorrow evening on two sites that don't have 'book' types, however I have another site with books and uses the great book_access module also (still no available Symfonised Drupal version of book_access, it's a Drupal 7 only module, I highly recommend this module).