Problem/Motivation

A bug in libraries-override #2451411: Add libraries-override to themes' *.info.yml makes it difficult to override a library or library asset that has already been overridden. You have to know the theme which last overrode a library and then use the full path to the overriding theme asset. This presents a WTF to themers and makes it difficult to reuse theme code. It also makes the sub-theme dependent on the internal file structure of the base theme.

In normal circumstances overriding an already overridden library would be and edge case, but with the introduction of the Stable base theme, a lot of system libraries have already been overridden. This also implies that possible future changes in the asset file structure of Stable could break sub-themes that override libraries already overridden by Stable.

Workaround

The current workaround is explained in #2642122-4: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides and #2642122-18: Overriding already overridden libraries-override requires knowledge of previous libraries-overrides.

Proposed resolution

1. Fix the bug so that the libraries-override declaration should use the same keys as the libraries declaration in the original libraries.yml file.
2. Maintain a BC layer so that themes that have already used the workaround will not break immediately. Remove the BC layer in D9 (#2852314: Remove BC layer for libraries-override that is already overridden)

Remaining tasks

Reviews
Commit

User interface changes

None

API changes

Method signatures of 3 protected methods in LibraryDiscoveryParser are changed

Data model changes

None.

Comments

hass created an issue. See original summary.

hass’s picture

Issue summary: View changes
Cottser’s picture

For now you need to use the full path. See test_theme.info.yml for examples. http://cgit.drupalcode.org/drupal/tree/core/modules/system/tests/themes/...

almaudoh’s picture

#3: In other words, after stable overrides the system/base library, the key is replaced with a drupal-root-relative path to the NEW stable css file, this is the key you should use in overriding.
In Stable we have

  system/base:
    css:
      component:
        ...
        css/components/autocomplete-loading.module.css: css/system/components/autocomplete-loading.module.css
        ...

So your version should be:

libraries-override:
  system/base:
    css:
      component:
        /core/themes/stable/css/system/components/autocomplete-loading.module.css: false  

For now, the path has to be hard-coded until dynamic path names like '@stable/css/...' are implemented.

hass’s picture

Thanks for your hints and the examples. This works now, but it is also not really intuitive and inconsistent.

It is also not documented in

For stable I may use the full path, but for other contrib themes this may not work as these could reside in different directories. This means we need @foo for dynamic paths asap. It also means that every core theme library change in future will seriously break a lot of contrib themes.

This sounds more reliable to me until @ is supported:

/**
 * Implements hook_library_info_alter().
 */
function yaml_library_info_alter(&$libraries, $extension) {
  // @TODO: Workaround for https://www.drupal.org/node/2642122
  if ($extension == 'system' && isset($libraries['base'])) {
    unset($libraries['base']['css']['component']['css/components/autocomplete-loading.module.css']);
  }
}
almaudoh’s picture

OTOH, using streamwrappers is more elegant than using dynamic @ theme names. #1308152: Add stream wrappers to access extension files

hass’s picture

Don't you think a stream wrapper is not a bit overkill for referencing a theme?

almaudoh’s picture

Don't you think a stream wrapper is not a bit overkill for referencing a theme?

Perhaps. But consider that there is more than just one use case for streamwrappers and this happens to be one of the stronger ones.

Cottser’s picture

And in many cases you would not have to use the stream wrapper but it does seem like a compelling option to have available! Thanks for that @almaudoh.

rbaprado’s picture

Not working for me:

name: Mercante
type: theme
description: Tema para o site Mercante.co
core: 8.x
stylesheets-remove:
  - @stable/css/system/components/ajax-progress.module.css
libraries-override:
  system/base:
    css:
      component:
        css/components/ajax-progress.module.css: false
        /core/themes/stable/css/system/components/ajax-progress.module.css: false
        core/themes/stable/css/system/components/ajax-progress.module.css: false
Cottser’s picture

@rbaprado I tested that .info.yml file, worked fine for me. Here is a more minimal one that also worked for me. I tested with 8.0.x (commit dbf84c49d57156be0a3527f2808caa6dc44aa168) and Standard profile.

name: Mercante
type: theme
description: Tema para o site Mercante.co
core: 8.x
libraries-override:
  system/base:
    css:
      component:
        /core/themes/stable/css/system/components/ajax-progress.module.css: false

Edit: And it should probably be said, make sure you clear caches :)

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Cottser’s picture

Title: Cannot override already overridden libraries » When overriding a library via libraries-override that is already overridden, you need to use the full path from DRUPAL_ROOT
Priority: Major » Normal

