Opening this as a follow-up to #636454: Cache tag support and #730060: Replace CACHE_TEMPORARY, cache_clear_all() and minimum cache lifetime with cache tags support, especially the first issue has a lot of background discussion that is worth going through on how this might be applied.

We need to do the following:

- apply cache tags to rendered entities.

- collect those cache tags on rendering (will likely be similar to the #attached support in drupal_render())

- clear those tags on entity updates.

- additional review all uses of cache_invalidate() that are currently in core to determine whether they're appropriate/necessary.

- probably as a follow-up to this issue, determine how to apply cache tags to lists of content - i.e. where changes to content will affect what shows up in the list. This is a lot more complicated to figure out, and doesn't affect the initial implementation of tagging individual content pieces and aggregating them.

Putting this against base system since this is going to be part drupal_render(), part entity system most likely, and it probably won't affect cache backends at all.

CommentFileSizeAuthor
#202 1605290-202.patch1.89 KBAnonymous (not verified)
#200 1605290-200.patch1.82 KBAnonymous (not verified)
#194 1605290-194.patch56.48 KBamateescu
#194 interdiff.txt582 bytesamateescu
#192 1605290-192.patch56.37 KBamateescu
#192 interdiff.txt1.75 KBamateescu
#188 1605290-188.patch55.68 KBAnonymous (not verified)
#188 interdiff.txt1.93 KBAnonymous (not verified)
#178 interdiff.txt1.43 KBamateescu
#177 1605290-177.patch55.23 KBamateescu
#170 1605290-170.patch54.35 KBamateescu
#170 interdiff.txt1.71 KBamateescu
#167 1605290-167.patch54.76 KBamateescu
#167 interdiff.txt688 bytesamateescu
#164 1605290-164.patch54.64 KBamateescu
#164 interdiff.txt3.34 KBamateescu
#163 1605290-163.patch54.95 KBamateescu
#163 interdiff.txt1.7 KBamateescu
#160 1605290-160.patch56.35 KBamateescu
#160 interdiff.txt4.12 KBamateescu
#158 1605290-158.patch54.28 KBamateescu
#158 interdiff-149-158.txt4.2 KBamateescu
#158 interdiff.tests_.txt13.18 KBamateescu
#149 1605290-149.patch40.31 KBmsonnabaum
#149 interdiff.txt4.8 KBmsonnabaum
#143 1605290-143.patch40.31 KBWim Leers
#143 interdiff.txt925 bytesWim Leers
#136 1605290-136.patch40.25 KBamateescu
#135 1605290-135.patch40.47 KBamateescu
#135 interdiff.txt4.94 KBamateescu
#132 1605290-132.patch45.41 KBamateescu
#132 interdiff.txt1.03 KBamateescu
#130 1605290-130.patch45.48 KBamateescu
#130 interdiff.txt649 bytesamateescu
#128 1605290-128.patch45.36 KBAnonymous (not verified)
#128 interdiff.txt1.93 KBAnonymous (not verified)
#126 1605290-126.patch44.04 KBamateescu
#126 interdiff.txt1.09 KBamateescu
#122 1605290-122.patch46.82 KBamateescu
#122 interdiff.txt1.98 KBamateescu
#120 1605290-120.patch42.1 KBamateescu
#120 interdiff.txt9.86 KBamateescu
#119 1605290-119.patch9.86 KBamateescu
#116 1605290-116.patch34.3 KBamateescu
#116 interdiff.txt12.87 KBamateescu
#111 1605290-111.patch34.09 KBmsonnabaum
#108 1605290-108.patch34.09 KBmsonnabaum
#100 1605290-100.patch34.14 KBmsonnabaum
#100 interdiff.txt5.35 KBmsonnabaum
#96 interdiff_1.txt29.56 KBamateescu
#96 interdiff_2.txt5.45 KBamateescu
#96 1605290-95.patch31.7 KBamateescu
#92 1605290-92.patch59.27 KBdamiankloip
#92 interdiff-1605290-92.txt893 bytesdamiankloip
#90 1605290-entity_render_cache-89.patch58.39 KBmsonnabaum
#90 interdiff.txt987 bytesmsonnabaum
#84 1605290-84.patch58.33 KBdamiankloip
#79 1605290-entity_render_cache-78.patch53.67 KBmsonnabaum
#77 1605290-entity_render_cache-77.patch53.66 KBmsonnabaum
#75 1605290-entity_render_cache-75.patch52.19 KBmsonnabaum
#67 interdiff.txt10.32 KBCaseledde
#66 1605290-entity_render_cache-66.patch50.79 KBCaseledde
#51 1605290-entity_render_cache-51.patch51.63 KBamateescu
#49 1605290-entity_render_cache-49.patch52.86 KBamateescu
#49 interdiff.txt4.72 KBamateescu
#46 interdiff.txt20.5 KBamateescu
#45 1605290-entity_render_cache-45.patch49.83 KBamateescu
#42 1605290-entity_render_cache-42.patch41.36 KBBerdir
#34 1605290-entity_render_cache-34.patch41.47 KBamateescu
#34 interdiff.txt7.69 KBamateescu
#32 1605290-entity_render_cache-32.patch38.28 KBamateescu
#32 interdiff.txt10.9 KBamateescu
#29 1605290-entity_render_cache-29.patch28.29 KBamateescu
#29 interdiff.txt16.95 KBamateescu
#25 1605290-entity_render_cache-25.patch14.13 KBamateescu
#25 interdiff.txt6.98 KBamateescu
#21 lazy-cache-clearing-poc.txt2.41 KBamateescu
#20 1605290-entity_render_cache-20.patch14.27 KBamateescu
#20 interdiff.txt3.72 KBamateescu
#17 1605290-entity_render_cache-17.patch13.72 KBamateescu
#17 interdiff.txt1.68 KBamateescu
#13 1605290-entity_render_cache-13.patch13.79 KBamateescu
#11 interdiff.txt11.78 KBamateescu
#11 1605290-entity_render_cache-11.patch13.79 KBamateescu
#5 1605290-entity_render_cache.patch9.45 KBamateescu
#1 cache_tags_drupal_render.patch2.42 KBcatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Status: Active » Needs work
FileSize
2.42 KB

First, untested, draft for drupal_render() support.

Anonymous’s picture

Issue tags: +D8 cache tags

ah, nice. perhaps we should use a tag to keep track of follow up cache tags issues? i've added the tag 'D8 cache tags', feel free to change it.

moshe weitzman’s picture

Assigned: Unassigned » moshe weitzman

I'll try to move this along since Views is working on adding cache tags to its cache plugins. #1712456: How to leverage cache tags in Views

catch’s picture

Moshe - any update on this?

amateescu’s picture

Here's a start on the entity side of things :) Not sure if I'll be able to get back to it before next week so I'm not assigning the issue to me just yet.

catch’s picture

Status: Needs work » Needs review
+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.phpundefined
@@ -156,10 +175,38 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+      // Cache the rendered output if permitted by the view mode settings.
+      // @todo Should we cache the 'default' view mode by default? Modules won't
+      // really have a way to alter this because 'default' is not defined in
+      // hook_entity_view_mode_info().
+      if ($view_mode == 'default' || $this->viewModesInfo[$view_mode]['cache']) {
+        $build[$key]['#cache'] = array(
+          'keys' => array('entity_view', $this->entityType ,$entity->id(), $view_mode),
+          'granularity' => DRUPAL_CACHE_PER_ROLE,
+          'bin' => $this->cacheBin,
+          'tags' => array(
+            $this->entityType . '_view' => TRUE,
+            $this->entityType => array($entity->id()),
+          ),
+        );

We only need the entity ID tag here afaik. Not sure there's a use case for entity type.

Can't find much else to complain about though.

catch’s picture

Talked to amateescu, there's a decent case for having the entity type as a tag - for example updating display settings you'd be able to clear the cache just for that entity type. Also just one tag per entity type is a relatively small number to be fetching each request.

amateescu also mentioned that we need to account for child elemends such as field items adding cache tags for parents, without being cached themselves - say a reference field etc. It looks like #cache['tags'] should support this fine - drupal_render() already allows you to have #cache set but no cid or cid parts.

amateescu’s picture

Assigned: moshe weitzman » amateescu

My friday just cleared up, so I'll work on merging #1 and #5 + what was discussed above sooner than expected.

Status: Needs review » Needs work

The last submitted patch, 1605290-entity_render_cache.patch, failed testing.

catch’s picture

Title: Apply specific cache tags to drupal_render()/page/block caching and clear them » Enable entity render caching with cache tag support
amateescu’s picture

Status: Needs work » Needs review
FileSize
13.79 KB
11.78 KB

Here's some more progress on this.

The most important change is that I figured out we also need to clear the render cache when creating new entities, not only on update. I can easily find at least two scenarios where we need this:
- when a new comment is posted, we want the node page associated with it to be updated
- when we create a new node and tag it with a taxonomy term, we want the taxonomy/term/ page to be updated

The patch attached should be pretty much feature-complete, but probably will need to be postponed on all the entity ng conversions..

Status: Needs review » Needs work

The last submitted patch, 1605290-entity_render_cache-11.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
13.79 KB

I hope that's not a legitimate install failure, so let's try a re-roll on latest HEAD first.

Status: Needs review » Needs work

The last submitted patch, 1605290-entity_render_cache-13.patch, failed testing.

catch’s picture

@@ -529,6 +544,9 @@ public function save(EntityInterface $entity) {
           $this->saveRevision($entity);
         }
         $this->resetCache(array($entity->id()));
+        // Reset the render cache as well.
+        $this->resetRenderCache(array($entity));

This should invalidate the cache tags, then it'll invalidate any cached item that's been tagged correctly (i.e. views as well).

I can't find the comment invalidation in the patch, but it'd make sense to me for comment_save() to invalidate the cache tag for the node as well as for the comment.

amateescu’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.php
@@ -156,10 +175,54 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+      $tags = array();
+      foreach ($entities as $entity) {
+        $tags[$this->entityType][] = $entity->id();
+
+        // @todo Remove when all entities are converted to EntityNG.
+        if (!$entity->getPropertyDefinitions()) {
+          continue;
+        }
+
+        // Add all the referenced entity types and IDs to the tags that will be
+        // cleared.
+        foreach ($entity->getPropertyDefinitions() as $name => $definition) {
+          if ($definition['type'] == 'entity_reference_field') {
+            foreach ($entity->$name->getValue() as $target_id) {
+              $tags[$definition['settings']['target_type']][$target_id] = $target_id;
+            }
+          }
+        }
+      }

This is the part that invalidates all references.

Now that I look at it again, I guess it should only do this for new entities, because when we update an entity, the relevant caches should already be tagged with its id.

I'll do that in the next re-roll.

EDIT: Oh, and we also need a global switch for this. The current way of cache clearing is fine if you insert/update one entity, but it's really bad for bulk operations (e.g. Migrate, VBO).

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.68 KB
13.72 KB

Silly me :/ This should get us past the installer again.

catch’s picture

Status: Needs review » Needs work

I'd forgotten comments reference nodes now :)

For clearing - in the implementation we might want to keep all the cleared tags during the request then clear them all at once.

Another option would be to do all the tag clearing only in the form submission callbacks (as we do for cache_clear_all()) - if you call $entity->save() from somewhere else you'd be responsible for clearing the tags too, or not.

moshe weitzman’s picture

Another option would be to do all the tag clearing only in the form submission callbacks (as we do for cache_clear_all()) - if you call $entity->save() from somewhere else you'd be responsible for clearing the tags too, or not.

Yes please. We can't have cache clears firing all over the place for data migrations and so on.

amateescu’s picture

Status: Needs work » Needs review
FileSize
3.72 KB
14.27 KB

We can't have cache clears firing all over the place for data migrations and so on.

That's exactly what I said a few lines above :)

Now that we got into the fun territory of cache clearing, I have to say that I would prefer option 1) from #18 because, let's be honest, bulk operations and data migrations are the minority use case here, there are far more individual $entity->save() calls and I'd prefer to offer a better DX for this case.

How about something along the lines of _menu_clear_page_cache()?

function _entity_clear_render_cache($force = FALSE) {
  if ($force) {
    cache_invalidate_tags($collected_entity_render_cache_tags);
  }
  else {
    drupal_register_shutdown_function('cache_invalidate_tags', $collected_entity_render_cache_tags);
  }
}

In the meantime, let's see how many fails we have left after I fixed some broken code in resetCache().

amateescu’s picture

FileSize
2.41 KB

Actually, forget the pseudo-code from #20, this is what I mean.

catch’s picture

For end of request clearing we should do that in the storage controller (or if we split cache tag handling out of the direct storage controllers maybe in a base class for that), for me that makes sense for all tags not just entity ones.

Even if we do this though I'd still go for doing the tag clearing in the submit handler - keeping the tags around in memory is going to mean a massive query at the end of migration, more memory etc.

Status: Needs review » Needs work

The last submitted patch, 1605290-entity_render_cache-20.patch, failed testing.

xjm’s picture

Issue tags: +D8 cacheability
amateescu’s picture

Status: Needs work » Needs review
FileSize
6.98 KB
14.13 KB

Removed auto cache clearing from the storage controllers and fixed a couple of other stuff. Let's see how we stand with tests..

amateescu’s picture

One thing that worries me about not doing any automatic cache clearing is that we have to touch every single test that deals with entity view output. How about introducing a system.performance config variable for enabling entity render caching?

amateescu’s picture

Double post.

Status: Needs review » Needs work

The last submitted patch, 1605290-entity_render_cache-25.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
16.95 KB
28.29 KB

Maybe this won't be as bad as I thought.

Status: Needs review » Needs work

The last submitted patch, 1605290-entity_render_cache-29.patch, failed testing.

tstoeckler’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityFormController.php
@@ -203,7 +203,14 @@ public function submit(array $form, array &$form_state) {
+    $entity = $this->getEntity($form_state);
+    try {
+      \Drupal::service('plugin.manager.entity')->getRenderController($entity->entityType())->resetCache(array($entity));
+    }
+    catch (\Exception $e) {
+      // Nothing to do if the entity type doesn't have a render controller.

I think throwing (and catching) an exception whenever saving a non-renderable entity is a bit costly. I think we should go the extra mile here of \Drupal::service('plugin.manager.entity')->getDefinition($entity->entityType()); and check whether that has a 'render controller' key. (Alternatively we could add a $nothrow option or something getRenderController() (or, actually, getControllerClass()))

Other than that, looks pretty sweet!

amateescu’s picture

Status: Needs work » Needs review
FileSize
10.9 KB
38.28 KB

I'll be away for a week so I'm posting my progress so far. One major pain point remaining is.. of course, comments :) More specifically: comments are cached individually, as they should, but they're also cached in the node's render array ($node->comments).

A solution for that could be to allow #attached callbacks to alter their $elements array. Then we could simply change comment_node_view() to do something like this instead:

if ($node->comment && $view_mode == 'full' && node_is_page($node) && empty($node->in_preview)) {
      $node->content['#attached']['comment_node_page_additions']['node'] = $node;
    }

Re #31: From those two options, I would prefer the latter; introduce an extra argument to get*Controller() and getControllerClass() similar(/identical) to ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE, ContainerInterface::NULL_ON_INVALID_REFERENCE and ContainerInterface::IGNORE_ON_INVALID_REFERENCE.

Status: Needs review » Needs work

The last submitted patch, 1605290-entity_render_cache-32.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
7.69 KB
41.47 KB

Merged HEAD and fixed a few more tests..

Status: Needs review » Needs work

The last submitted patch, 1605290-entity_render_cache-34.patch, failed testing.

catch’s picture

Priority: Normal » Major

This is at least major.

Wim Leers’s picture

Agreed. Considering that ESI is a crucial part of getting D8 to perform well, I'd say it's even critical (assuming render cache and high render cache hit ratio, to achieve low amortized CPU time per served page).

Anonymous’s picture

"Considering that ESI is a crucial part of getting D8 to perform well"

if ESI is a crucial part of getting D8 to perform well, then are we saying that the 99%* of sites that won't use ESI are just toast?

please, no, can we stop this right here? IMO, we can't ship D8 if the only way to make it perform well is to use ESI. or we can ship it, but wow, that is an admission of some very serious design fail with Scotch and Whiskey etc.

* i may have made that figure up, it's probably higher than 99%

also, Wim, i'm not picking on you :-) i've seen variations of this from lots of devs in the D8 cycle, this just pushed me over the edge.

effulgentsia’s picture

I think some of the confusion is from casual lumping of "enable Drupal render caching by default for blocks and entities" and "allow for caching blocks in a caching layer outside of Drupal" into "ESI". Technically, ESI is only a subset of the 2nd, and while I would still really really love to have core support it out of the box, I agree might not hold up a release if not done in time, and should not be required for decent performance, only for extreme scalability. The 1st, however, may end up being the only way to make D8 have not worse performance than D7, but fortunately, is available to all sites, not just the 1% using out-of-Drupal caching layers.

Wim Leers’s picture

[offtopic]
#38: Hehe, no worries :) I feel more or less the same way — I'm just echoing what I've read elsewhere. As #39 indicates, it's more subtle than that though: the things that enable ESI also enable better caching *within Drupal itself*. I.e. ESI is about making blocks/entities etc. (as #39 says) individually retrievable and cacheable, so they can be integrated with super high-end, super expensive ESI solutions. But it also means you can leverage that same granular cacheability (and corresponding cache clearing) for doing "ESI" with your Varnish or nginx. And it also means that even without any of that, Drupal has to do less work per page load if we have this sort of granular caching working well.
So: it's about caching individual parts/components/pieces/blocks/… of a page. It'll have a big effect, no matter if you're using just Drupal, a reverse proxy in front of Drupal, or a very high-end CDN in front of Drupal.
I hope that made sense :)
[/offtopic]

Anonymous’s picture

so i read the patch, and wow, i'm really, really off-topic with my comments.

nothing to do with ESI in there, at all. it just collects tags from render arrays. +1 from me, and sorry for the noise.

Berdir’s picture

Status: Needs work » Needs review
FileSize
41.36 KB

Here's another re-roll.

Haven't really read how you want to solve the... "comment/nested-entity"-problem but that's going to be interesting :) The node contains a list of comments that might or might not be visible. But even if we add the cache tags for those to the node build array, there is still no way to detect if a comment is actually in there or not, we can just track explicit cache clears and then again for all users.

