A node cannot be displayed in different view mode on its own page

Whoa, subscribe.

We removed the $page parameter, and I still don't know why. Currently, we are determining if "the node is being displayed as a page" using:

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

(introduced by #658314: $page variable in node.tpl.php is buggy)

This is just broken in the case you spotted.

For reference, the $page argument was removed by #350984: Kick comment rendering out of node module.

I think this is a duplicate of #445902: (bool) menu_get_object() is *not* an equivalent of $page

which was marked duplicate of #658314: $page variable in node.tpl.php is buggy

Which was marked 'fixed'. But #445902: (bool) menu_get_object() is *not* an equivalent of $page is still outstanding no?

So the actual problems are:

http://api.drupal.org/api/function/node_view/7 adds contextual links based on node_is_page(), instead of the view mode. (I wanted to use the view mode, but somehow got voted out.)

http://api.drupal.org/api/function/template_preprocess_node/7 uses node_is_page() to populate the $page variable that is used in http://api.drupal.org/api/drupal/modules--node--node.tpl.php/7/source to determine whether the node title should be displayed or not.
Simple fix: The view mode is available here, just replace the variable.
Proper fix: Make display of title configurable per view mode, just like any other field.

http://api.drupal.org/api/function/comment_node_view/7 uses node_is_page() to conditionally attach the comments to a node. The view mode is available here, so quick fix: Replace node_is_page() with $view_mode == 'full'.

Basically, we should just push down the full page display mode as BUILD_MODE instead of trying to infer what the coder might have wanted from the environment.

Yes, indeed.

Can we clarify what is being meant with "pushing down full page display mode as view mode", please? Do you want to introduce a new view mode "page" or "full_page"? So you can still have "full" and use that without all the presumed logic? (Technically, I'd say that this would probably be the most reliable solution... note that #553298: Redesign the 'Manage Display' screen basically allows to have many view modes + derive settings for them from other view modes)

Isn't one important question whether there are other view modes that an additional $page argument could be combined with? Say, 'teaser' + $page?

I don't think that this would make sense. Hence, if $page only makes sense in combination with 'full', then 'full' + $page could just be a new view mode. Or not - if we just take over and re-purpose the 'full' view mode.

In the end, all of these additions should be configurable. Both $title and $comments already live in the node output, so any workaround other than making them configurable counts as special-casing. We have an issue that tries to tackle this somewhere, but I can't find it currently.

"view mode + separate $page bool" has always been a WTF, IMO.
If 'teaser + $page' has a meaning, it means that $page is a misnomer. It actually means "do some hardcoded bunch of additional stuff".
What $page means will never be granular enough : hide title ? display comments ? display comment form ?

Note that the latest approaches in #553298: Redesign the 'Manage Display' screen precisely aim at configuring what's visible or not per view mode, including non-fields : title, possibly comments (not done in the patch - could even be configurable : N paginated comments, comment count, comment form...)
See screenshots in #553298-83: Redesign the 'Manage Display' screen.

Yeah, I also vote for adding another view mode called "page". If we want, for D7, we can just hardcode comment module to display comments only in the "page" view mode, or if it's more or less easily possible comment.module could hook into the (redesigned) "Manage fields" form to make comment display configurable per view mode.

We still need "full" and "teaser" as they were for node listings. You might want to have a "teaser" listing as well as a "full" listing, but both can differ from the "page" view mode.

Having a "page" view mode is IMO the clearest and most consistent solution. node_page() would just do a node_view($node, 'page'), and no menu-system black magic involved at all.

Is it too late to break comment rendering out into its own block? That would allow some sanity to be restored to the view mode

I reviewed the code and it looks like a great cleanup. i will rtbc once bot is happy. Its currently very red on this patch.

The problem is that the Field API assumes all entities have a view mode named 'full'. Probably not a great design decision, but that's the best I could come up with for the challenges at hand (entity-type agnostic aspects, display settings to use for a view mode before the user actually visited Field UI to configure it...)

With that patch, this 'full' view mode for nodes is not used anywhere in core. If it was not the hardcoded default, we wouldn't even be defining it in core.

#553298: Redesign the 'Manage Display' screen has the promise of getting rid of this hardcoded 'full' mode, and using an actual 'default' mode to hold default display settings. Entity types would then be completely free of what actual view modes they use.

For now, can't we stick to the 'full' view mode for 'node page', and say that a node displayed in a different location is a different (non core) view mode ?

As for the patch in #16,
- the 'page' view mode, if it stays, needs to be declared in node_entity_info()
- field UI needs to know where to tie the display settings for the 'page' mode : field_ui_field_ui_view_modes_tabs() (an atrocious D6 remnant that #553298: Redesign the 'Manage Display' screen wishes to eradicate)

Updated a new (still in progress) patch in #553298: Redesign the 'Manage Display' screen
From #98 over there :

- Stops special-casing 'full' as a hardcoded view mode required on all entities

TODO
- Now that it's not special anymore, possibly rename the existing 'full' view mode to something more meaningful entity type per entity type (eg 'page' for nodes, 'taxonomy listing page' for taxo terms, 'profile page' for users ... ?) - should probably be done in a followup to avoid bikesheding.

#16: full-page-mode-for-nodes.patch queued for re-testing.

Re-rolled @Gábor Hojtsy's patch.

This rerolls it to apply to HEAD. Haven't tried addressing the errors yet, so probably still very broken.

Oops, wrong format on that patch.

This should fix the "Undefined index" when things try to get the 'page' display.

Actually, looks like #28 might fix the other failures as well.

Patch looks great, RTBC if the test bot is green.

My remarks in #19 still apply.

I'd not read yched's #19 for a while but yeah if we're not using 'full' for nodes anywhere else in core, which we're not, then I don't see any point adding a new 'page' mode.

We already have a node_is_page() function, hook_page_build(), and plenty of other stuff that refers to a "page" and therefore becomes manifest in a clear meaning of "page" throughout Drupal core/contrib modules, and very importantly, also themes.

If I understood above remarks correctly, then we should rename the current "full" view mode to "page" to clarify its purpose, and possibly leave "full" for contrib.

This patch removes node_is_page().

#553298: Redesign the 'Manage Display' screen does the needed job of downgrading 'full' from its current special status of "default" view mode, present on all entity types.
When that's done, then 'full' is just a regular view mode, that can, and indeed should, be renamed to what actually makes sense entity type per entity type ('page' for nodes and taxo terms, 'i_dont_really_know' for comments, etc).

'full' was awkward to begin with (my bad), because it started off as a derivative of the 'teaser / full node' dichotomy - while there's in fact no reason to assume that the node displayed on its own page is a "full" version (i.e displaying all fields). Even more inaccurate for all other entity types where we forced 'full' as the default (and often only) view mode.

Attempt at a reroll against HEAD, now that #553298: Redesign the 'Manage Display' screen is committed.

I think the goal here now is to rename the 'full' mode for nodes into 'page' - not to add a new 'page' mode.

yched: Yeah. I'm just getting this patch ported up, so that part is out of the way. This is still needs work.

Re-reading the thread after a while, and especially sun's #7, it seems the real issue is the hardcoding of the display of some node components : title, comments, comment form...

Does it mean that, now that #553298: Redesign the 'Manage Display' screen is in, we want to turn 'title', 'comments', 'comment_form' into real 'extra fields' (hook_field_extra_fields), that admins can hide / reorder view mode by view mode ?

Problem : currently on a default install, 'teaser' view mode is the only one that gets specialized display settings, all other modes use the 'default' set of display settings. This means that 'full' = 'rss' = 'search_index' ... = 'default', which is good since the view mode that you get presented when you visit the 'Manage display' UI (i.e 'default') is the one used for the main display (node on its own page).

Doing the above would mean that 'full' (possibly renamed 'page') needs to be also specialized, because it makes no sense that the 'default' mode hides the title and displays comments. In this case the 'default' mode, only deals with "less important" view modes (rss, search_index, contrib or custom modes...), and the important ones ('teaser', 'page') are configured in secondary subtabs...

Yes, I think it makes sense to turn those into real 'extra fields'.

The problem you describe is pretty suboptimal IMO. Can we enhance the view mode declaration so they have optional weights? Then we'd make 'page' a negative weight.

Could you elaborate:

Doing the above would mean that 'full' (possibly renamed 'page') needs to be also specialized, because it makes no sense that the 'default' mode hides the title and displays comments.

I read it about 10 times, but couldn't quite get what you mean.

Also what you describe as a problem, is only a problem, if we consider our secondary tabs to be not very visible (which I would say is true). That is a bug in itself, though, and not related to this issue IMO.

re #43 :
That was supposed to read "Doing the above would mean that 'full' *view mode* (possibly renamed 'page') needs ..."

#553298: Redesign the 'Manage Display' screen introduced the notion of 'default' display settings. They are what you configure on 'Manage Display' Those are used for all view modes, except the ones explicitly set to use a dedicated set of settings. Those modes are then configured on a separate page accessed by a dedicated secondary tab on the 'Manage display' page.
This reduces the amount of configuration you need to go through. This also reduces the uncertainty about what is displayed between the moment you enable a module that declares a new mode of its own (e.g 'print') and the moment you have the idea to visit the 'Manage display' page for this mode and actually configure it.

Currently, by default, only 'teaser' is configured to use dedicated settings, all the others use the defaults.
This means that 'full', 'rss', 'search_index', ... all display the same.

If we make the display of the node title and comment threads configurable this way (as opposed to currently hardcoded on cheesy criteria), then we need to say that in 'full' mode, title is hidden and 'comment thread' is visible, to match the current behavior. But this cannot be 'default' settings, because for instance we don't want comments displayed by default in 'rss' or on all newly enabled view modes.

So 'full' (or 'page' or whatever we rename it to) needs to use dedicated settings, and the 'default' settings, which are the most obvious in the UI, only involves less important view modes, while the two most important modes are hidden behind secondary tabs.

Ahhh, sorry I just couldn't grasp why on earth we would want to hide the title, but, yeah, I got in now...

I still think this should not speak against making title and comments real 'extra fields' but rather against the secondary tabs design in Seven.
I think if they are prominent and the user sees something like "Default | Page | Teaser" right away, then it's probably easily graspable how get where you want to go. Another idea would be to expand the "Custom display settings" fieldset by default.

The problem is on the function node_is_page().

My solution:
Mark view_mode to the object $node.
In the function node_is_page() add a condition view_mode == 'full'.

View my patch file for detail.

Retest it

Re upload file because last patch file cannot test?

New patch

Looks sane.

Hm, if we do it this way we have to be very careful to document the change - i.e., that 'full' is now a semi-reserved build mode that others shouldn't use lightly. For example, if Views in Drupal 7 will allow me to blindly put together a listing of 10 'full' nodes (like in Drupal 6), then we won't have progressed a whole lot from where the beginning of this issue started out.

I'm not sure if there's anywhere in core itself that needs this documentation or not - but definitely in the upgrade docs.

To the extent that this is an API change anyway, I wonder if it would make more sense to just go ahead and rename it to 'page' also (as was suggested earlier)? That's a much clearer name, and helps cement the fact that this is a lot different from 'full' in Drupal 6.

There's views, panels, there's also nodereference formatters that allow you to display referenced nodes as 'full' nodes.

there's also nodereference formatters that allow you to display referenced nodes as 'full' nodes

The other are blurry, but this example makes clear that we have to rename "full" to "page". Separate issue though.

+++ modules/node/node.module	3 Jun 2010 09:58:52 -0000
@@ -1256,6 +1256,8 @@ function node_view($node, $view_mode = '
+  // Add view_mode

Minor: Comments should end in a period. But could we maybe expand that out more? I can see below that that's what the code is doing. The question is why? Can you explain?

+++ modules/node/node.module	3 Jun 2010 09:58:52 -0000
@@ -1321,7 +1323,7 @@ function node_show($node, $message = FAL
+  return (!empty($page_node) && isset($page_node->view_mode) && $page_node->view_mode == 'full' ? $page_node->nid == $node->nid : FALSE);

Similarly, could we get a comment here explaining this? I actually wonder if the logic might be clearer if we stop trying to take the ternary operator beyond the point of ridiculouslness ;), and just do normal if/then stuff here.

60 critical left. Go review some!

Here is more readability code. You can modify the comment because my English skill is bad :(. Sorry about that.

My solutions is:

• On node view state, store a view_mode to the $node. Now the $node can know it view_mode
• On node_is_page check the view_mode. Because the page meaning the view_mode == full

Sorry about the mistake. Can you modify my comment for more meaning and add period to the end?

In drupal we don't like lines like this:

+  $is_full_view = $node->view_mode == 'full' ? TRUE : FALSE;

if then else would be better/more readable

Also in regards to #56 there is no need for ? TRUE : FALSE;

You can achieve the same with
$is_full_view = $node->view_mode == 'full';

And I don't think there is any reason to store this in the variable $is_full_view anyway

Thanks. And here is the merge path.

To reiterate the point in #57 about readability, perhaps a better approach would be

if (empty($page_node)) {
  return FALSE;
}
return $node->view_mode == 'full' && $page_node->nid == $node->nid;

Check it.

If we need to rename 'full' to 'page' or add page separately, then I'm not sure why we don't just do #40 here. If we did that it seems like we should actually be removing node_is_page() altogether though.

#61: node.patch queued for re-testing.

This patch solved the problem; all looks good.

#61: node.patch queued for re-testing.

Um. Can't we simply write foo and bar and baz.

Adding a $node->view_mode property sounds like a WTF. Other entities don't have it - it just happens that in core those other entities only have one view mode, but they will get more from contrib.

I agree with catch #62: cleaner to rename 'full' mode to 'page', and turn all if ($view_mode == 'full') { add title, comments, book nav, .. } into if ($view_mode == 'page') {.

That's missing an isset().

However, like catch and yched, I think that an approach like #40 would simplify things a lot more.

We discussed this with chx, and here is a little issue summary.

The root of the issue here is that each entity has a 'view mode' that defines the different ways it can be displayed. For nodes, we have an additional concept, which is the notion of 'I'm the main object displayed on my page'. This notion (currently node_is_page()) is what is used by the forum module to set the breadcrumb on 'node/123', by the book module to add navigation, etc.

The problem about this is two-fold:

• node_is_page() is dumb, and returns TRUE every time a node of nid = 123 is rendered on node/123
• The full view mode is close, but not exactly the same notion as node_is_page(), because you could want to display full nodes in a Views listing somewhere

We need to remove node_is_page(), which has long been identified as a bad idea. Here I agree with the approach in #40. From there we have two ways forward:

• Add a new 'page' view mode, replace node_is_page() by $view_mode == 'page', and do also implement the same for other entities that can be displayed in page mode (comments, users and taxonomy terms)
• Simply decide that, for nodes, the 'full' view mode gets all the decorations, and that you should create a new view mode if you want to display full nodes in listing

Agreed with the formulation in #69.
Option 1 ("Add a new 'page' view mode") leaves us with a useless 'full' view mode. We don't need a new view mode. In an earlier state, Field API required a 'full' view mode to exist for every entity type, but that's no longer the case.

So +1 on the second option ("Simply decide that, for nodes, the 'full' view mode gets all the decorations, and that you should create a new view mode if you want to display full nodes in listing"). But, doing so, rename 'full' to 'page'. Which is actually a mix of the formulations of options 1 and 2 :-p.

So here is a patch that does just that.

Do we need something for the upgrade path?

Looks good on a very quick glance, except for this:

   // displayed on its own page. Modules may alter this behavior (for example,
   // to restrict contextual links to certain view modes) by implementing
   // hook_node_view_alter().
-  if (!node_is_page($node)) {
+  if ($view_mode == 'page') {
     $build['#contextual_links']['node'] = array('node', array($node->nid));
   }

That logic needs to be reversed.

D'oh.

"Do we need something for the upgrade path?"

Not in core, but I'll write an upgrade func for HEAD2HEAD.

Actually, we also need to change the default view mode of various API functions in the node module.

At comment/reply/xxx we display the node on top of the comment form. When the default view mode is 'page', we end up with two comment forms on the same page. I recommend we default to 'teaser', which is generally what you want anyway.

Hm. This means that comment/reply/xxx is a case where we currently display the node 'as on its own page' but without the whole shazam. Having this page display the teaser version instead (in most cases, with a trimmed or shorter content) is a feature change...

This would also break node_unpublish_by_keyword_action(), btw.

Pondering...

Ouch. We really have a node_unpublish_by_keyword_action()? This looks broken by ways I cannot even count :)

Ok, so let's just add 'page' as a new view mode, and continue using 'full' in the other places.

We have pretty much arrived back to #40 that everyone seemingly agreed on but with some significant differences -- like laypeople can understand what the hell is going on by reading #69. This patch is a simple reroll of Damien's with a minute comment fix.

I'm sorry to be a pain, but having Field UI's 'Manage display' present users with a 'full' mode that they can't make any sense of doesn't seem great.

Lacking time today to reconsider, but I'm wondering if returning to
node_is_page($node) {return $menu_node = menu_get_object() && both nids match && $view_mode == 'full_renamed_to_page'}
would make sense - meaning, roughly #66, which I dismissed earlier :-/.

I'm thinking we'd need a proper summary of the current state of 'node page only' behaviors :
- what currently relies only on view mode == 'full'
- what currently relies on node_is_page()
- what are the exact contexts that currently display a node in 'full', and what
Cos' right now the situation is a litte bit confuse...

Looks like this needs more discussion based on #81

I tried to help here but its kinda hard to jump in given the uncertain state of #81. Perhaps yched can help us move forward here.

I must say that he simplicity of #66 is pretty attractive.

I dug some more, will try to summarize soon.

Boston D7 criticals code sprint is tomorrow. Any chance we can make a plan before then?

Sorry, post is a bit long...

So. Ideally, no behavior would be hard-coded on view modes. From the original issue in #409750: Overhaul and extend node build modes : "The ultimate goal IMO is to have 'modes' be just empty shells, with as little actual behavior hardcoded as possible (merely providing sensible defaults).[...] What actually happens in each mode is configurable".
I did not really expect we would get there in D7, though. The case of things tied to 'the node on its own page' is complex and tricky (and quite tied to drupal's history), and we still have a couple things hardcoded around the 'full' view mode.

This google spreadsheet lists the current behaviors we hardcode. Two flavors :

1. stuff depending on $view_mode = 'full' - anywhere the node is displayed in 'full' mode.
   In current core that is : node/%nid, comment/reply when commenting on the OP, unpublish_by_keyword() (sick concept in a world with CCK, beyond repair), + you'll get nodes displayed as 'embedded nodes' via noderef fields, Views set up to displaay a list of nodes in 'full' mode...
   --> Currently :
   • Book links ('add child page', 'prienter friendly version')
   • Book navigation
2. stuff depending on node_is_page() - apply when the node is displayed on its own page.
   --> Currently :
   • blog and forum breadcrumbs
   • comment thread + comment form
   • contextual links (everywhere *except* on the node's own page)
   • + the $page variable in node.tpl.php (and similarly in taxo-term.tpl.php...), hiding the node title if it's already displayed as the page title.

I tried to explain shortly in the doc why each behavior makes or doesn't make sense. IMO the only thing currently off is that Book navigation should fall into category 2), because we don't want it in noderef's embedded nodes.

So, bad news : both uses are legitimate. And we do need something like node_is_page() / taxonomy_term_is_page() (yes, terms use the same beloved mechanism :-p...) to tell us that we're displaying a node in context where it cannot be isolated from the rest of the page (breadcrumb, page title...).

Open questions :
a) is hook_node_view() still the place of choice for side effects like breadcrumb alteration in D7 ? wouldn't hook_page_alter() be more appropriate ?
b) $title hidden / displayed wrapped in h2 tags (or whatever the template sees fit), hardcoded in templates, and thus out of reach of Field UI's reordering / hiding, is a definite pain point from the past. It's been bugging me for more than 1 year now, but I have no better proposal, and now seems a bit late to change things there anyway.

+ one final remark :
c) As I mentioned in #41, we could remove a couple of those hardcoded stuff by exposing some of them (book navigation, comment listing + form) as 'extra fields', that can be turned on and off view mode by view mode. But :
- that would still not solve all cases - breadcrumb / title, see above.
- the amount of work is not trivial, namely :
let hook_field_extra_fields() specify display settings per view mode (API break)
specialize the 'full' view mode for nodes (currently 'full' mode uses 'default' display settings, like 'rss' or 'search')
add weights to view modes to determine the order of secondary tabs on 'Manage display' screens
add 'full' display settings for all fields created by a default display.
This won't happen within D7.

Unless we fix a) b) and c), node_page_title() is here to stay.

Based on this, here's my proposal :
- keep node_is_page($node) as it currently is : $node->nid == $menu_node->nid
- everything that currently uses if (node_is_page($node)) (list above) needs to use if ($view_mode == 'full' && node_is_page($node)).
- tweak : add book navigation if ($view_mode == 'full' && node_is_page($node)), instead of currently if ($view_mode == 'full')
- contrary to what I've advocated earlier, keep 'full' mode as 'full', do not rename to 'page'. Because as explained above, $view_mode and node_is_page() are orthogonal. There are cases where $view_mode == 'full' && node_is_page($node) == FALSE, and the same statement if the view mode is named 'page' is a definite WTF.

Should translate into a patch much shorter than this post :-/

For d7, I agree with 86 tail makes the most sense. For d8, I reallly really want C -- I'm quite sad finding out that I cannot configure title per build mode, as I've come up with several workarounds for this.

However, hard coded full means that modules cannot switch view mode of the current node (for example, if module did want a page view mode seperate from full. Display suite's node display does this in d6. Since $page was being used, build mode could be switched.), but shrug.

(Just thinking, how will having an hard coded stuff on being node page /full display affect stuff like views display comments settings for node views?)

Patch attach does what yched suggests in tail.

Of course, you still can't display a node in view_mode full on node pages outside of the node page >.O

Missed one + the term page

matches yched spec.

I agree that this makes sense and that it does what yched wants it to do. Committed to CVS HEAD. One critical bug less. Great job.

Gee, kudos for the fast reaction, folks !

Here's a small fix for two things that possibly got mixed up backwards in the committed patch from #88 :

- committed patch turned 'book links' ('add new page' / 'view printer-friendly version') from "whenever node displayed in 'full' mode" to "only for the node on its own page". Attached patch reverts that.
- reservedly, attached patch moves 'book navigation' to only display on the node's own page.

both points as per my proposal in #86.

and the patch.

Of course, you still can't display a node in view_mode full on node pages outside of the node page >.O

Yup. Just ran into that with Views, in fact :(

Still not clear what the solution to that problem should be. I created an issue in the Views quite to discuss it, though (#1077632: Views blocks that use the "full node" build mode display incorrectly on the node page) as that seems to be one of the main places where this problem will come up - and at the very least, something Views will likely have to find a way to work around for Drupal 7.