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
Comment | File | Size | Author |
---|---|---|---|
#14 | 2575735-14.patch | 5.08 KB | almaudoh |
#12 | 2575735-12.patch | 6.18 KB | almaudoh |
#12 | interdiff.txt | 510 bytes | almaudoh |
#9 | 2575735-8.patch | 5.68 KB | almaudoh |
#9 | 2575735-8-TEST-ONLY.patch | 3 KB | almaudoh |
Comments
Comment #2
almaudoh CreditAttribution: almaudoh commentedComment #3
almaudoh CreditAttribution: almaudoh commentedAs a matter of fact, we don't need
$this->cacheTagInvalidator->invalidateTags()
sinceLibraryDiscoveryCollector::clear()
does the same thing.Comment #7
Wim LeersThis is the change that really matters.
We need test coverage for it.
Comment #8
almaudoh CreditAttribution: almaudoh commentedThere 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.
Comment #9
almaudoh CreditAttribution: almaudoh commentedTests attached.
Comment #12
almaudoh CreditAttribution: almaudoh commentedThe cid for the
LibraryDiscoveryCollector
needs to be reset as well whenLibraryDiscoveryCollector::reset()
is called to ensure we don't use the wrong cid if for some reason the ActiveTheme was changed mid-request.Comment #13
almaudoh CreditAttribution: almaudoh commentedComment #14
almaudoh CreditAttribution: almaudoh commentedRe-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.
Comment #15
Wim LeersThis is fine, because of #3.
This seems wrong in two ways:
CacheCollectorInterface::reset()
says:$this->cid = NULL
?Comment #16
almaudoh CreditAttribution: almaudoh commentedBecause 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.
Comment #17
Wim Leers#16: That totally makes sense! Can you add a comment for that? :)
Comment #18
Wim LeersSo #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:
… and what this is actually testing is that
$this->cid = NULL
line — without that, the wrong cache item is retrieved inLibraryDiscoveryCollector
: it retrieves whichever theme was first active (test_theme
) in this case.Comment #19
almaudoh CreditAttribution: almaudoh commented#18: Ha! :)
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, whenclassy
is the base theme, thetest_theme_library_info_alter()
is not called again becausehook_library_info_alter()
is only invoked for the active theme (in this case classy). So thekitten
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
orlibraries-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. NowLibraryDiscoveryCollector
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.Comment #20
almaudoh CreditAttribution: almaudoh commentedLongish title :P
Comment #21
Wim Leers@almaudoh++
Thank you for always writing so well: clear sentences, proper punctuation, proper formatting. It makes working with you a true pleasure :)
Comment #22
almaudoh CreditAttribution: almaudoh commented@WimLeers: aw, shucks :) Thanks for the great reviews too.
Comment #23
alexpottDiscussed with committers and we agree that this is an RC target because this would be an extremely hard bug to debug and avoid.
Comment #24
catchCommitted/pushed to 8.0.x, thanks!
Comment #26
Wim LeersGreat work here, @almaudoh!