Problem/Motivation

During #2375673: Split Bartik's CSS into SMACSS style components we've found that if we add a CSS file to a library with a name that already exists, that CSS never loads. It seems like the core CSS is overriding the theme CSS. This is a big WTF for themers.

We already have created way to override CSS files #575298: Provide non-PHP way to reliably override CSS but it has to be changed because its designed to remove a single file. If this functionality is left unchanged people would unintentionally override multiple files which has the same name.

Proposed resolution

Don't allow to override stylesheets anymore explicitly, so remove support for stylesheets-override in theme info files.
Therefore remove those information from the ActiveTheme object as well

Don't override CSS files that have the same name, let them to live in peace together. Stylesheet-override and stylesheet-remove have to be changed so themers can specify the CSS file they want to remove based on the location of the file:

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

The location of modules can change. In order to make it more stable we could add a placeholder that includes the location of the module or theme:

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

API Changes

stylesheets-override will be removed because the way its built doesn't support theme hierarchy. There is follow-up issue to address this API change later: #2451411: Add libraries-override to themes' *.info.yml.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug
Issue priority Critical because an important part of the theme system is not working properly.
Disruption This will be a disruptive change, because we will modify the syntax for stylesheets-remove and stylesheets-override will be removed. This mus be done to fix the critical bug.
Files: 
CommentFileSizeAuthor
#172 core_and_base_theme_css-2389735-172.patch20.74 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,691 pass(es). View
#172 interdiff.txt733 byteslauriii
#170 view-source_drupal8_dev_admin_content.png1.29 MBLewisNyman
#168 core_and_base_theme_css-2389735-168.patch20.27 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,622 pass(es). View
#157 interdiff.txt373 byteslauriii
#157 core_and_base_theme_css-2389735-157.patch18.68 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,574 pass(es). View
#155 core_and_base_theme_css-2389735-155.patch18.58 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,399 pass(es). View
#155 interdiff.txt672 byteslauriii
#152 css-stuff-2389735.152.patch18.58 KBlarowlan
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,429 pass(es). View
#152 interdiff.txt699 byteslarowlan
#149 interdiff.txt2.13 KBlauriii
#149 core_and_base_theme_css-2389735-149.patch18.53 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,382 pass(es), 3 fail(s), and 0 exception(s). View
#145 interdiff.txt276 byteslauriii
#145 core_and_base_theme_css-2389735-143.patch18.65 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,392 pass(es). View
#141 interdiff.txt2.6 KBlauriii
#141 core_and_base_theme_css-2389735-141.patch18.65 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,383 pass(es). View
#136 core_and_base_theme_css-2389735-136.patch17.04 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,565 pass(es). View
#136 interdiff.txt1.36 KBlauriii
#134 core_and_base_theme_css-2389735-134.patch17.89 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,515 pass(es), 13 fail(s), and 7 exception(s). View
#134 interdiff.txt1.75 KBlauriii
#132 interdiff.txt2.44 KBlauriii
#132 core_and_base_theme_css-2389735-132.patch17.03 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,464 pass(es). View
#130 core_and_base_theme_css-2389735-130.patch16.65 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,442 pass(es). View
#117 interdiff.txt492 byteslauriii
#117 core_and_base_theme_css-2389735-117.patch14.92 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,645 pass(es). View
#113 interdiff.txt3.9 KBlauriii
#113 core_and_base_theme_css-2389735-113.patch14.29 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,430 pass(es), 1 fail(s), and 0 exception(s). View
#111 core_and_base_theme_css-2389735-110.patch14.09 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,423 pass(es), 6 fail(s), and 0 exception(s). View
#107 core_and_base_theme_css-2389735-106.patch14.93 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core_and_base_theme_css-2389735-106.patch. Unable to apply patch. See the log in the details link for more information. View
#102 interdiff.txt7.98 KBlauriii
#101 core_and_base_theme_css-2389735-101.patch13.17 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,387 pass(es), 2 fail(s), and 1,017 exception(s). View
#96 interdiff.txt3.41 KBlauriii
#95 core_and_base_theme_css-2389735-95.patch8.58 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,395 pass(es), 15 fail(s), and 0 exception(s). View
#76 interdiff-70to76.txt836 bytesdavidhernandez
#76 core_and_base_theme_css-2389735-76.patch5.19 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,404 pass(es). View
#70 interdiff-68to69.txt1.07 KBdavidhernandez
#70 core_and_base_theme_css-2389735-69.patch5.33 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,493 pass(es). View
#68 interdiff-64to68.txt4.87 KBdavidhernandez
#68 core_and_base_theme_css-2389735-68.patch6.14 KBdavidhernandez
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,502 pass(es), 2 fail(s), and 0 exception(s). View
#64 interdiff.txt1.06 KBlauriii
#64 core_and_base_theme_css-2389735-64.patch7.17 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,483 pass(es). View
#52 core_and_base_theme_css-2389735-52.patch7.13 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,510 pass(es). View
#49 core_and_base_theme_css-2389735-49.patch7.31 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,972 pass(es), 10 fail(s), and 451,269 exception(s). View
#47 core_and_base_theme_css-2389735-47.patch7.48 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core_and_base_theme_css-2389735-47.patch. Unable to apply patch. See the log in the details link for more information. View
#4 core--base-theme-load-order-2389735-4.patch667 bytesloopduplicate
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,829 pass(es). View
#18 core--base-theme-load-order-2389735-18.patch3.61 KBiMiksu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,229 pass(es), 5 fail(s), and 0 exception(s). View
#18 core--base-theme-load-order-2389735-18-TESTSONLY.patch2.96 KBiMiksu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,261 pass(es), 2 fail(s), and 0 exception(s). View
#27 core--base-theme-load-order-2389735-27-tests.patch2.82 KBiMiksu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,306 pass(es), 3 fail(s), and 0 exception(s). View
#30 core--base-theme-load-order-2389735-30-tests.patch3.89 KBiMiksu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,237 pass(es), 1 fail(s), and 0 exception(s). View
#36 core_and_base_theme_css-2389735-36.patch7.46 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core_and_base_theme_css-2389735-36.patch. Unable to apply patch. See the log in the details link for more information. View
#38 core_and_base_theme_css-2389735-38.patch6.19 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,388 pass(es), 18 fail(s), and 17 exception(s). View

Comments

Wim Leers’s picture

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.

LewisNyman’s picture

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. There's no way to change this behaviour without breaking stylesheets-override?

Wim Leers’s picture

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.

loopduplicate’s picture

FileSize
667 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,829 pass(es). View

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.

lauriii’s picture

Status: Active » Needs review

Setting to needs review to trigger test bot

Gábor Hojtsy’s picture

Issue tags: +Needs tests

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

Gábor Hojtsy’s picture

Title: Core CSS files override theme CSS files with the same name » Core and base theme CSS files override theme CSS files with the same name

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

benjy’s picture

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.

Gábor Hojtsy’s picture

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

LewisNyman’s picture

hm, I recreated the original problem I found in #2375673: Split Bartik's CSS into SMACSS style components but it still doesn't load Bartik's CSS file.

  1. Rename Bartiks's dropbutton.component.css to dropbutton.css
  2. Change the reference to the file in bartik.libraries.yml
  3. Bartik's dropbutton.css should load

Anything I'm missing?

Gábor Hojtsy’s picture

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

davidhernandez’s picture

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?

Gábor Hojtsy’s picture

@loopduplicate: can you still reproduce this one?

benjy’s picture

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

markcarver’s picture

Status: Needs review » Needs work

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.

davidhernandez’s picture

Priority: Major » Critical

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.

iMiksu’s picture

Assigned: Unassigned » iMiksu

I'll try to write test for this.

iMiksu’s picture

Assigned: iMiksu » Unassigned
Status: Needs work » Needs review
FileSize
3.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,229 pass(es), 5 fail(s), and 0 exception(s). View
2.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,261 pass(es), 2 fail(s), and 0 exception(s). View

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.

dawehner’s picture

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

davidhernandez’s picture

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.

lauriii’s picture

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

The last submitted patch, 18: core--base-theme-load-order-2389735-18.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 18: core--base-theme-load-order-2389735-18-TESTSONLY.patch, failed testing.

iMiksu’s picture

Assigned: Unassigned » iMiksu

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!

hass’s picture

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

davidhernandez’s picture

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

iMiksu’s picture

Assigned: iMiksu » Unassigned
FileSize
2.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,306 pass(es), 3 fail(s), and 0 exception(s). View

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.

lauriii’s picture

Status: Needs work » Needs review

Setting to needs review to trigger testbot

Status: Needs review » Needs work

The last submitted patch, 27: core--base-theme-load-order-2389735-27-tests.patch, failed testing.

iMiksu’s picture

Status: Needs work » Needs review
FileSize
3.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,237 pass(es), 1 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 30: core--base-theme-load-order-2389735-30-tests.patch, failed testing.

Cottser’s picture

Issue tags: +Twig

+1 to #26.

LewisNyman’s picture

+10 to 26

dawehner’s picture

+100 to 26

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

Jeff Burnz’s picture

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

lauriii’s picture

Status: Needs work » Needs review
FileSize
7.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core_and_base_theme_css-2389735-36.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Status: Needs review » Needs work

The last submitted patch, 36: core_and_base_theme_css-2389735-36.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
6.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,388 pass(es), 18 fail(s), and 17 exception(s). View

Ups.. fixed gitignore so should be better now

Status: Needs review » Needs work

The last submitted patch, 38: core_and_base_theme_css-2389735-38.patch, failed testing.

markcarver’s picture

Title: Core and base theme CSS files override theme CSS files with the same name » Bartik does not override Classy's layout.css
Component: theme system » Bartik theme

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.

markcarver’s picture

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.

davidhernandez’s picture

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

Cottser’s picture

Title: Bartik does not override Classy's layout.css » Core and base theme CSS files in libraries override theme CSS files with the same name
Component: Bartik theme » theme system
Related issues: +#2377397: Themes should use libraries, not individual stylesheets

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

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.

Cottser’s picture

Cottser’s picture

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.

lauriii’s picture

Status: Needs work » Needs review
FileSize
7.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core_and_base_theme_css-2389735-47.patch. Unable to apply patch. See the log in the details link for more information. View

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

Status: Needs review » Needs work

The last submitted patch, 47: core_and_base_theme_css-2389735-47.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
7.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,972 pass(es), 10 fail(s), and 451,269 exception(s). View

Should apply now

Status: Needs review » Needs work

The last submitted patch, 49: core_and_base_theme_css-2389735-49.patch, failed testing.

Jeff Burnz’s picture

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.

lauriii’s picture

Status: Needs work » Needs review
FileSize
7.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,510 pass(es). View

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.

lauriii’s picture

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.

Cottser’s picture

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

Cottser’s picture

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

LewisNyman’s picture

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.

Actually we are using the SMACSS file naming conventions in themes, in modules we are using the MAT naming convention. MAT makes less sense in themes as as they don't need to be overridden by other themes, unless you have a base theme.

I can't imagine it being obvious/desirable to themers that they have to prefix all their stylesheets with the name of their theme.

lauriii’s picture

Issue tags: -Needs tests

We have tests, thanks @iMiksu!

Jeff Burnz’s picture

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!

dawehner’s picture

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

lauriii’s picture

Issue tags: +Needs change record
Wim Leers’s picture

Status: Needs review » Needs work

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.

lauriii’s picture

Assigned: Unassigned » lauriii
lauriii’s picture

Assigned: lauriii » Unassigned
Status: Needs work » Needs review
FileSize
7.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,483 pass(es). View
1.06 KB

Fixed 1 & 2

davidhernandez’s picture

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.

Cottser’s picture

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

davidhernandez’s picture

Assigned: Unassigned » davidhernandez

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.

