Consolidate library extension caches into a single cache entry

Since this is by extension, we'll need to have the active theme in the cache key, none of these should be requested before the theme is set so should be OK.

Also assuming this is for all discovered libraries. Some might not be used, or only on rarely visited admin pages etc. so might be worth CacheCollector here?

Yeah we should come up with a scenario and have a look at the needed amount of memory for it.

Not only the active, but all enabled themes.

The memory consumption should be small. Even smaller if we'd find a way to omit the default/empty properties (e.g., by caching the original/unprocessed data instead of the processed data).

Postponing this on #2228291: Move all plugin and other discovery data into a cache_discovery bin too. With that and the PhpStorage cache backend we might have come up with a way to avoid the i/o memory trade-off.

Moving this to duplicate. If the local cache stuff turns out to be a dead end, we can re-open this.

Yeah I don't mind doing this additionally to the other one. It's a lot less complex than config pre-loading.

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryCollector.php
@@ -0,0 +1,75 @@
+/**
+ * Class LibraryDiscoveryCollector.
+ */
+class LibraryDiscoveryCollector extends CacheCollector {

Shit really? In general I really wonder whether it is worth to split this up into 3 classes tbh. As long you change the tests properly this though should work fine.

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryCollector.php
@@ -0,0 +1,75 @@
+ * Class LibraryDiscoveryCollector.
...
+class LibraryDiscoveryCollector extends CacheCollector {

at least the doc does not tell you anything wrong :P

1. +++ b/core/core.services.yml
   @@ -810,7 +810,15 @@ services:
   +  library.discovery.parser:
   ...
   +  library.discovery.collector:
   

   Let's order these two alphabetically.

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryCollector.php
   @@ -0,0 +1,75 @@
   +    $this->storage[$key] = $this->discoveryParser->buildLibrariesByExtension($key);
   

   "buildLibrariesByExtension" is very long.

   $this->discoveryParser->getByExtension($name) ?

   Is there actually a different way than by extension, or do we intend to add one in the future?

   If not, just LibraryParser->get($extension)?

3. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -43,85 +33,14 @@ class LibraryDiscovery implements LibraryDiscoveryInterface {
   +   *   The moduel handler.
   

   Typo in "moduel"

4. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -135,8 +54,13 @@ protected function setCache($extension, $information) {
   +  public function buildLibrariesByExtension($extension) {
   +    if (isset($this->libraries[$extension])) {
   +      return $this->libraries[$extension];
   +    }
   

   Hm. If we're going to split it all up this radically, then I wonder why there is still a separate/custom/duplicate property cache on the parser?

   Shouldn't the parser just parse when told to do so?

5. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -309,4 +220,18 @@ protected function parseLibraryInfo($extension, $library_file) {
   +  protected function drupalGetPath($type, $name) {
   

   Btw, we still need to inject some $type knowledge into all these classes...

   Two possible approaches:

   A. Use the legacy $type, $name pattern of drupal_get_path() & Co.

   B. Use Extension objects. (they have ->getType() built-in)

Thanks!

Yeah, let's definitely create a follow-up for the extension type issue - that magic type detection hack was definitely meant to be temporary only...

It would also be great to create a separate follow-up issue to (1) rename the service IDs into 'asset.library_discovery*' and optionally (2) move the classes into a \Asset\Discovery subnamespace and remove "Discovery" from the class names.

Committed c5942ee and pushed to 8.x. Thanks!

The follow-up that never happened: #3536163: Library discovery Parser should allow a version constant for modules and themes