When viewing a book page through /node/view/, the book block doesn't show up. Using /book/view/ does work.

Sounds like a tricky thing to fix though.

e.g.:
http://drupal.org/book/view/1
http://drupal.org/node/view/1

CommentFileSizeAuthor
#13 book_1.patch38.89 KBJonBob
#9 book_block.patch1.32 KBmoshe weitzman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

could be fixed with the much discussed 'global context' feature which will tell modules about the node currently being viewed

Boris Mann’s picture

Also doesn't show up if a book node is accessed through an alias.

One possible fix will be to check the node type instead of the URL. This will only work with nodes that have actually been created as book pages, not for other types of content that have been organized in a book.

Looking at the database, couldn't we just see if the current node id exists in the book table? If it does, then it's part of a book.

moshe weitzman’s picture

we could check the book table. but where and how to do so? we don't want to create a special case for books in the core engine.

killes@www.drop.org’s picture

Can't we access the un-aliased url somehow? This is also a problem when setting up block paths in general.

Stefan Nagtegaal’s picture

Well, my question is: why do we access every node using 'node/view/$nid'?
Why not;
- stories 'story/view/$nid';
- books 'book/view/$nid';
- pages 'page/view/$nid';
- and so on...

Why is it only with books that we use the 'book/view/$nid' and not with every other $node->type?

moshe weitzman’s picture

It seems that the best solution to this need is to wait for a nodeapi('view') hook which can query to see whether the currently viewed node is part ofa book. it should query only if we $main = 0 or $page = 1 or something like that.

moshe weitzman’s picture

Assigned: Unassigned » moshe weitzman

it is clear that the admins and users expect the book block to appear no matter how the user gets there. Thus I propose the attached patch which adds one fast query to most page views (when book block is enabled). I am using this patch with success on my intranet.

killes@www.drop.org’s picture

I'd sure like to see the patch. :)

moshe weitzman’s picture

FileSize
1.32 KB

with patch

JonBob’s picture

Dries’s picture

Not sure that patch is correct -- it looks as if it duplicates some code for no particular reason.

JonBob’s picture

Viewing book pages using the book/view/52 path doesn't display the node tabs, either. Is there any way we could consolidate these back into node/52 now, to solve both these issues at once? We do have a nodeapi('view') at long last, so that might change the situation.

JonBob’s picture

Assigned: moshe weitzman » JonBob
FileSize
38.89 KB

Here's a new patch that unifies the node/52 and book/view/52 paths for nodes. It involves a small change to hook_view(), which is discussed first:

Currently hook_view() expects node modules to return a themed node. However, each module does this the same way; they modify $node as necessary, then call theme('node', $node) and return the result. We can refactor this so that the calling function node_view() calls theme('node') instead. By doing this, it becomes possible for hook_nodeapi('view') to be called after hook_view() where the node contents are filtered, and before theme('node') where the body is enclosed in other HTML. This way the book module can insert its navigation into the body right before the theming.

Advantages of this refactoring:
- I can use it for book.module to remove the extra viewing path.
- The function of hook_nodeapi('view') becomes more like hook_view(), as neither will expect a return value.
- We more closely follow the flow of other nodeapi calls, which usually directly follow their corresponding specific node type hooks (instead of preceding them).
- The attachment.module people could use it to append their attachments in a list after the node.
- Gabor could use it instead of his filter perversion for his "articles in a series" module.
- A little less code in each view hook.
- The content hook is no longer needed, so that means even less code.

Disadvantages:
- Any modules written to use nodeapi('view') could be affected (but these would all be post-4.4 modules).
- Implementations of hook_view() would need to be updated (but return values would be ignored, so most would work without updates anyway).

Now the patch takes advantage of this API shift to inject its navigation at the end of all book nodes, regardless of the viewing path. In fact, since the paths become identical, I've removed the book/view handler entirely. We should probably provide an .htaccess rewrite for this (one is still needed for node/view/nn anyway). At the same time, there is a check in book_block() that shows the block appropriately on these pages.

There is a slight performance hit in the form of an extra database call to check for whether to display the block, but this is offset by the removal of much duplicate code in book.module and the fixing of three significant book-related bugs (this one, http://drupal.org/node/view/7867 and the missing tab issue).

Dries’s picture

I tested this against a local copy of the Drupal handbook and it works like a charm. Committed.

Anonymous’s picture

kbahey’s picture

I have a question regarding this patch: does it fix the alias issue with books?

I mean the one I reported in http://drupal.org/node/view/7867 as well as others who reported the same.

If it does fix this issue, then does it also fix the issue of xtemplate mission not displayed when a home page other than "node" is used? As in: http://drupal.org/node/view/7685

JonBob’s picture

If you look at the issue URL you posted, you'll see that it was indeed marked fixed due to this patch.

The XTemplate mission block has nothing to do with book view paths.