There are two ways to render an entity through the Entity API

Either do

    $nodes = \Drupal::entityTypeManager()->getStorage('node')->loadMultiple([1]);
    $build['nodes'] = \Drupal::entityTypeManager()->getViewBuilder('node')->viewMultiple($nodes, 'teaser');

or

    $nodes = \Drupal::entityTypeManager()->getStorage('node')->loadMultiple([1]);
    foreach ($nodes as $node) {
      $build['nodes'][] = \Drupal::entityTypeManager()->getViewBuilder('node')->view($node, 'teaser');
    }

The problem with viewMultiple() is that render cache is bypassed in a subtle way. Even though you won't see any changes, the pre_render callback will call all entity build/view/alter hooks .. so if anything is expensive going on there, well, you're doing it again even though you have a hit.

I've attached a simple module which you can use to see the problem.

1. turn on and make sure you have a node with id 1
2. turn off page and dynamic page cache, make sure no other caching mechanism is in place
3. surf to /test
4. check that the entity is render cache
5. open testing.module and uncomment the 'die' line (line 9)
6. reload page, everything is fine
7. open src/TestController and comment the viewMultiple() line and uncomment the foreach
8. reload: wsod ..

CommentFileSizeAuthor
#58 interdiff_53-58.txt1.85 KBPrem Suthar
#58 2843565-58.patch10.84 KBPrem Suthar
#54 2843565-53-interdiff.txt504 bytesBerdir
#54 2843565-53.patch10.72 KBBerdir
#51 interdiff_50-51.txt2.43 KBranjith_kumar_k_u
#51 2843565-51.patch10.7 KBranjith_kumar_k_u
#50 interdiff_44-50.txt3.42 KBAndyF
#50 2843565-50.patch10.68 KBAndyF
#41 2843565-41.patch10.64 KBswentel
#41 2843565-41-interdiff.txt705 bytesswentel
#39 2843565-38.patch8.84 KBmarthinal
#34 2843565-34.patch10.05 KBmarthinal
#24 2843565-24-interdiff.txt1.74 KBBerdir
#24 2843565-24.patch10.09 KBBerdir
#23 2843565-23-interdiff.txt2.53 KBBerdir
#23 2843565-23.patch9.98 KBBerdir
#9 2843565-9-test-only.patch4.72 KBBerdir
#9 2843565-9.patch7.7 KBBerdir
#6 2843565-6-interdiff.txt1.7 KBBerdir
#6 2843565-6.patch6.18 KBBerdir
#3 2843565-3-fail.patch4.48 KBswentel
testing.tar_.gz1.32 KBswentel
#44 2843565-44.patch13.52 KBswentel
#44 2843565-44-interdiff.txt3.56 KBswentel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel created an issue. See original summary.

swentel’s picture

Assigned: Unassigned » swentel

working on test fail patch now

swentel’s picture

Status: Active » Needs review
FileSize
4.48 KB

So stepping through the render pipeline was already painful to uncover this bug, turns out writing a test isn't fun either ..
So very basic web test for now, which is simple and elegant in a way to prove the bug.

swentel’s picture

Assigned: swentel » Unassigned

Status: Needs review » Needs work

The last submitted patch, 3: 2843565-3-fail.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.18 KB
1.7 KB

I guess I know what the problem is.

viewMultiple() sets a #pre_render *outside* of the entities, but the render caches are inside. So we call that #pre_render even on cache hits.

We're basically lying out ourself here, we discussed and accepted the drawback that we lose multiple rendering by using render caching we just didn't really want to believe it :)

I guess the good thing is that views doesn't actually use viewMultiple() so views aren't affected and due to that, not much other than custom code probably is..

Here's a quickfix, passes the new test. We could consider to deprecate buildMultiple, implementing the same in build would only require 50% of the code or so wih all that optimization to group by view mode and so on. One problem is that by not using buildMultiple() anymore, we kind of make this an API change, entity view builders might override the method and expect it to be called. I know we do that in Paragraphs atm.