davidhernandez’s picture

Assigned: davidhernandez » Unassigned
FileSize
6.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,502 pass(es), 2 fail(s), and 0 exception(s). View
4.87 KB

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

Status: Needs review » Needs work

The last submitted patch, 68: core_and_base_theme_css-2389735-68.patch, failed testing.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
5.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,493 pass(es). View
1.07 KB
davidhernandez’s picture

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?

Cottser’s picture

Agree.

davidhernandez’s picture

Issue tags: -Needs change record
lauriii’s picture

Status: Needs review » Reviewed & tested by the community

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

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

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.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
5.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,404 pass(es). View
836 bytes

Fixing #75 1.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

#75 fixed

Wim Leers’s picture

RTBC+1

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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

davidhernandez’s picture

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

davidhernandez’s picture

Issue summary: View changes

Adding beta eval.

alexpott’s picture

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

davidhernandez’s picture

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

alexpott’s picture

Status: Needs review » Needs work
Issue tags: +Triaged D8 critical

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

lauriii’s picture

lauriii’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
davidhernandez’s picture

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?

dawehner’s picture

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.

larowlan’s picture

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

LewisNyman’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

I've updated the issue summary with Lauriii

lauriii’s picture

Issue summary: View changes
lauriii’s picture

Assigned: Unassigned » lauriii

davidhernandez’s picture

Issue summary: View changes
Issue tags: +Needs change record

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.

lauriii’s picture

Status: Needs work » Needs review
FileSize
8.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,395 pass(es), 15 fail(s), and 0 exception(s). View

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.

lauriii’s picture

FileSize
3.41 KB

Ups forgot to attach interdiff

Status: Needs review » Needs work

The last submitted patch, 95: core_and_base_theme_css-2389735-95.patch, failed testing.

hass’s picture

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.

davidhernandez’s picture

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" %}
hass’s picture

Interresting. Wasn't aware of this.

lauriii’s picture

Status: Needs work » Needs review
FileSize
13.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,387 pass(es), 2 fail(s), and 1,017 exception(s). View

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

lauriii’s picture

FileSize
7.98 KB

Status: Needs review » Needs work

The last submitted patch, 101: core_and_base_theme_css-2389735-101.patch, failed testing.

lauriii’s picture

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.

xjm’s picture

Issue tags: +D8 Accelerate Dev Days
lauriii’s picture

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

lauriii’s picture

Status: Needs work » Needs review
FileSize
14.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch core_and_base_theme_css-2389735-106.patch. Unable to apply patch. See the log in the details link for more information. View

Status: Needs review » Needs work

The last submitted patch, 107: core_and_base_theme_css-2389735-106.patch, failed testing.

LewisNyman’s picture

I've posted a comment asking for some feedback from the contributors to #575298: Provide non-PHP way to reliably override CSS

I am a little concerned that we are removing something we added for good reason, themers do this all the time and don't like writing PHP ;-)

What would the change record look like for this issue?

davidhernandez’s picture

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 ?

lauriii’s picture

Status: Needs work » Needs review
FileSize
14.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,423 pass(es), 6 fail(s), and 0 exception(s). View
yannickoo’s picture

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

lauriii’s picture

FileSize
14.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,430 pass(es), 1 fail(s), and 0 exception(s). View
3.9 KB
lauriii’s picture

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

The last submitted patch, 111: core_and_base_theme_css-2389735-110.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 113: core_and_base_theme_css-2389735-113.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
14.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,645 pass(es). View
492 bytes
lauriii’s picture

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.

LewisNyman’s picture

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.

I still think we should assess the php we are expecting themers to write to recreate this functionality.

davidhernandez’s picture

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.

lauriii’s picture

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.

lauriii’s picture

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';
      }
    }
  }
}
davidhernandez’s picture

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.

Wim Leers’s picture

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

alexpott’s picture

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.

lauriii’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

We need to add API changes to the issue summary.

Wim Leers’s picture

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

lauriii’s picture

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

davidhernandez’s picture

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.