Discussed this with @effulgentsia, @alexpott, @xjm, @joelpittet, @lauriii, @catch and we agreed that this is a bug but is not major.

For now we should create a follow-up issue to document the different ways you can use libraries-override (including the full path way). Then we would update those docs based on the solution that we come up with here.

Cottser’s picture

Issue tags: +Needs followup

Oops, tagging for the followup docs issue to be created.

Jacine’s picture

I cannot get this working for the life of me.

Everyone says use stable at the very least for a base theme, so I'm doing that, but that is making this more complex as far as knowing what keys I should be overriding here. And all I want to do here is:

1 - Override one of the 3 CSS files.
2 - Remove the other two.

I have tried everything I can think of, but neither of these work:

libraries-override:
  contextual/drupal.contextual-links:
    css:
      component:
        /core/modules/contextual/css/contextual.module.css: false
      theme:
        /core/modules/contextual/css/contextual.theme.css: css/overrides/contextual.theme.css
        /core/modules/contextual/css/contextual.icons.theme.css: false
libraries-override:
  stable/drupal.contextual-links:
    css:
      component:
        /core/themes/stable/css/contextual/contextual.module.css: false
      theme:
        /core/themes/stable/css/contextual/contextual.theme.css: css/overrides/contextual.theme.css
        /core/themes/stable/css/contextual/contextual.icons.theme.css: false

It tried all types of variations with the paths, including the full path, not including it, including the preceding slash, not including it. I tried explicitly setting base theme: stable in case there was some weirdness around that. Nothing.

No matter what, I still get all 3 files from Stable as if I did nothing.

davidhernandez’s picture

I'm compelled to argue for this being a major. I understand that the bug itself of overriding a library that has already been overridden might not be considered major, but the problem we have is that Stable overrides practically everything. The end result is most people are starting from a point where these are already overridden. That is tantamount to the api being broken.

davidhernandez’s picture

@Jacine, looking at what Cottser wrote in #11, you have to leave the original library name, but use the full path of the new CSS fie. So contextual lib, with stable css file path.

Jacine’s picture

Ah, Drupal TX is brutal.

What David suggested is working (thank you!).

To help others that run into this, this is what worked:

libraries-override:
  contextual/drupal.contextual-links:
    css:
      component:
        /core/themes/stable/css/contextual/contextual.module.css: false
      theme:
        /core/themes/stable/css/contextual/contextual.theme.css: css/overrides/contextual.theme.css
        /core/themes/stable/css/contextual/contextual.icons.theme.css: false

And this is the logic:

  1. Use the original module (or core) namespace for library name.
  2. Use the path of the most recent override (in my case Stable) as the key
  3. Use the full path to the file
hass’s picture

I guess we are only waiting for #1308152: Add stream wrappers to access extension files to implement an easy way that always works.

Today it is a pure nightmare if you have a contrib theme in a folder and another theme overrides again. You are completly lost as you have no idea in what folder structure the theme is saved. For overriding core the above may work, but it will also break if you move the theme to /theme/contrib/foo and another one has it in /theme/foo and so on. This is really a serious issue.

davidhernandez’s picture

Why would using a stream wrapper be more elegant? Have those been used anywhere else in a yaml file? It seems weird to use it there, versus using the @ token which we use for other things. Twig extends, for example.

almaudoh’s picture

Why would using a stream wrapper be more elegant?

I guess I'm seeing this from a programmer's standpoint. Streamwrappers are already part of php and so we wouldn't need to write additional code to parse the @themename tokens.

I'll look into a fix for this bug.

Have those been used anywhere else in a yaml file?

Not yet, because we don't have the streamwrappers in core yet. But IMHO, using a string like theme://stable/css/contextual/contextual.module.css: false looks just as nice as using @stable/css/contextual/contextual.module.css: false. The difference being that the streamwrappers approach would be usable in any context (not just in yaml files).

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pixelmord’s picture

I ran into that issue and noticed that even if I provide the full path to the css file (in stable) like mentioned in #18, the file provided by stable does not get removed. I end up having both files linked/imported the one that stable provides and the one that my theme provides as an override.

To make this work I had to put both the library_override AND a stylesheet_remove in the info.yml file to get the desired behaviour of replacing the css file.

Cottser’s picture

@pixelmord can you please provide the YAML you used? Otherwise we may not be able to help or reproduce.

pixelmord’s picture

FileSize
162.05 KB

@Cottser regarding #24:

I use core 8.2.4

OK so the issue occurs with this yml ( The theme is currently sub-theme of Seven, which itself is a sub-theme of Stable):

name: Thunder Admin
type: theme
core: 8.x
base theme: seven

stylesheets-remove:
 - '@thunder_article/css/article-form.css'

libraries-override:
  seven/install-page:
    css:
      theme:
        css/theme/install-page.css: css/install-page.css
  node/drupal.node:
    css:
      layout:
        /core/themes/stable/css/node/node.module.css: css/content-form-layout.css

libraries-extend:
  tour/tour-styling:
    - thunder_admin/tour-styling
  paragraphs/drupal.paragraphs.admin:
    - thunder_admin/paragraphs.admin

libraries:
  - thunder_admin/global-styling

regions:
  header: 'Header'
  pre_content: 'Pre-content'
  breadcrumb: Breadcrumb
  highlighted: Highlighted
  help: Help
  content: Content
  page_top: 'Page top'
  page_bottom: 'Page bottom'
  sidebar_first: 'First sidebar'
regions_hidden:
  - sidebar_first

If I configure like above shown, I still have both stylesheets linked:

If I add the stylesheet that I wanted to override to te stylesheets_remove array, everything works as intended:

stylesheets-remove:
 - '@stable/css/node/node.module.css'
 - '@thunder_article/css/article-form.css'
davidhernandez’s picture

To be clear, you did a cache rebuild and all that fun stuff?

pixelmord’s picture

Sure I cleared the cache.
I also tried that override again with another css file that I specifically created for testing, so I can be sure that the file shown in the above image was not attached someplace else. Same issue, after specifying the override in the info.yml file I get both files attached the one from stable and the test.css.

Just now I checked and tried to just remove the file by setting it to false, that did not work either, the node.module.css is still linked.

That is what I specified for that test:

libraries-override:
  seven/install-page:
    css:
      theme:
        css/theme/install-page.css: css/install-page.css
  node/drupal.node:
    css:
      layout:
        /core/themes/stable/css/node/node.module.css: false

It might have to do with the double inheritance of: Stable > Seven > Thunder Admin

I will fire up my debugger and see what I can find out..

davidhernandez’s picture

It shouldn't because Seven uses Classy, not Stable directly. Stable > Classy > Seven. So any theme using Classy would have the same problem. That would be easy to test.

pixelmord’s picture

Yeah @David found that out as well.

And I did some step through debugging and found out that the methods in Drupal\Core\Asset\LibraryDiscoveryParser work totally fine!

The only thing that does not work totally fine is my brain, because after some serious thinking what else could be a reason for this happening is that stable overrides TWO libraries that attach the file and my theme just did override one of those libraries, because I just did not think of that!!
What makes that worse is the fact that for the node form both those libraries are required for some reason.

So the following YML works and successfully removes the file:

libraries-override:
  seven/install-page:
    css:
      theme:
        css/theme/install-page.css: css/install-page.css
  node/drupal.node:
    css:
      layout:
        /core/themes/stable/css/node/node.module.css: false
  node/form:
    css:
      layout:
        /core/themes/stable/css/node/node.module.css: false

So no additional bug here other than what was originally reported, sorry.
However it might be interesting to find out why stable/core-node-module has two libraries that require the same css file and both get attached to the node form but that is an issue for another day...

almaudoh’s picture

Assigned: Unassigned » almaudoh

Let me work on this over the weekend. Seeing as streamwrappers are not remotely on the horizon, let's see if we can make @theme token work.

almaudoh’s picture

Status: Active » Needs review
FileSize
2.01 KB

We start with a test that should fail...

Status: Needs review » Needs work

The last submitted patch, 31: 2642122-31-FAIL.patch, failed testing.

almaudoh’s picture

More strict YAML parser in testbots. Still expecting 1 test fail only.

almaudoh’s picture

Embarrassing when fail patches refuse to fail :(. Seems like the @ token is causing problems with the PECL Yaml parser on the testbot. I'm using Symfony Yaml parser on my local machine, so I'll remove the @ token just to confirm the patch fails.

almaudoh’s picture

A more careful look at the test result shows that the test actually fails, it's just that DrupalCI does not report it. https://dispatcher.drupalci.org/job/default/281509/consoleFull
Testbot result

So here is a full patch with the fix.

This patch does the following:
1. Allows the use of the original overridden asset path when overriding (no matter how many parent themes have already overridden already). So for the example in #4, the override can now be written as below and will work irrespective of whether base themes have overridden them already or not.

libraries-override:
  system/base:
    css:
      component:
        css/components/autocomplete-loading.module.css: false

2. In order to maintain BC with the current behavior (which isn't very themer-friendly), it will also work if the previous overriding path is used. It will also work if using the @theme token to represent the previous overriding theme path. This approach is not necessary anymore and is not recommended.

3. If a base theme had removed an asset using path/to/original/asset.css: false, the sub-theme can reinstate it or still override that with path/to/original/asset.css: path/to/new/asset.css. This is currently not possible in HEAD, (except you use library-extends).

The entry on the left-hand side of the : is always relative to the original library location while the one on the right-hand side is always relative to the sub-theme's location. (Well that might be another WTF, but how to fix that).

almaudoh’s picture

Assigned: almaudoh » Unassigned
davidhernandez’s picture

So if I'm reading #1 right comment #35, the patch solves the original bug? Why do we need the dynamic path token?

Also...almaudoh to the rescue!

almaudoh’s picture

Why do we need the dynamic path token?

We actually don't need that anymore, so I can remove it in the next patch.

What do you think about maintaining the current behavior? Should that be removed now or deprecated to remove later? I looks like a lot of .info.yml files have already been written based on that behavior.

Also...almaudoh to the rescue!

Always happy to contribute when I can :)

davidhernandez’s picture

I think we'll need to leave the existing behavior for backwards compatibility. A lot of people are already doing it as a work around, so we don't want to break their themes.

almaudoh’s picture

Ok. I have removed the dynamic path tokens and retained the existing behavior for BC.

hass’s picture

Removing the @themename is incorrect!

The problem is that themes can be saved in every directory e.g. sites/all/themes/foo or sites/all/themes/contrib/foo or /themes/foo or sites/all/themes/basetheme/foo and the directory is not predictable.

Therefore we need this "@foo" token or the path may not overridden. Theme developer do not know where all the thousands of their users install the theme.

davidhernandez’s picture

The path doesn't have to do with where the library is located. The path is defined by the original library and is internal to the theme or module where it came from. That path is always the same.

If you want to override the system/base library, you always use "css/components/autocomplete-loading.module.css", no matter where your theme is. This path comes from the system module, not your theme.

hass’s picture

I guess #4 is the root cause of my confusion... if the /core/ folders are not added all should be good I guess.

But "stylesheets-remove" will not work without the @theme_name. Consistency wise not so perfect.

almaudoh’s picture

I guess #4 is the root cause of my confusion...

#4 was just a work-around for the bug. After this patch, you no longer need to use that approach. Just what @davidhernandez says in #42 will work.

But "stylesheets-remove" will not work without the @theme_name.

The @theme_name token for "stylesheets-remove" is not affected by this patch and still works as before. Also, I believe "stylesheets-remove" was deprecated in D8 for "libraries-override" and "libraries-extend". So it shouldn't be used for new themes.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

almaudoh’s picture

This patch still needs manual test and review so it can go into 8.3.0. RTBC anyone?

kostyashupenko’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
123.1 KB

I've tested last patch (#40) on 8.3.x branch at bartik/seven themes and it works for me

libraries-override:
  system/base:
    css:
      component:
        css/components/autocomplete-loading.module.css: false

screen

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
    @@ -39,6 +39,26 @@ class LibraryDiscoveryParser {
    +   * @todo Remove when BC-break is allowed.
    

    This @todo should have an issue filed against it for 9.x and the issue should be linked here.

  2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
    @@ -143,6 +163,11 @@ public function buildByExtension($extension) {
    +          // Update to the overridden source in libraries-override.
    +          if (!($source = $this->getLatestOverridePath($source, $type, $id, $extension))) {
    +            // Don't bother if the override says 'false'.
    

    The value assignment in the if is unnecessarily tricksy to read cause of the negating and additional brackets. Also the comment inside the if code by more descriptive.

    $source = $this->getLatestOverridePath($source, $type, $id, $extension);
    if (!$source) {
      // Some better comment about what a FALSE value means.
      continue;
    }
    
  3. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
    @@ -444,13 +470,16 @@ protected function setOverrideValue(array &$library, array $sub_key, array $over
        * @param string $theme_path
    ...
        * @return string
    
    @@ -460,4 +489,45 @@ protected function resolveThemeAssetPath($theme_path, $overriding_asset) {
    +   * @return string
    +   *   The full path to the effective libraries-override asset or $original_path
    +   *   if the asset is not overridden.
    

    I think this can return FALSE no?

  4. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
    @@ -460,4 +489,45 @@ protected function resolveThemeAssetPath($theme_path, $overriding_asset) {
    +   * @param string $new_path
    +   *   The Drupal-root-relative path to the overriding library asset.
    

    I think this can be FALSE - no?

  5. ~As far as I can see the actual bug outlined in the issue summary is not tested - i.e setting an overridden value to false
  6. The follow needs to created before commit - see #13
  7. Does this change require a change record - not sure
almaudoh’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs followup
FileSize
7.43 KB
15.76 KB

Thanks for the review @alexpott
1. Opened #2852314: Remove BC layer for libraries-override that is already overridden. Also added link in @todo
2. Fixed
3. Fixed
4. Fixed
5. Added test coverage
6. #2852317: Document the different ways 'libraries-override' can be used (including the full path way)
7. There was already a CR from the parent issue https://www.drupal.org/node/2497313 which documented the correct way of doing it. The buggy behavior or workarounds for it were not really documented anywhere. I don't think we need a CR either.

Hope it's okay to re-RTBC this since the only real code change is #48.2 which is really minor.

almaudoh’s picture

Small CS indentation adjustment...

alexpott’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Needs review

@almaudoh given the added test in #49 convention would mean that setting to needs review would be better than re-rtbcing yourself.

Reading the test made we wonder if we have test coverage of what happens is a base theme sets something to false but the sub theme has an override?

Also this is likely to miss the window for getting in to 8.3.0 (because there is only 5 minutes to go). The current patch is making changes to protected methods that we wouldn't make in a bug fix release like 8.3.1. In order to get this in to 8.3.x we'd need to rework the fix with a tighter focus on BC in mind (if possible). So moving to 8.4.x.

alexpott’s picture

The issue summary could also to with an update to detail exactly what the issue is - much is implied rather than explained. Plus the proposed resolution needs fleshing out.

almaudoh’s picture

Title: When overriding a library via libraries-override that is already overridden, you need to use the full path from DRUPAL_ROOT » Overriding already overridden libraries-override requires knowledge of previous libraries-overrides
Issue summary: View changes
Issue tags: -Needs issue summary update
Related issues: +#2852314: Remove BC layer for libraries-override that is already overridden

Updated the issue summary.

@almaudoh given the added test in #49 convention would mean that setting to needs review would be better than re-rtbcing yourself.

Sorry, forgot about the test.

In order to get this in to 8.3.x we'd need to rework the fix with a tighter focus on BC in mind (if possible)

@alexpott, what exactly do we need to do to get this in 8.3.x? I'm considering that more themes will end up having to use the workaround for the next 6months until 8.4.x?

almaudoh’s picture

Issue summary: View changes
almaudoh’s picture

Issue summary: View changes
gpap’s picture

Patch #50 does not fix this bug on 8.3.2, at least for Seven sub themes.

DuneBL’s picture

FileSize
8.38 KB

The following (without the patch) doesn't work from a subthemed bootstrap theme (8.4):
Top of prog_tpm.info.yml

core: 8.x
type: theme
base theme: bootstrap

override in prog_tpm.info.yml

libraries-override:
  toolbar/toolbar:
    css:
      theme:
        /core/themes/stable/css/toolbar/toolbar.theme.css: css/prog_tpm.toolbar.theme.css

cache cleared...

davidhernandez’s picture

Bootstrap doesn't use Stable. Unless Bootstrap overrides one, you'll be getting libraries and assets directly from core.

DuneBL’s picture

@davidhernandez
My bad... I was looking in an admin page!!!

I would like to override a toolbar stylesheet for my theme and my admin theme...
For the theme (not admin), I can add the libraries-override in info.yml, but for the admin theme, should I create a subtheme and add the same libraries-override in the subtheme.info.yml just for that?
If yes, how could I make it point to the same overrided css file used in my site theme or, maybe, I must duplicate it? (or is there any other more clever solution?)

Dennis Cohn’s picture

I can confirm #56:
Patch #50 does not fix this bug on 8.3.2, at least for Seven sub themes.
Still got issues with these files in my subtheme of seven. I need to include these files below in my own subtheme.info.yml to make the status report page to look nice.

libraries-override:
  system/base:
    css:
      component:
        /core/themes/stable/css/system/components/system-status-counter.css: css/components/system-status-counter.css
        /core/themes/stable/css/system/components/system-status-report-counters.css: css/components/system-status-report-counters.css
        /core/themes/stable/css/system/components/system-status-report-general-info.css: css/components/system-status-report-general-info.css