Add libraries-override to themes' *.info.yml

I'm curious whether we could do something like the following:

We store libraries per theme, so that the cached libraries are processed with the defined entries in stylesheets-overrides and stylesheets-remove.
This would allow you to get rid of using that logicon runtime, but rather keep using libraries everywhere.

I mentioned on IRC that I think sticking with paths and wildcards would be more conventional. Similar to how the magic module syntax works right now.

I definitely would this to work using namespace or full path, and not by library.

So I assume by namespace it would be?:

# Remove a CSS file:
stylesheets-remove:
  - @node/node.css

and with path:

# Remove a CSS file:
stylesheets-remove:
  - core/node/css/node.css

By name is preferred.

I think it makes sense to be able to remove single css file from a library because if it would be base on libraries you would have to replace whole library. Anyway the solution we have now doesn't fullfil the expectations of people using it which at least for me in most of the cases is that I want to override/remove single CSS. However after all you are going to replace all instances of that CSS file existing in the Drupal installation.

After changing this to be based on the path of the file we might be able to remove the stylesheet-override because you would be able to remove single CSS file and then include it using libraries.

As long as library names are globally unique – which I think they are?

I'm pretty sure they're not. I can create a subtheme of Bartik with a library called "global-styling".

so if you just remove CSS from a library and add your own, it won't be loaded the same times as the original library.

I hadn't thought of that. In that case, the override makes sense.

We discussed this before at some point. I think the solution lies in fully embracing the asset-library-based Drupal 8, and thus removing any remnants from the individual-asset-based past.

That means:

libraries-remove

The ability to remove entire libraries.

libraries-remove:
  - subtheme/library
  - core/modernizr

libraries-override

The ability to override entire libraries and parts of libraries:

libraries-override:
  # Replace an entire library.
  core/drupal.collapse: mytheme/collapse
  # Replace one particular library asset with another.
  subtheme/library/css/theme/css/layout.css: css/layout.css
  # Remove one particular asset.
  drupal/dialog/css/theme/dialog.theme.css: false

Note that we could even omit libraries-remove if we'd allow

libraries-override:
  # Remove an entire library.
  core/modernizr: false

That would cover the use cases of stylesheets-override and stylesheets-remove.

(Note that this is basically a declarative form of hook_library_info_alter().)

… but we can go further, and consider improvements through API additions:

