Contrib.social

Page and dynamic page cache entries not cleared automatically when new rules added

Open on Drupal.org →
Created on 10 November 2020, about 4 years ago
Updated 25 June 2024, 7 months ago

Problem/Motivation

When adding a CSS injector file, cached pages on the site do not get cleared. A site admin must perform a full site cache clear.

Steps to reproduce

  1. Configure site so that all caching is enabled (Dynamic Page Cache, Page Cache). Make sure caching is not disabled via the local.settings.php file. For good measure, make sure CSS/JS aggregation are enabled as well.
  2. In Browser A, visit the homepage as an anonymous user.
  3. In Browser B, login as an admin and add a CSS Injector rule that adds something obvious like setting background color of body to red. Have it apply to all pages/themes.
  4. In Browser B, view the homepage as an admin (which bypasses page caches) and observe the change in color (good)
  5. In Browser A (anonymous user), reload homepage and observe no change (bad)
  6. Clear site cache
  7. In Browser A (anonymous user), reload homepage and observe no change (good)

Proposed resolution

The cache tag for "library_info" is cleared when updates to a CSS injector rule are made, but this cache tag doesn't appear to be bubbled up to page cache and dynamic page cache entries, so they are not invalidated.

In this module's hook_page_attachments, which adds the asset libraries to the page as attachments, there's this comment:

* Note that the list_cache_tags (library_info) are not added here and need not,
* as tha caller already does it. Setting asset entityies' list_cache_tags to
* library_info makes the library-info invalidate on asset change.
* While changing and deleting of assets will trigger invalidation by their
* individual cache tags, the list cache tags guarantees invalidation on new
* asset creation.

So it's not adding library_info cache tag, but it probably should be. Maybe core was doing this at one point, and stopped?

We should consider adding this tag explicitly.

Remaining tasks

User interface changes

API changes

Data model changes

🐛 Bug report
Status

Needs work

Version

2.0

Component

Code

Created by

🇺🇸United States bkosborne New Jersey, USA

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Open in Jenkins → Open on Drupal.org →
    Core: 10.2.1 + Environment: PHP 8.1 & MySQL 8
    last update 9 months ago
    8 pass
  • Comment 9 months ago →
    🇺🇸United States bkosborne New Jersey, USA

    Okay, that makes sense. I didn't realize I could pull the tags and contexts from that interface. Here's an updated patch that does that.

  • Status changed to Needs work 8 months ago
  • Comment 8 months ago →
    🇩🇪Germany Anybody Porta Westfalica

    Thanks @bkosborne patch LGTM so far, perhaps it would make sense to proceed using a MR instead of patches? Would you do that?

    Furthermore some simple tests might be good to ensure it works as expected.

  • First commit to issue fork.
  • Merge request !23Issue #3181674 by bkosborne, pookmish, geek-merlin: Page and dynamic page... → (Open) created by hswong3i
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.2.1 + Environment: PHP 8.1 & MySQL 8
    last update 8 months ago
    8 pass
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.2.1 + Environment: PHP 8.1 & MySQL 8
    last update 8 months ago
    4 fail
  • Comment 8 months ago →
    🇭🇰Hong Kong hswong3i

    @bkosborne #11 not working for me and report error message as below:
    NOTICE: PHP message: Uncaught PHP Exception TypeError: "Drupal\Core\Render\Renderer::addCacheableDependency(): Argument #1 ($elements) must be of type array, null given, called in /var/www/html/modules/contrib/asset_injector/asset_injector.module on line 141" at /var/www/html/core/lib/Drupal/Core/Render/Renderer.php line 690

    Rolling back with #7 looks much better ?_?

  • Comment 8 months ago →
    🇺🇸United States bkosborne New Jersey, USA

    I don't see how that's possible. $elements is an array typed property on the function. If it were null, PHP should have thrown a different error when _asset_injector_add_element_libraries was invoked. What version of Asset Injector are you using, and what other patches (if any) do you have applied?

  • Comment 7 months ago →
    🇩🇪Germany Anybody Porta Westfalica

    @hswong3i comparing your MR's diff with the patch from #11 you can see they are different. Please first fix the MR to be 1:1 like #11

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024