Problem/Motivation

  // The 'page' variable is set to TRUE in two occasions:
  //   - The view mode is 'full' and we are on the 'node.view' route.
  //   - The node is in preview and view mode is either 'full' or 'default'.
  $variables ['page'] = ($variables ['view_mode'] == 'full' && (node_is_page($node)) || (isset($node->in_preview) && in_array($node->preview_view_mode, array('full', 'default'))));

node_is_page() looks like this:
https://api.drupal.org/api/drupal/core%21modules%21node%21node.module/fu...

So it should add the route context.

And also $node->in_preview - this probably needs a cache context too.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

I wonder if we need that at all?

And also $node->in_preview - this probably needs a cache context too.

I'd say max-age = 0.

Wim Leers’s picture

I wonder if we need that at all?

I thought it was only used in esoteric cases, but that's apparently not the case:

  {% if not page %}
    <h2{{ title_attributes }}>
      <a href="{{ url }}" rel="bookmark">{{ label }}</a>
    </h2>
  {% endif %}

(In node.html.twig.)

Then we need to support $variables['#cache']['contexts'], just like we already support $variables['#attached'].

Berdir’s picture

in_preview already opts out of caching completely in NodeViewBuilder.

To reproduce this, it should be possible to switch the frontpage view to use the default/full view mode. then node/1 and the same node on the frontpage would be cached together but one is expected to show the title and the other one not.