Berdir’s picture

\Drupal\block_content\BlockContentViewBuilder is actually an example that we break with this change, but not sure what else we should do.

As a start, we could actually implement it like that (without a #pre_render) by default if render_cache is disabled for an entity type.

Also looking at viewMultiple() calls, aggregator calls it, comments call it, so we should be able to reproduce with comments in core, interesting that we didn't notice this earlier. But that's it, nothing else outside of tests.

swentel’s picture

Quickly tested the patch, works great.

Briefly talked with berdir in IRC, we both feel to to go for a minimal fix to get this in as quickly as possible and create a follow up to consider deprecating/dropping buildMultipel(). This is especially a great fix for comments for instance.

Berdir’s picture

Ok, updated BlockContentViewBuilder. It still worked, as it still explicitly calls buildMultiple(), just didn't properly unset the #pre_render callback anymore, so probably did it twice.

Also cleaned up the test and switched to use entity_test. Was thinking about using a kernel test, but I think it's actually good to have an actual web test for this to avoid anything messing with the behavior by doing shortcuts/workarounds.

Not too much point in an interdiff as renaming/changing shuffled around pretty much everything in the test. Also new test-only patch to ensure that it still fails.

Status: Needs review » Needs work

The last submitted patch, 9: 2843565-9-test-only.patch, failed testing.

jibran’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -108,16 +108,7 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    -    // The default ::buildMultiple() #pre_render callback won't run, because we
    -    // extract a child element of the default renderable array. Thus we must
    -    // assign an alternative #pre_render callback that applies the necessary
    -    // transformations and then still calls ::buildMultiple().
    

    Should we move this as well? It seems like important bit of information.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -126,7 +117,6 @@ public function view(EntityInterface $entity, $view_mode = 'full', $langcode = N
    -      '#pre_render' => array(array($this, 'buildMultiple')),
    

    Can we please add a @todo with follow up?

Berdir’s picture

Status: Needs work » Needs review

1. No, because this information is simply no longer relevant. It was a hack we needed because we weren't really thinking, now there is no hack anymore and single vs multiple viewing is the same.

2. Possibly, but I'm not exactly sure yet about what that todo would even be. BlockContentViewBuilder is one obvious example why dropping buildMultiple() isn't an option for Drupal 8 anyway.

Lets wait for some more reviews, failed/passed as expected I just messed up the patch order.

yched’s picture

we discussed and accepted the drawback that we lose multiple rendering by using render caching we just didn't really want to believe it :)

Yup, I remember @Berdir pointing out some time ago that, by moving the build step to #pre_render to benefit on render cache, we basically lost the benefits of multiple-entity view building. I don't remember that we identified that stray #pre_render to be problematic though, good catch :-/

We could consider to deprecate buildMultiple, implementing the same in build would only require 50% of the code or so wih all that optimization to group by view mode and so on

Not only that, but the whole FormatterInterface::prepareView() step becomes moot, since its sole purpose was to allow front-loading of all EntityRefs across the multiple entities being viewed, rather than entity-by-entity (best we can have is multiple-loading of EntityRef fields of cardinality > 1 within each entity, for each entity)

EntityReferenceFormatterBase provides a default implementation for that, for all EntityRef (and subclassed field types : file, image, media, paragraphs) formatters to reuse, so arguably the existence of FormatterInterface::prepareView() didn't require any actual work from contrib/custom formatters to implement. I don't know if some formatter somewhere had to override that, I'm not sure there is a case (the one that comes to mind is entity_reference_revisions, but it seems they sidestepped the question)

Still, quite some convoluted multiple-handling logic between EntityViewBuilder::viewMultiple(), EntityViewDisplay::buildMultiple() and EntityReferenceFormatterBase, that exists for nothing :-)

But yeah, doesn't look like something that can be removed in 8.x...

yched’s picture

On the patch itself, I'm too much out of the loop atm to offer a valid opinion - just :

So yeah, we probably have to keep the multiple-entity logic in EntityViewBuilder::buildMultiple() to avoid potential BC breaks on subclasses, and we also probably have to keep the method public.

But if it is in fact only ever called by ::build() with degenerate single-entity cases, it might be worth a comment explaining the historical reason for the "Dude, why so convoluted ?" WTF :-)