Maybe we need to build a cache key based on the contained entities, so that a different group of entities will be cached differently?

Status: Needs review » Needs work

The last submitted patch, 1605290-entity_render_cache-42.patch, failed testing.

catch’s picture

I'd probably have any comment addition/update clear the cache tag for the node ID. That'll force things like comment count in node teasers to be updated as well as listings like most commented. If we individually cache the generated HTML for each comment as well then rebuilding the full list of comments for the node is still going to be cheap-ish.

amateescu’s picture

Status: Needs work » Needs review
FileSize
49.83 KB

This should get us back to just a few failing tests: comment 'new' indicator and comment pager.

For the former, @catch opened #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user and the latter is the problem that @Berdir was referring to in #42. The problem with comment pager is that when we initially do nested caching on the first page, subsequent pages have no way of telling the render controller that we are displaying a different set of comments.

I also opened and patched #1965208: Convert TaxonomyTermReferenceItem to extend EntityReferenceItem and #1996714: Convert FileItem and ImageItem to extend EntityReferenceItem so we can have a cleaner way of gathering entity ids from entity / taxonomy / field / image reference fields in EntityRenderController::resetCache().

amateescu’s picture

FileSize
20.5 KB

Forgot the interdiff.

Status: Needs review » Needs work

The last submitted patch, 1605290-entity_render_cache-45.patch, failed testing.

Berdir’s picture

Given that there are various problems with comments currently anyway (new mark, links, ..) could we simply disable the render cache on the entity-level if there are comments. There probably needs to be an easy way to disable caching anyway, not sure if we have that already?

amateescu’s picture

Status: Needs work » Needs review
FileSize
4.72 KB
52.86 KB

Adding cache clearing in user_role_grant/revoke_permissions() was a very bad idea, don't know what I was thinking. This one should really get us to only comment 'new' and pager fails.

Re #48: I'm not sure what that buys us, the hard work is already done, so waiting for / fixing those two remaining issues doesn't sound too bad to me..

Status: Needs review » Needs work

The last submitted patch, 1605290-entity_render_cache-49.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
51.63 KB

Straight up reroll. Still waiting for those comment issues to be fixed...

Wim Leers’s picture

Status: Needs review » Needs work
+++ b/core/includes/common.incundefined
@@ -5064,6 +5064,57 @@ function drupal_render_collect_attached($elements, $return = FALSE) {
+function drupal_render_collect_cache_tags($elements, $return = FALSE) {

This function is of the *utmost* importance. It's short, it's simple, but it is also the basic building block by which render caching stands or falls.

I think it's essential to have unit test coverage for this.

+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.phpundefined
@@ -62,6 +91,21 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
+        'granularity' => DRUPAL_CACHE_PER_ROLE,

Playing the devil's advocate: are we absolutely certain we can assume this in all cases?
Or, which I think is more likely, we are defaulting to this, but modules/entity types can override this default.
IOW: I should interpret this as a default?

+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.phpundefined
@@ -164,4 +208,35 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+  /**
+   * Implements \Drupal\Core\Entity\EntityRenderControllerInterface::resetCache().

{@inheritdocs}

+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.phpundefined
@@ -164,4 +208,35 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+        // Add all the referenced entity types and IDs to the tags that will be
+        // cleared.
+        foreach ($entity->getPropertyDefinitions() as $name => $definition) {
+          if ($definition['type'] == 'entity_reference_field' && $field_values = $entity->get($name)->getValue()) {
+            foreach ($field_values as $value) {
+              $tags[$definition['settings']['target_type']][$value['target_id']] = $value['target_id'];
+            }
+          }

I understand this is necessary, but… it's not very nice to see a specific field type check in here. Is there no other way?

+++ b/core/modules/comment/comment.pages.incundefined
@@ -80,6 +80,7 @@ function comment_reply(EntityInterface $node, $pid = NULL) {
+      unset($build['comment_node']['#cache']);

A comment here would be nice.

+++ b/core/modules/comment/lib/Drupal/comment/CommentRenderController.phpundefined
@@ -79,9 +79,10 @@ protected function alterBuild(array &$build, EntityInterface $comment, EntityDis
-      // Add 'new' anchor if needed.
+      // Add 'new' anchor and disable render cache if needed.

"Add 'new' anchor if needed (and if so, disable render cache)."
?

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentAnonymousTest.phpundefined
@@ -148,6 +148,8 @@ function testAnonymous() {
+    \Drupal::entityManager()->getRenderController('node')->resetCache();
+    \Drupal::entityManager()->getRenderController('comment')->resetCache();
     $this->drupalGet('node/' . $this->node->nid);
     $this->assertPattern('@<h2[^>]*>Comments</h2>@', 'Comments were displayed.');
     $this->assertLink('Log in', 1, 'Link to log in was found.');
@@ -158,6 +160,8 @@ function testAnonymous() {

@@ -158,6 +160,8 @@ function testAnonymous() {
       'post comments' => TRUE,
       'skip comment approval' => TRUE,
     ));
+    \Drupal::entityManager()->getRenderController('node')->resetCache();
+    \Drupal::entityManager()->getRenderController('comment')->resetCache();

These are changes to keep existing tests working; ideally we'd also have tests to prove that the entity render cache is working.

+++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.phpundefined
@@ -126,7 +126,14 @@ public function save() {
+    if (\Drupal::entityManager()->hasController($this->targetEntityType, 'render')) {
+      \Drupal::entityManager()->getRenderController($this->targetEntityType)->resetCache();
+    }

This is again for entity reference fields?

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterBase.phpundefined
@@ -97,6 +97,14 @@ public function view(EntityInterface $entity, $langcode, array $items) {
+      // Gather cache tags from reference fields.
+      foreach ($items as $item) {
+        if (isset($item['entity'])) {
+          $info['#cache']['tags'][$item['entity']->entityType()][] = $item['entity']->id();
+          $info['#cache']['tags'][$item['entity']->entityType() . '_view'] = TRUE;
+        }

Again.

But why is it this time not necessary to check the type or some other property? The code reads like it's being applied to all fields, yet the comment says differently.

+++ b/core/modules/node/node.moduleundefined
@@ -1012,16 +1012,18 @@ function node_revision_delete($revision_id) {
+    // Don't use the render cache when a revision is displayed.
+    unset($nodes['nodes'][$node->id()]['#cache']);

Why? Why not set a short expiration time to prevent filling the cache endlessly?

amateescu’s picture

About testing: yeah, even though the patch is thoroughly tested already by existing tests, I agree that we need some that are dedicated to this functionality.

+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.phpundefined
@@ -62,6 +91,21 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
+        'granularity' => DRUPAL_CACHE_PER_ROLE,

Playing the devil's advocate: are we absolutely certain we can assume this in all cases?
Or, which I think is more likely, we are defaulting to this, but modules/entity types can override this default.
IOW: I should interpret this as a default?

Every entity type can override getBuildDefaults() in its own render controller, and the render array built here can be altered in a lot of places.

{@inheritdocs}

Yeah, this patch was started long before that policy and needs to be updated. And it's {@inheritdoc}, without S.

I understand this is necessary, but… it's not very nice to see a specific field type check in here. Is there no other way?

As I already said in #45, I opened and provided patches for all reference-like fields to have a target_id property (instead of 'tid' or 'fid'). After that, we will just need to check if a specific field is an instance of \Drupal\Core\Entity\Field\Type\EntityReferenceItem (not the configurable one provided by the Entity reference module).

+++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.phpundefined
@@ -126,7 +126,14 @@ public function save() {
+    if (\Drupal::entityManager()->hasController($this->targetEntityType, 'render')) {
+      \Drupal::entityManager()->getRenderController($this->targetEntityType)->resetCache();
+    }

This is again for entity reference fields?

This has nothing to do with entity reference fields? $targetEntityType is a property of the EntityDisplayBase object.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterBase.phpundefined
@@ -97,6 +97,14 @@ public function view(EntityInterface $entity, $langcode, array $items) {
+      // Gather cache tags from reference fields.
+      foreach ($items as $item) {
+        if (isset($item['entity'])) {
+          $info['#cache']['tags'][$item['entity']->entityType()][] = $item['entity']->id();
+          $info['#cache']['tags'][$item['entity']->entityType() . '_view'] = TRUE;
+        }

Again.

But why is it this time not necessary to check the type or some other property? The code reads like it's being applied to all fields, yet the comment says differently.

And again, this has nothing to do with entity reference! All reference-like fields (entity ref, taxo ref, file, image) have the 'entity' property for their items already in HEAD.

+++ b/core/modules/node/node.moduleundefined
@@ -1012,16 +1012,18 @@ function node_revision_delete($revision_id) {
+    // Don't use the render cache when a revision is displayed.
+    unset($nodes['nodes'][$node->id()]['#cache']);

Why? Why not set a short expiration time to prevent filling the cache endlessly?

And that short lived cache entry would help.. who exactly? Not to say that we'd have to take it into account in the cache key, which will special case entity types that are revisionable.

Wim Leers’s picture

I'm a relative outsider to Field API and figured it'd be useful to have a pair of eyes look at this that doesn't know what else has happened. Apologies if my review was frustrating to you — just trying to help.

I missed #45. Hence most of my reference field remarks are indeed incorrect, with the exception of the one on this piece of code I think:

if ($definition['type'] == 'entity_reference_field'

That one is *not* generic, right?

And that short lived cache entry would help.. who exactly?

I'm just questioning the assumptions in this patch, that's all! This is probably the sanest default. But everywhere where assumptions are embedded, especially WRT caching, IMO the comment should convey *why* this is the sanest default: not only the *what* but also the *why*.

amateescu’s picture

Yep, that's the one that is not generic and that the issues/patches mentioned in the last paragraph of #45 will address.

Fair point that we need an extra comment there to explain why we're only caching the current (active) revision and not all others. Will do that along with all your other remarks in the next iteration.

Thanks for trying to help and sorry for the tone.. :)

effulgentsia’s picture

Priority: Major » Critical

Raising to critical, because while we can still optimize some of the OOP overhead we've added to D8 in general, and to Entities in particular, I don't think we can optimize uncached entity rendering to be faster than it was on D7. Happy to be proven wrong, and if we do manage to optimize sufficiently before getting this in, I'd be ok with this then being downgraded back to major, but I suspect we'll get this in first.

catch’s picture

Agreed on critical.

Wim Leers’s picture

+1

Caseledde’s picture

There are a lot of per-role, per-user and per-request aware fields (like Fivestar, Flag, etc). There already is an issue for the 'new'-mark on comments: #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user.

If we cache an entity, we have to be aware of those fields / extra_fields.

In the context of #1875974: Abstract 'component type' specific code out of EntityDisplay we are able to handle all fields in a different way.

So here is a resolution propose:

  1. Cache the whole rendered entity.
  2. Add a ['field_granularity']-like attribute to all field component types. There we can set the cache granularity of this component.
  3. During rendering the entity, checking ['field_granularity']. If its not 'default' a placeholder-token will be rendered instead of the field component.
  4. If the cached entity is called, the token will be replaced by the rendered field component.
  5. Cache the rendered field component like the entity by using the granularity for this field getting from ['field_granularity'].

In this way, we are able to provide context aware link sections, 'new'-marks for comments etc. without touching the cache granularity of the parent entity.

catch’s picture

Fivestar, flag etc. should move to the same mechanism that edit/contextual module are using in core - which doesn't require changing markup based on the current user role (those checks are done via AJAX requests and replacement). This is about the same as what you're suggesting, except that the replacement is done in JavaScript rather than PHP. It's not possible to rely on PHP replacement due to reverse proxies.

moshe weitzman’s picture

That AJAX technique is ideal. If not possible for some reason, those modules just need to add cache_tags for each role/group/whatever. The cache system handles retrieving the right cached HTML after that.

catch’s picture

Cache tags don't affect cache IDs at all, only invalidation.

Since #cache is set on the render array, anyone altering the render array ought to also be able to alter the cache ID/cid parts though so as long as that's exposed properly it's an option for modules that can't do js-replacement for some reason.

moshe weitzman’s picture

Oops, yes thats what I should have said. Thanks.

effulgentsia’s picture

anyone altering the render array ought to also be able to alter the cache ID/cid parts

Yes, but providing a field type doesn't involve altering the entity render array directly. Rather, field.module sits as an intermediary for that. Creating some API for field types to inform field.module of their cache granularity and then letting field.module alter the #cache of the entity render array sounds like a nice idea, but I think can be a noncritical, post-API-freeze follow up, since it would be a small API addition, not a BC break. If not done in time for D8 core, then field type modules can fall back to implementing the corresponding alter hooks themselves.

Caseledde’s picture

There is no need to change the cache granularity of the entity because of a single field. If we do, we still have issues like #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user or #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links. If we use tokens to be replaced in the rendered entity, we do not need granularity for the entity. For example, the granularity for entites comes in handy, if we have to be aware of field access. The we can use roles as granularity for the entity. But we are still albe to cache the content of a single field separatly and just decide to display or not. No rendering needed anymore.

The second point is, to return the cached html before the renderable array will be build. We cache the html, because it is not necessary to rebuild it again and again. The renderable array should'nt be rebuild as well. In this way, we can't alter the render array after the entity was cached, because the alteration will not be called anymore.

So we need an API to enable field developers to return markup during the rendering of an entity, which will be cached, and during the display of this entity, if the field displays dynamic content.

If we can provide an generic API to tag a field component with attributes like

  • Field Granularity: Clould be 'default' (to be cached with the entity) or DRUPAL_NO_CACHE (to exclude the field from display)
  • Get default value: Returns a value, which can be cached within the entity. (links-section without active classes)
  • Get dynamic value: Returns the dynamic value. (links-section with active classes)

The field API has to call this methods during the rendering of the entity and during the display via ajax to replace the generic tokens build by the field module by react of the 'Field Granularity'-tag.

Caseledde’s picture

Updated patch from #51. It was out of date and not longer applyable.

Caseledde’s picture

FileSize
10.32 KB

Interdiff between #51 and #66.

catch’s picture

Yes, but providing a field type doesn't involve altering the entity render array directly. Rather, field.module sits as an intermediary for that. Creating some API for field types to inform field.module of their cache granularity and then letting field.module alter the #cache of the entity render array sounds like a nice idea, but I think can be a noncritical, post-API-freeze follow up, since it would be a small API addition, not a BC break.

What we should likely do is collect the cid parts recursively in the same way that #attached and #cache_tags are - then adding cid parts to a field formatter will affect the entity rendering as well.

catch’s picture

Except that's a completely stupid idea from me in #68 - it's OK for cache tags and #attached 'cos you can combine the result of whatever was cached, but cid parts you need to know before you even request from cache, so yeah that does need to be calculated up front, or otherwise circumvented.

Caseledde’s picture

Status: Needs work » Needs review

Needs review to run testbot.

What should our next steps be? I can imagine, that my proposal is al little bit big, but i think we have to do something anyway, so I think we can do it in a generic way right away.

Status: Needs review » Needs work

The last submitted patch, 1605290-entity_render_cache-66.patch, failed testing.

tstoeckler’s picture

Re #70/@Caseledde: I'm not sure I've understood your #65 100% correctly, but either way I think we all agree that getting this patch in as is would be a great step forward.

I agree with you and @effulgentsia in #64 that - even if we enforce modules to strictly only return markup that is dependent on the set of available roles - we should provide facilities that help with doing per-user-markup-but-only-via-ajax without having to re-invent the heavy lifting each time. That should definitely be discussed in a follow-up issue, though.
I don't know in fow far #2005644: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash is related, also.

Caching the render arrays themselves, instead of building them each time, is very interesting idea, that I haven't seen discussed in the core queue so far. The granularity question is the same one there, but if we want to enforce role-based markup that needs to happen on a render array level anyway. I've seen promising benchmarks based on https://drupal.org/sandbox/Caseledde/1970904 that this is in fact worth investigating, but again I would suggest you open a follow-up once this is in.

Wim Leers’s picture

#2005644: Use client-side cache tags & caching to eliminate 1 HTTP requests/page for in-place editing metadata, introduce drupalSettings.user.permissionsHash is essential for client-side caching of these AJAX-retrieved, user- or role-specific pieces of content. Otherwise, these have to be retrieved every single time, as is the case right now in Drupal core (i.e. for rendering contextual links and in-place editing metadata).

tstoeckler’s picture

Ahh, that makes perfect sense. Thanks for explaining that!

msonnabaum’s picture

Status: Needs work » Needs review
FileSize
52.19 KB

Should fix comment and filter fails.

Status: Needs review » Needs work

The last submitted patch, 1605290-entity_render_cache-75.patch, failed testing.

msonnabaum’s picture

Here's a new one that should fix more comment tests.

It takes a different approach than previous patches in that it adds the logic directly into the entity rather than the form/render controllers. The logic of when an entity changes and what entities are related to that entity are firmly within the responsibilities of the entity itself, so it makes sense to handle it there.

I added two methods to entity, changed() and relationships(). The names aren't great, but they express the basic intention of declaring that something changed and getting a list of related entities that need to change as well.

mikeytown2’s picture

Status: Needs work » Needs review
msonnabaum’s picture

Ignore last patch.

amateescu’s picture

No no no, no :P This was also my original approach until #18, where @catch and @moshe weitzman convinced me that clearing caches every time an entity is saved is the wrong thing to do.

I think relationships() is a good concept to have in general (maybe getRelatedEntities()/Data() would be a better name?), but changed() needs to go :)

P.S. An interdiff would be very helpful to see exactly what was changed..

effulgentsia’s picture

msonnabaum’s picture

@amateescu - I spoke with Moshe yesterday and he no longer thinks this is necessary. Perhaps catch still feels this way, but I just dont see how it's sustainable at all. If we're leaving invalidation up to the controllers, we're violating the boundaries of the entity. Only it can truly know when it has changed, because it's the owner of it's save/delete methods.

The remaining failures are still quite unique. We haven't solved the basic problems of this patch, so I think we should focus on doing whatever will get these cases passing.

Status: Needs review » Needs work

The last submitted patch, 1605290-entity_render_cache-78.patch, failed testing.

damiankloip’s picture

Assigned: amateescu » Unassigned
FileSize
58.33 KB

Here a patch with some earlier work in. I have moved the new methods into Entity instead, as we will need to use this cache invalidation with config entities too; such as when a field instance is updated, all entities that it is attached to need to have their cache invalidated.

amateescu’s picture

@damiankloip, what do you mean by "earlier work"? Earlier from a previous patch from here? Which one?

Doesn't anyone use interdiffs these days? :(

@msonnabaum, if we go with that line of thinking, then we also need to move the static cache invalidation out of the storage controller? And we'll have to get back to the global switch idea as well: "Hey, I'm trying to do a migration here, stop messing with all those caches." ... (hard work on the db) ... "Ok, I'm done. Carry on and clear everything."

msonnabaum’s picture

Sorry, I wasnt posting interdiffs because it kept telling me they were impossible and producing nonsense.

And sure, it might be nice to have the global switch thing, but IMO it's a total distraction from the very difficult problems in the current fails. Lets fix those first.

msonnabaum’s picture

Status: Needs work » Needs review
amateescu’s picture

That's what I've been saying for a couple of months already (since #45) :)

I suspect that no one actually read the previous comments, so I'll say it again. There are three problems:
- comment 'new' marker -> easy to fix as we already have and established pattern of how it should be done
- comment links -> @catch and @Wim Leers discussed this in another issue, can't remember which one atm
- comment pager -> see the comment in #45, nfi how to fix it..

Can we please postpone this one and stop chasing our tails? :)

Status: Needs review » Needs work

The last submitted patch, 1605290-84.patch, failed testing.

msonnabaum’s picture

Status: Needs work » Needs review
FileSize
987 bytes
58.39 KB

There are a lot more than 3 remaining problems in this patch…

Wim had a workable solution for the "comment new" patch today. Hopefully he'll post something soon in the other issue.

Here's an inelegant solution to the pager issue. Hopefully we can do better, but this should work for now.

Status: Needs review » Needs work

The last submitted patch, 1605290-entity_render_cache-89.patch, failed testing.

damiankloip’s picture

FileSize
893 bytes
59.27 KB

I think the ViewsUI class is ruining the party at the moment, with INTERDIFF...

catch’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity.phpundefined
@@ -637,4 +640,43 @@ public function initTranslation($langcode) {
+    foreach ($this->relationships() as $related_entity) {
+      $related_entity_type = $related_entity->entityType();
+      if (!isset($tags[$related_entity_type . '_view'])) {
+        $tags[$related_entity_type . '_view'] = array();
+
+      }
+      $tags[$related_entity_type . '_view'][] = $related_entity->id();

This shouldn't be necessary.

If an entity (say a node) gets rendered with a reference field (say taxonomy terms), the entity reference formatter should add cache tags for those entities to the render array. Since tags are collected recursively, those term IDs then end up in the cache tags for the node render, and if any term ID gets updated, the rendered node cache gets invalidated.

This looks like it's doing it the other way 'round - when a term references a node, delete the cache tags for the node, but that's not necessary and a lot more complex.

amateescu’s picture

Assigned: Unassigned » amateescu

@catch is correct in #93, the latest iterations introduced a lot of unneeded complexity in this patch. I'm working to get it back to a sane state.

msonnabaum’s picture

@catch I added that to fix the issue where a node_view cache wasn't updated when a new comment was added. Since the comment is new, the node was never tagged with it. Not sure how else to handle that.

amateescu’s picture

Status: Needs work » Needs review
FileSize
31.7 KB
5.45 KB
29.56 KB

This gets us back to more or less what we had in #17 (yes, five months ago), before the madness with cache clearing in form controllers started.

The first interdiff is the 'get back to a sane state' part, and the second one is starting the 'clear caches on save and delete' path.

Status: Needs review » Needs work

The last submitted patch, 1605290-95.patch, failed testing.

catch’s picture

@msonnabaum that makes sense now.

'm not sure if it makes sense for anything other than comments though - they're the only entity that references an entity, then gets rendered by the entity they reference, other entity references work backwards from that - or don't get nested within the referenced entity (i.e. node listings on taxonomy terms aren't within the entity render).

amateescu’s picture

Assigned: amateescu » msonnabaum

As discussed in IRC, that case should already be handled by the logic in resetCache() from the render controller, which should pick up the referenced node by looking at the 'nid' property of the new comment. Mark is working on merging the patches now.

msonnabaum’s picture

Status: Needs work » Needs review
FileSize
5.35 KB
34.14 KB

Ok, here's a merging of the two patches. I kept the methods on the entity because think it's a bit cleaner, but we can move the actual cache tags clearing out eventually. I think an event listener might work best.

Status: Needs review » Needs work

The last submitted patch, 1605290-100.patch, failed testing.

msonnabaum’s picture

Status: Needs work » Needs review
msonnabaum’s picture

#100: 1605290-100.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D8 cache tags, +D8 cacheability

The last submitted patch, 1605290-100.patch, failed testing.

amateescu’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -646,2 +644,49 @@
+    // @todo: Move cache tag invalidation to an event listener so that the
+    // service isn't required here.
+    Cache::deleteTags($tags);

After our discussion on IRC about this, I still don't see why clearing these tags is the responsability of the entity object, since the render controller is the one that defines the cache bin and only it knows how to handle its own cache...

msonnabaum’s picture

Status: Needs work » Needs review
Issue tags: -D8 cache tags, -D8 cacheability

#100: 1605290-100.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D8 cache tags, +D8 cacheability

The last submitted patch, 1605290-100.patch, failed testing.

msonnabaum’s picture

FileSize
34.09 KB

Re-roll.

msonnabaum’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1605290-108.patch, failed testing.

msonnabaum’s picture

Status: Needs work » Needs review
FileSize
34.09 KB

Thought the bot was failing, was a dumb mistake.

msonnabaum’s picture

Also, I don't entirely disagree with #105, I'd just rather leave any more refactoring until after we're closer to passing.

Status: Needs review » Needs work

The last submitted patch, 1605290-111.patch, failed testing.

amateescu’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -642,4 +647,51 @@ public function initTranslation($langcode) {
+    // Add all the referenced entity types and IDs to the tags that will be
+    // cleared.
+    foreach ($this->getProperties() as $name => $definition) {
+      $property = $this->get($name)->offsetGet(0);
+      if ($property instanceof EntityReferenceItem && $entity = $property->entity) {
+        $relationships[] = $entity;
+      }
+    }

The purpose of the original code from EntityRenderController (where the comment is actually correct about what the code is doing) was to get entity ids only, not full entity objects. Since $property->entity is a computed property, this means that we're now loading a bunch of entities for no reason, since we only need their ids for cache clearing.

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -642,4 +647,51 @@ public function initTranslation($langcode) {
+    foreach ($this->relatedEntities() as $related_entity) {
+      $related_entity_type = $related_entity->entityType();
+
+      if (!isset($tags[$related_entity_type])) {
+        $tags[$related_entity_type] = array();
+      }
+
+      $tags[$related_entity_type][] = $related_entity->id();
+    }

This is not how cache tags work, the second dimension of the array needs to be keyed by entity ids. Edit: Bah, I'm not sure that's true, I might have keyed the array for different purposes.

tstoeckler’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.php
@@ -62,6 +92,22 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
+    $view_mode_is_cacheable = !isset($this->viewModesInfo[$view_mode]) || (isset($this->viewModesInfo[$view_mode]) && $this->viewModesInfo[$view_mode]['cache']);

I haven't dug into EntityRenderController::getBuildDefaults() so sorry if this is obvious from the context, but: Why would $this->viewModesInfo[$view_mode] not be set?

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterBase.php
@@ -95,8 +95,21 @@ public function view(EntityInterface $entity, $langcode, array $items) {
diff --git a/core/modules/file/config/entity.view_mode.file.full.yml b/core/modules/file/config/entity.view_mode.file.full.yml

diff --git a/core/modules/file/config/entity.view_mode.file.full.yml b/core/modules/file/config/entity.view_mode.file.full.yml
new file mode 100644
index 0000000..4d4ed1f

index 0000000..4d4ed1f
--- /dev/null

--- /dev/null
+++ b/core/modules/file/config/entity.view_mode.file.full.yml

+++ b/core/modules/file/config/entity.view_mode.file.full.yml
+++ b/core/modules/file/config/entity.view_mode.file.full.yml
@@ -0,0 +1,5 @@

@@ -0,0 +1,5 @@
+id: file.full
+label: File default
+status: '0'
+cache: '1'
+targetEntityType: file

It seems this is a merge conflict. The file.full view mode was recently removed in the view mode UI issue.

+++ b/core/modules/taxonomy/config/entity.view_mode.taxonomy_term.full.yml
@@ -1,4 +1,5 @@
diff --git a/core/modules/taxonomy/config/entity.view_mode.taxonomy_vocabulary.full.yml b/core/modules/taxonomy/config/entity.view_mode.taxonomy_vocabulary.full.yml

diff --git a/core/modules/taxonomy/config/entity.view_mode.taxonomy_vocabulary.full.yml b/core/modules/taxonomy/config/entity.view_mode.taxonomy_vocabulary.full.yml
new file mode 100644
index 0000000..8961a6f

index 0000000..8961a6f
--- /dev/null

--- /dev/null
+++ b/core/modules/taxonomy/config/entity.view_mode.taxonomy_vocabulary.full.yml

+++ b/core/modules/taxonomy/config/entity.view_mode.taxonomy_vocabulary.full.yml
+++ b/core/modules/taxonomy/config/entity.view_mode.taxonomy_vocabulary.full.yml
@@ -0,0 +1,5 @@

@@ -0,0 +1,5 @@
+id: vocabulary.full
+label: Taxonomy vocabulary
+status: '0'
+cache: '1'
+targetEntityType: taxonomy_vocabulary

Same here.

amateescu’s picture

Assigned: msonnabaum » amateescu
Status: Needs work » Needs review
FileSize
12.87 KB
34.3 KB

Also, I don't entirely disagree with #105, I'd just rather leave any more refactoring until after we're closer to passing.

The thing is.. that's not refactoring, it's just coming back to the "default" state of this patch. Here's how it should work if we want to keep relationship gathering in the entity class. The only big change is that resetCache() does not need full entity objects anymore, just ids.

The rest of the interdiff is just cleaning up bad rerolls that accumulated mistakes over time.

Beside the usual comment failures, at least Drupal\serialization\Tests\EntitySerializationTest (probably Drupal\taxonomy\Tests\TermTest too) fails because of #114.1.

@tstoeckler:

Why would $this->viewModesInfo[$view_mode] not be set?

Because some tests (mostly in Views IIRC) like to request non-existent view modes and I didn't feel like fixing everything in here, so I chose the easier way out :) We can try to remove that condition in a patch testing issue if you want to see exactly what was wrong.

Edit: I intentionally left that debug() in there to prove that a test is failing because of needless entity loading.

Status: Needs review » Needs work

The last submitted patch, 1605290-116.patch, failed testing.

tstoeckler’s picture

@amateescu: Ahh, that's interesting, thanks. I guess a simple comment/@todo would be enough to satisfy me, for now.

amateescu’s picture

Status: Needs work » Needs review
FileSize
9.86 KB

This should fix all DUTB tests, let's see what's left..

amateescu’s picture

FileSize
9.86 KB
42.1 KB

Oops, that was only the interdiff.

Status: Needs review » Needs work

The last submitted patch, 1605290-120.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.98 KB
46.82 KB

Keeping up with HEAD.

Status: Needs review » Needs work

The last submitted patch, 1605290-122.patch, failed testing.

amateescu’s picture

Assigned: amateescu » Unassigned
dlu’s picture

Component: base system » entity system

Moved to entity system per #2050763-16: Refine "base system" component (notes on refactoring of "base system" category here: https://docs.google.com/a/acquia.com/spreadsheet/ccc?key=0AusehVccVSq2dF...).

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
44.04 KB

Rerolled and disabled the render cache for nodes and comments. Let's see what happens.

Status: Needs review » Needs work

The last submitted patch, 1605290-126.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.93 KB
45.36 KB

hopefully this fixes the two fails.

dixon_’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
@@ -326,4 +326,16 @@ public function isTranslatable();
+  /**
+   * @todo
+   *
+   * @return array
+   */
+  public function relatedEntities();

I hate to come in late in this issue, but it feels wrong to have relatedEntities() in EntityInterface. To me it feels like we should create a separate service for this, or maybe tuck it into EntityManager (but I'd prefer a separate service I think). This code shouldn't need to be extended per entity type really, it should be fairly generic, no?

It would also be quite useful to have the related/dependent entity logic in a service so that contrib easily can extend it independently of each entity class. We need something like this for content staging in Deploy module. In D7 I created Entity Dependency module for this.

amateescu’s picture

FileSize
649 bytes
45.48 KB

@dixon_, that is indeed a good point. I would prefer to see it in EntityManager rather than a separate service, and I'm sure others would like it to stay in EntityInterface. Given that, are you ok with discussing this in a followup? This patch has been through too many bikesheds already..

Documented the new methods from EntityInterface, so I think we're finally ready for a rtbc here :)

Wim Leers’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityRenderController.php
@@ -62,6 +92,24 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
+        'keys' => array('entity_view', $this->entityType ,$entity->id(), $view_mode, $request->getQueryString()),

s/ ,/, /

And more importantly: making the entire query string part of the key is problematic, because it makes it easy for a malicious user to trigger the generation of new cache entries, thus always hitting the server *and* filling the cache with identical cache values with different keys … leaving no room for "proper" cache entries.

I feel this was discussed before, but maybe that was a separate issue.

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.03 KB
45.41 KB

And more importantly: making the entire query string part of the key is problematic, because it makes it easy for a malicious user to trigger the generation of new cache entries, thus always hitting the server *and* filling the cache with identical cache values with different keys … leaving no room for "proper" cache entries.

That was added by Mark in #90 as a 'dirty fix' for comment paging, but since we disabled the cache for nodes and comments, we don't need to worry about it in this patch.

catch’s picture

ecause it makes it easy for a malicious user to trigger the generation of new cache entries, thus always hitting the server *and* filling the cache with identical cache values with different keys … leaving no room for "proper" cache entries.

This is no different with the page cache - we cache 404s in the page cache as well.

Sites that are concerned about this should use a cache that handles LRU or similar.

Wim Leers’s picture

since we disabled the cache for nodes and comments, we don't need to worry about it in this patch.

Wow. I see it in the patch now. Is this just temporary, until #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user is fixed? The test results of #122 — the last patch before you disabled this, seem to confirm that that *is* the main reason?

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentAnonymousTest.php
@@ -148,6 +148,8 @@ function testAnonymous() {
+    \Drupal::entityManager()->getRenderController('node')->resetCache();
+    \Drupal::entityManager()->getRenderController('comment')->resetCache();
+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentAnonymousTest.php
@@ -158,6 +160,8 @@ function testAnonymous() {
+    \Drupal::entityManager()->getRenderController('node')->resetCache();
+    \Drupal::entityManager()->getRenderController('comment')->resetCache();

If node/comment render caching is disabled, then why are lines like these necessary?

+++ b/core/modules/statistics/statistics.module
@@ -73,6 +73,17 @@ function statistics_node_view(EntityInterface $node, EntityDisplay $display, $vi
+function statistics_node_view_alter(&$build, EntityInterface $node, EntityDisplay $display) {
+  // If statistics were added to the node render array, we can't use the render
+  // cache.
+  if (isset($build['links']['statistics'])) {
+    unset($build['#cache']);
+  }
+}

Can't this be done in statistics_node_view()?

Finally, and most importantly: I don't see any additional test coverage. Wouldn't it make sense to do a full render cache life cycle on an entity? I.e. create it, view it, view it again and ensure that it's served from the render cache, modify it, ensure that it's re-rendered.

amateescu’s picture

FileSize
4.94 KB
40.47 KB

Wow. I see it in the patch now. Is this just temporary, until #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user is fixed? The test results of #122 — the last patch before you disabled this, seem to confirm that that *is* the main reason?

Yep, we want to get the initial patch in and then work out everything needed for nodes and comments.

If node/comment render caching is disabled, then why are lines like these necessary?

Because I'm dumb :/

Can't this be done in statistics_node_view()?

We don't have access to the $build array there.

And yes, I guess we need some dedicated tests using the entity_test entity type.

amateescu’s picture

Wim Leers’s picture

Issue tags: +Needs tests

And yes, I guess we need some dedicated tests using the entity_test entity type.

Having discussed this with amateescu in IRC, this is currently the only thing remaining.

Wim Leers’s picture

Status: Needs review » Needs work

For #1991684-40: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user, I've reviewed this patch in depth.

Overall review

  1. Unit tests for drupal_render_collect_cache_tags(), because the render cache's reliability depends on it.
  2. Tests that ensure drupal_render() is indeed using the render cache on entities, and that saving an entity clears the corresponding cache entries.
  3. This patch will cause there to be as many cache entries as there are entities on the site. On many if not most sites, that will result in numerous cache entries. Hence I think we should have a new cache table: cache_render. It's reasonable to say it should be a follow-up, but at the same time it is this patch that will cause the render cache to be used much more widely. It's a small change. If you choose not to do it here, then please create a follow-up.

Code review

  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -634,4 +638,45 @@ public function initTranslation($langcode) {
    +    // @todo Remove when all entities are converted to EntityNG.
    

    Link to an issue?

  2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -634,4 +638,45 @@ public function initTranslation($langcode) {
    +      $field_item = $this->get($name)->offsetGet(0);
    

    What about other offsets? If we don't care about other offsets, then there should be a comment explaining why.

  3. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -634,4 +638,45 @@ public function initTranslation($langcode) {
    +  public function changed() {
    +    $related_entity_ids = array(
    +      $this->entityType() => array($this->id() => TRUE),
    +    );
    +
    +    foreach ($this->relatedEntities() as $related_entity) {
    +      $related_entity_ids[$related_entity->entityType()][$related_entity->id()] = TRUE;
    +    }
    +
    +    foreach ($related_entity_ids as $entity_type => $entity_ids) {
    +      if (\Drupal::entityManager()->hasController($entity_type, 'render')) {
    +        \Drupal::entityManager()->getRenderController($entity_type)->resetCache(array_keys($entity_ids));
    +      }
    +    }
    +  }
    

    $related_entity_ids is first initialized to contain the entity whose changed() method is being invoked.

    Then, the method iterates over all related entities and adds those to $related_entity_ids.

    Finally, we iterate over $related_entity_ids by entity type, clearing all render cache entries for entity IDs of a given entity type.

    So, clearly, $related_entity_ids is the set of all affected entities whose render cache entries should be cleared.

    Hence I think $related_entity_ids should be renamed to $affected_entity_ids.

  4. +++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.php
    @@ -619,4 +619,20 @@ public function initTranslation($langcode) {
    +  public function changed(array $tags = array()) {
    +    $this->decorated->changed($tags);
    +  }
    

    The $tags parameter here should go away, I think this is a leftover from earlier patches?

  5. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -326,4 +326,17 @@ public function isTranslatable();
    +  public function relatedEntities();
    

    Like catch in #93, I'm very confused why this is necessary. If an entity is changed, then all cache entries containing that entity should be cleared, and nothing else. E.g. when creating a node, why should the author's profile page be cleared?

    But then I read this in #11:

    The most important change is that I figured out we also need to clear the render cache when creating new entities, not only on update. I can easily find at least two scenarios where we need this:
    - when a new comment is posted, we want the node page associated with it to be updated
    - when we create a new node and tag it with a taxonomy term, we want the taxonomy/term/ page to be updated

    In the first case, I would say it is the comment entity's responsibility to know on which entity it was posted and thus the comment entity should just clear the render cache entry for the entity it is for.
    This is also what #44 says.

    But in the second case, it's a lot harder: displaying all nodes that reference a taxonomy term is displaying the inverse of the relationship "ENTITY references TERM", which is the one that we really are manipulating.

    So, without having stored "backreferenced", we may still have those (and it's easy create those using Views).
    We want to be certain that we don't render any stale content/state, so rather than demanding optimal (minimum) cache clearing, we cast a wide net to make sure everything that might be affected, is cleared.

    I think that is fine for this initial version — we don't want to make this too complex.

    So, while I think this can be improved, I also think that this is the simplest possible solution that can work. Even though it may be suboptimal, a more optimal solution is likely to bring overhead with it as well. KISS.

    One thing that I'd like to see changed though is the method name. "related" is a very broad term. "referenced" is much more narrow, precisely describes what happens, and is actually the term used in the comment.
    So let's rename relatedEntities() to referencedEntities()?

  6. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -326,4 +326,17 @@ public function isTranslatable();
    +  /**
    +   * Acts on an entity after it was saved or deleted.
    +   */
    +  public function changed();
    

    Note that \Drupal\Core\Entity\Entity implements ComplexDataInterface, which already has a onChange() method. At the very least, it is confusing to see both onChange() and changed() on \Drupal\Core\Entity\Entity.

  7. +++ b/core/lib/Drupal/Core/Entity/EntityRenderController.php
    @@ -22,8 +22,37 @@ class EntityRenderController implements EntityRenderControllerInterface {
    +    $this->entityInfo = entity_get_info($entity_type);
    

    This function is deprecated.
    \Drupal\Core\Entity\EntityManager::getDefinitions() should be used directly.

  8. +++ b/core/lib/Drupal/Core/Entity/EntityRenderController.php
    @@ -62,6 +91,23 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
    +      $return['#cache'] = array(
    +        'keys' => array('entity_view', $this->entityType, $entity->id(), $view_mode),
    +        'granularity' => DRUPAL_CACHE_PER_ROLE,
    +        'bin' => $this->cacheBin,
    +        'tags' => array(
    +          $this->entityType . '_view' => TRUE,
    +          $this->entityType => array($entity->id()),
    +        ),
    

    It'll be possible to manipulate #cache to suit your site's specific characteristics (e.g. to change granularity) by using hook_entity_view_alter().
    Good.

  9. +++ b/core/modules/comment/comment.pages.inc
    @@ -81,6 +81,7 @@ function comment_reply(EntityInterface $node, $pid = NULL) {
    +      unset($build['comment_node']['#cache']);
    

    Can use a comment.

  10. +++ b/core/modules/comment/lib/Drupal/comment/CommentRenderController.php
    @@ -79,9 +79,10 @@ protected function alterBuild(array &$build, EntityInterface $comment, EntityDis
    +        unset($build['#cache']);
    

    Can use a comment.

amateescu’s picture

Assigned: Unassigned » msonnabaum

tl;dr version: add tests, a few comments and rename two methods :P

changed() and relatedEntities() are not mine in this patch, so let's try this first.

amateescu’s picture

Assigned: msonnabaum » Unassigned

It seems I offended Moshe with the issue assignment. nevermind me :)

sdboyer’s picture

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -634,4 +638,45 @@ public function initTranslation($langcode) {
+    // Gather a list of related entities.
+    foreach ($this->getProperties() as $name => $definition) {
+      $field_item = $this->get($name)->offsetGet(0);
+      if ($field_item instanceof EntityReferenceItem && $entity = $field_item->entity) {
+        $relationships[] = $entity;
+      }
+    }
+

so while i'm gonna ultimately concede, i find this horrifying, so i'm gonna raise the issue anyway.

the fact that we are tying contained/referenced entities in to choices about the render cache for the referencing entity is nuts. if this were a system for caching the raw data of an entity, that would be one thing: one could make an argument for caching the entity + its references together as an aggressive denormalization technique, justified by the fact that the consumers of that cached data actually MAY have a need to access anything in that composite datastructure.

however, in the case of caching rendered output...ugh. no. this is a really unfortunate intermixing of data with presentation; the presentation layer should know precisely what data its referencing, rather than having to fall back on this global, wildly oversensitive strategy. this is what you get when you mix data with presentation.

Wim comments that he's fine with this for an initial version. i guess i am, too. i can't disagree that this seems like the simplest possible solution that will work. but i think a more complex, granular solution that is also really reliable won't be possible until we run the hell away from this tight coupling of rendering logic directly with the entities themselves. that's what presentational elements are for - like say, blocks!

amateescu’s picture

however, in the case of caching rendered output...ugh. no. this is a really unfortunate intermixing of data with presentation; the presentation layer should know precisely what data its referencing, rather than having to fall back on this global, wildly oversensitive strategy. this is what you get when you mix data with presentation.

Either you haven't actually read the patch or I don't understand this comment at all :)

Of course the presentation layer knows what data is referencing, that's how it gathers the cache tags. The snippet you pasted is only used for cache invalidation.

Wim Leers’s picture

FileSize
925 bytes
40.31 KB

#141:

1. about the render cache entry for e.g. a node containing the cache tags for all of its referenced entities.
So you'd like to see render cached entries for each individual thing, e.g. the taxonomy terms referenced by a node, and then when that node gets rendered, it just retrieves those render cache entries, assembling them as part of the rendered node.
I think this is an example of where normalization seems to make sense. But it doesn't. It'd make sense if the presentation of referenced entities were the same everywhere. But it is not. Not only is the presentation different that the "full" entity view, but it's also entirely possible to use the theme layer to customize the presentation of referenced entities to suit the particular presentation needs of this view mode of this entity.
So what you're saying is nice in theory, but not useful/achievable in practice, I'm afraid.
2. cache tag clearing
*This* is where I believe the problem lies. See below.

So it seems almost everybody is confused by the changed() and relatedEntities(). Note that they have been changed/fixed, so earlier comments in the issue are not relevant anymore — I got confused by this too.

In an attempt to bring everybody on the same page, I stepped through a whole bunch of scenarios that will hopefully make cache tag clearing the situation more clear.

The render cache entries for the listed entities will be reset. In each case, we want to make sure that simply all referenced entities are listed — that is the goal of this issue.

Value of $related_entity_ids in Entity::changed() when creating or editing a …

1. Term

	$related_entity_ids = array(
		'taxonomy_term' => array(<TERM_ID> => TRUE),
	);

2. "article" Node, no tags, no comments yet

	$related_entity_ids = array(
		'node' => array(<NODE_ID> => TRUE),
		'user' => array(<USER_ID> => TRUE),
	);

3. "article" Node, one tag, no comments yet

	$related_entity_ids = array(
		'node' => array(<NODE_ID> => TRUE),
		'user' => array(<USER_ID> => TRUE),
		'taxonomy_term' => array(<TERM_ID> => TRUE),
	);

4. "article" Node, two tags, no comments yet

	$related_entity_ids = array(
		'node' => array(<NODE_ID> => TRUE),
		'user' => array(<USER_ID> => TRUE),
		'taxonomy_term' => array(
			<TERM_ID_1> => TRUE,
			<TERM_ID_2> => TRUE,
		),
	);

5. Comment on Node that references two Terms

	$related_entity_ids = array(
		'comment' => array(<COMMENT_ID> => TRUE),
		'node' => array(<NODE_ID> => TRUE),
		'user' => array(<USER_ID> => TRUE),
	);

(Note that the the Terms referenced by the Node are not listed, which is correct.)

6. Comment on Comment (i.e. comment reply) on Node that references two Terms

	$related_entity_ids = array(
		'comment' => array(
			<COMMENT_ID> => TRUE,
			<PARENT_COMMENT_ID> => TRUE,
		),
		'node' => array(<NODE_ID> => TRUE),
		'user' => array(<USER_ID> => TRUE),
	);

(Note that the the Terms referenced by the Node are not listed, which is correct.)

7. "article" Node, two tags, has one comment

	$related_entity_ids = array(
		'node' => array(<NODE_ID> => TRUE),
		'user' => array(<USER_ID> => TRUE),
		'taxonomy_term' => array(
			<TERM_ID_1> => TRUE,
			<TERM_ID_2> => TRUE,
		),
	);

(Note that the Comment on the Node is not listed.)

Conclusion

Everything is working as intended AFAICT.

BUT!

Case 1 makes sense.

But in case 2: why the User? Because any "nodes by User X" listing will only have cache tags for the specific nodes (e.g. node:x, node:y) in the listing as well as the user's cache tag (user:a).

Concern: this will clear any render cache entry by that user, the majority of which probably has not changed. I.e. many false positives. To reduce false positives, we could introduce " references " cache tags, so: "user:a references node:article". If we'd then clear that cache tag, we'd reduce the number of false hits significantly.

In other words: more granularity could reduce false positives. We can do that by not only having cache tags for a single entity, but also having cache tags for relationships. Because only those things that depend on that relationship will be affected.

Similarly in case 3: Not clearing the "user:a" tag but clearing "user:a references node:article" would be better.
And not clearing the "term:b" tag but clearing "term:b references node:article".

Similarly in cases 4–7.

Thus: we can reduce false positives, but that should be a follow-up issue, because we have to establish clear rules for that. Preferably it'd be auto-generated somehow (maybe as part of EntityQuery?), to prevent human errors.


I did fix one bug that I reported: #138.2 — otherwise the results wouldn't have been correct.

amateescu’s picture

How about KISS rather than overcomplicating everything with that "references" cache tag? Anyway, as long as it's discussed in a followup, I won't complain.

sdboyer’s picture

Status: Needs work » Needs review

changing status so Wim's patch gets reviewed.

re: #142 - yeah, i chose a weird snippet to focus on for that comment. it was at least two layers removed from my actual objection, sorry. i blame the several beers i had just imbibed :P

@Wim Leers -

I think this is an example of where normalization seems to make sense. But it doesn't. It'd make sense if the presentation of referenced entities were the same everywhere. But it is not. Not only is the presentation different that the "full" entity view, but it's also entirely possible to use the theme layer to customize the presentation of referenced entities to suit the particular presentation needs of this view mode of this entity.

yeah, my objection here is really outside the scope of this issue - i dislike view modes being tightly bound with the entity object itself, and would prefer that they exist in a separate (presentational) layer...where we'd have essentially the same logic as what's here. i should have bitten my tongue.

your explanation in #143 is good, though - i had been under the impression that the clearages were more pervasive than they actually are. seems like they're actually much more targeted, which is great.

so, carry on, +1.

Wim Leers’s picture

#144:

How about KISS rather than overcomplicating everything with that "references" cache tag? Anyway, as long as it's discussed in a followup, I won't complain.

Indeed, KISS here. I merely explained how it could be more efficient, but it should only be done in a follow-up, not here.


#145: Yay — glad to read #143 was not in vain.


SO: the remarks in #138 should be addressed, then this should be RTBC.

catch’s picture

With updating a user clearing the node, yes that's tricky. Often the only thing about a user that appears on the node is the username and maybe the image, but we have no way to link changes to specific fields with invalidating the tags (unless you removed the tag in custom code then added it back conditionally).

On the other hand, if a user is deleted and their nodes get reassigned to anonymous, or they change their username and/or the path alias to their profile page is updated, then you definitely want all the nodes they ever posted invalidated - and this is still considerably better than the Drupal 7 equivalent.

While I'm here a reminder to me that we want to add cache tags to the page cache (based on all the cache tags in all the cached items for that request) so we can clear that the same way - but that's a follow-up enhancement once this is in.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -354,7 +355,9 @@ public function getTranslationLanguages($include_default = TRUE) {
   public function save() {
-    return \Drupal::entityManager()->getStorageController($this->entityType)->save($this);
+    $return = \Drupal::entityManager()->getStorageController($this->entityType)->save($this);
+    $this->changed();
+    return $return;
   }

As discussed, it's not safe to override save(). See #2078517: Document that entity business logic belongs in preSave(), postSave(), preDelete() and postDelete(), not save()/delete(). Let's call this in postSave() and postDelete(). We might need to ensure that existing implementations of those methods call the parent implementation.

msonnabaum’s picture

FileSize
4.8 KB
40.31 KB

New patch that resolves some merge conflicts and addresses some of the concerns above.

I strongly dislike the idea that we can't guarantee that save() and delete() get called on the entity, but I went ahead and made the changes in #148 since anything else will require entity api refactoring.

I changed relatedEntities to referencedEntities. The original was named without realizing that all related entities are now entity references, so I agree this makes more sense.

I didn't change entity_get_info to EntityManager::getDefinitions(). Really, that data should be passed in to the render controller, we don't need to add a reference to the manager here.

The name conflict with changed() and onChange() is troublesome, but I don't quite know what to do about it. Typed data is just taking a useful method name from a subclass yet again. Any name I think of is worse and less descriptive. I honestly don't care though at this point, so whatever someone else wants to come up with I'll probably be ok with.

msonnabaum’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1605290-149.patch, failed testing.

Wim Leers’s picture

#1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user got committed, which means the hacks/work-arounds/limitations in this patch for comments can be removed; full entity render caching should now be possible! :)

catch’s picture

I don't think that's true yet, there's still edit/delete links on the comments themselves.

Wim Leers’s picture

amateescu just pointed that out on Twitter too. Stupid that I'd forgotten about that :( I hadn't heard/read about that in months, that's probably why. I actually can't find an issue for it — can you point me to it?

catch’s picture

I can't find it either. If I remember correctly there was an issue to add back contextual links to comments which was discussing all the links and how viable it'd be to use contextual links for them, but it's not coming up in search.

amateescu’s picture

Assigned: Unassigned » amateescu

@Wim Leers, I don't think there's an issue for that.. yet. I googled a bit and I found where this topic was discussed between you and @catch: #1882482-105: [meta] Unify editing approaches in Drupal.

Anyway, I'll spend some quality time on rerolling and fixing this patch.

catch’s picture

That's the one!

amateescu’s picture

Assigned: amateescu » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
13.18 KB
4.2 KB
54.28 KB

The first interdiff has fixes for the test failures in #149 and the second one is for test coverage.

I added separate tests for drupal_render_collect_cache_tags() and for the whole entity render cache flow. Also found a bug in the process where new (unsaved) entities were generating a cache entry, and they shouldn't since they might not have an ID yet.

I think we have everything now and we're ready for final reviews. One thing that still bugs me is the changed() method, which doesn't quite fit the scope of this issue.

Status: Needs review » Needs work

The last submitted patch, 1605290-158.patch, failed testing.

amateescu’s picture

Assigned: Unassigned » catch
Status: Needs work » Needs review
FileSize
4.12 KB
56.35 KB

And last round of fixes. All this wouldn't be necessary if we wouldn't load entities when we just need their ids.

Edit: I also opened a spin-off issue because this patch fixes it only for taxonomy terms: #2087995: All entity post*() and pre*() methods should call their parent implementation

Anonymous’s picture

using drupal_static() and return flags in drupal_render_collect_cache_tags() looks a bit suspect. perhaps something like this:

function drupal_render_collect_cache_tags($elements, $tags = array()) {
  if (isset($elements['#cache']['tags'])) {
    foreach ($elements['#cache']['tags'] as $namespace => $values) {
      if (is_array($values)) {
        foreach ($values as $value) {
          $tags[$namespace][$value] = $value;
        }
      }
      else {
        if (!isset($tags[$namespace])) {
          $tags[$namespace] = $values;
        }
      }
    }
  }
  if ($children = element_children($elements)) {
    foreach ($children as $child) {
      $tags = drupal_render_collect_cache_tags($elements[$child], $tags);
    }
  }
  return $tags;
}

in drupal_render_collect_cache_tags(), why do we care about 'first non-array value for a tag namespace wins'? reading the code, it seems we care, but i have NFI why.

also, why do we munge arrays for a given tag namespace into arrays with identical keys and values? reading the code, it seems we care, but i have NFI why. we test for that type of array, but we pass in 'tags' => array('render_cache_tag_child' => array(1 => 1, 2 => 2)). if the munging is important, perhaps pass 'tags' => array('render_cache_tag_child' => array(1, 2)) and test for array((1 => 1, 2 => 2)) in the output.

+function entity_view(EntityInterface $entity, $view_mode, $langcode = NULL, $reset = FALSE) {
...
+function entity_view_multiple(array $entities, $view_mode, $langcode = NULL, $reset = FALSE) {

why do we need $reset here? if calling code wants to reset the cache, they should call resetCache() on the render controller themselves IMO.

+    $view_mode_is_cacheable = !isset($this->viewModesInfo[$view_mode]) || (isset($this->viewModesInfo[$view_mode]) && $this->viewModesInfo[$view_mode]['cache']);
+    if (!$entity->isNew() && !isset($entity->in_preview) && $this->entityInfo['render_cache'] && $view_mode_is_cacheable) {

put $view_mode_is_cacheable first in the condition, seeing as we went to all the trouble to calculate it...

dixon_’s picture

amateescu’s picture

FileSize
1.7 KB
54.95 KB

Re #161:

  1. drupal_render_collect_cache_tags(): I assume the drupal_static() usage is there to be inline with drupal_render_collect_attached(), but I don't know the historic reasons for that.
  2. Fixed the test.
  3. why do we need $reset here? if calling code wants to reset the cache, they should call resetCache() on the render controller themselves IMO.

    Because entity_load() has it :)

  4. Fixed.

Patch size is smaller because the Entity BC decorator was removed in the meantime.

amateescu’s picture

FileSize
3.34 KB
54.64 KB

@catch said in IRC that the reason for drupal_static() is probably bogus (and related to some crazy stuff going on with the collection at the time) so I replaced it with the code from #161 which is nice indeed.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

coolio, i think this is RTBC.

Anonymous’s picture

Status: Reviewed & tested by the community » Needs review

woops. we need some comments explaining why we mess with the tags array.

after that, rtbc.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
688 bytes
54.76 KB

Done!

Wim Leers’s picture

I thought in #152 that having fixed #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user, this issue could finally enable render caching on nodes & comments. But sadly, comment ops links prevent that. For stopping comment links from breaking render cache, see #2090783: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user.

So this patch only brings render caching for other entities, like Taxonomy Terms. That's still a big step forward though.

+1 for RTBC on overall status of this patch.


I do have some remarks though: one potential problem (which could be fixed in a follow-up), but mostly @todo nitpicks. So I'll leave this as RTBC to let a committer decide — I don't want to hold up this patch.

  1. +++ b/core/lib/Drupal/Core/Entity/EntityRenderController.php
    @@ -62,6 +91,23 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
    +    // @todo Fix the tests that require non-existent view modes and remove the
    +    // isset() checks below.
    

    It'd be better if this linked to an issue.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityRenderController.php
    @@ -62,6 +91,23 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
    +          $this->entityType . '_view' => TRUE,
    

    Shouldn't the bundle be included here? The per-view mode entity display config is stored in files like entity.display.node.article.teaser.yml. Right now, cache entries with a different view mode than the one affected (changed) would be deleted/invalidated as well.

    (Discovered while working on a talk about D8 performance — see http://wimleers.com/talk-really-fast-drupal-8/.)

  3. +++ b/core/modules/block/lib/Drupal/block/BlockRenderController.php
    @@ -62,4 +62,10 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    +    // @todo Move block render caching logic to this controller?
    

    I'm not sure if this should be solved here or elsewhere, but it feels strange to add a question as a todo.

  4. +++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.php
    @@ -219,6 +219,7 @@ public function getReplyForm(Request $request, NodeInterface $node, $pid = NULL)
    +        unset($build['comment_node']['#cache']);
    
    +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
    @@ -36,6 +36,7 @@
    + *   render_cache = FALSE,
    
    +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
    @@ -40,6 +40,7 @@
    + *   render_cache = FALSE,
    

    Should get a @todo pointing out that fixing https://drupal.org/node/2090783 should enable comment & node render caching?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
1.71 KB
54.35 KB

Rerolled and adressed #168:

  1. I tried to remove the isset() checks in a patch testing issue and I remebered why they're necessary: because 'default' is not an actual view mode. Replaced the @todo with a proper comment.
  2. This was discussed way back in #6 (and #7) and we decided that it would mean carrying too many cache tags on each page request for very little gain.
  3. Removed the @todo because BlockRenderController is not our concern here.
  4. There are at least 3 people (@catch, me and you) who will always have this in the back of our heads, so I'm pretty sure it won't be forgotten :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1605290-170.patch, failed testing.

Wim Leers’s picture

Thanks for the clarifications. I think the two exceptions are unrelated to this patch, so re-testing.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: -D8 cache tags, -D8 cacheability

#170: 1605290-170.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1605290-170.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

#170: 1605290-170.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D8 cache tags, +D8 cacheability

The last submitted patch, 1605290-170.patch, failed testing.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
55.23 KB

Rerolled and fixed those new exceptions.

amateescu’s picture

FileSize
1.43 KB

And here's the correct interdiff :/

catch’s picture

Thought it was strange that the comment change fixed the exception ;)

I'll try to commit more or less as soon as it's back green from the bot. Has been RTBC for five days excepting re-rolls.

Status: Reviewed & tested by the community » Needs work
Issue tags: -D8 cache tags, -D8 cacheability

The last submitted patch, 1605290-177.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

#177: 1605290-177.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1605290-177.patch, failed testing.

Fabianx’s picture

Status: Needs work » Needs review
Issue tags: +D8 cache tags, +D8 cacheability

#177: 1605290-177.patch queued for re-testing.

catch’s picture

Status: Needs review » Needs work

The last submitted patch, 1605290-177.patch, failed testing.

Anonymous’s picture

in case this helps someone who has a clue about Entity/Fields API, the fails happen here:

/**
 * Implements hook_entity_prepare_view().
 */
function entity_test_entity_prepare_view($entity_type, array $entities, array $displays) {
  // Add a dummy field item attribute on field_test_text if it exists.
  if ($entity_type = 'entity_test_render') {
    foreach ($entities as $entity) {
      if ($entity->getPropertyDefinition('field_test_text')) {
        foreach ($entity->get('field_test_text') as $item) {
          $item->_attributes += array('data-field-item-attr' => 'foobar');
        }
      }
    }
  }
}

leads to this:

PHP Fatal error: Unsupported operand types in /var/www/8-drupal/webroot/core/modules/system/tests/modules/entity_test/entity_test.module on line 510

i guess we had some API change and we need a new incantation now?

Anonymous’s picture

derp derp. i missed the obvious bug here:

if ($entity_type = 'entity_test_render') {

is missing a '=', and was introduced over in #1778122-120: Enable modules to inject attributes into field formatters, so that RDF attributes get output. i've posted a patch there, though unfortunately it doesn't fix the fail here.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.93 KB
55.68 KB

here's an updated patch that fails in a way that someone better able to shave entity system yacks than i may be able to fix.

Status: Needs review » Needs work
Issue tags: -D8 cache tags, -D8 cacheability

The last submitted patch, 1605290-188.patch, failed testing.

Fabianx’s picture

Status: Needs work » Needs review

#188: 1605290-188.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D8 cache tags, +D8 cacheability

The last submitted patch, 1605290-188.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
56.37 KB

Figured out the problem with @yched.

Edit: deleted the bad rebase comment, ignore me :)

Status: Needs review » Needs work

The last submitted patch, 1605290-192.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
582 bytes
56.48 KB

It was a bad merge after all, there's one thing that I needed to add back in the test.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Latest interdiff was lost in a re-roll and was already there before. The actual fix for EntityViewControllerTest looks good.

catch’s picture

Title: Enable entity render caching with cache tag support » Change notice: Enable entity render caching with cache tag support
Priority: Critical » Major
Status: Reviewed & tested by the community » Active

Very happily committed/pushed to 8.x.

Could use a change notice - how this gets enabled/disabled for entity types and to introduce the change in general. We should also cross-reference [#2086767] for how to not break this.

amateescu’s picture

Status: Active » Needs review

YAY!

Here's the change notice: https://drupal.org/node/2095167

jibran’s picture

Title: Change notice: Enable entity render caching with cache tag support » Enable entity render caching with cache tag support
Assigned: catch » Unassigned
Priority: Major » Critical
Status: Needs review » Fixed

Yay!! Thank you everybody for working on this. Thanks @amateescu for change notice it makes a lot of sense.

Wim Leers’s picture

Great change notice, amateescu — thanks for your hard work on this!

Anonymous’s picture

Status: Fixed » Needs review
FileSize
1.82 KB

looks like we accidentally landed the $reset param to entity_view_multiple and entity_view ?

attached patch fixes that.

jibran’s picture

Can we please do it in followup issue? We are already at #201.

+++ b/core/includes/entity.inc
@@ -623,14 +623,11 @@ function entity_render_controller($entity_type) {
   if ($reset) {

I think we have to remove this as well.

Anonymous’s picture

FileSize
1.89 KB

woops, good catch.

amateescu’s picture

Status: Needs review » Fixed

Nope, that was not by accident. See #163 and it's also in the change notice.

Anonymous’s picture

ok, i'll open another issue. "because entity_load() has it" is complete rubbish IMO.

a load call shouldn't reset a cache, period.

yched’s picture

I just noticed the Entity::referencedEntities() method that got added here. It currently relies on loading entities in separate single loads, which sounds pretty bad ?
#2073661: Add a EntityReferenceField::referencedEntities() method is adding a method on ER FieldItemList class to (multi)load the entities referenced in the field.
It looks like we should:
- unify method names : referencedEntities() ? targetEntities() ?
- make the method in Entity rely on the one in ERFieldItemList ?

andypost’s picture

Filed critical follow-up #2099105: Clean-up render cache when permission changes
Bacase render cache should care about permissions

catch’s picture

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

Wim Leers’s picture

Enabling node render caching is being done at #2151459: Enable node render caching. The many blockers to be able to do that, are listed in that issue.

YesCT’s picture

Issue summary: View changes
Related issues: +#1743590: Isolated Block Rendering

#1743590: Isolated Block Rendering was postponed on this. since this is done, opening that one.

yched’s picture

yched’s picture