According to http://api.drupal.org/api/function/hook_node_view/7 (and confirmed by looking at the code), hook_node_view() has a $teaser parameter but no longer has a $page parameter.

However, I noticed that many implementations of this hook throughout Drupal core still assume a $page parameter is being passed.

This should be relatively trivial to fix, but some of the implementations still have code that only runs when $page is TRUE, and it's not immediately clear to me if that code is relevant anymore (because if it is, it needs to be moved somewhere else). For example, in book_node_view():

if ($page) {
  menu_set_active_trail(book_build_active_trail($node->book));
  menu_set_active_menu_name($node->book['menu_name']);
}

Also note that in this particular case, the menu_set_active_menu_name() function no longer even exists in Drupal (it was recently changed to menu_set_active_menu_names).

Marking as critical for now, since I'm reasonably sure something is seriously broken here, just not sure what :)

CommentFileSizeAuthor
#2 remove_page_from_remaining_hook_node_view.patch3.36 KBbrianV

Comments

brianV’s picture

brianV commented

This is done as part of #350984: Kick comment rendering out of node module.

Some quick grepping of the code shows the following functions need to be updated:

modules/book/book.module:738:function book_node_view($node, $teaser, $page) {
modules/trigger/trigger.module:247:function trigger_node_view($node, $teaser, $page) {
modules/upload/upload.module:343:function upload_node_view($node, $teaser, $page) {
modules/statistics/statistics.module:104:function statistics_node_view($node, $teaser, $page) {

I have a patch in the works for this. Trigger, upload and statistics all just need $page pulled out of the function declaration. book.module is a bit more work...

The issue that changed this never should have been committed without updating all the function calls to these functions...

brianV’s picture

brianV commented
StatusFileSize
new remove_page_from_remaining_hook_node_view.patch3.36 KB

Patch attached to fix up these remaining modules.

In the patch committed in #350984: Kick comment rendering out of node module, they replace

 if ($page) { 
  ...
}

by calling 

 if ((bool)menu_get_object()) {
 ...
}

.

I've followed that convention in this patch. This is a pretty ugly way of doing it, but that is a different issue!

marcingy’s picture

marcingy commented
Status: Active » Needs review

Setting to code needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I do not understand why you do not simply replace $page with !$teaser...?

Frando’s picture

Frando commented

We cannot simply replace $page with !$teaser because on a listing of full nodes, we have $page = FALSE and $teaser = FALSE, I think. So replacing $page with menu_get_object() looks good to me (and has already been done a couple of times in D7 IIRC).

David_Rothstein’s picture

David_Rothstein commented
Status: Needs work » Fixed

It looks like all these were fixed in a recent CVS commit. (I guess someone wrote a patch to fix them without checking the queue for existing issues first, oh well...)

damien tournoud’s picture

damien tournoud commented

I confirm I fixed the book implementation that as part of #444920: Book breadcrumbs are broken.

However please note that #445902: (bool) menu_get_object() is *not* an equivalent of $page: think about a full-node view called displayed on a node/xxx page.

brianV’s picture

brianV commented

Damien - is there any dependable method then of determining if we are on a page or not?

brianV’s picture

brianV commented

never mind - I am reading the issue you linked.

Status: Fixed » Closed (fixed)

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