lauriii’s picture

Status: Needs work » Needs review
FileSize
16.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,442 pass(es). View

Rerolled and fixed Seven themes overrides.

LewisNyman’s picture

I like Wim's proposal. I'm happy to remove this functionality and later we can implement that. The code implementation is not good but if Alex is ok with it I am as well.

lauriii’s picture

FileSize
17.03 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,464 pass(es). View
2.44 KB

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

dawehner’s picture

  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?

lauriii’s picture

FileSize
1.75 KB
17.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,515 pass(es), 13 fail(s), and 7 exception(s). View

Thanks @dawehner for the review and suggestions!

Status: Needs review » Needs work

The last submitted patch, 134: core_and_base_theme_css-2389735-134.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
17.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,565 pass(es). View

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.

Wim Leers’s picture

Pinged @dawehner for review.

dawehner’s picture

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.

Wim Leers’s picture

Can we inject the module handler, please then?

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

lauriii’s picture

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.

lauriii’s picture

FileSize
18.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,383 pass(es). View
2.6 KB

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

lauriii’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

We still need change record

dawehner’s picture

Issue summary: View changes
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

lauriii’s picture

Issue tags: -Needs change record
FileSize
18.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,392 pass(es). View
276 bytes

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

davidhernandez’s picture

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.

lauriii’s picture

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

alexpott’s picture

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

lauriii’s picture

Status: Needs work » Needs review
FileSize
18.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,382 pass(es), 3 fail(s), and 0 exception(s). View
2.13 KB

Fixed @alexpotts points

Status: Needs review » Needs work

The last submitted patch, 149: core_and_base_theme_css-2389735-149.patch, failed testing.

joelpittet’s picture

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

larowlan’s picture

Status: Needs work » Needs review
FileSize
699 bytes
18.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,429 pass(es). View

Fixes fails

lauriii’s picture

Assigned: lauriii » Unassigned
alexpott’s picture

+++ 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);
}
lauriii’s picture

FileSize
672 bytes
18.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,399 pass(es). View
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

lauriii’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
18.68 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,574 pass(es). View
373 bytes

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

alexpott’s picture

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

Let's add a test for that then.

alexpott’s picture

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

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

alexpott’s picture

Issue tags: +Needs change record
+++ 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.

davidhernandez’s picture

Status: Needs review » Needs work

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.

lauriii’s picture

lauriii’s picture

Issue tags: -Needs change record

CR is done

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.77 KB
20.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,621 pass(es). View

Fixed #161

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

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

joelpittet’s picture

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

lauriii’s picture

Assigned: Unassigned » lauriii
Status: Reviewed & tested by the community » Needs work

I will still manually test this

lauriii’s picture

FileSize
20.27 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,622 pass(es). View
1.39 KB

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

lauriii’s picture

Status: Needs work » Needs review
LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
1.29 MB

I applied the patch and one of the differences are the loading order of the jqueryui/theme.css file. It doesn't cause a problem though. The other side effect is that classy's layout.css file is loaded now, because it was previously being replaced by Seven's layout.css. The order of the base theme CSS is wrong, but that is already wrong in HEAD.

davidhernandez’s picture

Status: Reviewed & tested by the community » Needs work

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.

lauriii’s picture

Status: Needs work » Needs review
Related issues: +#2474091: CSS files from themes are being loaded in reversed order
FileSize
733 bytes
20.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,691 pass(es). View

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

b0unty’s picture

Status: Needs review » Reviewed & tested by the community

latest patch seems to fix #171

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed 0fb8f6b on 8.0.x
    Issue #2389735 by lauriii, iMiksu, davidhernandez, larowlan,...
davidhernandez’s picture

I published the change records.

Wim Leers’s picture

neclimdul’s picture

Status: Fixed » Needs review

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.

neclimdul’s picture

Status: Needs review » Needs work

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.

dawehner’s picture

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

joelpittet’s picture

Assigned: lauriii » Unassigned

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

davidhernandez’s picture

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?

davidhernandez’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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