Follow-up to #2389735: Core and base theme CSS files in libraries override theme CSS files with the same name

Problem/Motivation

stylesheets-override and stylesheets-remove as is don't necessarily give you the level of granularity needed to meet real world use cases.

Proposed resolution

From @Wim Leers in #9

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-override
The ability to override entire libraries and parts of libraries:
libraries-override:
  # Replace an entire library
  core/drupal.collapse: mytheme/collapsed
  # 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().)

Remaining tasks

  1. Add libraries-override

User interface changes

N/A

API changes

API additions.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it is an API addition. Nothing is broken.
Issue priority Major this would be a huge themer experience win and needed to help provide functionality that was lost when stylesheets-override was removed. Not critical because nothing is broken.
Unfrozen changes Unfrozen because it is an API addition.
Disruption This should not be a disruptive change unless we decide to remove stylesheets-remove.
Files: 
CommentFileSizeAuthor
#166 libraries-override-2451411-166.patch37.93 KBalmaudoh
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,589 pass(es). View
#166 interdiff.txt4.34 KBalmaudoh
#165 libraries-override-2451411-165.patch39.03 KBalmaudoh
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,281 pass(es), 4 fail(s), and 1 exception(s). View
#165 interdiff.txt3.58 KBalmaudoh
#163 libraries-override-2451411-163.patch37.97 KBalmaudoh
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,268 pass(es), 1 fail(s), and 0 exception(s). View
#163 interdiff.txt1.52 KBalmaudoh
#160 libraries-override-2451411-160.patch38.38 KBalmaudoh
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,764 pass(es). View
#160 interdiff.txt11.48 KBalmaudoh
#157 libraries-override-2451411-157.patch39.12 KBalmaudoh
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,525 pass(es). View
#157 interdiff.txt997 bytesalmaudoh
#154 libraries-override-2451411-150.patch38.15 KBalmaudoh
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#154 interdiff.txt997 bytesalmaudoh
#150 libraries-override-2451411-150.patch38.15 KBalmaudoh
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#147 libraries-override-2451411-147.patch38.15 KBalmaudoh
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#147 interdiff.txt5.93 KBalmaudoh
#142 libraries-override-2451411-142.patch36.69 KBalmaudoh
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,164 pass(es), 11 fail(s), and 0 exception(s). View
#142 interdiff.txt24.24 KBalmaudoh
#138 add_libraries_override-2451411-138.patch34.8 KBalmaudoh
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,659 pass(es), 1 fail(s), and 0 exception(s). View
#121 add_libraries_override-2451411-121.patch35.03 KBborisson_
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,523 pass(es). View
#121 interdiff.txt1.33 KBborisson_

Comments

Cottser’s picture

Title: stylesheets-override and stylesheets-remove should work by library or extension » stylesheets-override and stylesheets-remove should work by library or extension (module/theme name)
Issue summary: View changes
dawehner’s picture

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.

LewisNyman’s picture

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.

davidhernandez’s picture

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.

lauriii’s picture

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.

Cottser’s picture

I think override still makes sense and is needed because libraries are #attached only in certain cases, 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.

It seems pretty possible to me to allow for overriding/removing a single CSS file from a library. And I think if all CSS is added by libraries it probably makes sense to override/remove that way rather than by extension. As long as library names are globally unique – which I think they are?

davidhernandez’s picture

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

davidhernandez’s picture

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.

Wim Leers’s picture

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.

davidhernandez’s picture

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.

Cottser’s picture

Indeed, I would love to embrace libraries, what @davidhernandez said :)

Cottser’s picture

Priority: Normal » Major
Issue tags: +DX (Developer Experience)

Arguably a major improvement to be had in the granularity and control we are talking about here :)

Wim Leers’s picture

Title: stylesheets-override and stylesheets-remove should work by library or extension (module/theme name) » stylesheets-override and stylesheets-remove should work by library

It looks like we agree on that.

sqndr’s picture

I agree with #10 as well. :)

davidhernandez’s picture

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

