views\Plugin\views\field\Field::getItems() calls a field fromatter, but ditches the top-level array and only renders children piecemeal (for example in separate cells in a table).
It takes care of carrying of cacheability metadata, but not #attached assets.

Meaning, as reported in a comment in the change notice for #attached :

Note that when adding #attached assets in a field formatter, it needs to be #attached to each element individually, not once for the whole elements() array. Otherwise the assets will not be added when the formatter is invoked as part of a view

Which is fairly counter-intuitive :-)

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug: Formatters using #attached assets break when used in field-by-field views
Issue priority Major: The bug only affects fields rendered in Views, not in regular $entity->view(). It's thus very easy to write a formatter that seems to work in the typical test cases but breaks when used in Views
Disruption None, internal fix.
CommentFileSizeAuthor
#26 2496039_Views_formatter_attached-26.patch10.37 KByched
#18 interdiff.txt1.72 KByched
#18 2496039_Views_formatter_attached-18.patch10.37 KByched
#16 2496039_Views_formatter_attached-10.patch9.97 KByched
#16 2496039_Views_formatter_attached-10-test-only.patch8.13 KByched
#10 2496039_Views_formatter_attached-10.patch9.97 KByched
#10 2496039_Views_formatter_attached-10-test-only.patch8.13 KByched
#1 2496039_Views_formatter_attached-1.patch1.84 KByched

Comments

yched’s picture

yched commented
Status: Active » Needs review
StatusFileSize
new 2496039_Views_formatter_attached-1.patch1.84 KB

Looks like we just need to merge BubbleableMetadata instead of CacheableMetadata ?

