Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#10 | 2528314-10.patch | 4.28 KB | Wim Leers |
Comments
Comment #1
Wim LeersI wonder if we need that at all?
I'd say
max-age = 0
.Comment #2
Wim LeersI thought it was only used in esoteric cases, but that's apparently not the case:
(In
node.html.twig
.)Then we need to support
$variables['#cache']['contexts']
, just like we already support$variables['#attached']
.Comment #3
Berdirin_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).
Comment #4
Wim Leers#2351015: URL generation does not bubble cache contexts added that yesterday!
Indeed.
That should do the trick, but it doesn't. Because
NodeViewController
specifiesfull
instead ofdefault
, and Views only offersdefault
, notfull
.That seems like a bug, impeding cacheability?
+100. Can we still do that?
So, in conclusion, two things:
\Drupal\Core\Entity\EntityManager::getDisplayModeOptions()
, every entity has adefault
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 specifyingfull
ordefault
? Apparently we have afull
view mode, but itsstatus
isfalse
. What does that even mean? And why doesn't/admin/structure/types/manage/article/display
show thefull
view mode? Actually, if thefull
view mode is disabled, then why canNodeViewController
still use that view mode anyway? And where is thedefault
view mode defined then? AFAICT it's just mapped to thefull
view mode?This is one huge clusterfuck that we should untangle before release. It's impossible to understand.
page
template variable for nodes?Comment #5
catch#2 I think we could, but not in a minor release - so it is now or never.
Comment #6
dawehnerFeels really similar to #2529344: Twig handles bubbleable metadata when rendering preprocessed variables, but not with if statements
Comment #7
catchComment #8
swentel CreditAttribution: swentel commentedMaking 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.
Comment #9
xjmReading the comments, I think @catch meant this based on #5.
Comment #10
Wim LeersHere'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.Comment #13
BerdirHm. 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:
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 :(
Comment #14
yched CreditAttribution: yched commented'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 ?
Comment #15
yched CreditAttribution: yched commentedRe @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...
Comment #16
BerdirYeah, 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.
Comment #17
yched CreditAttribution: yched commentedRight, EM::getDisplayModeOptions() force-adds 'default'. I might have let that slip through in #2322503: getDisplayModeOptions() returns only full or teaser regardless of the status of the entity display. Unless that was #2154711: Move entity_get_(form/view)_mode_options() functions to EntityManager and add get(Form/View)ModeOptions() methods.
Comment #18
yched CreditAttribution: yched commentedWell, "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.
Comment #19
yched CreditAttribution: yched commented#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 :-/)
Comment #20
yched CreditAttribution: yched commentedSo, #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 :
The current patch here basically changes it to just
$variables['view_mode'] == 'full'
: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 ?
Comment #21
yched CreditAttribution: yched commentedI 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 ?
Comment #22
BerdirI 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.
Comment #23
yched CreditAttribution: yched commentedYeah, 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 ?
Comment #24
yched CreditAttribution: yched commentedSo 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)" ?
Comment #25
Wim LeersUgh, 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?
Comment #27
paranojik CreditAttribution: paranojik as a volunteer commentedIn 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)
Comment #29
Wim LeersI don't think this qualifies as a major bug. But it's a bug for sure.
Comment #40
catchComment #43
catch