Follow-up to #2451411: Add libraries-override to themes' *.info.yml

Problem/Motivation

Sometimes you want to alter the assets of a library by adding in your own additional library assets whenever a library is attached.

The most common use case in core is the Classy base theme, see #2489460: [Meta] Move module.theme.css files to Classy or Seven.

Other use cases include:

  1. BigPipe (in core or contrib), where it is crucial because it means a theme's CSS extending other CSS is loaded not as part of the initial theme CSS, but as part of the CSS for the actual component. Remember that the component may be rendered *after* the initial page! So this ensures that we don't end up with 1. theme CSS, 2. component CSS, with 2 overriding 1, which is a problem in HEAD. With support for this, we completely avoid that problem.
  2. The last point also quite clearly shows how this makes Drupal 8 ready for a Web Component world.

Neither stylesheets-remove nor libraries-override meet this specific use case.

There is also a big front-end performance win: rather than loading all the CSS extending existing CSS all the time, this ensures it's only loaded when necessary.

Proposed resolution

Allow code like the following in a theme's (and potentially module's?) *.info.yml file so that libraries can extend other libraries:

# Extend drupal.user: add assets from classy's user libraries.
libraries-extend:
  core/drupal.user: 
    - classy/user1
    - classy/user2

Remaining tasks

Patch
Patch review
Commit

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
  1. Major because this would be a huge themer experience win and assist in moving Classy forward, not critical because nothing is broken.
  2. This would be a big front-end performance win.
  3. Last but not least: this is crucial for BigPipe (in core or contrib).
Prioritized changes The main goal of this issue is TX, but there's also a strong front-end performance aspect, and an even stronger "unblock BigPipe" aspect.
Disruption Unblocks much of the work in #2489460: [Meta] Move module.theme.css files to Classy or Seven, otherwise not disruptive.

Comments

jaxxed’s picture

Your initial proposed syntax should probably be an array format (or allow for an array format)

If you wanted to extend a library by adding more than one library, you'd need something like:

# Extend drupal.user: add assets from classy's user libraries.
libraries-extend:
  core/drupal.user: 
    - classy/user1
    - classy/user2

I am not sure what the Drupal YAML standards are for variable elements (can a node be a string OR an array of strings.)

Cottser’s picture

Great point, thanks @jaxxed. I can't remember who came up with that syntax originally, but we are having similar discussions on the libraries override issue.

almaudoh’s picture

How about if we wanted to extend a library with just a component from a theme or elsewhere. We could do something like:

# Extend drupal.user: add assets from my theme's custom css
libraries-extend:
  core/drupal.user: 
    css:
      theme:
        my/custom/css/button.css: { }

This allows to add some new css or js to the existing library, without having to necessarily define a new library.

Cottser’s picture

Yup that seems valid as well, worth considering at least :)

jaxxed’s picture

@almaudoh

I still think that it needs to be an array per library:

# Extend drupal.user: add assets from my theme's custom css
libraries-extend:
  core/drupal.user: 
    - css:
        theme:
          my/custom/css/button.css: { }

I started by following the overrides code, but it diverges from our needs pretty quickly.

I'm not ready to patch for this deeper extension syntax, but right now I am playing with injecting library dependencies for an library-extensions. There would be some notable behaviour that would come with such an approach, the most significant being that dependencies are loaded before the library that they're attached to.

Is this something that we're thinking about just for one of the subthemes, or are we going right for the core? If it's the core then I can look at altering the library discoverer right away, but if it's not in the raw core, then it will require overloading or adding more code.

Cottser’s picture

I don't think we want to add dependencies here, there are already ways to do that I'm pretty sure. By extending we want to add the opposite of dependencies.

@jaxxed what do you mean by right for the core/raw core? Just Drupal core? Used by core themes?

almaudoh’s picture

So where do we stand on the syntax here. Considering that there is mention that this particular capability may be needed sooner rather than later, we could start work on a patch here.

davidhernandez’s picture

So why did we split this off into its own thing instead of making it part of libraries-override? It seems like override just needs an add asset syntax.

almaudoh’s picture

So combining #1 and #5, we could have something like:

libraries-extend:
  core/drupal.user:    # The name of the library being extended
    - classy/user1      # Add all the assets defined by 'classy/user1'
    - classy/user2      # and also all those defined by 'classy/user2'
    - css:          # Add a specific asset available in this theme
        theme:
          my/custom/css/button.css: { }