I'm not too fond of the
if (isset($build_list['#cache']) || isset($build_list['#attached']) || isset($build_list['#post_render_cache'])) {
it feels like brittle abstraction leaking about what constitutes a bubbleable property.

I kind of fear the perf impact of unconditionally merging for each field in each row, tough :-/

dawehner’s picture

dawehner
Primary language German
commented

Is that the only property we care about? I'm curious whether we could sort of solve things generically

dawehner’s picture

dawehner
Primary language German
commented

Interesting fix!!

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue tags: +Needs tests

Let's add tests as well.

yched’s picture

yched commented

Turns out #2474121: CacheableMetadata should get BubbleableMetadata's merge/applyTo/createFromRenderArray/createFromObject methods switched from merging BubbleableMetadata to only merging CacheableMetadata, which is basically what the patch reverts. Asked for feedback over there.

yched’s picture

yched commented

Is that the only property we care about? I'm curious whether we could sort of solve things generically

Hard to tell, but I'd tend to think that if you chose to not render an element but only render children individually, then the only #properties you are interested in keeping are the bubbleable ones ? The other properties from the parent shouldn't affect the rendering of the children, right ?

fabianx’s picture

fabianx commented

This is fine, but needs tests.

#post_render_cache will be hopefully gone soon, then there is only #attached and #cache, which is way more sane.

yched’s picture

yched commented

Thinking about it a bit more, what I think happened is :

- #2099137: Entity/field access and node grants not taken into account with core cache contexts did :
if (isset($parent['#cache'])) {
BubbleableMetadata::createFromRenderArray($parent)->merge($children)->applyTo($children)
}
Meaning : it only bothered about #cache, and used BubbleableMetadata just because CacheableMetadata didn't have the proper create / merge / apply helper methods natively back then.

- #2474121: CacheableMetadata should get BubbleableMetadata's merge/applyTo/createFromRenderArray/createFromObject methods added those missing methods, and since the code snippet above only seemed to care about #cache, it moved it to CacheableMetadata ionstead it now had the create / merge / apply features

- We in fact do want to take care of anything bubbleable (#attached in addition to #cache), so should revert to BubbleableMetadata create / merge / apply, but updating the surrounding if() so that it doesn't only trigger if there's a #cache property

yched’s picture

yched commented

- (yched #1) I'm not too fond of the
if (isset($build_list['#cache']) || isset($build_list['#attached']) || isset($build_list['#post_render_cache'])) {
it feels like brittle abstraction leaking about what constitutes a bubbleable property.
- (Fabianx #7) #post_render_cache will be hopefully gone soon, then there is only #attached and #cache, which is way more sane.

Would be cleaner indeed. What do you guys think of adding BubbleableMetadata::hasMetadata($build) / CacheableMetadata::hasMetadata($build) ?

Tests : sure, but I won't be able to do that today - anyone feel free to beat me to it ;-)

yched’s picture

yched commented
StatusFileSize
new 2496039_Views_formatter_attached-10-test-only.patch8.13 KB
new 2496039_Views_formatter_attached-10.patch9.97 KB

With a test. Interdiff is the test-only.patch

Still interested in feedback from @Fabianx / @Wim Leers about #9 (adding BubbleableMetadata::hasMetadata($build) / CacheableMetadata::hasMetadata($build)) ?

yched’s picture

yched commented

As a side note:

The test formatter attaches 'foo/fake_library' on its render array.
When bubbled to the overall view render array rendering 5 entities, this results in:
$view_build['#attached']['library'] = ['foo/fake_library', 'foo/fake_library', ... 5 times]

I'm probably missing something, but I would have expected the bubbled ['#attached']['library'] to contain a merged de-duplicated list, instead of repeating a library once per child that attaches it ?

yched’s picture

yched commented
Issue tags: -Needs tests
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
+++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -835,11 +835,11 @@ public function getItems(ResultRow $values) {
+      if (isset($build_list['#cache']) || isset($build_list['#attached']) || isset($build_list['#post_render_cache'])) {
+        BubbleableMetadata::createFromRenderArray($build_list)
+          ->merge(BubbleableMetadata::createFromRenderArray($items[$delta]['rendered']))
           ->applyTo($items[$delta]['rendered']);
       }

Let's instead do this:

  1. don't do any isset() checks
  2. move the BubbleableMetadata::createFromRenderArray($build_list) out of the loop
  3. unconditionally merge it with the bubbleable metadata for the current delta

The last submitted patch, 10: 2496039_Views_formatter_attached-10-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 10: 2496039_Views_formatter_attached-10.patch, failed testing.

yched’s picture

yched commented
Status: Needs work » Needs review
StatusFileSize
new 2496039_Views_formatter_attached-10-test-only.patch8.13 KB
new 2496039_Views_formatter_attached-10.patch9.97 KB

@Wim : See #2 - I'm a bit worried about the perf cost of doing the merge for each delta in each column in each result row, while 95% formatters don't have anything to bubble anyway. That's why #2099137: Entity/field access and node grants not taken into account with core cache contexts added that isset() check in the first place before merging, I guess.

Will try to run a couple profiling runs.

For now, reuploading the patches from #10 for the bot hiccup.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

It's going to be render cached anyway, so … the amortized cost would be negligible.

yched’s picture

yched commented
StatusFileSize
new 2496039_Views_formatter_attached-18.patch10.37 KB
new interdiff.txt1.72 KB

With @Wim's proposal from #13.

Profiled a view listing 50 nodes, 10 number fields each (with a formatter that doesn't add any #attached or #cache), I couldn't get xhprof-kit to get consistent diffs (varied between +0.2% and +1.2% cpu, but more consistently towards the former). That was with render cache disabled, I'm fine with calling this a wash when bringing render cache in the mix :-)

The last submitted patch, 16: 2496039_Views_formatter_attached-10-test-only.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

I think these numbers will get much better, once we have the optimization to not perform the heavy merging logic, as it will be much cheaper, but 500 times.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Yep, #2483433: Optimize CacheableMetadata::merge() + BubbleableMetadata::merge() will make that 0.2–1.2% increase (pre-render caching) even smaller.

RTBC+1

yched’s picture

yched commented

Oh, I wasn't aware of #2483433: Optimize CacheableMetadata::merge() + BubbleableMetadata::merge(), so yeah, definitely +1 on the latest approach :-)

yched’s picture

yched commented
Issue summary: View changes

Added beta evaluation summary

yched’s picture

yched commented
Issue summary: View changes

Aaaand it seems I can't write basic HTML

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2496039_Views_formatter_attached-18.patch, failed testing.

yched’s picture

yched commented
Status: Needs work » Needs review
StatusFileSize
new 2496039_Views_formatter_attached-26.patch10.37 KB

Testbot fluke ? Rebased on latest HEAD just in case.

yched’s picture

yched commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Priority: Normal » Major
Status: Reviewed & tested by the community » Fixed

Committed 46e2034 and pushed to 8.0.x. Thanks!

I agree with the beta evaluation this is a major bug.

diff --git a/core/modules/views/tests/modules/views_test_formatter/views_test_formatter.info.yml b/core/modules/views/tests/modules/views_test_formatter/views_test_formatter.info.yml
index 4ac1e65..91658d8 100644
--- a/core/modules/views/tests/modules/views_test_formatter/views_test_formatter.info.yml
+++ b/core/modules/views/tests/modules/views_test_formatter/views_test_formatter.info.yml
@@ -1,4 +1,4 @@
-name: 'Views Test ormatter'
+name: 'Views Test Formatter'
 type: module
 description: 'Provides test field formatters.'
 package: Testing

Fixed on commit.

+++ b/core/modules/views/src/Tests/Handler/FieldFieldTest.php
@@ -266,6 +266,30 @@ public function testSimpleRender() {
+    foreach ($this->entities as $entity) {
+      $expected_attachments['library'][] = 'foo/fake_library';
+      $expected_attachments['drupalSettings']['AttachmentIntegerFormatter'][$entity->id()] = $entity->id();
+    }

This looks funky that foo/fake_library is added multiple times. I guess uniqueness is sorting out later. Not the fault of this patch though.

  • alexpott committed 46e2034 on 8.0.x 
    Issue #2496039 by yched: Formatter's #attached assets are not carried...
yched’s picture

yched commented

This looks funky that foo/fake_library is added multiple times. I guess uniqueness is sorting out later. Not the fault of this patch though

Agreed. I asked about that in #11 above, but got no answer :-)
@wim, what do you think ?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Yes, uniqueness is applied at a later stage: when it is actually rendered. The code building render arrays generally just appends to [#attached][library]. The only way to make it not have duplicates at that stage is by requiring the code building render arrays use a helper function. Which is too late to require at this stage, and goes against the ingrained habits for no good reason AFAICT :)

yched’s picture

yched commented

@wim: Thanks, makes sense. I guess we don't deduplicate each time we merge/bubble up, but rather once just before attaching the actual assets at the page level, right ? That probably makes sense perf-wise :-)

I guess the test could explicitly call that deduplicate step to make the list of $expected libraries closer to what you'd intuitively expect, but I'm not sure there would be much sense in that.

Also, I have to confess when writing the test I didn't really figure out at which step in the Views pipeline the nested attachments got bubbled to the top-level of the render array. For the sake of the test, just obtaining a render array with the expected bubbled '#attach's on top was good enough for me.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Yep, related to that is #2483433: Optimize CacheableMetadata::merge() + BubbleableMetadata::merge(). It moves the existing attachment merging tests from a KTB to a unit test, but one of the things it tests is that when attachments are merged, no deduplication is happening yet.

Status: Fixed » Closed (fixed)

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