yched’s picture

On the overall "entity render cache kills multiple-entity view building" issue :

We kind of accepted that tradeoff after we discovered it, on the grounds that the multiple-view optimization we lost only impacted render-cache misses. But that's assuming there is a reasonably high hit ratio on entity render cache :-)

If I'm not mistaken, that assumes :
- uncacheable / high-granularity formatters do the right job of being placeholdered
- the entities' cacheability (and notably the cacheability of 'view' AccessResults) is not too granular either.

Also, EntityViewBuilder only leverages render cache to begin with if "$this->isViewModeCacheable($view_mode) && $this->entityType->isRenderCacheable())", so this leaves out a few cases :-)

So the tradeoff is probably alright in the 80% cases, but I'm not sure we can safely say it's always good enough ?

OTOH :

- Retaining the multiple-view optimization for the cache misses, would require enriching the render cache API and code flow to allow something like : "your $element's children each have a ['#cache']['keys'], I got cache hits for N of those, here's a callback so that you can do something about the remaining ones as a whole".

That's me doing some heavy handwaving here, with no real clue on how it might actually look like and how easy/awful that might be :-), but feature-wise this doesn't seem totally unreasonable ?

- Views not using EVB::viewMultiple() anyway sure hinders any efforts in that direction :-/. I can't say I remember why Views only does single-entity view() though.

Berdir’s picture

We've discussed before about an ability to do a cache getMultiple() if there are multiple #cache definitions in multiple children. That would kind of go into the same direction. I'm a bit worried about introducing even more complexity into the render cache system though.

So if we do want to consider that, then I think we should at least do it step by step. Fix this, then cache get multiple for hits, then handling of multiple misses?

And then we're also back to #12.2. A @todo, but what exactly are we going to write? If we are considering to make it useful again, then we don't need a todo to remove it.

Maybe we should just write down facts about how it is now, without already trying to define a plan/idea for how we want it to be? That might be enough for this issue?

yched’s picture

So if we do want to consider that, then I think we should at least do it step by step. Fix this, then cache get multiple for hits, then handling of multiple misses?

Oh totally, that sounds like the right thing to do right now. The issue at hand here should get fixed regardless of hypothetical render cache enhancements to come.