joelpittet’s picture

Title: stylesheets-override and stylesheets-remove should work by library » Add libraries-remove and libraries-override to *.info.yml
Issue summary: View changes
joelpittet’s picture

Issue summary: View changes
davidhernandez’s picture

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.

mducharme’s picture

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?

davidhernandez’s picture

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.

andypost’s picture

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

jstoller’s picture

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

almaudoh’s picture

Assigned: Unassigned » almaudoh

Working on this for the next couple of hours.

almaudoh’s picture

Title: Add libraries-remove and libraries-override to *.info.yml » Add libraries-remove and libraries-override to themes' *.info.yml
Assigned: almaudoh » Unassigned
Status: Active » Needs review
FileSize
11.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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.

Status: Needs review » Needs work

The last submitted patch, 24: libraries-override-2451411-24.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 24: libraries-override-2451411-24.patch, failed testing.

Cottser’s picture

+++ b/core/modules/system/tests/themes/test_theme/test_theme.info.yml
@@ -20,3 +20,13 @@ regions:
+  'core/drupal.dialog/css/theme/misc/dialog.theme.css': FALSE

+++ b/core/themes/bartik/bartik.info.yml
@@ -5,10 +5,10 @@ description: 'A flexible, recolorable theme with many regions and a responsive,
+  classy/base/css/theme/css/layout.css: false

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?

almaudoh’s picture

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.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
12.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,602 pass(es), 0 fail(s), and 3 exception(s). View
3.15 KB

Fixed the test fails and added some code comments.

almaudoh’s picture

FileSize
4.1 KB

Previous interdiff was wrong.

Status: Needs review » Needs work

The last submitted patch, 30: libraries-override-2451411-30.patch, failed testing.

Cottser’s picture

Oops, missed that part. Great! Looks very close to green :)

almaudoh’s picture

Status: Needs work » Needs review
FileSize
928.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
1.58 KB

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.

almaudoh’s picture

FileSize
13.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,602 pass(es). View

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

The last submitted patch, 34: libraries-override-2451411-34.patch, failed testing.

Cottser’s picture

(I canceled the patch from #34)

@almaudoh awesome! I'm reluctant to extend the scope of this issue but we could definitely use your help in implementing libraries-extend! I haven't had a chance to create the issue but you are welcome to if you'd like :)

Not sure right now on the nomenclature, if we went with library-overrides would that make it library-extensions or something then?

Cottser’s picture

A couple small questions, sorry not a full review at this point.

  1. +++ b/core/modules/system/tests/themes/test_theme/test_theme.info.yml
    @@ -20,3 +20,13 @@ regions:
    +  core/drupal.collapse: 'test_theme/collapse'
    ...
    +  classy/base/css/theme/css/layout.css: 'css/test_theme_layout.css'
    

    Are the quotes needed? Probably not, right, because YAML?

  2. +++ b/core/modules/system/tests/themes/test_theme/test_theme.info.yml
    @@ -20,3 +20,13 @@ regions:
    +  core/drupal.progress: FALSE
    ...
    +  core/drupal.dialog/css/theme/misc/dialog.theme.css: FALSE
    

    I think we use lowercase booleans in YAML.

davidhernandez’s picture

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.

almaudoh’s picture

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.

Cottser’s picture

To follow up on #38:

+++ b/core/themes/bartik/bartik.info.yml
@@ -5,10 +5,10 @@ description: 'A flexible, recolorable theme with many regions and a responsive,
-stylesheets-remove:
-  - '@classy/css/layout.css'

This only had quotes because @classy is some kind of token according to the YAML spec. No @, no quotes needed - #2481183: Quote tokens in theme .info.yml files.

almaudoh’s picture

Title: Add libraries-remove and libraries-override to themes' *.info.yml » Add libraries-override to themes' *.info.yml (and remove stylesheets-remove)
FileSize
19.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,602 pass(es). View
8.7 KB

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.

almaudoh’s picture

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

almaudoh’s picture

FileSize
20.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,600 pass(es). View
3.43 KB

Fixed #43 and added test for it.

davidhernandez’s picture

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.

almaudoh’s picture

Issue summary: View changes
Cottser’s picture

Issue tags: +Needs beta evaluation

I agree with @davidhernandez. Thanks for all the work here @almaudoh!

I think this issue can still switch over all (or some) of the uses of stylesheets-remove, but yes that should probably be removed in a separate issue (if at all because of where we are in the release).

Wim Leers’s picture

Status: Needs review » Needs work

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.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
25.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,276 pass(es). View
14.94 KB

Thanks for the reviews and comments.


#45 / #47: @davidhernandez, @Cottser: Beta evaluation guidelines allow "follow-ups from a recent critical or major change" as prioritized changes. I know there's not much disruption for core (one or two lines in Bartik, Seven and Stark and the test themes). I don't know exactly how many themes in contrib use stylesheets-remove at this time. IMO, i think we should remove it completely (or at least convert all core usages and maybe leave in a BC-shim). I haven't reverted it in the patch yet, still awaiting the decision of the framework managers.
#48: @WimLeers: Thanks for the great review and nice 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.

Wim Leers’s picture

Status: Needs review » Needs work

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().

almaudoh’s picture

Status: Needs work » Needs review
FileSize
25.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,275 pass(es). View
6.07 KB

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.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

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.

joelpittet’s picture

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)?

