Problem/Motivation

EntityReferenceFormatterBase::view() is applying directly the field level access cacheability to the render array which is removing any already present cacheable metadata.

This could happen if you have a field formatter extending from this one and e.g. in ::viewElements add not only the children to the render array, but also cacheable metadata at the first level of the render array. In this case the cacheable metadata will simply be overwritten by EntityReferenceFormatterBase::view(), but it should be merged instead.

Proposed resolution

Replace
$field_level_access_cacheability->applyTo($elements);
with
$field_level_access_cacheability->merge(CacheableMetadata::createFromRenderArray($elements))->applyTo($elements);

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#10 interdiff-7-10.txt1.69 KBMaouna
#10 cache-formatter_merge_cache_data-2854454-10.patch3.07 KBMaouna
#10 cache-formatter_merge_cache_data-2854454-10-FAIL.patch2.19 KBMaouna
#7 cache-formatter_merge_cache_data-2854454-7.patch3.16 KBMaouna
#7 3-7-interdiff.txt1.82 KBMaouna
#6 cache-formatter_merge_cache_data-2854454-6-FAIL.patch2.19 KBMaouna
#3 2854454-3.patch903 byteshchonov

Comments

hchonov created an issue. See original summary.

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented
Issue summary: View changes
hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented
StatusFileSize
new 2854454-3.patch903 bytes
hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented
Issue tags: +Needs tests, +DevDaysSeville, +D8 cacheability
Maouna’s picture

Maouna commented
Assigned: Unassigned » Maouna
Status: Needs review » Needs work
Maouna’s picture

Maouna commented
Status: Needs work » Needs review
StatusFileSize
new cache-formatter_merge_cache_data-2854454-6-FAIL.patch2.19 KB

Failing test for demonstration.

Maouna’s picture

Maouna commented
Issue tags: -Needs tests
StatusFileSize
new 3-7-interdiff.txt1.82 KB
new cache-formatter_merge_cache_data-2854454-7.patch3.16 KB

Added the test.

The last submitted patch, 6: cache-formatter_merge_cache_data-2854454-6-FAIL.patch, failed testing.

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented 
  1. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldFormatter/EntityTestReferenceCustomCacheTagFormatter.php
@@ -0,0 +1,32 @@
+ *   label = @Translation("Adding a custom cache tag"),

    "Custom cache tag" should be sufficient. 

  2. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldFormatter/EntityTestReferenceCustomCacheTagFormatter.php
@@ -0,0 +1,32 @@
+class EntityTestReferenceCustomCacheTagFormatter extends EntityReferenceIdFormatter  {

    Why do you extend from EntityReferenceIdFormatter instead from EntityReferenceFormatterBase?

  3. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldFormatter/EntityTestReferenceCustomCacheTagFormatter.php
@@ -0,0 +1,32 @@
+    $elements = parent::viewElements($items, $langcode);
+
+    $elements['#cache']['tags'][] = 'custom_cache_tag';
+
+    return $elements;

    The empty lines are not really needed.

Maouna’s picture

Maouna commented
StatusFileSize
new cache-formatter_merge_cache_data-2854454-10-FAIL.patch2.19 KB
new cache-formatter_merge_cache_data-2854454-10.patch3.07 KB
new interdiff-7-10.txt1.69 KB

Thanks @hchonov for the feedback. I adapted the patches accordingly.

The last submitted patch, 10: cache-formatter_merge_cache_data-2854454-10-FAIL.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me, fix makes sense and has a test.

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented
Assigned: Maouna » Unassigned

  • catch committed 5df2ddd on 8.4.x 
    Issue #2854454 by Maouna, hchonov: EntityReferenceFormatterBase::view()...

  • catch committed 2dfc5de on 8.3.x 
    Issue #2854454 by Maouna, hchonov: EntityReferenceFormatterBase::view()...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Fixed this on commit:

FILE: ...ld/FieldFormatter/EntityTestReferenceCustomCacheTagFormatter.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 19 | ERROR | [x] Expected 1 space before opening brace; found 2
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

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

Status: Fixed » Closed (fixed)

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