(I'd tend to think "multiple-render-cache-get for children" and "single build callback for multiple children cache misses" could be worth designing hand-in-hand even if actually done in separate steps, as they indeed seem related, at least to figure out what shape the API could look like)

Maybe we should just write down facts about how it is now, without already trying to define a plan/idea for how we want it to be? That might be enough for this issue?

+1. Just clarifying that the multiple-view code is kept around because BC even though we only single-view at the moment sounds good enough.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Ok, let's see what core committers think of it.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record
+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -108,16 +108,7 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
-    $build_list = $this->viewMultiple(array($entity), $view_mode, $langcode);
-
-    // The default ::buildMultiple() #pre_render callback won't run, because we
-    // extract a child element of the default renderable array. Thus we must
-    // assign an alternative #pre_render callback that applies the necessary
-    // transformations and then still calls ::buildMultiple().
-    $build = $build_list[0];
-    $build['#pre_render'][] = array($this, 'build');
-
-    return $build;
+    return $this->viewMultiple(array($entity), $view_mode, $langcode)[0];

+++ b/core/modules/block_content/src/BlockContentViewBuilder.php
@@ -14,19 +14,14 @@ class BlockContentViewBuilder extends EntityViewBuilder {
-    unset($build_list['#pre_render'][0]);
+    foreach ($entities as $key => $entity) {
+      unset($build_list[$key]['#pre_render'][0]);
+    }

This change has tricky BC implications. Not sure if there is a BC way to go so that custom implementations of viewMultiple or view don't have to change. Regardless we probably need a CR here.

swentel’s picture

+++ b/core/modules/system/src/Tests/Entity/EntityRenderCacheHookTest.php
@@ -0,0 +1,47 @@
+    $this->drupalGet('test-view/' . $entity->id());
+    $this->assertNoText('custom hook invocation called');

Maybe we should add a positive assert here as well, like check that we're actually seeing content of the entity ?

swentel’s picture

Shy attempt at a change record at https://www.drupal.org/node/2846946 - needs a bit more verbosity obviously.

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

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
9.98 KB
2.53 KB

Updated the documentation.

I added a @todo without an explicit issue reference, but since we do not actually know yet what we want/can/will do exactly, this seems like the best choice to me.

Berdir’s picture

Added more asserts and lots of verbosity to the change record.

Re #19: Yes, this is a bit a problem, but I'm not aware of a single other example in contrib or custom code doing what BlockContentViewBuilder does or anything else with #pre_render. Plus, even if you would do something like that, it is very likely that it would mostly still work, e.g. no tests were failing for BlockContent, we were just building twice.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Ok, looks very good to me!

  • catch committed 3f9882f on 8.4.x
    Issue #2843565 by Berdir, swentel, yched: getViewBuilder('node')->...

  • catch committed eca7f3a on 8.3.x
    Issue #2843565 by Berdir, swentel, yched: getViewBuilder('node')->...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

Also opened #2852164: Multiple render caching to preserve the discussion.

jibran’s picture

Just published the change record https://www.drupal.org/node/2846946.

Berdir’s picture

Status: Fixed » Needs work

Aw. We noticed a problem with this in our project. this causes comment view builder to build each comment separately. But with nested comments, in relies on them being all built together.

Adding the same change as in BlockContentViewBuilder fixes it:


  /**
   * {@inheritdoc}
   */
  public function viewMultiple(array $entities = array(), $view_mode = 'full', $langcode = NULL) {
    $build_list = parent::viewMultiple($entities, $view_mode, $langcode);
    unset($build_list['#pre_render'][0]);
    return $this->buildMultiple($build_list);
  }

As I mentioned once here, I was wondering if we should actually do that by default if entity types are uncacheable, but the trick with comments is that they are only uncacheable if threading is enabled.

@alexpott is going to revert this, then we can fix it with test coverage. Or figure out something else for better BC.

  • alexpott committed 78068b6 on 8.4.x
    Revert "Issue #2843565 by Berdir, swentel, yched: getViewBuilder('node...
alexpott’s picture

Also reverted this on 8.3.x - see 49092a8

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

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

marthinal’s picture

yogeshmpawar’s picture

Status: Needs work » Needs review
marthinal’s picture

Status: Needs review » Needs work

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

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

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

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

marthinal’s picture

FileSize
8.84 KB

Rerolled

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

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

swentel’s picture

The last submitted patch, 41: 2843565-41.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

swentel’s picture

So I get the drupal_set_message fail, but the other ones are curious, checking.

swentel’s picture

fix the tests

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

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

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

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

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

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

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

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

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

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

AndyF’s picture

Reroll for 9.x. It removes the legacy test changes. Thanks!

ranjith_kumar_k_u’s picture

Fixed CS errors

Wim Leers’s picture

Status: Needs review » Needs work

Thanks! 🙏

Not yet passing tests though 😅 Bit more work needed!

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

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

Berdir’s picture

Version: 9.5.x-dev » 9.4.x-dev
Status: Needs work » Needs review
FileSize
10.72 KB
504 bytes

Incompatible test module is what caused this to fail so badly.

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

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

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

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

nod_’s picture

Status: Needs review » Needs work

Patch fails lint on 10.1.x

Prem Suthar’s picture

Solved the #54 Custom CMD Failed Patch.
Also, Add the Interdiff Of the 53-58 patches.

Prem Suthar’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs issue summary update

Test coverage appears to be there (good)
Issue summary should be updated though with proposed solution, remaining tasks, any api changes, etc.

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

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