(IMHO that is page stuff should have been removed a long time ago and title become a normally configurable field that's not shown on default and shown on teaser. Would simplify a lot and then this behavior would simply be by design).

Wim Leers’s picture

Then we need to support $variables['#cache']['contexts'], just like we already support $variables['#attached'].

#2351015: URL generation does not bubble cache contexts added that yesterday!

in_preview already opts out of caching completely in NodeViewBuilder.

Indeed.

To reproduce this, it should be possible to switch the frontpage view to use the default/full view mode. then node/1 and the same node on the frontpage would be cached together but one is expected to show the title and the other one not.

That should do the trick, but it doesn't. Because NodeViewController specifies full instead of default, and Views only offers default, not full.
That seems like a bug, impeding cacheability?

(IMHO that is page stuff should have been removed a long time ago and title become a normally configurable field that's not shown on default and shown on teaser. Would simplify a lot and then this behavior would simply be by design).

+100. Can we still do that?


So, in conclusion, two things:

  1. The craziness of "full" vs "default" for nodes: it has never made any sense to me whatsoever, but now it seems to actually get in the way. According to \Drupal\Core\Entity\EntityManager::getDisplayModeOptions(), every entity has a default view mode. Fine. But, when used, that should be mapped to an actual view mode, right? So that the CID is the same, regardless of specifying full or default? Apparently we have a full view mode, but its status is false. What does that even mean? And why doesn't /admin/structure/types/manage/article/display show the full view mode? Actually, if the full view mode is disabled, then why can NodeViewController still use that view mode anyway? And where is the default view mode defined then? AFAICT it's just mapped to the full view mode?
    This is one huge clusterfuck that we should untangle before release. It's impossible to understand.
  2. Can we still remove the page template variable for nodes?
catch’s picture

#2 I think we could, but not in a minor release - so it is now or never.

catch’s picture

Issue tags: +rc target
swentel’s picture

Making the title configurable on Field UI would indeed we cool and not that hard. Adding setDisplayConfigurable('view', TRUE) exposes it already, but I guess we need to either add an option in StringFormatter to select the wrapper (say, h1, h2, h3 etc) or add another formatter.

The default view mode is there to make sure that in case some module renders the node with a view mode which is not enabled, you at least see something (unless you have disabled all fields on the default view mode, which is possible of course). So it's some acting as a fallback. I guess yched has the whole history behind this since this option is there since like the first version of CCK back in drupal 4/5.

xjm’s picture

Issue tags: -rc target +rc deadline

Reading the comments, I think @catch meant this based on #5.

Wim Leers’s picture

Status: Active » Needs review
FileSize
4.28 KB

Here's a very simple approach to doing #4.2 per #5, but without making the title field configurable, which I'm pretty sure would lead to a very big issue.

The proposal is simple: the full view mode is the one that is only intended to be used on the canonical entity route, i.e. on the "full node page". We've always called it the "full node page" while talking about it, so it makes sense to just associate that view mode with it. No special checks necessary then.

Status: Needs review » Needs work

The last submitted patch, 10: 2528314-10.patch, failed testing.

The last submitted patch, 10: 2528314-10.patch, failed testing.

Berdir’s picture

Hm. That is an API change, and without the ability to configure the title, it's a somewhat problematic one.

We have a hook right now that allows you to switch out the view mode based on something. I'm using that in my project for a paywall and I've used it in the past in https://www.drupal.org/project/userpoints_nodeaccess, which is basically the same. A different view mode allows me to easily configure different field displays and hide most of them, but it's still the full page and some parts are relying on it and the node_is_page() function. book.module and statistics.module are still calling it, for example.

On the other side:

So it should add the route context.

That would be pretty bad. If we for example show a list of node teasers in a block, then we have to cache them differently on every single page. Another idea would be a variant of what @moshe proposed in the comment links issue. Have a cache context for node.is_page:5 or so. But again, if you display 100 nodes on a page, you end up with 100 cache contexts :(

yched’s picture

'page' and figuring whether the title should be displayed in the node.tpl has long been a bit painful.

I vaguely remember some hairy issue late in the D7 cycle about that, that had to undo some oversimplification that was made earlier. Off-hand I don't remember what that oversimplification was though, wondering if that was "display title everywhere except on full mode" :-/

Sorry for not being more specific right now. IIRC it was Gabor who pointed the issue back then, maybe he remembers best ?

yched’s picture

Re @wim #4, 'default' :
There is actually no 'default' view mode, there is a 'default' EntityDisplay, used when no display has been configured for a view mode. This allows entity types to provide many "display flavors", for each "context" where the entity gets displayed (rss, search result, etc...), without forcing site admins to explicitly configure each of them on each bundle (which would be daunting)

The original intent was that your're not really supposed to render an entity in the 'default' view mode, you render it in an actual view mode, and depending on config it might end up using the display for that view mode, or the the 'default' display.

It's possible things have shifted a bit from this original design, and we let users select 'default' in places where they can pick a view mode. Not sure that's really a good thing...

Berdir’s picture

Yeah, I was wondering too where that default is coming from in views :)

Looks like we somehow merged those concepts together... it calls getViewModeOptions() on entity manager, but that actually calls getDisplayModeOptions() internally. So what you actually get is a list of all enabled display modes on an entity type not view modes. Not views fault then :)

Still, you can use that to reproduce this, you just need to tick the checkbox for full on manage display so that you can select full.

yched’s picture

yched’s picture

Looks like we somehow merged those concepts together. So what you actually get is a list of all enabled display modes on an entity type not view modes

Well, "display modes" is the generic term for "view modes + form modes".
So getDisplayModeOptions() is still about *modes* (i.e. "available flavors for rendering"), not about Displays (i.e. configured settings for fields of a given bundle in a given mode)

Still, 'default' shouldn't be in there.

yched’s picture

#721754: A node cannot be displayed in different view mode on its own page is the D7 issue I was talking about (sorry, just dug the issue # from git history, but have to run right now, couldn't actually go through it :-/)

yched’s picture

So, #721754: A node cannot be displayed in different view mode on its own page was the D7 issue that adjusted the visibility of a couple elements
- node title within node tpl,
- some node links (book links...)
- breadcrumbs (e.g on forums...)
- contextual links
based on various combinations of node_is_page() and $view_mode == 'full'
(see https://www.drupal.org/files/issues/721754_drupal_node_is_page_full_miss... - yay, CVS patch !)

The patch applied various logic to the various elements listed above, for node title specifically it did :

@@ -1355,7 +1355,7 @@ function template_preprocess_node(&$vari
-  $variables['page']      = node_is_page($node);
+  $variables['page']      = $variables['view_mode'] == 'full' && node_is_page($node);

The current patch here basically changes it to just $variables['view_mode'] == 'full' :

@@ -588,10 +588,6 @@ function template_preprocess_node(&$variables) {
-  $variables['page'] = ($variables['view_mode'] == 'full' && (node_is_page($node)) || (isset($node->in_preview) && in_array($node->preview_view_mode, array('full', 'default'))));
 
@@ -70,7 +69,7 @@
-  {% if not page %}
+  {% if not view_mode == 'full' %}
       [... display the title ...]

No real opinion on the change atm, just bringing some historical perspective :-)

Also :
- what we do on the node side, will probably need to be reflected on the taxo_term side, because template_preprocess_taxonomy_term() / taxonomy_term_is_page() / taxonomy-term.html.twig currently use the exact same logic to display the term name.
- more generally, any entity type whose entities have a dedicated "view as main content" URL, have to deal with the same question ("should the entity tpl display the label, or is it already displayed as the page title ?") - and will thus potentially face the same issue about cacheability and cache poisoning ?

yched’s picture

I think I'm +1 to the approach ('full' is hardcoded to not display the title in the node template), and we should do it for taxo_term as well.

There is no generic entity.tpl, and also no universally-defined "full" view mode across entity types that means "used on the entity's page", so it remains up to each entity type to implement this behavior in their own [my_entity_type].tpl and preprocessor according to their own semantics of view modes.

Maybe we should check the other content entities in core ?
- do they have a "view entity on own page" URL ?
- do their template/preprocessor implement a cache-safe logic for the "show label or not" question ?

Berdir’s picture

I think all other core examples are way worse and have no concept for this at all :)

For example, there's a long-standing issue that it's not possible to view a rendered user that actually contains the user name, without messing with the user template.

yched’s picture

Yeah, I kind of feared that would be the case :-)

So maybe for this issue here, let's just take care of nodes and taxo_terms, which currently work the same way ?

yched’s picture

I think I'm +1 to the approach ('full' hardcoded to not display the title in the node template)

So yeah, except for the use case @Berdir described in #13 : use of hook_entity_view_mode_alter() to dynamically swap the view_mode based on some runtime condition (e.g on the entity).

So the $view_builder->view($node, 'full') that is done on node/{nid} can end up being rendered using a 'full_alt' view mode, and the tpl would then show the node title, duplicating the page title.

@Berdir : but isn't that also the case in HEAD, since node.tpl currently does "hide the title if (node_is_page() && 'full' mode)" ?

Wim Leers’s picture

Ugh, we totally lost track of this issue :( Then again, not sure we could've done this during RC anyway.

Suggestions on how to move forward?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

paranojik’s picture

Version: 8.1.x-dev » 8.2.x-dev
Issue tags: -rc deadline

In my opinion #3 explains what this issue should focus on. That is: doing stuff in twig templates based on conditions not included in the cache context. The whole debate about view_mode/is_page and how to display the title is a different issue.
If I understand this correctly then (in the node case) the (view_mode == "full" && $page == TRUE) and (view_mode == "full" && $page == FALSE) are two different "displays" and should each be cached separately.

(found this while working on #2712111: Embed specific attributes are cached along with the entity with @slashrsm)

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Priority: Major » Normal

I don't think this qualifies as a major bug. But it's a bug for sure.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.