This looks sensible to me and easy for everyone to understand.

@davidhernandez: There is a slight difference to the libraries-override syntax which doesn't specify attributes for the assets. But also it was to make the patches easier to review.

Wim Leers’s picture

So why did we split this off into its own thing instead of making it part of libraries-override? It seems like override just needs an add asset syntax.

Because we wanted to keep the scope as small as possible over there, I think that was mostly discussed/agreed upon in IRC, but #2451411-37: Add libraries-override to themes' *.info.yml and #2451411-63: Add libraries-override to themes' *.info.yml suggest this at least.

Fabianx’s picture

If it were me I would keep it stupidly simple and just allow extending a library with another library for now.

The use case of directly adding CSS is very nice, but could internally be still be implemented as a hash over the contents of the definition.

core/autogenerated_[HASH]

and create a library on the fly.

Overall however I don't see much difference in:

libraries-extend:
  core/drupal.user:    # The name of the library being extended
    - classy/user1      # Add all the assets defined by 'classy/user1'
    - classy/user2      # and also all those defined by 'classy/user2'
    - css:          # Add a specific asset available in this theme
        theme:
          my/custom/css/button.css: { }

vs.

libraries:
  - foo:
    - css:          
        theme:
          my/custom/css/button.css: { }

libraries-extend:
  core/drupal.user:    # The name of the library being extended
    - classy/user1      # Add all the assets defined by 'classy/user1'
    - classy/user2      # and also all those defined by 'classy/user2'
    - mytheme/foo    # Just add the library from above

While being way more flexible to work with.

e.g. another module or subtheme could decide to just add something to mytheme/foo - regardless if that is used in one or in several libraries as extension.

override is a different "beast" in that that should allow to override / remove or change specific CSS files.

This issue should IMHO be purely about libraries extending other libraries and nothing more - and hence keep things really simple.

Cottser’s picture

Agreed with #11, that is in line with the original proposal in the issue summary and nicely keeps things inside the actual library definition rather than bloating the theme's .info.yml file. Overriding is indeed a different beast with different requirements which I don't think makes sense to try to incorporate into *.libraries.yml. I like how it's been implemented in the overrides issue.

Put differently, adding the ability to add individual CSS and JS files from the theme's *.info.yml kind of undoes https://www.drupal.org/node/2228783 and similar changes.

almaudoh’s picture

+1 to #11. So basically we're looking at libraries-extend: in foo.libraries.yml instead of foo.info.yml. But how do we explain to someone new to Drupal 8 why libraries-extend is in foo.libraries.yml while libraries-override are in foo.info.yml.

Cottser’s picture

I actually completely missed that it was being proposed to add to libraries.yml. That does feel somewhat inconsistent.

I think the extends should be "mapped" in the info file similar to how you can add a library to every page, and the libraries to use as "extensions" declared normally in libraries.yml.

Fabianx’s picture

#13, #14: Yes, I was unclear there.

I meant:

- libraries to *.libraries.yml
- libraries-extend to *.info.yml

Modules can use the good ol' hook_library_info_alter() to do the same.

Cottser’s picture

Good then, at least a few of us agree there! :)

almaudoh’s picture

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

Ok. So #1, #11 and #15 makes the implementation relatively simple.
Here's a patch rolled on top of the latest patch at #2451411: Add libraries-override to themes' *.info.yml. The do-not-test.patch is the patch for this issue with one test. More tests still needed.

Some key points:

  1. I guess libraries-override takes precedence over libraries-extend. But the implementation libraries-extend will happen after override, so overrides must always take place on the library where it is defined, not where it is extended.
  2. Does libraries-extend need to check if there is already existing assets as it is trying to extend and throw an exception? That might be good for TX, but I don't know
  3. Do we need to do a recursion check? For example if a library tries to extend another library that is extending the former library. That results in an infinite recursion. May be far fetched... :)

Status: Needs review » Needs work

The last submitted patch, 17: libraries-extend-2497667-17.patch, failed testing.

almaudoh’s picture

Status: Needs work » Needs review
FileSize
4.78 KB
10.39 KB
45.2 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,291 pass(es). View

Fixed the test fails and added some docs. Still needs more tests.

The last submitted patch, 17: libraries-extend-2497667-17.patch, failed testing.

almaudoh’s picture

Issue summary: View changes
Fabianx’s picture

Overall looks great, one detail:

+++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
@@ -185,6 +185,21 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
+    foreach ($base_themes as $base) {
+      if (!empty($base->info['libraries-extend'])) {
+        foreach ($base->info['libraries-extend'] as $library => $extend) {
+          $values['libraries_extend'][$library] = $extend;
+        }
+      }
+    }
+    // Add libraries extend declared by this theme.
+    if (!empty($theme->info['libraries-extend'])) {
+      foreach ($theme->info['libraries-extend'] as $library => $extend) {
+        $values['libraries_extend'][$library] = $extend;
+      }
+    }

I thibk this should extend the libraries to extend and not override.

Or merge in other words.

I.e. if a base theme extends a component, why should the theme have to do the same and not just extend more?

almaudoh’s picture

FileSize
9.86 KB
15.56 KB
49.72 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,511 pass(es). View

#22.1 Great point! Fixed. Also added some more tests. This should be ready now.

Wim Leers’s picture

FileSize
15.56 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,116 pass(es), 428 fail(s), and 22 exception(s). View

Wow, great work here! :)

#2451411: Add libraries-override to themes' *.info.yml landed. Reuploading without that part, i.e. reuploading #23's "do-not-test" patch. Zero changes made.

Wim Leers’s picture

FileSize
15.58 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,666 pass(es). View
2.46 KB

This looks excellent. I was only able to find nitpicks, fixed those myself.

  1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryCollector.php
    @@ -124,8 +126,50 @@ protected function getLibraryDefinitions($extension) {
    +        // If libraries are not overridden, then apply libraries extend.
    

    s/libraries extend/libraries-extend/

  2. +++ b/core/lib/Drupal/Core/Theme/ActiveTheme.php
    @@ -219,4 +221,14 @@ public function getLibrariesOverride() {
    +   *   The list of libraries extend.
    

    s/libraries extend/libraries-extend definitions/

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

    Libraries extensions (so it is consistent with the wording in the preceding code which says "libraries overrides").

Committer: no need to credit me for these language nitpicks.

Wim Leers’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +front-end performance, +BigPipe, +rc deadline, +minor version target

From the beta evaluation:

  1. This would be a huge themer experience win.
  2. This would be a big front-end performance win: rather than loading all the CSS extending existing CSS all the time, this ensures it's only loaded when necessary.
  3. Last but not least: this is crucial BigPipe (in core or contrib), because it means a theme's CSS extending other CSS is loaded not as part of the initial theme CSS, but as part of the CSS for the actual component. Remember that the component may be rendered *after* the initial page! So this ensures that we don't end up with 1. theme CSS, 2. component CSS, with 2 overriding 1, which is a problem in HEAD. With support for this, we completely avoid that problem.
  4. The last point also quite clearly shows how this makes Drupal 8 ready for a Web Component world.

We have strong +1 from Cottser.

The last submitted patch, 24: libraries-extend-2497667-24.patch, failed testing.

Fabianx’s picture

Strong RTBC + 1

Yes, this usage pattern is crucial for BigPipe, AJAX, ESI + Co and one of the big enablers.

No more !important in the theme for AJAX CSS loaded afterwards ...

almaudoh’s picture

Issue summary: View changes

Moved Wim's strong arguments in #26 up higher in the IS.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Can we get a quick followup to actually use this in Classy... the theme has an @todo on this issue.

We've got test coverage and this is an addition so less risky than a change. Committed b95d2c8 and pushed to 8.0.x. Thanks!

  • alexpott committed b95d2c8 on 8.0.x
    Issue #2497667 by almaudoh, Wim Leers, Cottser, Fabianx, jaxxed: Add...
Wim Leers’s picture

WOOOHOOO! Thank you!

Can we get a quick followup to actually use this in Classy... the theme has an @todo on this issue.

Done: #2580255: Remove (classy|seven)_library_info_alter() in favor of libraries-extend.

davidhernandez’s picture

Issue tags: +Needs change record

This should have a change record.

I'll see if I can work on one later, but anyone else can feel free to do so.

almaudoh’s picture

Issue tags: -Needs change record

Updated existing libraries-override CR https://www.drupal.org/node/2497313 to incorporate libraries-extend.

Wim Leers’s picture

I also updated the CR to link to #2580255: Remove (classy|seven)_library_info_alter() in favor of libraries-extend, to point readers to the fact that Seven and Classy are now doing the right thing :)

Status: Fixed » Closed (fixed)

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