Consider not executing hook_library_(info)_alter() *per* library, but once for all

In case we do that, we should do it for both library hooks.

Afaik, libraries are discovered when requested, by module and not all together? That's makes it impossible to implement a single alter hook for hook_library_info_alter() unless we change that pattern?

@WimLeers: We already do that function hook_library_info_alter(&$libraries, $module) {, and $libraries is documented as The JavaScript/CSS libraries provided by $module.

If you look at head standard install you will see in total 87 calls to \Drupal::moduleHandler()->alter() caused by \Drupal\Core\Asset\LibraryDiscovery::getLibrariesByExtension
which for example results in 87 calls to \Drupal::theme() which calls in 87 $container->get() calls, so in total we talk about about 1% of a cached request.

Doing this, saves me 2.2 ms

1. +++ b/core/modules/quickedit/quickedit.module
   @@ -85,21 +85,21 @@ function quickedit_page_attachments(array &$page) {
   +function quickedit_library_alter(array &$libraries, $theme = NULL) {
   +  // Retrieve the admin theme.
   +  if (!isset($theme)) {
   +    $theme = Drupal::config('system.theme')->get('admin');
   +  }
   +  if (isset($libraries['quickedit/quickedit'])) {
   

   No idea where the $theme is supposed to be coming from here...?

2. +++ b/core/modules/system/theme.api.php
   @@ -765,20 +765,21 @@ function hook_library_info_alter(&$libraries, $module) {
   -function hook_library_alter(array &$library, $name) {
   -  if ($name == 'core/jquery.ui.datepicker') {
   +function hook_library_alter(array &$libraries, $name) {
   +  if (isset($libraries['core/jquery.ui.datepicker'])) {
   +    $library = &$libraries['core/jquery.ui.datepicker'];
   

   $name doesn't make sense anymore, then?

No idea where the $theme is supposed to be coming from here...?

quickedit_library_alter() calls itself ...

$name doesn't make sense anymore, then?

Valid point.

Add a related issue

As you maybe have seen or maybe haven't seen is that even with that patch, LibraryDiscovery::getLibrariesByExtension() and LibraryDiscoveryParser
still act on a per extension base.

Even this is kinda an API change, but we should somehow execute just a single alter hook for all library definitions used on a page.
Can we do that somehow? This alter hook than probably has to be removed from the LibraryDiscovery again

This patch changes the library parser to just fire one alter hook for all modules.

• Worked on rewriting the existing tests for the library parser
• While rewriting the LibraryDiscovery I realized that the cache collector has the advantage of providing locking, so we could put the runtime hook into that.

Note: the current patch is in the middle of realizing the point 2)

mhh this is really tricky, storing both by extension and not by extension due to Parser::build() feels just wrong.

2ms on stock head makes this major.

Fine with an API change here.

So far, here is a simple version, which should work already.

Note: We optimize here for the common case: Get a library by name, not by extension. On an actual request
you reference to specific libraries, not to all extensions of a specific library.

Fixed it.

Who could have guessed that we have tests :)

This could be it ...

would be great to use overrides for libraries in theme, but separate issue. Themers like to override libraries in yml

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
   @@ -56,27 +74,51 @@ public function __construct(CacheCollectorInterface $library_discovery_collector
   +  protected function loadLibraryDefinitions() {
   

   I'd use findDefinitions() as DefaultPluginManager use

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
   @@ -56,27 +74,51 @@ public function __construct(CacheCollectorInterface $library_discovery_collector
   +    // @todo Should themes be able to take part in this process as well?
   +    $this->moduleHandler->alter('library', $libraries);
   

   good question! needs @todo and issue

3. +++ b/core/modules/system/theme.api.php
   @@ -786,20 +785,21 @@ function hook_library_info_alter(&$libraries, $module) {
   - * @param array $library
   - *   The JavaScript/CSS library that is being added.
   + * @param array $libraries
   + *   The Javascript/CSS libraries which are added, keyed by library name.
     * @param string $name
     *   The name of the library.
   ...
   -function hook_library_alter(array &$library, $name) {
   ...
   +function hook_library_alter(array &$libraries) {
   

   removed @name still documented

4. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
   @@ -392,20 +411,20 @@ public function testLibraryThirdPartyWithMissingLicense() {
   -    $library = $libraries['no-license-info'];
   +    $library = $libraries['licenses/no-license-info'];
   

   great to see that fixed

Thank you for you review!

Alright fixed your reviews ...

I'd use findDefinitions() as DefaultPluginManager use

just changed it.

Awesome!

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
   @@ -56,27 +74,52 @@ public function __construct(CacheCollectorInterface $library_discovery_collector
   +    // @todo store that a library does not exist.
   

   I think you're doing that with the below code?

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
   @@ -56,27 +74,52 @@ public function __construct(CacheCollectorInterface $library_discovery_collector
   +    if (empty($this->libraryDefinitions[$library_name])) {
   

   I think we want isset() there? As empty(FALSE) will always return TRUE. We don't need to keep setting it.

@damiankloip is right, as always.

!isset() :)

Here you go.

Ha, it failed, good to know!

As the person that did the initial conversion to reduce the cache calls, this is looking great!

I am ok to rtbc I think, I only changed one character in the last patch.

+++ b/core/modules/system/src/Form/ThemeAdminForm.php
@@ -52,6 +52,7 @@ public function buildForm(array $form, FormStateInterface $form_state, array $th
+    \Drupal::cache('discovery')->delete('library_info');

This hardcodes how and where caches are stored in a form submit function. Should we add a clearCachedLibraries() method or so to a service, similar to how we clear plugin caches?

This hardcodes how and where caches are stored in a form submit function. Should we add a clearCachedLibraries() method or so to a service, similar to how we clear plugin caches?

Let me quick explain why this change is needed.
Previously we retrieved library information per extension, so that enabling/disabling a theme did not caused issues, because the entry
in the cache collector simply did not existed yet. Once you just use a simple cache, this is no longer true and we need to invalidate it somehow.
Anyone has a better idea?

This bit is going to conflict with #2382533: Attach assets only via the asset library system.

Well sure, this is life and this is why we use git.

Why is this necessary?

Oh well, the reason is the following: We now parse core libraries always, so comparing with an empty result doesn't work that simple anymore.

core is moving, that is great!!

@berdir
Note: I also fixed your review, see the interdiff.

Maybe this is all you need.

While measuring the performance I realized that HEAD is broken in the sense that the cache entry for libraries is not saved.
Given that we save 150ms as anonymous user.

In case you have HEAD fixed, the difference is 6.37ms (2.5% of the request)

#2354705: Mark a couple of asset services as non public introduced that regression :(

Mh, k, maybe a reason this is critical.

So while the CacheCollector being made private caused the 150ms (!) regression, I'm not sure about removing it here.

We might want to further optimize the CacheCollector (add role to the cid so that anonymous users don't get things like contextual links or quickedit loaded? Make it per-library instead of per module?) but given #2381473: Decrease the size of the asset library cache item going back to one monolothic cache entry doesn't seem great.

Why are we marking this critical instead of just rolling back #2354705: Mark a couple of asset services as non public, if that's what introduced the testbot slowdowns? That was a normal "clean-up" style task that does not make D8 closer to release in any way.

@webchick
That itself is fair, but I also think that executing alter hooks for all of them will more and more be a problem on real sites,
with a lot of contrib modules, for core itself its maybe just the tip of the iceberg.

https://github.com/symfony/symfony/issues/12924 is the symfony issue which is also kinda broken, to be honest.

Yeah, don't get me wrong, I think this issue is probably also good to do, but I worry about us trying to "rush" it in in order to fix a regression in time for a big sprint that was caused by some other (normal) issue. Alex Pott is doing a revert of that one though, so we have some time to evaluate this one on its own merits.

Moving back to Major since the revert will take care of the critical.

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -41,9 +50,50 @@ class LibraryDiscoveryParser {
   +    $libraries_core = (array) $this->buildByExtension($core_extension);
   

   Why..

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -41,9 +50,50 @@ class LibraryDiscoveryParser {
   +    foreach ((array) $this->moduleHandler->getModuleList() as $extension) {
   

   all..

3. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -41,9 +50,50 @@ class LibraryDiscoveryParser {
   +    foreach ((array) $this->themeHandler->listInfo() as $extension) {
   

   ..the casts to array? Don't these return empty arrays if nothing is there anyway? If not they should.

I'm still not sure on dropping the cache collector - all core implementations check if the library exists before attempting to alter - so running it on cached libraries, then individually on uncached libraries as they're retrieved seems quite possible.

Yes let's postpone.