$page variable in node.tpl.php is buggy

Here is a first patch, i think this still needs a test.

I tested it with eval php in a node body, but i'm sure your way is better :)

I used === because 1. its faster, 2. drupal should return the same database always, but i don't have a special reason why i chosed this.

I think we need this check in a couple module places. Lets put this check in own function like node_is_detail_page() or somesuch. Perhaps you can find similar cases by grep for menu_get_object().

Here is new version, which fixes on every(i hope) place.

Patch looks good to me, but it would be nice to have yched confirm that it would enable progress in #658150-3: Nodereference D7 port.

I think this would be a bit clearer in node_is_detail_page():

return isset($page_node->nid)) && $page_node->nid === $node->nid

I agree that === is non standard and should be removed until we require type consistency in other places as well.

Here it is

What about node_is_own_pager?

I wanted to say "node_is_own_page"

Let's stick with detail_page and fix the phpDoc.

This is actually a duplicate of #445902: (bool) menu_get_object() is *not* an equivalent of $page, but I'll kill the other one instead.

One thing I don't really understand: why do we need that at all? Isn't that something that build modes should cover completely? Why do we need to make a difference between "A node displayed on a node/xxx page by itself" and "A full node displayed somewhere"?

Reading #658150-3: Nodereference D7 port, I understand why we need that. Shouldn't we refactor the node template file? We could have:

- node.tpl.php: the node without its title
- node-wrapper.tpl.php: a wrapper with the title

I don't really understand why we removed the $page parameter. After all, it *was* useful.

+++ modules/node/node.module
@@ -1323,6 +1323,19 @@ function node_show($node, $message = FALSE) {
+function node_is_detail_page($node) {

Why not node_is_full_page() ?

Or if the consideration was that this clashes too much with the build mode, then why not even

node_is_page() ?

I'm on crack. Are you, too?

+1 for node_is_page()

+1 for node_is_page() here too

new version

Let's update the documentation, and get this committed! :)

how bout this

An aside, I don't like "I agree that === is non standard". As the author pointed out, it is significantly faster as it avoids running PHP's typecasting comparison. It really should be used by default, and avoids all sorts of nasty bugs like 0 == 'foo'.

+++ modules/node/node.module	16 Dec 2009 15:05:10 -0000
@@ -1323,6 +1323,20 @@ function node_show($node, $message = FAL
+ * Checks if the current page is displaying this sepcific node's 
+ * full view (ex. node/%nid) as opposed to its teaser.

PHPDoc summary should be on one line. Either shorten this or split it into a short summary and a separate description.

+++ modules/node/node.module	16 Dec 2009 15:05:10 -0000
@@ -1323,6 +1323,20 @@ function node_show($node, $message = FAL
+function node_is_page($node) {
+  $page_node = menu_get_object();
+  return (isset($page_node->nid)) ? $page_node->nid == $node->nid : FALSE;

I don't really see how $page_node->nid could ever not be set if there is a $page_node. menu_get_object() can, however, return NULL or FALSE. Thus, I'd suggest to change the first condition to !empty($page_node).

This review is powered by Dreditor.

EDIT: ignore this patch ... forgot the "!"

use this patch instead of #24

+++ modules/node/node.module	16 Dec 2009 18:41:24 -0000
@@ -1323,6 +1323,19 @@ function node_show($node, $message = FAL
+ * Check if the current page displays this sepcific node's full view (ex. node/%nid).
...
+ * @return
+ *   Is the current page displaying the full view of this specific node.

Typo in 'specific'... Also, 'this' is missing its context here.

However, since this is a simple helper function, we want to consolidate @return into the PHPDoc summary and remove @return.

"Returns whether the current page is the full page view of the passed in node."

+++ modules/node/node.module	16 Dec 2009 18:41:24 -0000
@@ -1323,6 +1323,19 @@ function node_show($node, $message = FAL
+  return (!empty($page_node)) ? $page_node->nid == $node->nid : FALSE;

We either want to drop the additional parenthesis around !empty() or wrap the entire ternary operator condition. The latter is preferred, because it clarifies where the condition starts and ends, and what is going to be returned.

I'm on crack. Are you, too?

Great - thank you

I agree that this is great. Plus, it unblocks the work on nodereference. Committed to CVS HEAD.

+function node_is_page($node) {
+  $page_node = menu_get_object();
+  return (empty($page_node) ? $page_node->nid == $node->nid : FALSE);
+}

Hm.

I rolled back the patch. That empty() should probably be !empty().

blarrg!! Sorry about that.

I beg to differ, testbot.

Committed to CVS HEAD. Thanks.

We have one test failure that is probably related to this patch:

Node preview title is equal to node title.  Node  node.test  1146  NodeTitleTestCase->testNodeTitle()  Fail

I have traced that failure back to #654854: Clean up conditions in comment_reply(), avoid node_view() when unnecessary., actually. Aside from the chronology, there is also a clear reason for why that patch causes the failure (node is not displayed on comment reply page). So this issue is innocent.

Unfortunately this resulted in the critical issue that the same node now cannot be displayed in multiple build modes on its own page. Eg. a node cannot appear in both a summary/teaser block and its own full page at the same time. Both will render the full page: #721754: A node cannot be displayed in different view mode on its own page.