Wim Leers’s picture

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.

davidhernandez’s picture

Issue summary: View changes
Issue tags: -Needs beta evaluation
davidhernandez’s picture

Issue tags: -Needs change record

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

almaudoh’s picture

Status: Needs work » Needs review

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.

almaudoh’s picture

FileSize
25.78 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,309 pass(es). View
10.88 KB

and the patch...

almaudoh’s picture

Title: Add libraries-override to themes' *.info.yml (and remove stylesheets-remove) » Add libraries-override to themes' *.info.yml
Wim Leers’s picture

Status: Needs review » Needs work

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?

almaudoh’s picture

Status: Needs work » Needs review
FileSize
25.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,446 pass(es). View
1.89 KB

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.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Alright. Thanks!

Splendid work here! :)

Cottser’s picture

Here's the issue for libraries-extend: #2497667: Add libraries-extend to themes' *.info.yml

alexpott’s picture

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

lauriii’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
27.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,592 pass(es), 1 fail(s), and 0 exception(s). View
4.57 KB

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.

Status: Needs review » Needs work

The last submitted patch, 66: add_libraries_override-2451411-66.patch, failed testing.

The last submitted patch, 66: add_libraries_override-2451411-66.patch, failed testing.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
33.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,422 pass(es), 87 fail(s), and 12 exception(s). View
13.66 KB

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.

Status: Needs review » Needs work

The last submitted patch, 70: libraries-override-2451411-70.patch, failed testing.

almaudoh’s picture

FileSize
33.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,516 pass(es), 3 fail(s), and 0 exception(s). View
3.16 KB

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.

almaudoh’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 72: libraries-override-2451411-72.patch, failed testing.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
33.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,694 pass(es). View
2.29 KB

Was too sleepy to post this yesterday...

lauriii’s picture

Status: Needs review » Needs work

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’s picture

Assigned: Unassigned » Shamsher_Alam
Shamsher_Alam’s picture

Status: Needs work » Needs review
FileSize
30.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,727 pass(es), 0 fail(s), and 4 exception(s). View
Shamsher_Alam’s picture

Assigned: Shamsher_Alam » Unassigned
Issue tags: +SrijanSprintNight
lauriii’s picture

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

Status: Needs review » Needs work

The last submitted patch, 78: libraries-override-2451411-78.patch, failed testing.

almaudoh’s picture

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

Shamsher_Alam’s picture

Status: Needs work » Needs review
FileSize
5.22 KB

@lauriii
Inter diff of patch.

Shamsher_Alam’s picture

FileSize
33.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,735 pass(es). View
almaudoh’s picture

Status: Needs review » Needs work
  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[]/

