Problem/Motivation

There is a bug in the implementation of LibraryDiscoveryCollector::reset() in HEAD. The $cid is not reset meaning that there is a possibility of retrieving the assets/libraries for the wrong theme if the active theme is changed after LibraryCacheCollector::reset() or LibraryDiscoveryCollector::clear() is called

Also, LibraryDiscovery::clearCachedDefinitions() uses both $this->cacheTagInvalidator->invalidateTags() and LibraryDiscoveryCollector::clear() to clear the persistent cache backend. These two methods essentially do the same thing, hence it makes sense to remove the $cacheTagInvalidator to simplify the class.

Proposed resolution

Fix the code.

Remaining tasks

Patch
Reviews
Commit

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

almaudoh created an issue. See original summary.

almaudoh’s picture

Status: Active » Needs review
FileSize
520 bytes
almaudoh’s picture

As a matter of fact, we don't need $this->cacheTagInvalidator->invalidateTags() since LibraryDiscoveryCollector::clear() does the same thing.

The last submitted patch, 2: 2575735-2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2575735-3.patch, failed testing.

Status: Needs work » Needs review

almaudoh queued 3: 2575735-3.patch for re-testing.

Wim Leers’s picture

Title: LibraryDiscovery::clearCachedDefinitions() does not properly clear caches » LibraryDiscovery::clearCachedDefinitions() does no clear its static cache
Status: Needs review » Needs work
Issue tags: +Needs tests
+++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
@@ -86,7 +72,8 @@ public function getLibraryByName($extension, $name) {
+    $this->libraryDefinitions = [];

This is the change that really matters.

We need test coverage for it.

almaudoh’s picture

We need test coverage for it.

There is already a test in #2451411: Add libraries-override to themes' *.info.yml which implicitly covers this. But I guess explicit test coverage can never be bad. Will work on one tomorrow.

almaudoh’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3 KB
5.68 KB

Tests attached.

The last submitted patch, 9: 2575735-8-TEST-ONLY.patch, failed testing.

The last submitted patch, 9: 2575735-8-TEST-ONLY.patch, failed testing.

almaudoh’s picture

The cid for the LibraryDiscoveryCollector needs to be reset as well when LibraryDiscoveryCollector::reset() is called to ensure we don't use the wrong cid if for some reason the ActiveTheme was changed mid-request.

almaudoh’s picture

Title: LibraryDiscovery::clearCachedDefinitions() does no clear its static cache » LibraryDiscovery::clearCachedDefinitions() does not clear its static cache
almaudoh’s picture

Issue tags: +rc target triage
Parent issue: » #2451411: Add libraries-override to themes' *.info.yml
FileSize
5.08 KB

Re-roll.
After #2451411: Add libraries-override to themes' *.info.yml, this is more or less a clean-up, even though there is a subtle bug as mentioned in #12.

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
    @@ -84,7 +69,6 @@ public function getLibraryByName($extension, $name) {
    -    $this->cacheTagInvalidator->invalidateTags(['library_info']);
    

    This is fine, because of #3.

  2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryCollector.php
    @@ -172,4 +171,13 @@ protected function applyLibrariesExtend($extension, $library_name, $library_defi
    +  public function reset() {
    +    parent::reset();
    +    $this->cid = NULL;
    +  }
    

    This seems wrong in two ways:

    1. CacheCollectorInterface::reset() says:
        /**
         * Resets the local cache.
         *
         * Does not clear the persistent cache.
         */
        public function reset();
      
    2. Why set $this->cid = NULL?
almaudoh’s picture

Why set $this->cid = NULL?

Because if the active theme changes before (or after) the static cache is cleared, the LibraryDiscoveryCollector should load the libraries for the current active theme in ::lazyLoadCache(). So the cid needs to be recalculated otherwise it will load assets for the wrong theme.

It's not clearing the persistent cache, just ensuring that the right libraries for the current active theme are loaded from there.

Wim Leers’s picture

#16: That totally makes sense! Can you add a comment for that? :)

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs issue summary update

So #16 + #17 show why my remark at #15.2.2 was wrong. But my remark at #15.2.1 was also wrong (it doesn't touch persistent caches at all, it only makes sure the original CacheCollector::reset() logic also runs). Which makes #15.2 completely wrong. And #15.1 was just an observation.

Which then means I have no more remarks. Except that the IS is not very clear yet.

AFAICT this is the key problem that is being fixed:

+++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
@@ -36,11 +36,17 @@ protected function setUp() {
+    // Now make classy the active theme and assert that library is not added.
+    $this->activateTheme('classy');
+    $this->assertFalse($this->libraryDiscovery->getLibraryByName('test_theme', 'kitten'));

… and what this is actually testing is that $this->cid = NULL line — without that, the wrong cache item is retrieved in LibraryDiscoveryCollector: it retrieves whichever theme was first active (test_theme) in this case.

almaudoh’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

#18: Ha! :)

AFAICT this is the key problem that is being fixed:

Exactly. Except now there is another interesting thing I've discovered while trying to write a failing patch (which would just be #14 with the hunk mentioned in #15.2 removed).

The test doesn't fail because the library definitions get rebuilt after LibraryDiscovery::clearCachedDefinitions(), so in the second assertion, when classy is the base theme, the test_theme_library_info_alter() is not called again because hook_library_info_alter() is only invoked for the active theme (in this case classy). So the kitten library doesn't exist, so the test passes. Can't tell if that is a bug or by design.

The only way to test this case I can think of is to use libraries-override or libraries-extend, which is kinda ironic since those two did not exist when the first patch was made.


The issue was originally about LibraryDiscovery not clearing its static cache when ::clearCachedDefinitions() is called. Now LibraryDiscoveryCollector using the wrong $cid to fetch the wrong cached copy if the active theme is changed, that is something new. Updating IS to capture this.
almaudoh’s picture

Title: LibraryDiscovery::clearCachedDefinitions() does not clear its static cache » LibraryDiscoveryCollector::reset() does not properly reset its $cid, resulting in loading wrong library assets if the active theme changes

Longish title :P

Wim Leers’s picture

@almaudoh++

Thank you for always writing so well: clear sentences, proper punctuation, proper formatting. It makes working with you a true pleasure :)

almaudoh’s picture

@WimLeers: aw, shucks :) Thanks for the great reviews too.

alexpott’s picture

Issue tags: -rc target triage +rc target

Discussed with committers and we agree that this is an RC target because this would be an extremely hard bug to debug and avoid.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 758cf21 on 8.0.x
    Issue #2575735 by almaudoh: LibraryDiscoveryCollector::reset() does not...
Wim Leers’s picture

Great work here, @almaudoh!

Status: Fixed » Closed (fixed)

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