Contrib.social

Module can pollute the render cache when used with a field item that has cache ids set

Open on Drupal.org →
Created on 24 May 2023, almost 2 years ago

Problem/Motivation

When used with a field formatter where each field item has a cache key set (such as on an entity reference field with the rendered entity formatter) the suffix is appended to this item which can put incorrect markup into the render cache for the key on the field item.

Reporting as a new issue - even though I suspect https://www.drupal.org/project/field_delimiter/issues/3321484 → is a result of this issue there is not enough details in that issue to be sure this is a duplicate.

Steps to reproduce

Using this module with standard install profile

  • On article default display - configure the tags field to use Rendered entity, use default view mode and add a delimiter
  • On article teaser display - configure the tags field to use Rendered entity, use default view mode and add a different delimiter than above
  • Add an article with more that 1 tag.
  • View the article in the full view mode
  • View the article in the teaser view mode

Observe that the field is rendered on the teaser view mode with the settings from the default view mode.

This is because the field item will have cache keys set - e.g

["entity_view", "taxonomy_term", "1", "default"]

And the preprocess function is adding #suffix to that render array - so if it is the first time being rendered it will cache the suffix as part of the render array.

When rendered elsewhere, this information which should be related only to field / view mode of the host entity is now part of the render cache for the entity being referenced.

Proposed resolution

I initially thought of adding the delimiter as the final part of the cache key - but I wasn't able to find explicit docs on what characters are supported in cache keys and how the module would sanitize this. This would also reduce the cacheability of rendered entities.

So perhaps an alternative is to use a nested render array instead of a suffix. Given the delimiter is already sanitized it could so something like:

$content_with_suffix = [
  $item['content'],
  ['#markup' => $safe_delimiter],
];
$item['content'] = $content_with_suffix;

Remaining tasks

Agree on approach
Patch
Review
Commit

User interface changes

n/a

API changes

n/a

Data model changes

Pending approach taken.

🐛 Bug report
Status

Active

Version

1.0

Component

Code

Created by

🇳🇿New Zealand ericgsmith

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @ericgsmith
  • Comment almost 2 years ago →
    🇳🇿New Zealand ericgsmith

    Attached is a patch and test with the approach of using a nested render array.

    There are no automated tests setup on this module so these need to be run offline.

  • Status changed to Needs review almost 2 years ago
  • Comment almost 2 years ago →
    🇳🇿New Zealand ericgsmith
  • Comment almost 2 years ago →
    🇳🇿New Zealand ericgsmith
  • Status changed to RTBC almost 2 years ago
  • Comment almost 2 years ago →
    🇳🇿New Zealand RoSk0 Wellington

    I've followed the steps to reproduce and:
    configured Tags field on the Default display mode to be Rendered entity, Rendered as Default and Delimited by: -|-
    configured Tags field on the Teaser display mode to be Rendered entity, Rendered as Default and Delimited by: ,,
    cleared the caches with drush cr, just in case
    visited /node/1, which is just created article with 3 tags
    and observed - in addition to my -|- delimiter I can see a comma
    exactly same output is visible on the home page, which is teaser view mode

    After applying the patch and clearing the caches default display haven't changed, teaser did however - no it shows three commas!

    So it looks like the patch does fix cache pollution, but the problem of extra comma is still present. Will see if I can find where is that coming from.

    Patch and test looks good.

  • Comment almost 2 years ago →
    🇳🇿New Zealand RoSk0 Wellington

    No issues here - that additional comma is hard coded for the tags field in the Olivero theme.

  • Comment over 1 year ago →
    solideogloria

    This patch is just what we needed. Hope this gets committed soon. Thanks.

  • Comment over 1 year ago →
    solideogloria

    The patch also applies to 2.0.x

  • Merge request !8Issue #3362292: Module can pollute the render cache when used with a field... → (Open) created by ericgsmith
  • Comment about 1 month ago →
    🇳🇿New Zealand ericgsmith

    Moved patch to gitlab to hopefully see this get committed - we have been using this patch for coming up 20 months without issue.

    Tests are not running in gitlab but still pass locally, I will open another issue to get CI setup.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024