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.In the nodeapi 'load' operator this is done:
function book_nodeapi(&$node, $op, $teaser, $page) {
switch ($op) {
case 'load':
// Note - we cannot use book_link_load() because it will call node_load()
$info['book'] = db_fetch_array(db_query('SELECT * FROM {book} b INNER JOIN {menu_links} ml ON b.mlid = ml.mlid WHERE b.nid = %d', $node->nid));
if ($info['book']) {
$info['book']['href'] = $info['book']['link_path'];
$info['book']['title'] = $info['book']['link_title'];
$info['book']['options'] = unserialize($info['book']['options']);
return $info;
}
break;
In the settings you can limit the type of nodes which can be used for books
variable_get('book_allowed_types', array('book'))
There for it would save a lot of queries (for example the forum's overview page because topic's won't be a book type most of the times) if it would be made like this:
function book_nodeapi(&$node, $op, $teaser, $page) {
switch ($op) {
case 'load':
if (in_array($node->type, variable_get('book_allowed_types', array('book')))) {
// Note - we cannot use book_link_load() because it will call node_load()
$info['book'] = db_fetch_array(db_query('SELECT * FROM {book} b INNER JOIN {menu_links} ml ON b.mlid = ml.mlid WHERE b.nid = %d', $node->nid));
if ($info['book']) {
$info['book']['href'] = $info['book']['link_path'];
$info['book']['title'] = $info['book']['link_title'];
$info['book']['options'] = unserialize($info['book']['options']);
return $info;
}
}
break;
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | revert-251239-7x.patch | 1.56 KB | pwolanin |
| #7 | book_fix.patch | 1.01 KB | R.Muilwijk |
| #4 | book_nodeapi_load-6-dev.patch | 1.37 KB | R.Muilwijk |
| book_nodeapi_load.patch | 1.37 KB | R.Muilwijk | |
Comments
Comment #1
R.Muilwijk CreditAttribution: R.Muilwijk commentedComment #2
Dries CreditAttribution: Dries commentedI think this is a good suggestion, so I've committed it to CVS HEAD after some testing. I think it would be good to backport this to D6, but only after another good look at the patch. Changing the version from D7 to D6.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedBack to "needs review", then, in order for the review process to take place.
The suggestion makes sense, and the patch (on core review) looks good. I'm only concerns by the fact that the patch now conditionally defines
$node->book, which might break not E_ALL-proof codes elsewhere.Comment #4
R.Muilwijk CreditAttribution: R.Muilwijk commentedPatch for 6-dev
Comment #5
R.Muilwijk CreditAttribution: R.Muilwijk commentedcould someone please review the patch for 6-dev.
Comment #6
pwolanin CreditAttribution: pwolanin commentedThis patch is not correct - "Users with the administer book outlines permission can add all content types."
Thus any node of any type *may* be part of the book outline (in D5 as well as D6). So, either the D7 patch should be reverted or a bigger patch that changes this behavior needs to be submitted.
For D6/D7 one could maintain a variable listing all node types that currently ARE part of the book outline, but I think that might be a bit hard to maintain. Easy to add them, but hard to know when to remove them from the list? Well, maybe not if you keep a track of the number of them via a keyed array with the values equal to the #of posts?
Comment #7
R.Muilwijk CreditAttribution: R.Muilwijk commentedHmmz pwolanin makes sense. I think this patch should fix it for head.
Comment #8
R.Muilwijk CreditAttribution: R.Muilwijk commentedComment #9
pwolanin CreditAttribution: pwolanin commentedNo, please revert the initial patch for now until and unless there is a complete and well-tested solution. You are totally ignoring a second permission, and ignoring the "Outline" tab.
Comment #10
R.Muilwijk CreditAttribution: R.Muilwijk commentedHmmz, okay that is fine however what would be the complete solution?
My thoughts:
Always only use the node types that have been marked as 'book' type. To limit access so some people can only add certain node types for a book and admin can add more make permissions for each node type that has been marked as 'book' type. This how you can give permissions to which node types can be added by certain people. This will delete the access for 'administer book outlines' to add all type of nodes. Discussion could be whether or not more modularity is important or performance. My vote is for performance because book types are most of the time limited to 'image' and 'page' and thus saving a lot of queries for 'views' and 'forums'.
I'll wait for the input for this before I make the patch.
Comment #11
pwolanin CreditAttribution: pwolanin commentedHmm, but that solution is problematic also - since the current setting allows many users to add nodes of the defined type, and admins to add anything else to the book. I think a solution would need to still keep some separation between which types users with the two different permissions can add. In addition, such a solution would need to take the update path into consideration, since existing sites from Drupal 4.7, 5, 6, etc may already have nodes of any type in the hierarchy.
Apparently the current book module tests are not checking this functionality, so I'll try to write a new test in a separate issue.
In Drupal 7 the loaded node is likely to be cached in some form, so I really think this should just be reverted for now.
The initial patch does not revert cleanly for me - attached patch should do it.
Comment #12
pwolanin CreditAttribution: pwolanin commentedComment #13
Dries CreditAttribution: Dries commentedReverted and committed to HEAD.
Comment #14
pwolanin CreditAttribution: pwolanin commentedThanks Dries. If we want to address this as a D6 performance issue, (which I'm not sure is needed, since this is a very simple query) my best thought so far remains a variable that tracks the number of nodes of each type that exist in the book table.
Comment #15
R.Muilwijk CreditAttribution: R.Muilwijk commentedpwolanin, I would want to see this as a performance issue. Offcourse it is a simple query. However on for example the forum page you show 25 nodes. 25 nodes do a node_load(). With alot of Drupal modules they just add their information in node_laod whether the node type is relevant or not. On one install with some contrib we had 11 queries for each node_load! This means 25*11= 275 queries.
With some patches this was brought down to 4 relevant for each node load. Saving 175 queries which were just useless! I think we should give the right example for using nodeapi load the right way which means only for nodes that matter. If there is a setting which lets you limit these node types then use it and don't give some adminster the right to put everything in a book. There has to be some kind of filter.
I still think adding a permission for each node type, which defaults to true, which is selected on the book settings interface is the way to go.
Comment #16
catchIt'd be nice to standardise on a ui for stuff like this - for example forums allows you to specify which node types can be used in forums - and does this via the vocabulary settings (although that's by no means ideal).
Comment #17
R.Muilwijk CreditAttribution: R.Muilwijk commentedcatch, that would be hard because Forum is just plain old taxonomy. It's worth to think about but I would not like to do that in this issue. Do you have any comments on this specific issue?
Comment #18
catchWith node_load_multiple() this is much less of an issue - since we only do one query per page for book module regardless of how many nodes are loaded, so won't fixing this.
Comment #19
catchHmm, would still make a small difference on individual node views, so worth looking at. I think the variable should be automatically updated on node_save() rather than configuration though.
Comment #20
pwolanin CreditAttribution: pwolanin commentedI don't think this is feasible (as suggested above) since any node of any type can be in a book.