(copied verbatim from #2377397-17: Themes should use libraries, not individual stylesheets)

1. a libraries-extend property in theme *.info.yml files:
   

   libraries-extend:
     contextual/drupal.contextual-links:
       css:
         theme:
           mytheme-fancy-contextual-links.css
       js:
         mytheme-fancy-contextual-links.js
   

   … to attach additional CSS/JS to existing libraries (if those existing libraries are present)

2. a attach-library-to-hook property in theme *.info.yml files:
   

   attach-library-to-hook:
     menu_local_action:
       - core/modernizr
     block___menu:
       - mytheme/fancymenublock
   

   … to automatically attach a library instead of having to implement hook_preprocess_HOOK() for each.

Looking at #9, I'm fine with doing it by library if we retain the ability to target individual files. There is no objection to the library concept, only to being limited to removing/overriding the whole library without flexibility.

It looks like we agree on that.

I agree with #10 as well. :)

@alexpott suggests we merge this with #2389735: Core and base theme CSS files in libraries override theme CSS files with the same name because that critical fix exposes underlying brokenness that this issue intends to fix. If we do that, do we still want to tackle using libraries or just fix stylesheets-remove/override? I wasn't sure if Wim's suggestion in #9 would remove stylesheets-remove, or the library functionality would just be an API addition. If it is an addition, we can fix stylesheets remove in the other issue and do the addition work here.

Regardless, we need to decide on how to specify the assets, so we can do it the same in both places. If we are going to break an API, lets only do it once.

I talked to xjm and alexpott about this real quick. There shouldn't be a problem getting this into 8.1 if we don't get it done in time. We should still try to get it done before RC, but if we run out of time we don't need to postpone it to 9.

Since stylesheets-override was removed in https://www.drupal.org/node/2389735 should we also consider dropping the libraries-override and simply rely on libraries-remove for consistency?

We want libraries-override. The stylesheets-override removal happened because we couldn't fix a critical bug while maintaining its functionality. Libraries-override would allow a theme to replace a particular asset, without removing the whole library. For example, if you want to replace just one CSS file, but keep the functionality of the rest of the library, especially its conditional inclusion.

libraries-override is needed for vertical-tabs and print.css

This appears to be needed for #2489564: Move user.theme.css to Classy.

Working on this for the next couple of hours.

Eventually took longer than a couple of hours. After much thought, I figured the best approach is to implement individual component overrides/removes in the LibraryDiscoveryParser while deferring entire library overrides/removes to the LibraryDiscovery.

I'm going with #9. libraries-remove is implemented by specifying libraries-override: false

First patch just to get things going...Added tests to confirm functionality. Also left in stylesheets-remove for now. Can be removed later.

Just a thought (and may be overkill…) but should we provide library granularity here so that an asset included in multiple libraries can be treated differently?

That's what this does actually. The classy/base refers to the library definition, not the file path. The file path starts from css/layout.css relative to the library where it was defined. So if you define that same .css file in another library, it will get included via that library.

Fixed the test fails and added some code comments.

Previous interdiff was wrong.

Forgot to create the css and js files referenced in the new test_theme.libraries.yml. This should be green now.

Was just wondering if we could extend the scope of this issue to include libraries-extend as well - seems that is really needed at this time too.

@Cottser: are we ok with the nomenclature here? I know libraries-override came from stylesheets-override, but I think library-overrides would sound more correct.

Oops, sorry wrong patch. Correct one attached. Interdiff is correct.

If we stick with convention, that would mean using plurals. "libraries" since you can manipulate more than one library at a time. The match works for removal of a library so we don't need "libraries-remove". Is there a way to work extending into the same grouping? I'm guessing that would be hard with the "something: something" syntax.

Is there a way to work extending into the same grouping?

It would not allow for adding new stuff into an existing library. The syntax in #9 would be more flexible and therefore would need to be in a separate group.

One more iteration. Addressed comments in #38 and #41. I'm always confused by this uppercase true/false - lowercase TRUE/FALSE dichotomy. Left to me we should just use lowercase throughout.

Also ripped out stylesheets-remove completely, though I retained the tests to confirm equivalent functionality is still provided by libraries-override.

Looks much better now.

Self-review.

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
@@ -314,6 +314,61 @@ protected function parseLibraryInfo($extension, $path) {
+          NestedArray::unsetValue($libraries[$name], $parents);
+          if ($override) {
+            // Replace with an override if specified.
+            NestedArray::setValue($libraries[$name], $new_parents, []);

This will result in loss of settings attached to the original component. Re-use existing settings for now.

Some libraries define additional settings e.g. normalize library defines assets/vendor/normalize-css/normalize.css: { every_page: true, weight: -20 }. This would be lost in the code block below. Will fix in next patch.

That also exposes the fact that the current libraries-override syntax would not allow for overriding those kinds of settings. Are there possible use-cases for this, or maybe we can do that in libraries-extend

Fixed #43 and added test for it.

We should leave stylesheets-remove in, at least until we get confirmation from a framework manager that we can take it out. Removing it changes this from an api addition to a disruption change, so we need to verify we can do that at this late stage in the beta phase.

Also, I'm adding needs change record. The addition of libraries-override will need one. If we do remove stylesheets-remove we'll need a separate one for that as well.

One of the best, clearest, cleanest, most well-tested patches I've seen in a while. Awesome work, @almaudoh. Thank you! :)

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -87,7 +87,7 @@ public function buildByExtension($extension) {
   -    $libraries = $this->parseLibraryInfo($extension, $path);
   +    $libraries = $this->applyLibrariesOverrides($this->parseLibraryInfo($extension, $path), $extension);
   

   Hrm, let's do this in two steps for clarity:

   $libraries = $this->parseLibraryInfo($extension, $path);
   $libraries = $this->applyLibrariesOverrides($libraries, $extension);
   

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -314,6 +314,62 @@ protected function parseLibraryInfo($extension, $path) {
   +          // An asset within the library.
   

   "Active theme defines an override for an asset within this library."

   (For similarity to the if-branch comment at line 333.)

3. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -314,6 +314,62 @@ protected function parseLibraryInfo($extension, $path) {
   +          if ($component == 'css') {
   

   Let's use strict equality.

4. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -60,18 +60,18 @@ class ActiveTheme {
   +   * The libraries or library components overridden by the theme.
   
   @@ -177,4 +168,10 @@ public function getBaseThemes() {
   +   * Returns the libraries or library components overridden by the active theme.
   

   What are "library components"?

5. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -177,4 +168,10 @@ public function getBaseThemes() {
   +  }
    }
   

   Needs a newline in between.

6. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -161,27 +161,24 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
   +    // Grab libraries-overrides from base theme.
   ...
   +      if (!empty($base->info['libraries-override'])) {
   

   "overrides" vs. "override"

7. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -42,4 +43,70 @@ public function testElementInfoByTheme() {
      }
    
   +
   +  /**
   

   One newline too many.

8. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -42,4 +43,70 @@ public function testElementInfoByTheme() {
   +  public function testLibrariesOverride() {
   

   Excellent test coverage!

9. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -42,4 +43,70 @@ public function testElementInfoByTheme() {
   +    // Assert that library component was correctly overridden.
   +    $library = $library_discovery->getLibraryByName('classy', 'base');
   +    $this->assertAssetInLibraryComponent($library['css'], 'base', 'core/modules/system/tests/themes/test_theme/css/test_theme_layout.css');
   ...
   +    // Assert that library component was correctly removed.
   +    $library = $library_discovery->getLibraryByName('core', 'drupal.dialog');
   +    $this->assertNoAssetInLibraryComponent($library['css'], 'drupal.dialog', 'core/misc/dialog.theme.css');
   ...
   +    // Assert that overridden library component still retains attributes.
   

   Where did you find this "component" term? If it's used elsewhere, it's fine, but so far, the terminology has been to my knowledge: "asset library", which contains "assets". No mention of "components".

10. +++ b/core/modules/system/tests/themes/test_subtheme/test_subtheme.info.yml
    --- /dev/null
    +++ b/core/modules/system/tests/themes/test_theme/css/collapse.css
    
    +++ b/core/modules/system/tests/themes/test_theme/css/collapse.css
    --- /dev/null
    +++ b/core/modules/system/tests/themes/test_theme/js/collapse.js
    

    AFAIK these files don't even need to exist!

    If you want to keep them, then please capitalize "css" and "js" to "CSS" and "JS", respectively.

11. +++ b/core/modules/system/tests/themes/test_theme/test_theme.info.yml
    @@ -14,9 +14,20 @@ description: 'Theme for testing the theme system'
    +libraries-override:
    

    More excellent test coverage! Thank you! :)

12. +++ b/core/modules/system/tests/themes/test_theme/test_theme.info.yml
    @@ -14,9 +14,20 @@ description: 'Theme for testing the theme system'
    +  # It works for js as well.
    

    s/js/JSS/

13. +++ b/core/modules/system/tests/themes/test_theme/test_theme.info.yml
    @@ -14,9 +14,20 @@ description: 'Theme for testing the theme system'
    +
    

    Unwanted additional newline.

#43: great point. IMO there are two ways to go about this:

1. Fix it. This could be tricky. Look at \Drupal\Core\Render\Renderer::mergeAttachments() how drupalSettings is supposed to be merged.
2. Consider it impossible to override drupalSettings in a library, and throw an exception in case somebody tries to do so. Add test coverage that verifies that this happens.

I think option 2 is fine, especially because it could simply be a non-API breaking feature addition in a future version of Drupal 8.

Thanks for the reviews and comments.

1. Done.
2. Done.
3. Done.
4. Assets actually. :) Didn't know the terminology to use when I started the patch.
5. Done.
6. Fixed.
7. Fixed.
8. :)
9. Changed to "assets", didn't change the helper functions in the test though.
10. Changed the case.TwigTransTest fails if the files don't exist because it installs both test_theme theme and locale module. Locale module tries to load JS files in hook locale_js_alter() when it calls locale_js_translate() and fails if the file doesn't exist.
11. :)
12. Fixed.
13. Fixed.

On #43 I was actually referring to the attributes specified for JS/CSS assets (Used the wrong word again :P). However, your point on drupalSettings makes sense - and I agree to option 2 as well. Implemented.

Added explicit check for malformed libraries-override asset specification and throwing an exception if not well formed.

10. Aha, ok!

This is now very close IMO.

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -343,20 +346,28 @@ protected function applyLibrariesOverrides($libraries, $extension) {
   +            throw new \LogicException(SafeMarkup::format('Library asset %asset is not correctly specified.', ['%asset' => $asset]));
   

   Let's say what is wrong in the exception message.

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -343,20 +346,28 @@ protected function applyLibrariesOverrides($libraries, $extension) {
   +            // drupalSettings should not be overridden.
   +            throw new \LogicException(SafeMarkup::format('drupalSettings cannot be overridden in libraries-override. Trying to override %asset.', ['%asset' =>$asset]));
   

   s/should not/may not/
   s/cannot/may not/

   In the exception message, I'd point the user to hook_library_info_alter() instead. There, they can do this.

3. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -62,27 +63,91 @@ public function testLibrariesOverride() {
        $this->assertAssetInLibraryComponent($library['css'], 'base', 'core/modules/system/tests/themes/test_theme/css/test_theme_layout.css');
   

   Can you also update the name of this assertion, so that no "component" stuff is left?

4. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -62,27 +63,91 @@ public function testLibrariesOverride() {
   +      if ($e->getMessage() === 'drupalSettings cannot be overridden in libraries-override. Trying to override <em class="placeholder">core/drupal.ajax/drupalSettings/ajaxPageState</em>.') {
   +        $this->pass('Throw LogicException when trying to override drupalSettings');
   +      }
   +      else {
   +        $this->fail('Throw LogicException when trying to override drupalSettings');
   +      }
   ...
   +      if ($e->getMessage() === 'Library asset <em class="placeholder">core/drupal.dialog/css</em> is not correctly specified.') {
   +        $this->pass('Throw LogicException when specifying invalid override');
   +      }
   +      else {
   +        $this->fail('Throw LogicException when specifying invalid override');
   +      }
   

   The $this->pass() + $this->fail() can be replaced with a single $this->assertEquals().

Thanks @WimLeers:
1. Done. Hope this is good enough.
2. Done.
3. Done.
4. Nice one. Didn't see that. Thanks.

Still waiting for framework managers review and comments on whether to keep stylesheets-remove or not.

Unless we already have consensus, we can remove stylesheets-remove in a follow-up. Then we can get this in. Progress matters!

Please add a change record and a beta evaluation. Once that's present, this is RTBC IMO. Just a bunch of minor things:

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -88,6 +89,7 @@ public function buildByExtension($extension) {
   +    $libraries = $this->applyLibrariesOverrides($libraries, $extension);
   
   +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -60,18 +60,18 @@ class ActiveTheme {
   +  protected $librariesOverride;
   

   This is plural, but elsewhere it's singular. Let's be consistent.

2. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -177,4 +168,11 @@ public function getBaseThemes() {
   +  /**
   +   * Returns the libraries or library assets overridden by the active theme.
   +   */
   +  public function getLibrariesOverride() {
   

   Missing @returns.

3. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -161,27 +161,24 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
   +    // Prepare libraries overrides from this theme and ancestor themes.
   

   s/overrides/override/

4. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -42,4 +45,125 @@ public function testElementInfoByTheme() {
   +   * Tests that libraries-overrides are applied to library definitions.
   

   Plural, even though the key in YAML files is actually singular.

5. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -42,4 +45,125 @@ public function testElementInfoByTheme() {
   +   * Tests libraries-override on drupalSettings.
   ...
   +   * Tests libraries-override on malformed assets.
   

   Here you got it right.

I'm with the other theme system co-maintainers on this. We should probably not remove stylesheets-remove at this point. Definitely remove it from this patch to unblock it.

Maybe deprecate it to discourage use and as a reminder to remove it in 9.x.

Unless there is a compelling case to remove it (aka breaks things... or other beta evaluation conditions)?

I think the key question is: Does stylesheets-remove break something?. I think the answer is No..

But, because stylesheets-remove is now a subset of what libraries-override does, we could potentially internally map stylesheets-remove to libraries-override. That would then allow the logic to be simplified, and to just have one bit of (easily removable in D9) code that translates one to the other.

Added a draft change record for the libraries-override part.

Ok. Fixed review comments in #52 and restored stylesheets-remove.

we could potentially internally map stylesheets-remove to libraries-override.

The one thing that makes it hard is that stylesheets-remove uses relative paths and placeholders like '@classy/css/layout.css' to specify stylesheet paths. I don't think it makes sense to write new code (including tests) to map this back to the correct libraries for something we are deprecating. I have therefore left in the code and tests as it is in HEAD.

One change though is moving the stylesheets-remove piece in ThemeInitialization to a separate (private) method so we can easily delete later on.

Also added todo's for removal in D9.

and the patch...

The one thing that makes it hard […]

Yep, that's what I feared. So +1 to your solution!

I think this will be the last review round :)

1. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -177,4 +188,24 @@ public function getBaseThemes() {
   +   * ¶
   ...
   +   * ¶
   

   There are trailing spaces on these 2 lines.

2. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -238,8 +238,10 @@ protected function getExtensions() {
   -  protected function resolveStyleSheetPlaceholders($css_file) {
   +  private function resolveStyleSheetPlaceholders($css_file) {
   

   I don't think we ever make these private. Let's keep it at protected.

3. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -253,4 +255,42 @@ protected function resolveStyleSheetPlaceholders($css_file) {
   +  private function prepareStylesheetsRemove(Extension $theme, $base_themes) {
   

   Similarly, let's make this protected

4. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -253,4 +255,42 @@ protected function resolveStyleSheetPlaceholders($css_file) {
   +  }
    }
   

   Needs newline in between.

5. +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
   @@ -93,7 +93,7 @@ public function testGetRegistryForModule() {
   diff --git a/core/themes/bartik/bartik.info.yml b/core/themes/bartik/bartik.info.yml
   
   diff --git a/core/themes/bartik/bartik.info.yml b/core/themes/bartik/bartik.info.yml
   index fb8f2fb..fa73bcb 100644
   
   index fb8f2fb..fa73bcb 100644
   --- a/core/themes/bartik/bartik.info.yml
   
   --- a/core/themes/bartik/bartik.info.yml
   +++ b/core/themes/bartik/bartik.info.yml
   
   +++ b/core/themes/bartik/bartik.info.yml
   +++ b/core/themes/bartik/bartik.info.yml
   @@ -5,10 +5,10 @@ description: 'A flexible, recolorable theme with many regions and a responsive,
   
   @@ -5,10 +5,10 @@ description: 'A flexible, recolorable theme with many regions and a responsive,
    package: Core
    version: VERSION
    core: 8.x
   -stylesheets-remove:
   -  - '@classy/css/layout.css'
    libraries:
      - bartik/global-styling
   +libraries-override:
   +  classy/base/css/theme/css/layout.css: false
    ckeditor_stylesheets:
      - css/base/elements.css
      - css/components/captions.css
   @@ -35,4 +35,3 @@ regions:
   
   @@ -35,4 +35,3 @@ regions:
      footer_third: 'Footer third'
      footer_fourth: 'Footer fourth'
      footer_fifth: 'Footer fifth'
   -
   diff --git a/core/themes/seven/seven.info.yml b/core/themes/seven/seven.info.yml
   
   diff --git a/core/themes/seven/seven.info.yml b/core/themes/seven/seven.info.yml
   index 99a3ec1..daf74a0 100644
   
   index 99a3ec1..daf74a0 100644
   --- a/core/themes/seven/seven.info.yml
   
   --- a/core/themes/seven/seven.info.yml
   +++ b/core/themes/seven/seven.info.yml
   
   +++ b/core/themes/seven/seven.info.yml
   +++ b/core/themes/seven/seven.info.yml
   @@ -8,9 +8,9 @@ version: VERSION
   
   @@ -8,9 +8,9 @@ version: VERSION
    core: 8.x
    libraries:
     - seven/global-styling
   -stylesheets-remove:
   -  - core/assets/vendor/jquery.ui/themes/base/dialog.css
   -  - '@classy/css/layout.css'
   +libraries-override:
   +  core/jquery.ui.dialog/css/component/assets/vendor/jquery.ui/themes/base/dialog.css: false
   +  classy/base/css/theme/css/layout.css: false
    quickedit_stylesheets:
      - css/components/quickedit.css
    regions:
   diff --git a/core/themes/stark/stark.info.yml b/core/themes/stark/stark.info.yml
   
   diff --git a/core/themes/stark/stark.info.yml b/core/themes/stark/stark.info.yml
   index 69337d5..ed70cd6 100644
   
   index 69337d5..ed70cd6 100644
   --- a/core/themes/stark/stark.info.yml
   
   --- a/core/themes/stark/stark.info.yml
   +++ b/core/themes/stark/stark.info.yml
   
   +++ b/core/themes/stark/stark.info.yml
   +++ b/core/themes/stark/stark.info.yml
   @@ -6,5 +6,5 @@ version: VERSION
   
   @@ -6,5 +6,5 @@ version: VERSION
    core: 8.x
    libraries:
      - stark/global-styling
   -stylesheets-remove:
   -  - core/assets/vendor/normalize-css/normalize.css
   +libraries-override:
   +  core/normalize/css/base/assets/vendor/normalize-css/normalize.css: false
   

   Doesn't this mean we have no test coverage left for stylesheets-remove?

Thanks @WimLeers. #60:
1. Fixed
2. Done. I noticed we never use private in Drupal, even in cases like this where we don't want the method to be sub-classed. But no harm though.
3. Done.
4. Fixed.
5. There's still test coverage in ThemeInfoTest::testStylesheets() using the test_basetheme.info.yml and test_subtheme.info.yml which I reverted in the last patch (see the interdiff).
Since we're deprecating stylesheets-remove, I don't think we should be using it in core themes. Note that this test is still useful for libraries-override so can be left in when stylesheets-remove is eventually, well, removed.

Alright. Thanks!

Splendid work here! :)

I think we should deprecate stylesheets-remove a schedule it for removal in Drupal 9.0. Which is the approach the current patch takes - great! And we've still got test coverage in Drupal\system\Tests\Theme\ThemeInfoTest. Still to review.

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -314,6 +316,71 @@ protected function parseLibraryInfo($extension, $path) {
   +            throw new \LogicException(SafeMarkup::format('Library asset %asset is not correctly specified. It should be in the form "extension/library_name/sub_key/path/to/asset.js".', ['%asset' => $asset]));
   ...
   +            throw new \LogicException(SafeMarkup::format('drupalSettings may not be overridden in libraries-override. Trying to override %asset. Use hook_library_info_alter() instead.', ['%asset' =>$asset]));
   

   Let's use sprintf here - exception messages don;t need to go through SafeMarkup.

2. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -161,27 +161,27 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
        foreach ($base_themes as $base) {
   -      $base_theme_path = $base->getPath();
   -      if (!empty($base->info['stylesheets-remove'])) {
   -        foreach ($base->info['stylesheets-remove'] as $css_file) {
   -          $css_file = $this->resolveStyleSheetPlaceholders($css_file);
   -          $values['stylesheets_remove'][$css_file] = $css_file;
   +      if (!empty($base->info['libraries-override'])) {
   +        foreach ($base->info['libraries-override'] as $library => $override) {
   +          $values['libraries_override'][$library] = $override;
            }
          }
        }
    
   -    // Add stylesheets used by this theme.
   -    if (!empty($theme->info['stylesheets-remove'])) {
   -      foreach ($theme->info['stylesheets-remove'] as $css_file) {
   -        $css_file = $this->resolveStyleSheetPlaceholders($css_file);
   -        $values['stylesheets_remove'][$css_file] = $css_file;
   +    // Add libraries-override declared by this theme.
   +    if (!empty($theme->info['libraries-override'])) {
   +      foreach ($theme->info['libraries-override'] as $library => $override) {
   +        $values['libraries_override'][$library] = $override;
          }
   

   Do we test base theme inheritance of library overrides anywhere? I couldn't spot it.

3. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -42,4 +45,125 @@ public function testElementInfoByTheme() {
   +
   +  protected function assertAssetInLibrary($library_item, $library_name, $asset, $message = NULL) {
   +    if (!isset($message)) {
   +      $message = SafeMarkup::format('Asset @asset found in library @library', ['@asset' => $asset, '@library' => $library_name]);
   +    }
   +    foreach ($library_item as $definition) {
   +      if ($asset == $definition['data']) {
   +        $this->pass($message);
   ...
   +      }
   +    }
   +    $this->fail($message);
   +  }
   +
   +  protected function assertNoAssetInLibrary($library_item, $library_name, $asset, $message = NULL) {
   +    if (!isset($message)) {
   +      $message = SafeMarkup::format('Asset @asset not found in library @library', ['@asset' => $asset, '@library' => $library_name]);
   +    }
   +    foreach ($library_item as $definition) {
   +      if ($asset == $definition['data']) {
   +        $this->fail($message);
   +        return;
   +      }
   +    }
   +    $this->pass($message);
   +  }
   

   Missing docblocks and an asserts these should return a bool. Just do return $this->pass($message); and return $this->fail($message);

4. +++ b/core/modules/system/tests/themes/test_theme/test_theme.info.yml
   @@ -14,8 +14,18 @@ description: 'Theme for testing the theme system'
   -stylesheets-remove:
   -  - '@system/css/system.module.css'
   

   We need to ensure stylesheets-remove is still tested - how about leaving this in and removing another stylesheet in the libraries-override.

There is not tests for the theme inheritance and libraries-override yet. Fixed other points from Alexpotts comment and might be fixing the tests little later.

Thanks @laurii for #66
Working from there, added test coverage for basetheme/subtheme libraries-override inheritance. Nice catch @alexpott!! This revealed some bugs which has led to a gamut of fixes.

In this patch:
1. Fixed problems with parsing of protocol-free and full-protocol URI's in libraries-override and added tests.
2. Fixed HEAD's mangling of full-protocol URI's if 'external' is not specified (a bug??) and added a test.
3. Fixed some more calls to SafeMarkup::format() in exceptions and assertions.

Oh... TwigTransTest :/ Changed all the test files to css to avoid locale_js_alter() hook's file checks.

In the process I've found that _locale_parse_js_file() rejects files specified with streamwrappers e.g. public://dir/file.js - that's another issue I guess.

Was too sleepy to post this yesterday...

Thanks a lot for working on this! Here's what I found for now:

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -185,6 +187,13 @@ public function buildByExtension($extension) {
   +            else if ($this->isValidUri($source)) {
   

   s/else if/elseif

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -314,6 +323,71 @@ protected function parseLibraryInfo($extension, $path) {
   +    return $libraries;
   

   Even though its not a coding standard in Drupal I'd prefer a extra line before return :)

3. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -161,27 +161,27 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
   +    // Prepare libraries overrides from this theme and ancestor themes.
   +    // This allows child themes to easily remove CSS files from base themes and
   

   This could be on the first line I think

4. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -161,27 +161,27 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
   +    $values['libraries_override'] = array();
   

   s/array()/[]/

5. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -253,4 +255,77 @@ protected function resolveStyleSheetPlaceholders($css_file) {
   +   * @return array
   

   Maybe string[]

Shamsher_Alam: Thank you for working on this issue! Maybe if you have time you could still add an interdiff for the patch you posted? Here's tutorial how that happens: https://www.drupal.org/documentation/git/interdiff

@ Shamsher_Alam: I think you've lost some parts of the patch. The patch at #75 is 33KB while yours is 30KB.

@lauriii
Inter diff of patch.

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -314,6 +323,72 @@ protected function parseLibraryInfo($extension, $path) {
   +    ¶
   

   Extra space

2. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -225,7 +225,7 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
        if (!isset($this->extensions)) {
   -      $this->extensions = array_merge($this->moduleHandler->getModuleList(),  $this->themeHandler->listInfo());
   +      $this->extensions = array_merge($this->moduleHandler->getModuleList(), $this->themeHandler->listInfo());
        }
   

   This change is out of scope.

3. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -253,4 +255,77 @@ protected function resolveStyleSheetPlaceholders($css_file) {
   +   * @return string
   

   s/string/string[]/

Fixed #85

I am rerolling this.

reroll

This is not an exhaustive review.

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
   @@ -9,8 +9,6 @@
   -use Drupal\Core\Extension\ModuleHandlerInterface;
   -use Drupal\Core\Theme\ThemeManagerInterface;
   

   This looks like an unrelated change.

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -327,4 +402,11 @@ protected function fileValidUri($source) {
   +    return count(explode('://', $string)) === 2;
   

   Is there not already a core function for testing valid URIs that we could use here?

3. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -198,4 +209,24 @@ public function getRegions() {
   +   * @return array
   

   This needs a return comment.

4. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -256,4 +258,77 @@ protected function resolveStyleSheetPlaceholders($css_file) {
   +   * @return string[]
   

   This needs a return comment.

5. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -256,4 +258,77 @@ protected function resolveStyleSheetPlaceholders($css_file) {
   +   * @return string
   

   This needs a return comment.

6. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -42,4 +45,211 @@ public function testElementInfoByTheme() {
   +   * Tests library assets with edge-casey paths.
   

   Non-native speakers of English may not understand the invented word "edge-casey".

7. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -42,4 +45,211 @@ public function testElementInfoByTheme() {
   +   * @return bool
   

   A comment for this return is needed.

8. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -42,4 +45,211 @@ public function testElementInfoByTheme() {
   +   * @return bool
   

   A comment for this return is needed.

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -7,6 +7,7 @@
   +use Drupal\Component\Utility\SafeMarkup;
   

   Is this use statement needed by the added code?

2. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -7,6 +7,9 @@
   +use Drupal\Component\Utility\SafeMarkup;
   +use Drupal\Core\Extension\Extension;
   +use Drupal\Core\Theme\ActiveTheme;
   

   These do not seem to be used in the class.

#92:
1. Not really related, just code cleanup ;)
2. Didn't find any.
3. Well, I thought that would just be redundant considering what's already in the doc block. But no problem. Fixed.
4. Fixed
5. Fixed
6. Fixed
7. Fixed
8. Fixed
1. Fixed
2. Fixed

Thanks for the review @cilefen

Patch

We should get a little manual testing on this to make sure it is working well and to make sure how it operates makes sense for themers. You can use the change record to see how it works, or we can probably add some instructions if needed.

Re-roll after #2349711: Remove all visual from stark + fixed #96 nitpicks.

Thanks for the review. Still needs manual testing per #95. Looks RTBC from my end though.

Waiting on this to go in so we can progress a patch for #2497667: Add libraries-extend to themes' *.info.yml

I think sub_key is fine as that was what it was. I was thinking group or type... that may be a slight improvement... but unless someone has better or likes one of those, I think we can leave that as sub_type too. @Cottser follow-up maybe?

Type could be easily confused with other meanings so I didn't go with that. I think group or sub-group sounds better. I didn't really like sub-key myself.

To add to my last comment, should we also mention it needs to include the category for CSS? That could easily throw people.

We are fixing that in #2389203: Validate the CSS categories in libraries.yml files..

Should the "entire library override" option check to make sure the replacement library exists (or is false)? Otherwise it fails silently from what I can tell.
Should we check to make sure that the library being specified on the left hand side exists? Otherwise a typo will fail silently.
Should we check to make sure an individual asset on the left side exists?

These make sense. TX++. Will add in the next patch.

This should throw an exception if a non-existent library is assigned on the right-hand side.
It also throws an exception for a non-existent library or library asset.

It doesn't yet verify that the libraries and assets on the left side exists. The code for that appears quite complex.

Next, work on the tests.

Fixed the buggy code. This should:

1. throw an exception if a non-existent library is assigned on the right-hand side.
2. throw an exception for a non-existent library on the left-hand side
3. throw an exception for a non-existent library asset specified on the left-hand side

Added tests for 2) and 3). Will add test for 1) tomorrow.

Re #18, we have a non-passing patch. I feel like we should postpone to 8.1.

@cilefen: let's try one more time... if we can't get the DX improvements requested in #102 to pass, then maybe we just do without those. Patch was passing before then.

Thinking through this, I just realized I was doing it the wrong way. The LibraryDiscoveryParser::applyLibrariesOverride() only knows about the libraries in one extension at a time, so there is no clean way to verify that all libraries overrides for all extensions are correctly specified.

Again, considering some more, I realized that #102 is better implemented using assertions (https://drupal.org/node/2492225) since it's for the themer writing the theme and not the user.

I've therefore opened a follow-up #2562839: Follow-up to [#2451411]: Add assertions in Library(Discovery)Parser to prevent typos. to do that using assertions and removed it from this patch. Added an interdiff against #97 to see what has actually changed.

This looks awesome. Mostly nitpicks, and a few code organization questions/remarks.

Which tests are still missing?

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
   @@ -79,7 +78,34 @@ public function getLibrariesByExtension($extension) {
   +          // Remove the library definition if false is given.
   

   Nit: s/false/FALSE/ for improved clarity.

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -185,6 +187,13 @@ public function buildByExtension($extension) {
   +            // A regular URI (e.g., http://example.com/example.js) without
   +            // 'external' explicitly specified, which may happen if, e.g.
   +            // libraries-override is used.
   +            elseif ($this->isValidUri($source)) {
   

   Shouldn't this instead throw an exception? That sounds like an invalid override to me.

3. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -314,6 +323,76 @@ protected function parseLibraryInfo($extension, $path) {
   +        // Active theme defines an override for this library.
   +        if ($asset === "$extension/$name") {
   ...
   +        elseif (strpos($asset, "$extension/$name/") !== FALSE) {
   +          // Active theme defines an override for an asset within this library.
   

   If possible, it'd be great if the code in these if-tests could be moved into helper method, that'd make it much easier to follow.

4. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -198,4 +209,25 @@ public function getRegions() {
   +   *   The list of libraries-override.
   

   s/libraries-override/libraries overrides/

5. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -161,27 +161,27 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
   +    // Prepare libraries overrides from this theme and ancestor themes.
   +    // This allows child themes to easily remove CSS files from base themes and
   

   Nit: "This" fits on the previous line, perhaps even "allows".

6. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -161,27 +161,27 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
   +    // Get libraries-override declared by base theme.
   ...
   +    // Add libraries-override declared by this theme.
   

   s/libraries-override/libraries overrides/
   s/base theme/base themes/

7. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -42,4 +43,213 @@ public function testElementInfoByTheme() {
   +    /** @var \Drupal\Core\Theme\ThemeInitializationInterface $theme_initializer */
   +    $theme_initializer = $this->container->get('theme.initialization');
   +
   +    /** @var \Drupal\Core\Theme\ThemeManagerInterface $theme_manager */
   +    $theme_manager = $this->container->get('theme.manager');
   +
   +    /** @var \Drupal\Core\Render\ElementInfoManagerInterface $element_info */
   +    $library_discovery = $this->container->get('library.discovery');
   +
   +    $theme_manager->setActiveTheme($theme_initializer->getActiveThemeByName('test_theme'));
   ...
   +    $this->container->get('theme_handler')->install(['test_theme_libraries_override_with_drupal_settings']);
   +
   +    /** @var \Drupal\Core\Theme\ThemeInitializationInterface $theme_initializer */
   +    $theme_initializer = $this->container->get('theme.initialization');
   +
   +    /** @var \Drupal\Core\Theme\ThemeManagerInterface $theme_manager */
   +    $theme_manager = $this->container->get('theme.manager');
   +
   +    /** @var \Drupal\Core\Render\ElementInfoManagerInterface $element_info */
   +    $library_discovery = $this->container->get('library.discovery');
   +
   ...
   +    $this->container->get('theme_handler')->install(['test_theme_libraries_override_with_invalid_asset']);
   +
   +    /** @var \Drupal\Core\Theme\ThemeInitializationInterface $theme_initializer */
   +    $theme_initializer = $this->container->get('theme.initialization');
   +
   +    /** @var \Drupal\Core\Theme\ThemeManagerInterface $theme_manager */
   +    $theme_manager = $this->container->get('theme.manager');
   +
   +    /** @var \Drupal\Core\Render\ElementInfoManagerInterface $element_info */
   +    $library_discovery = $this->container->get('library.discovery');
   ...
   +    $this->container->get('theme_handler')->install(['test_theme']);
   +
   +    /** @var \Drupal\Core\Theme\ThemeInitializationInterface $theme_initializer */
   +    $theme_initializer = $this->container->get('theme.initialization');
   +
   +    /** @var \Drupal\Core\Theme\ThemeManagerInterface $theme_manager */
   +    $theme_manager = $this->container->get('theme.manager');
   +
   +    /** @var \Drupal\Core\Render\ElementInfoManagerInterface $element_info */
   +    $library_discovery = $this->container->get('library.discovery');
   ...
   +    $this->container->get('theme_handler')->install(['test_subtheme']);
   +
   +    /** @var \Drupal\Core\Theme\ThemeInitializationInterface $theme_initializer */
   +    $theme_initializer = $this->container->get('theme.initialization');
   +
   +    /** @var \Drupal\Core\Theme\ThemeManagerInterface $theme_manager */
   +    $theme_manager = $this->container->get('theme.manager');
   +
   +    /** @var \Drupal\Core\Render\ElementInfoManagerInterface $element_info */
   +    $library_discovery = $this->container->get('library.discovery');
   

   Can't you move this into a helper method? It'd make the tests much easier to read.

8. +++ b/core/modules/system/tests/themes/test_basetheme/test_basetheme.info.yml
   --- /dev/null
   +++ b/core/modules/system/tests/themes/test_theme/css/collapse.css
   
   +++ b/core/modules/system/tests/themes/test_theme/css/collapse.css
   --- /dev/null
   +++ b/core/modules/system/tests/themes/test_theme/js/collapse.js
   

   IIRC these dummy files are not necessary.

9. +++ b/core/modules/system/tests/themes/test_theme/test_theme.info.yml
   @@ -18,6 +18,25 @@ stylesheets-remove:
   +  # Use streamwrappers.
   

   s/streamwrappers/stream wrappers/

10. +++ b/core/modules/system/tests/themes/test_theme/test_theme.info.yml
    @@ -18,6 +18,25 @@ stylesheets-remove:
    +  # Use protocol-free URI.
    

    s/protocol-free/protocol-relative/

Thanks for the review @Wim Leers:
1. Fixed
2. This was introduced to allow e.g. to override a script with some version located in another domain. Are you saying this is not a valid use case?
3. How's this?
4. Done.
5. Done.
6. Done.
7. Will do in next patch.
8. See #49.10 where I responded to same query in #48.10
9. Done.
10. Done.

This patch re-organizes the test per #117.7 to improve readability.

Self-review: could only find some nits, can be fixed on commit - or the next re-roll.

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -327,4 +407,39 @@ protected function fileValidUri($source) {
   +   * Checks if the specified definition asset is a full library .
   

   Extra space...

2. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -17,29 +18,207 @@
   +    // Activate test theme that has subthemes
   

   Period at comment end.

Fixed the nitpicks and reviewed the code, looks good to me.

This now has tests, no need for the tag:)

Let's get some people who want this feature to manually test to ensure it's doing what they expect!

Screenshots proving you manually tested and it worked would be appreciated.

I was trying to override and remove a CSS from both module and theme but couldn't get neither working. Am I doing something wrong? This is what I added to my theme info.yml file:

libraries-override:
  classy/base/css/theme/css/layout.css: false
  system/base/css/components/button.theme.css: css/components/button.theme.css
  classy/base/css/components/pager.css: false

I'm also very worried about DX/TX of the current solution because it might work a little bit too much with magic. It looks first like a file path but it actually includes the theme/module name and the library name and then it has the path for the CSS file.

I'm also very worried about DX/TX of the current solution because it might work a little bit too much with magic. It looks first like a file path but it actually includes the theme/module name and the library name and then it has the path for the CSS file.

That's what had been proposed right from the issue inception. Do you hav something better?

libraries-override:
  classy/base/css/theme/css/layout.css: false
  system/base/css/components/button.theme.css: css/components/button.theme.css
  classy/base/css/components/pager.css: false

The first should work. The next two don't looked like they correctly specify existing library assets. (You're right - maybe we need to improve the syntax :))

The issue #2562839: Follow-up to [#2451411]: Add assertions in Library(Discovery)Parser to prevent typos. would help improve TX/DX by throwing exceptions when invalid entries are made.

Can you do another test. I'll so manual testing of my own.

CNW for further reviews / manual testing by others.

Would it be possible to use a different separator? I'm not too familiar with YML conventions.

Something like theme:library:path/to/css/style.css ?

theme:library:path/to/css/style.css

That'd be inconsistent/confusing too IMHO. Because theme/library already is a thing in library dependencies.

So, IMHO theme/library@path/to/css/style.css would be better, where @ is the separator we'd like to use.

We could also stick to something similar to what we already do in library files, which is just using separate lines?

libraries-override:
  # Replace an entire library
  core:
      drupal.collapse: mytheme/collapsed
  # Replace one particular library asset with another.
  subtheme:
      library-name:
          css/theme/css/layout.css: css/layout.css
  # Remove one particular asset.
  core:
      library-name:
          css/theme/dialog.theme.css: false

I'm not entirely sure how the last one would work.

#127++

Both #126 and #127 are great suggestions but #127 sounds a bit better for me

subtheme:
      library-name:
          css/theme/css/layout.css: css/layout.css

This still looks magical to non-"insiders". There's still potential to confuse css/theme/css/layout.css with an actual filesystem path. So if we're using the same kind syntax to existing libraries.info.yml, how about:

# Replace an entire library
core/drupal.collapse: mytheme/collapsed

# Replace a part of a library
subtheme/library-name:
 css:
    theme:
      css/layout.css: css/layout.css
module/library-name:
 js:
    js/script.js: js/my-script.js 

# Replace a part of a library
subtheme/library-name:
 css:
    theme:
      css/layout.css: css/layout.css

Oh I wasn't paying attention. I literally took that to be a system path. What you have in #130 makes sense. That looks to be exactly the same as what we do in the library file.

For example, from the system module:

maintenance:
  version: VERSION
  css:
    theme:
      css/system.maintenance.css: { weight: -10 }
  dependencies:
    - system/base
    - system/admin

Just to be clear, "theme" is the SMACSS category. So when overriding the CSS file you would just replicate the same category used by the original library (theme, component, base, layout), correct?

And to add, it looks the only different is that you have to specify the theme/module name, but that is because we don't have unique library names. I think that is ok.

but that is because we don't have unique library names

That's not really true. Library names are unique within an extension (module/theme). And global uniqueness is ensured because libraries are namespaced by the extension: extension/library.

That's why we have this syntax :)

That's why libraries are able to declare dependencies on any other library.

That's what I'm saying. The library name itself isn't global. We have to reference the extension, which is something we don't have to do when creating the library. I'm just remarking that it is slightly different syntax to creating the library, so we have to make sure it is documented.

Right, fair! Something like This looks exactly like a *.libraries.yml file, but rather than just specifying the library name, we need to specify a fully qualified library reference: extension/library, because we're able to override libraries from any extension.

Just to be clear, "theme" is the SMACSS category. So when overriding the CSS file you would just replicate the same category used by the original library (theme, component, base, layout), correct?

Correct!

Something like...

+1

Re-roll

Working on this now

This patch implements the syntax in #130, also extends the test coverage. Some interesting things in the course of making these changes:

1. The introduction of libraries-override means each time the active theme is changed, the libraries registry has to be rebuilt. Not sure if that should be incorporated into this patch or a follow-up.
2. The syntax for libraries-extend would be similar (ideas are forming in that issue #2497667: Add libraries-extend to themes' *.info.yml)

Expect some test fails as the LibraryDiscovery cache invalidation doesn't seem to be working fine in LibraryDiscoveryIntegrationTest. Will look into that later.

I was talking with @fabianx and he said that this might be useful tool to help people using libraries properly which is important for BigPipe to work correctly.

In trying to fix the test fails in #142, I found LibraryDiscovery::clearCachedDefinitions(), does not properly clear the static and persistent caches. So raised #2575735: LibraryDiscoveryCollector::reset() does not properly reset its $cid, resulting in loading wrong library assets if the active theme changes

Moved some of the code around to fix a hidden flaw in the logic.

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
@@ -79,7 +78,34 @@ public function getLibrariesByExtension($extension) {
   public function getLibraryByName($extension, $name) {
     $extension = $this->getLibrariesByExtension($extension);
-    return isset($extension[$name]) ? $extension[$name] : FALSE;
+    if (isset($extension[$name])) {
+      // Handle libraries that are marked for override or removal.
+      // @see \Drupal\Core\Asset\LibraryDiscoveryParser::applyLibrariesOverride()
+      if (isset($extension[$name]['override'])) {
+        $override = $extension[$name]['override'];
+        if ($override === FALSE) {
+          // Remove the library definition if FALSE is given.
+          unset($extension[$name]);
+          return FALSE;
+        }
+        else {
+          // Otherwise replace with existing library definition if it exists.
+          // Throw an exception if it doesn't.
+          list($new_extension, $new_name) = explode('/', $override);
+          if ($library = $this->getLibraryByName($new_extension, $new_name)) {
+            $extension[$name] = $library;
+          }
+          else {
+            throw new InvalidLibrariesOverrideSpecificationException(sprintf('The specified library %s does not exist.', $override));
+          }
+        }
+      }
+      return $extension[$name];
+    }
+    else {
+      return FALSE;
+    }
+
   }

This implementation is flawed in that a call to LibraryDiscovery::getLibrariesByExtension() would miss out on the overrides of entire libraries. And that method is a part of the LibraryDiscoveryInterface. This patch refactors the code to ensure that a call to either public interface methods returns similar results.

Also fixed the LibraryDiscovery::clearCachedDefinitions() bug so tests here can pass. Will transfer that hunk over to #2575735: LibraryDiscoveryCollector::reset() does not properly reset its $cid, resulting in loading wrong library assets if the active theme changes.

Re-roll

Don't give up hope, almaudoh. We're all seeing these updates and rooting you on!

Thanks @davidhernandez :). Almost there... Just a minor fix on a unit test.

Oops :( ... silly me. While the interdiff in #154 is correct I went and uploaded the exact same patch as in #150. Here's the right patch now, same interdiff, sorry for the noise.

Thanks for your hard work, @almaudoh!

The introduction of libraries-override means each time the active theme is changed, the libraries registry has to be rebuilt. Not sure if that should be incorporated into this patch or a follow-up.

Oh, wow, that sounds very bad. Unacceptable, even. #2575735: LibraryDiscoveryCollector::reset() does not properly reset its $cid, resulting in loading wrong library assets if the active theme changes cannot possibly fix that. But are you sure this is the case? The library "registry" already is cached per theme. Go to the front page and then to an admin page, then note how both library_info:bartik and library_info:seven exist in the cache.discovery cache bin (cache_discovery DB table).

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
@@ -62,23 +61,14 @@ public function __construct(CacheCollectorInterface $library_discovery_collector
   public function getLibrariesByExtension($extension) {
-    if (!isset($this->libraryDefinitions[$extension])) {
-      $libraries = $this->collector->get($extension);
-      $this->libraryDefinitions[$extension] = [];
-      foreach ($libraries as $name => $definition) {
-        $library_name = "$extension/$name";
-        $this->libraryDefinitions[$extension][$name] = $definition;
-      }
-    }
-
-    return $this->libraryDefinitions[$extension];
+    return $this->getLibraryDefinitions($extension);
   }

@@ -87,6 +77,39 @@ public function getLibraryByName($extension, $name) {
+  protected function getLibraryDefinitions($extension) {
+    if (!isset($this->libraryDefinitions[$extension])) {
+      $libraries = $this->collector->get($extension);
+      $this->libraryDefinitions[$extension] = [];
+      foreach ($libraries as $name => $definition) {
+        // Handle libraries that are marked for override or removal.
+        // @see \Drupal\Core\Asset\LibraryDiscoveryParser::applyLibrariesOverride()

The before vs after here is AFAICT the root cause of the caching problems you're seeing.

Note how \Drupal\Core\Asset\LibraryDiscovery in HEAD says this:

  /**
   * The final library definitions, statically cached.
   *
   * hook_library_info_alter() and hook_js_settings_alter() allows modules
   * and themes to dynamically alter a library definition (once per request).
   *
   * @var array
   */
  protected $libraryDefinitions = [];

i.e. the sole reason we have LibraryDiscovery on top of LibraryDiscoveryCollector, is for these uncacheable, super-dynamic alter hooks. They're only cacheable within a request (and therefore use static caching), and not across requests. For the things that are cacheable across requests, we have LibraryDiscoveryCollector.

And here's the key thing: libraries-override is cacheable across requests, so it should actually live in LibraryDiscoveryCollector, and not here. That already is cached per theme, and libraries overrides are also per theme, so that will then actually work perfectly :)

Incomplete review — I focused on the above part of my comment — reviewing the rest mostly needs people like @davidhernandez and @lauriii.

1. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -17,29 +18,228 @@
   +   *   The extension where-in the $library_name is defined.
   

   s/where-in/in which/

2. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
   @@ -17,29 +18,228 @@
   +   *   The library sub_key where the given asset is defined.
   

   s/sub_key/sub key/

3. +++ b/core/modules/system/tests/themes/test_theme/test_theme.info.yml
   @@ -18,6 +18,49 @@ stylesheets-remove:
   +  # Use Drupal relative paths.
   

   Nit: s/Drupal relative/Drupal-relative/

4. +++ b/core/modules/system/tests/themes/test_theme/test_theme.info.yml
   @@ -18,6 +18,49 @@ stylesheets-remove:
   +  # Use regular URI.
   

   s/regular/absolute/

5. +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
   @@ -93,7 +93,7 @@ public function testGetRegistryForModule() {
   -      'stylesheets_override' => [],
   +      'libraries_override' => [],
   

   I understand the addition, by why the removal?

Thanks for the review @Wim Leers.

But are you sure this is the case? The library "registry" already is cached per theme. The library "registry" already is cached per theme. Go to the front page and then to an admin page, then note how both library_info:bartik and library_info:seven exist in the cache.discovery cache bin (cache_discovery DB table).

I will do another check, but I think the second point below takes care of that.

And here's the key thing: libraries-override is cacheable across requests, so it should actually live in LibraryDiscoveryCollector, and not here. That already is cached per theme, and libraries overrides are also per theme, so that will then actually work perfectly :)

Agree completely. In fact, I was thinking of doing that in a follow-up, but we might as well just do that here.

So this patch moves the libraries-override cleanup code for entire libraries from LibraryDiscovery into LibraryDiscoveryCollector.

Fixed #158 1 - 4. #158.5 was removed because stylesheets-override was removed in #2389735: Core and base theme CSS files in libraries override theme CSS files with the same name. This is just a leftover which we might as well remove in this issue.

RTBC - looks great to me now!

Looks great!

Docs fixes

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryCollector.php
   @@ -79,9 +80,52 @@ protected function getCid() {
   +   *   The library definitions for $extension will overrides applied.
   

   s/will/with/

2. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
   @@ -198,4 +209,25 @@ public function getRegions() {
   +   * The array returned is in the form:
   +   * @code
   +   * [
   +   *   'core/drupal.collapse' => 'test_theme/collapse',
   +   *   'core/drupal.progress' => FALSE,
   +   *   'classy/base/css/theme/css/layout.css' => 'css/test_theme_layout.css',
   +   *   'core/drupal.dialog/css/theme/misc/dialog.theme.css' => FALSE,
   +   *   'core/jquery/js/assets/vendor/jquery/jquery.min.js' => 'js/collapse.js',
   +   * ]
   +   * @endcode
   

   Comment no longer valid or needed.

Some of the library files used in tests were moved around in #2566771: [Voltron patch] Move all remaining *.theme.css to Classy.

Sorry, patch in #165 is wrong (mixed up some work I'm currently doing on libraries-extend in there). Re-attached correct patch and interdiff.

Patch looks great to me. Ticking some credit boxes.

I think the patch already includes good test coverage and some manual testing of this has already been done. I also think that if manual testing uncovers problems during RC, they can be fixed. If I turn out to be wrong, we might need to revert this, but in the meantime, pushed to 8.0.x! Thanks for the great work on this.

Wooooot!!! Thanks.

Updated change record to match new syntax - https://www.drupal.org/node/2497313/revisions/view/8951259/8951961

This now unblocks #2497667: Add libraries-extend to themes' *.info.yml which already has a green patch. Hope we can get that in also before RC1.

Very awesome work by you, almaudoh. Thank you.

This patch has a bug, see #2642046: libraries-override does not update drupalSettings libraries array.

And one more issue #2642122: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides