Core and base theme CSS files in libraries override theme CSS files with the same name

AFAICT this is by design, to make the stylesheets-override feature work. Drupal has been requiring unique CSS file names for a long time for that exact reason.

If we are encouraging themers to split up their CSS files into meaningful libraries, then the chances of an accidental override increases when we no longer have just a single style.css.

I totally agree.

There's no way to change this behaviour without breaking stylesheets-override?

Actually, I think there is. :) Just require the full root-relative path to be used.

So, from this today:

stylesheets-override:
  foo.css
  bar.css

to:

stylesheets-override:
  core/modules/foo/css/foo.css
  sites/all/modules/bar/css/bar.css

… which immediately also shows how this breaks down: contributed modules can live in different places.

So we'd want extension-relative paths:

stylesheets-override:
  foo/css/foo.css
  bar/css/bar.css

This should work, since there can be only one extension with the same name at any time.

Caveat: before we change this, we should investigate why exactly stylesheet-override was made to work with filenames in the first place, to make sure we don't miss anything.

I think I'm experiencing the same thing while trying to get a subtheme of Bartik for the Drupal 8 Multilingual Demo profile to work. Classy's layout.css is called instead of Bartik's.

The order of the libraries being loaded is wrong, as I saw by putting a breakpoint at line 1395 of /core/includes/theme.inc... The order should be: Classy, Bartik, Demotheme. Instead it's going: Bartik, Classy, Demotheme.

I found the cause to be in /Drupal/Core/Theme/ThemeInitialization, line 102. the $base_themes array is constructed in a way which puts parent themes after child themes. I'm attaching a patch which puts parent themes before child themes instead.

Setting to needs review to trigger test bot

@Wim Leers: I think it was made to work with filenames originally because modules were supposed to provide their styles in one file, so you could override block.css or comment.css and there was no reason for paths to be provided. We are not on that basic level with CSS anymore :D

Note that @loopduplicate found this problem in #2392953: Update to beta4 - yml files out of date, adding role broken in our contrib distro that has a tiny subtheme of bartik and had issues with how theme stylesheets are merged in.

The patch needs a core test regardless even if we agree it is the way to go.

Extending title to cover what the patch does, hope it covers the original problem as well :)

Patch in #4 worked for me. I couldn't get my own CSS files to be included regardless of what they were called when subclassing Bartik.

We are now using this patch in the Multilingual demo distribution because it is required to make our contrib theme work, see https://www.drupal.org/project/multilingual_demo :)

#2414255: Subtheme template inheritance working in reverse order is a critical issues and essentially does the same thing for templates. Should we fold the two issues together? Join forces there, here?

With the fix here, #2414255: Subtheme template inheritance working in reverse order it looks a css file with the same name as a parent theme file will replace the parent. That sounds like an improvement, but I don't think it is solving this problem, and letting the two co-exist. Can someone else test it?

@loopduplicate: can you still reproduce this one?

@Gabor, my comment from #8 still applies but the patch in #4 no longer fixes it for me.

I can verify that this is indeed a major bug (perhaps even critical, but I'll let someone else touch that).

I just spent 1.5hrs and having a serious Drupal WTF moment trying to figure out why my sub-theme of Bartik wasn't loading Bartik's layouts.css file. On the off chance that this issue may fix it, I applied that patch and it indeed fixed my issue.

Turns out that sub-sub-theme library inheritance doesn't work that well? Classy already declares layout.css and it was loading that file instead of Bartik's. However, when switching back to Baritk, it loads... so not entirely sure if this is the correct approach in the patch, but setting to CNW regardless for tests.

Ok, I'll do it. This is not a nice to have fixed. This is a must fix for subtheming. Let's figure out all the scenarios and get it working right.

I'll try to write test for this.

I wrote an test which overrides parent's same CSS file (and same path) and it looks like it works now, maybe this issue is not valid anymore? However, it would be ideal to have tests for that, so I've posted now the tests only AND with the patch provided above in comment #4.

+++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
@@ -99,7 +99,7 @@ public function getActiveThemeByName($theme_name) {
     while ($ancestor && isset($themes[$ancestor]->base_theme)) {
       $ancestor = $themes[$ancestor]->base_theme;
-      $base_themes[] = $themes[$ancestor];
+      array_unshift($base_themes, $themes[$ancestor]);
     }
 
     $active_theme = $this->getActiveTheme($themes[$theme_name], $base_themes);

I kinda disagree with that bugfix ... the code in ThemeInitialization creates a ThemeActive object which explicitly has now documentation the order of base themes, which is not the way how you order it here.

IMiksu, I think the problems is we don't want to the subtheme CSS file to remove the parent theme CSS file. Both files should get included, even if they have the same name. Is that what you see happening?

The parent theme CSS file should only get overridden when using 'stylesheets-override' in the subtheme's info.yml file.

I think that changes the logic we used to have in Drupal 7. Are we sure we want to change that?

Both files should get included, even if they have the same name.

Right! Thanks for feedback, I'll change the tests accordingly, however I'm not sure yet how this will get fixed in the end, but tests will be needed anyhow!

If a subtheme adds the same file name like the base theme or a module it replaces the base theme or module file.

I believe that is why the .info overrides were added. Here is the change record. https://www.drupal.org/node/1876600

The overrides were added to make sure the theme/subtheme explicitly states what it does and does not want. If we continue to use name clashing as a way to override, it limits what a subtheme can name its files. It's also confusing, because you are removing the parent file even though you didnt use 'stylesheets-remove'.

This is now only the tests which should introduce two failures until both CSS files gets loaded as this issue proposes.

This still needs work on the fix, tests should be covered now hopefully.

Setting to needs review to trigger testbot

Nice, did not saw that coming. It should have failed only against Drupal\system\Tests\Theme\ThemeTest->testCSSWithSameNameAsInParent(), here's new patch testing this. Should have only one fail.

+1 to #26.

+100 to 26

I would vote for simply getting rid of the automatic override.

#26 agree, we don't need automagical file removal anymore.

This will most likely break stylesheet-override and stylesheet-remove functionalities. I talked sportly with @alexpott and he suggested that we could try to move that functionality to hook_library_info_alter. Lets see how many tests we break with this.

Ups.. fixed gitignore so should be better now

Ok, I can live with #26 (I wasn't aware of this new logic). The real "bug" then is in Bartik.

There's no stylesheets-override specified anywhere in bartik.info.yml:

stylesheets-override:
  - layout.css

I'm still curious though why Bartik's layout.css loads, even without this override, and a sub-theme of Bartik doesn't.

Here's the issue that introduced Classy's layout.css file as a library asset instead of a simple .info.yml stylesheet entry.

Some key notes from that discussion is that stylesheets added to .info.yml files take precedence over libraries and supposedly if they're all libraries, they're handled "automatically"? Curious what that means exactly.

I don't see why this was changed to Bartik. This is not a Bartik specific problem.

Stylesheets can't be added from .info.yml anymore: https://www.drupal.org/node/2379475

I just want to take a second to highlight #3 and #6.

At this time, I don't have a strong feeling on what the right way to go is here but I agree this is a more general bug than Bartik, so I'm reverting the metadata changes in #40 and adding "in libraries" to the issue title as a hopefully helpful keyword.

Building off of #3, what about something like this, using extension-based paths:

stylesheets-override:
  # Overrides @foo/css/foo.css with css/my-foo.css from this theme.
  - foo/css/foo.css: css/my-foo.css
  # Overrides @bar/css/bar.css with css/bar.css from this theme.
  - bar/css/bar.css: css/bar.css

stylesheets-remove:
  # Removes @baz/css/admin.css.
  - baz/css/admin.css
  # Removes @quux/css/admin.css.
  - quux/css/style.css

I'm still curious though why Bartik's layout.css loads, even without this override, and a sub-theme of Bartik doesn't.

I also have this exact same issue.

Digging through this related issue we come across some fun prior art like:

#575298-6: Provide non-PHP way to reliably override CSS
#575298-7: Provide non-PHP way to reliably override CSS

To follow up on #40 / #43, if there is a working bug fix for the Bartik issue alone, that can be handled in a separate issue IMO.

I removed all Javascript related changes because Javascripts are working how they should be. Lets see if this will fix the stylesheet-override an sytesheet-remove. Its dirty and slow but its progress :P

Should apply now

One thing I might mention is that while we are using SMACCS we abandon it at the file naming level in themes, so why is that? E.g. why do we even have layout.css, should it not be bartik.layout.css, i.e how all modules are working right now, e.g. forum.theme.css.

If this were the case this issue effectively goes away? Do we want a more complex override/remove in info files, which are really their for the less technically inclined to start with?

I don't actually disagree with #26, just thinking this through.

I removed all the crazy stuff I did for the stylesheet-override and remove because they were not broken. Sorry for misreading the code. This one should (finally) be green.

I had a discussion in DrupalCamp London with @manuee and @alexpott about stylesheet-override and stylesheet-remove which both are being little bit broken and might have to be replaced with something like library-remove where you could remove complete libraries. Probelm is that if someone removes a CSS file in a info.yml file it will not load any of the files with that name, even from the same theme that way it would cause breaking things very easily. Also the stylesheet-override doesn't make that much sense after we've moved to use libraries because with it you would end up adding CSS files outside libraries. Probably it would still has to be there so single CSS files could be replaced and library dependencies would be kept in the original library.

Indeed, the biggest downside I see there is that you would end up clobbering JS as well when a library adds CSS and JS.

Non-critical follow-up to discuss improvements to stylesheets-override and stylesheets-remove, I'm thinking we don't need to tackle that here:

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

We have tests, thanks @iMiksu!

Lewis, I'm agnostic about how things are done, although I have implemented a system that names spaces like this and it has not been an issue. I always saw MAT as just an implementation of SMACCS just adapted to our use case in terms of naming and grouping convention. Sorry for the noise if any!

If we want to do that, which sounds sane for me, we probably need a change record documenting this.

Patch looks sane and definitely very simple. Here's a quick review.

1. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -123,23 +123,19 @@ public function getCssAssets(AttachedAssetsInterface $assets, $optimize) {
   +          // Local and external files must keep their name as the associative
   +          // key so the same CSS file is not added twice.
   

   Depending on the type, some options may need some processing.

2. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -123,23 +123,19 @@ public function getCssAssets(AttachedAssetsInterface $assets, $optimize) {
   +              // Local CSS files needs the basename to be saved because its
   +              // being used for the stylesheet override and remove
   +              // functionalities.
   

   s/needs/need/
   s/its being/it is/
   s/the stylesheet override and remove functionalities/overriding and removing stylesheets/

3. +++ b/core/modules/system/tests/themes/test_theme/css/layout.css
   @@ -0,0 +1,7 @@
   + * This file is for testing CSS file named same as in parent theme through
   + * *.libraries.yml file.
   

   This phrase could use some clean-up.

4. +++ b/core/modules/system/tests/themes/test_theme/css/layout.css
   @@ -0,0 +1,7 @@
   + * Used in
   

   Incomplete comment.

Fixed 1 & 2

Just had a thought. Instead of testing CSS files with the same name using test_theme and Classy, we should use test_basetheme and test_subtheme. Add CSS files to them and test them. That is what they are there for, correct?

That way if we figure out how to get rid of that empty CSS file in Classy we won't need to keep it just for tests.

+1 to that, if we don't have to make the tests depend on Classy then let's not.

There is a method that already exists in Drupal\system\Tests\Theme\ThemeInfoTest for testing the presence of stylesheets in base theme and subtheme. I'll add it there.

This would remove the need to fix #62 3 and 4.

Since this just fixes the stylesheet linking bug, and the more developer facing change will actually happen in #2451411: Add libraries-override to themes' *.info.yml, I don't think this needs a change record. Agree or disagree?

Agree.

Points from #62 are fixed. Also improvements by David are good.

Only nits.

1. +++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
   @@ -123,23 +123,19 @@ public function getCssAssets(AttachedAssetsInterface $assets, $optimize) {
   -          // Add the data to the CSS array depending on the type.
   +          // Local and external files must keep their name as the associative
   +          // key so the same CSS file is not added twice. Depending on the type,
   +          // some options may need some processing.
   

   Replace with: "Depending on the type, some options may need some processing."
   I meant: remove Local and external files must keep their name as the associative key so the same CSS file is not added twice. — we *always* key it by full path; it's pointless to state it there, especially because it doesn't even happen there, it happens far lower.

   (As said in #62.)

2. +++ b/core/modules/system/src/Tests/Common/AttachedAssetsTest.php
   @@ -89,7 +89,7 @@ function testAddFiles() {
   +    $this->assertTrue(array_key_exists('core/modules/system/tests/modules/common_test/bar.css', $css), 'CSS files are correctly added.');
        $this->assertTrue(array_key_exists('core/modules/system/tests/modules/common_test/foo.js', $js), 'JavaScript files are correctly added.');
   

   A nice side-effect: now CSS & JS are consistent again!

Actual code changes + test coverage look perfect.

Fixing #75 1.

#75 fixed

RTBC+1

But doesn't this make stylesheets-override and stylesheets-remove work in a very odd way? Like the override would override 2 css entries?

@alexpott, is that not case currently? Local files are keyed by the basename, so it is not possible to add more than one stylesheet with the same name. We are fixing the add more than one problem, and hopefully fixing stylesheets/library-remove problems in #2451411: Add libraries-override to themes' *.info.yml.

Adding beta eval.

@davidhernandez but this patch changes the behaviour. Prior to this patch it was possible to only have one dialog.css loaded. So we you need an override you knew that you where overriding one file. Now, if you have 2 dialog.css files then it will replace both, right?

@alexpott, yes, both get removed.

(Assuming this patch)
If you create a CSS file with the same name as one added by a base theme or module, you can add your file. All will be linked. If you use stylesheets-override, all the files will be overridden and linked to the same file (not good.) If stylesheets-remove is used, all files with the name are removed.

We seemed to be stuck in a spot where the functionality is awkward, but the awkwardness was not created here, it was exposed here. I think part of it being that we are only partly embracing libraries, not completely.

We can have stylesheets-override check for uniqueness, but we need more specific keying for stylesheets-remove to target a specific file. Doing that turns this from a bug fix into a real API change to get specificity in the .info.yml file. How to do that is part of what is being discussed in #2451411: Add libraries-override to themes' *.info.yml. In that issue we've talked about embracing the library way, but I don't want to change an API here, and then change it again there.

Should we fix this one, and escalate #2451411?

@effulgentsia, @webchick, @xjm and I discussed this and agreed that this is a critical because we should allow different extensions to provide css with the same name. Also this bug would be really hard to track down.

Re #83 tbh I think we need to address #2451411: Add libraries-override to themes' *.info.yml in this issue. Since this changes what an override or remove can do. Because before you knew you were targeting the only instance of the css that was loaded - now you might be targeting multiple css files.

So we can get #2451411: Add libraries-override to themes' *.info.yml to this issue

Should we just fix stylesheets-override and -remove here and leave the library addition for the other issue, since that might be a lot more work?

Should we just fix stylesheets-override and -remove here and leave the library addition for the other issue, since that might be a lot more work?

This sounds for me like the right thing to do, honestly. One change of functionality is good, two separate ones are worse, as it also discourages people from thinking about one properly.

Can we get an update on remaining tasks/proposed resolution, thanks in advance

I prefer using the "@node/css/node.css" syntax, since it is the same method we use in other places like template extends. It is also a little easier to use.

I updated the beta evaluation. This is now a disruptive change.

Created the initial code to replace placeholders from the stylesheets-remove and stylesheets-override.

I think we need to test if this approach works with vendor stylesheet files.

Ups forgot to attach interdiff

David: we no longer need to write @ for template extends references. Theme registry manages all for you... and this is very good as it works like twig works by default.

That is correct now for most uses cases, because of the theme registry, but in cases where specificity is needed we still need the @ syntax. This is mostly for when the template has the same name as the one it extends. For example, in Bartik:

{% extends "@classy/block/block--system-menu-block.html.twig" %}

Interresting. Wasn't aware of this.

Some progress I've made. Probably need to talk to someone because removing overriden files is a bit problematic.

I talked with @alexpott about the stylesheet-override and we both agreed that the stylesheet-override should be removed in order to create sanity for the assets. The problem we have with the stylesheet-override comes when there is multiple base themes and someone wants to override a CSS in one theme and remove it later on in other theme. Anyway everything stylesheet-override is doing is easily achieved by using hook_library_alter.

Started removing stylesheet-override and realised that this would have to be fixed first: #2050269: hook_library_info_alter() is not called for themes

Agreeing with Lewis. The whole point of functionality like this is to allow themers to make these changes without writing code. Telling themers to use hook_library_alter to manage stylesheets is not an optimal solution.

What is the problem that occurs when the parent theme overrides (with lauriii's patch) and the child theme wants to remove? Is it just not possible? What is the result?

Can anyone predict if we will have these same problems in #2451411: Add libraries-override to themes' *.info.yml ?

Nice lauriii, your patch has also fixed #2442169: Views tabs styling is broken :)

 @davidhernandez things get complicated when there is multiple levels of themes. Lets say base_theme overrides a CSS file and subtheme wants to remove that, should it be removing the one that has been already overridden or the original one? Things get even complicated when there is more levels. I don't think that would be easy enough to understand what it does. I would say 90% of the themers who want to modify the CSS files coming from core wants to just nuke all or some of the core css files away without caring about dependencies etc. Anyway they are more important with Javascript. For those people who wants to care about the dependencies we could provide documentation how they can do it using PHP.

I discussed about this with @lewisnyman and he agreed that it might not be useful to have stylesheet-override as we could have it because it would be still hard to use and expected what the overrides do in more complicated situations.

should it be removing the one that has been already overridden or the original one?

@lauriii, do you mean what the themer targets or what the stylesheets-remove function targets? I wouldn't think there is a problem with the function, because we are increasing specificity. Using @base_theme/css/style.css means there should be no confusion as to what the function targets. It targets the file you tell it to target, in the module or theme you specifiy. I think in the case you brought up, you would probably have to add a remove line for both css files. Possibly annoying, but a lot better than adding a php function.

Wouldn't it be less annoying to write (copy-paste) few lines of PHP if you would then know what you are doing?
I mean it gets a lot complex because now someone can first remove something, then override on next level and remove after that.

Btw. what if override is added to base theme afterwards? It will remove its children's removes and overrides (same applies other way around)? On the override situation you wouldn't be only relying on the original CSS file but the base themes override.. In hook_library_alter you would be able to build whatever logic is needed and it would be easier to not break things.

I just doesn't feel like clever option to create huge amount of logic behind something that most of the people doesn't need. Also other point is that it would be way more fragile than hook_library_alter.

Using PHP this would be:

/**
 * Implements hook_library_info_alter().
 */
function seven_library_info_alter(&$attachments) {
  if (isset($attachments['drupal.vertical-tabs'])) {
    unset($attachments['drupal.vertical-tabs']['css']['component']['misc/vertical-tabs.css']);
    $attachments['drupal.vertical-tabs']['dependencies'][] = 'seven/vertical-tabs';
  }
}

OR

/**
 * Implements hook_library_alter().
 */
function seven_library_alter(&$attachment, $extension) {
  if ($extension == 'core/drupal.vertical-tabs') {
    foreach ($attachment['css'] as $key => $css) {
      if ($css['data'] == 'core/misc/vertical-tabs.css') {
        $attachment['css'][$key]['data'] = 'core/themes/seven/css/components/vertical-tabs.css';
      }
    }
  }
}

It might be easy for a coder that understands PHP, but there are lots of themers who look at 3 lines of PHP and get lost. Especially, for something that can be as complicated as an alter. If we are stuck using the alter as the only solution so we can fix a critical bug, we'll deal with it, but that is not a themer-centric solution. That is a develop-centric solution.

Just looking at that code snippet now. That is damn complicated for someone that doesn't understand how any of this works.

+++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
@@ -123,23 +123,17 @@ public function getCssAssets(AttachedAssetsInterface $assets, $optimize) {
               if (!isset($options['basename'])) {
                 $options['basename'] = drupal_basename($options['data']);
               }

This can now also be removed, right? It's not used anywhere.

The problem here is that we can't have our cake and eat it too. With theme inheritance and libraries we get a lot power - but that means we can't have a simple way of overriding an individual part of a library.

By providing a stylesheet-remove option, themes can easily get rid of stuff. If they need to override and don't want to write the necessary PHP to alter a library they can remove the CSS using the same option and just add the css to every page after removing the css.

We need to add API changes to the issue summary.

#125: the answer to that is IMHO something like #2451411-9: Add libraries-override to themes' *.info.yml:

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.

@Wim: I agree with you.

I think the question is that we can still keep libraries-override in its own issue and fix as an follow up for this? Or do we have to address the fix for that in this issue.

Yes, Wim's libraries solution in #2451411: Add libraries-override to themes' *.info.yml is the preferred endpoint. We're just going to end up stuck with a step in between. We need to fix this critical bug, which may leave us with only stylesheets-remove, and then try to work on the library issue. I'm concerned that since that issue isn't critical it may not get in. So what is done here might be what we have for release.

At worst, we can probably have it in 8.1, correct? Since it would be an addition.

Rerolled and fixed Seven themes overrides.

Changed the patch to use hook_library_info_alter because hook_library_alter will be removed from Drupal.

1. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -51,6 +58,10 @@ public function __construct($root, ThemeHandlerInterface $theme_handler, StateIn
   +    $this->extensions = \Drupal::service('module_handler')->getModuleList();
   

   Note: you can inject '%container.modules%' to get that kind of information from the container ...

2. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -228,4 +218,24 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
   +  public function resolveStyleSheetPlaceholders($css_file) {
   

   That seems to be a method which does not need to be public, right?

Thanks @dawehner for the review and suggestions!

Rollbacking other change because module_list doesn't provide proper way to get the path for a module. We also cannot inject moduleHandler through services container because that would cause circulal dependencies.

Pinged @dawehner for review.

Are you sure about that?

            'container.modules' => array(
                'basic_auth' => array(
                    'type' => 'module',
                    'pathname' => 'core/modules/basic_auth/basic_auth.info.yml',
                    'filename' => 'basic_auth.module',
                ),

1. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -51,6 +58,10 @@ public function __construct($root, ThemeHandlerInterface $theme_handler, StateIn
   +    $this->extensions = \Drupal::service('module_handler')->getModuleList();
   

   Can we inject the module handler, please then?

2. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -51,6 +58,10 @@ public function __construct($root, ThemeHandlerInterface $theme_handler, StateIn
   +    $this->extensions = array_merge($this->extensions,  $this->themeHandler->listInfo());
   

   Ideally we load the extensions on the fly ... as on a normal page request, we should not need to rebuild the active theme object, but rather use the stored entry in state.

Can we inject the module handler, please then?

lauriii said We also cannot inject moduleHandler through services container because that would cause circulal dependencies.

And the information we have in the container.modules would have to be putted into to Extension object which I don't think is very nice.

Thanks @dawehner for sorting this out for me on IRC!

We still need change record

Yeah sometimes things just work, if you do them right!

There was still single extra line on seven.theme file.

Can we add some reasoning to the change record? If people only look at the CR they will not understand why it was removed, since the new method is more complicated.

I added some reasoning and kitten pictures so themers don't have to be sad :)

1. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -47,10 +55,12 @@ class ThemeInitialization implements ThemeInitializationInterface {
   +
   

   Extra blank line

2. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -101,6 +111,7 @@ public function getActiveThemeByName($theme_name) {
   +    $this->prepareExtensions();
   
   @@ -228,4 +218,36 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
   +  /**
   +   * Gets all extensions.
   +   *
   +   * @return array
   +   */
   +  protected function prepareExtensions() {
   +    if (!isset($this->extensions)) {
   +      $this->extensions = array_merge($this->moduleHandler->getModuleList(),  $this->themeHandler->listInfo());
   +    }
   +    return $this->extensions;
   +  }
   ...
   +      return str_replace($token_candidate, $this->extensions[$token]->getPath(), $css_file);
   

   Let's only do the merge if we need to.

3. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -228,4 +218,36 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
   +   * @return array
   +   *   List of stylesheets where placeholders are replaced.
   

   Returns a string

4. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -47,10 +55,12 @@ class ThemeInitialization implements ThemeInitializationInterface {
       * @param \Drupal\Core\State\StateInterface $state
       *   The state.
       */
   -  public function __construct($root, ThemeHandlerInterface $theme_handler, StateInterface $state) {
   +  public function __construct($root, ThemeHandlerInterface $theme_handler, StateInterface $state, ModuleHandlerInterface $module_handler) {
   

   Missing ModuleHandler from @param docs.

5. +++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
   @@ -228,4 +218,36 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
   +   * @param array $list
   +   *
   

   This should be $css_file and is a string and needs docs.

Fixed @alexpotts points

@lauriii I have some fixes for the project loading issue that may be related: #2413695: Modules getting installed in the theme directory if they don't have a *.module file, it just needs some tests.

Fixes fails

+++ b/core/lib/Drupal/Core/Theme/ThemeInitialization.php
@@ -228,4 +218,39 @@ public function getActiveTheme(Extension $theme, array $base_themes = []) {
+    // Prime extensions.
+    $this->getExtensions();
+    if (isset($this->extensions[$token])) {
+      return str_replace($token_candidate, $this->extensions[$token]->getPath(), $css_file);
+    }

I think this would be better as:

$extensions = $this->getExtensions();
if (isset($extensions[$token])) {
  return str_replace($token_candidate, $extensions[$token]->getPath(), $css_file);
}

Well fair, I still like buildExtensions() a bit better, but its alright.

Just a very little change for sevens info.yml file.

Let's add a test for that then.

re #158 .... so there is no SevenThemeTest so let's handle that in a followup.

+++ b/core/themes/seven/seven.info.yml
@@ -10,7 +10,7 @@
 stylesheets-remove:
-  - dialog.css
+  - core/assets/vendor/jquery.ui/themes/base/dialog.css

We need another change record to tell people about this.

The comments in ThemeInfoTest.php should be updated, as they still mention stylesheets-override.

test_theme.info.yml has a stylesheets-remove line that I think needs updating.

Stark has a styleheets-remove line that needs updating.

CR is done

Fixed #161

If #155 was RTBC, #164 addresses everything after.

This simplifies things a lot! Big thanks @lauriii and @davidhernandez for taking this on and everybody for contributing to it's current state! Awesome work!

Edit: scratch that @lauriii already created the follow-up.
@alexpott re #158 ping me on IRC with what you'd like to see in the seven tests follow-up and I'll create the IS. My guess is "Test that core dialog css doesn't appear on page when seven theme is enabled"?

I will still manually test this

I will ask Lewis Nyman to review last changes because they mostly relate to Seven theme.

The other side effect is that classy's layout.css file is loaded now...

This is expected, since we fixed the problem that was causing it not to load. Ultimately, we want to get rid of that layout file, which is a separate issue, but we could add it to stylesheets-remove for both Seven and Bartik.

The order of the base theme CSS is wrong...

That is a problem. We had that fixed at one point. The screenshot shows Classy's layout file getting added after Seven's. It should be the other way around.

I fixed the first point. The order is wrong also on head and there is separated issue for that #2474091: CSS files from themes are being loaded in reversed order

The consistency of how css is inherited from base themes will be important in Drupal 8 since we have Classy. Thanks @lauriii for sticking with this and ironing out all the complexity. See you all in #2474091: CSS files from themes are being loaded in reversed order. This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 0fb8f6b and pushed to 8.0.x. Thanks!

I published the change records.

Next step: #2451411: Add libraries-override to themes' *.info.yml.

prefixing yaml tokens with @ is invalid. A bug in Symfony's yaml parser allows this but it will block other parsers from being able to read our yaml files.

Sorry, meant NW. I re-opened this because it was a specific design decision. A possible solution though would be to open a follow up to update CR, documentation, and code with quoting the string. This is what migrate did a couple weeks ago after I brought the same problem to their developers.

@neclimdul Would be using '@foo/bar' be okay instead? I see that the services.yml files do that all over the place, don't they?

Thanks for the report and +1 to #180 string and maybe document this gotcha in the theming guide.

If that syntax is correct, we can open a follow up issue and patch the core info files. Also, change update the change record and maybe add another one to clarify?

I created a follow up. #2481183: Quote tokens in theme .info.yml files