almaudoh’s picture

Status: Needs work » Needs review
FileSize
33.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,745 pass(es). View
1.38 KB

Fixed #85

Status: Needs review » Needs work

The last submitted patch, 86: libraries-override-2451411-86.patch, failed testing.

almaudoh’s picture

Status: Needs work » Needs review
cilefen’s picture

Status: Needs review » Needs work

I am rerolling this.

cilefen’s picture

Status: Needs work » Needs review
FileSize
33.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,900 pass(es). View

reroll

cilefen’s picture

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.

almaudoh’s picture

#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

almaudoh’s picture

FileSize
33.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,004 pass(es). View
3.7 KB

Patch

davidhernandez’s picture

Issue tags: +Needs manual testing

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.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

Warning: Lots of nitpicks. Overall this is looking fantastic to me and the test coverage seems very thorough from my perspective.

Note that the review doesn't follow source order, I've rearranged things so that my questions/code things are first, and then the rest is all nitpicks, all the time.

  1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
    @@ -79,7 +77,25 @@ public function getLibrariesByExtension($extension) {
    +        else {
    +          unset($extension[$name]);
    +          return FALSE;
    +        }
    

    I'm probably missing something but what is the purpose of the unset() here?

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

    Minor but I couldn't see why these uses were added.

  3. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
    @@ -314,6 +323,72 @@ protected function parseLibraryInfo($extension, $path) {
    +            throw new \LogicException(sprintf('Library asset %s is not correctly specified. It should be in the form "extension/library_name/sub_key/path/to/asset.js".', $asset));
    

    What is sub_key here? In other words could this exception message be improved so that it's easier to fix when someone runs into this?

  4. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
    @@ -314,6 +323,72 @@ protected function parseLibraryInfo($extension, $path) {
    +        else if (strpos($asset, "$extension/$name/") !== FALSE) {
    ...
    +          else if ($sub_key === 'css') {
    

    Minor: elseif per https://www.drupal.org/coding-standards#controlstruct.

  5. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
    @@ -256,4 +258,82 @@ protected function resolveStyleSheetPlaceholders($css_file) {
    +   *   A fully resolved theme asset path relative to the drupal directory.
    

    Minor: s/drupal/Drupal/

  6. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
    @@ -42,4 +45,213 @@ public function testElementInfoByTheme() {
    +   * Asserts that the given asset is in library.
    ...
    +   * Asserts that the given asset is not in library.
    

    Minor: "…in the library" would be a bit less terse here (2 instances).

  7. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
    @@ -42,4 +45,213 @@ public function testElementInfoByTheme() {
    +   *   The library where given asset should be.
    ...
    +   *   The library where given asset should not be.
    

    Similarly here, "where the given asset…" (2 instances).

  8. +++ b/core/modules/system/src/Tests/Asset/LibraryDiscoveryIntegrationTest.php
    @@ -42,4 +45,213 @@ public function testElementInfoByTheme() {
    +   * @param string $message
    +   *  (optional) A message to display with the assertion.
    ...
    +   * @return bool
    +   *  TRUE if the specified asset is found in the library.
    ...
    +   * @return bool
    +   *  TRUE if the specified asset is not found in the library.
    

    Minor: The parts underneath @param and @return here need be indented one extra space (3 instances).

  9. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
    @@ -167,7 +167,8 @@ function testCSSOverride() {
    +    $this->assertNoText('system.theme.css', "The theme's .info.yml file is able to remove a module CSS file from being added to the pagei using stylesheets-remove.");
    

    Minor: s/pagei/page/

  10. +++ b/core/modules/system/tests/themes/test_theme/css/collapse.css
    @@ -0,0 +1,4 @@
    + * @file
    + * Test CSS asset file for test_theme.theme
    
    +++ b/core/modules/system/tests/themes/test_theme/js/collapse.js
    @@ -0,0 +1,4 @@
    + * @file
    + * Test JS asset file for test_theme.theme
    

    Minor: I think the second lines here need to end in a period per https://www.drupal.org/node/1354#file.

  11. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
    @@ -138,6 +138,7 @@ public function getCssAssets(AttachedAssetsInterface $assets, $optimize) {
    +    // @todo Remove in Drupal 9.0.x
    
    +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
    @@ -161,27 +161,27 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
    +    // @todo Remove in Drupal 9.0.x
    
    @@ -241,6 +241,8 @@ protected function getExtensions() {
    +   * @todo Remove in Drupal 9.0.x
    
    @@ -256,4 +258,82 @@ protected function resolveStyleSheetPlaceholders($css_file) {
    +   * @todo Remove in Drupal 9.0.x
    

    Super minor: These should end in a period (4 instances).

almaudoh’s picture

Status: Needs work » Needs review
FileSize
32.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,070 pass(es). View
8.94 KB

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


#96:
1. This is for when you want to remove the entire library. This can't be done in the LibraryDiscoveryParser because the parser doesn't know about the $extension.
2. Fixed
3. sub_key is like 'js' or 'css'. A better name is welcome.
4. Fixed
5. Fixed
6. Fixed
7. Fixed
8. Fixed
9. Fixed
10. Fixed
11. Fixed

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

joelpittet’s picture

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?

Cottser’s picture

If sub_type can only be CSS or JS can we add something short to the end of the exception message to say (sub_key is either CSS or JS) or something? It just feels a bit cryptic now.

almaudoh’s picture

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.

Cottser’s picture

What about something like asset_type? And to expand a bit more on my phone-typed comment from earlier, I think an exception message along these lines would be more helpful, but this could be improved probably:

Library asset %s is not correctly specified. It should be in the form "extension/library_name/asset_type/path/to/asset.js" where asset_type is (for example) "css" or "js".

I'm doing some manual testing.

Cottser’s picture

Issue summary: View changes

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


After manual testing, some thoughts:

  1. 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.
  2. Should we check to make sure that the library being specified on the left hand side exists? Otherwise a typo will fail silently.
  3. Should we check to make sure an individual asset on the left side exists?

Maybe these should all fail silently so we don't have people creating dependencies on things they are wanting to remove/override but I accidentally ran into the second case while testing by typo'ing. Seems like we'd want to make sure the library exists at minimum.

Wim Leers’s picture

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

almaudoh’s picture

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.

almaudoh’s picture

Issue tags: +Needs tests
FileSize
4.38 KB
34.57 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 105,606 pass(es), 3,140 fail(s), and 364 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 105: libraries-override-2451411-104.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 105: libraries-override-2451411-104.patch, failed testing.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
14.58 KB
38.49 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,542 pass(es), 2,341 fail(s), and 353 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 109: libraries-override-2451411-109.patch, failed testing.

cilefen’s picture

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

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 109: libraries-override-2451411-109.patch, failed testing.

almaudoh’s picture

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

almaudoh’s picture

Status: Needs work » Needs review
FileSize
7.65 KB
11.14 KB
35.04 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,425 pass(es). View

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.

Cottser’s picture

Thanks @almaudoh! I think it's fine to punt on some of this stuff, did not mean to hold things up.

Wim Leers’s picture

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/

almaudoh’s picture

FileSize
8.94 KB
36.12 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,403 pass(es). View

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.

almaudoh’s picture

FileSize
14.53 KB
35.03 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,410 pass(es). View

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

almaudoh’s picture

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.

borisson_’s picture

FileSize
1.33 KB
35.03 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,523 pass(es). View

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

joelpittet’s picture

Issue tags: -Needs tests

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.

lauriii’s picture

Status: Needs review » Needs work

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.

almaudoh’s picture

Status: Needs work » Needs review

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.

davidhernandez’s picture

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 ?

Wim Leers’s picture

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.

davidhernandez’s picture

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.

Wim Leers’s picture

#127++

lauriii’s picture

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

almaudoh’s picture

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 
Cottser’s picture

That sounds pretty sensible to me @almaudoh.

davidhernandez’s picture

# 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?

davidhernandez’s picture

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.

Wim Leers’s picture

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.

davidhernandez’s picture

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.

Wim Leers’s picture

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.

almaudoh’s picture

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

almaudoh’s picture

FileSize
34.8 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,659 pass(es), 1 fail(s), and 0 exception(s). View

Re-roll

Status: Needs review » Needs work

The last submitted patch, 138: add_libraries_override-2451411-138.patch, failed testing.

The last submitted patch, 138: add_libraries_override-2451411-138.patch, failed testing.

almaudoh’s picture

Assigned: Unassigned » almaudoh

Working on this now

almaudoh’s picture

Assigned: almaudoh » Unassigned
Status: Needs work » Needs review
FileSize
24.24 KB
36.69 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,164 pass(es), 11 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 142: libraries-override-2451411-142.patch, failed testing.

The last submitted patch, 142: libraries-override-2451411-142.patch, failed testing.

lauriii’s picture

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.

almaudoh’s picture

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

almaudoh’s picture

Status: Needs work » Needs review
FileSize
5.93 KB
38.15 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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.

Status: Needs review » Needs work

The last submitted patch, 147: libraries-override-2451411-147.patch, failed testing.

The last submitted patch, 147: libraries-override-2451411-147.patch, failed testing.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
38.15 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Re-roll

Status: Needs review » Needs work

The last submitted patch, 150: libraries-override-2451411-150.patch, failed testing.

The last submitted patch, 150: libraries-override-2451411-150.patch, failed testing.

davidhernandez’s picture

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

almaudoh’s picture

Status: Needs work » Needs review
FileSize
997 bytes
38.15 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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

Status: Needs review » Needs work

The last submitted patch, 154: libraries-override-2451411-150.patch, failed testing.

The last submitted patch, 154: libraries-override-2451411-150.patch, failed testing.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
997 bytes
39.12 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,525 pass(es). View

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.

Wim Leers’s picture

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?

almaudoh’s picture

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.

almaudoh’s picture

FileSize
11.48 KB
38.38 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,764 pass(es). View

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.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - looks great to me now!

Wim Leers’s picture

Looks great!

almaudoh’s picture

FileSize
1.52 KB
37.97 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,268 pass(es), 1 fail(s), and 0 exception(s). View

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 163: libraries-override-2451411-163.patch, failed testing.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
3.58 KB
39.03 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,281 pass(es), 4 fail(s), and 1 exception(s). View

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

almaudoh’s picture

FileSize
4.34 KB
37.93 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,589 pass(es). View

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.

The last submitted patch, 165: libraries-override-2451411-165.patch, failed testing.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

The last submitted patch, 163: libraries-override-2451411-163.patch, failed testing.

The last submitted patch, 165: libraries-override-2451411-165.patch, failed testing.

effulgentsia’s picture

Patch looks great to me. Ticking some credit boxes.

  • effulgentsia committed 451bebc on 8.0.x
    Issue #2451411 by almaudoh, Shamsher_Alam, lauriii, borisson_, cilefen,...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

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.

almaudoh’s picture

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.

davidhernandez’s picture

Very awesome work by you, almaudoh. Thank you.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

hass’s picture

hass’s picture

Cottser’s picture

@hass something not acting exactly as you hoped is not always a bug. "suxxx" (in bold too) is not acceptable or respectful. Cut that out.

hass’s picture

Not being able to use "@classy/..." to wipe out conflicting css files is clearly a critical issue. You cannot deliver such contrib themes as they break or are broken out of the box just because of a path. This may be seen as feature, but it is more a critical bug.

Well - i may was a bit harsh after several hours of continued failures without any light. Every day with theaming in D8 is really hard as I find bugs and bugs. I'm close to 5 or more core patches and several unsolvable issues just to get one theme working properly. And if I count how often I need to presss clear all caches? 10.000 times? I have cache backend null enabled, twig and all other caches disabled, uninstalled the core cache modules, but it is not working...

Do you think this is theming fun? I do not. My current D8 theming frustration level is very high.