LibraryDiscoveryCollector::reset() does not properly reset its $cid, resulting in loading wrong library assets if the active theme changes

+++ 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.

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?

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

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++

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

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

Committed/pushed to 8.0.x, thanks!

Great work here, @almaudoh!