Updated: Comment #0

Problem/Motivation

The problem is identical to the one described and solved at #2090783: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user: op links on nodes may be personalized in some way: node access-based, time-dependent, etc. Such links would break the render cache.

Proposed resolution

Use the same solution as the one at #2090783: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user: leverage #post_render_cache.

A side benefit: statistics.module won't break the render cache anymore, so you can have that module enabled and still benefit from the render cache!

Remaining tasks

None.

User interface changes

None.

API changes

Node links should be altered not through hook_node_view() or hook_node_view_alter(), but through hook_node_links_alter(), which is called from a #post_render_cache callback and therefore can add personalized links without breaking the render cache.

CommentFileSizeAuthor
#7 Screenshot 2013-12-10 20.41.53.png18.51 KBlarowlan
#4 interdiff.txt4.57 KBwim leers
#4 node_ops_links-2151439-4.patch12.7 KBwim leers
#1 node_ops_links-2151439-1.patch11.51 KBwim leers

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new node_ops_links-2151439-1.patch11.51 KB

Et voila!

Status: Needs review » Needs work

The last submitted patch, 1: node_ops_links-2151439-1.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +beta blocker

Beta blocker as a blocker for #2151459: Enable node render caching.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new node_ops_links-2151439-4.patch12.7 KB
new interdiff.txt4.57 KB

This reroll makes all tests pass.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented 
  1. +++ b/core/modules/node/lib/Drupal/node/NodeViewBuilder.php
@@ -35,33 +35,14 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
+        '#context' => array(

    Any reason why this doesn't just include the node and save the second entity_load in the callback? 

  2. +++ b/core/modules/statistics/statistics.module
@@ -73,17 +79,6 @@ function statistics_node_view(EntityInterface $node, EntityDisplay $display, $vi
-    unset($build['#cache']);

    Nice!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
  1. Because then the entire node would be serialized to the render cache. That's very bad.
  2. :)
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Screenshot 2013-12-10 20.41.53.png18.51 KB

Then I think this is ready

Manually tested on simplytest.me with book module.
Screenshot:
pony sticks and monkey bricks

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

This looks good so I committed it to 8.x. Curious how much impact this will have. We'll have to do extensive profiling later once all the pieces fall into place.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -sprint

I, too, am very curious how much impact node render caching will have! We've been working towards it for months now, step by step, so I sure hope it will pay off!

Change notice at https://drupal.org/node/2152957 updated to also explain hook_node_links_alter().

Status: Fixed » Closed (fixed)

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