Problem

  • The current stylesheets[] theme .info property allows to override stylesheets, but an overridden stylesheet is appended to the end of the stack, instead of retaining its original position.

    Changing a stylesheets position breaks its expected effect. Stylesheets cannot be moved to a different loading position, by design.

    Also, the fact that stylesheets[], the facility to add stylesheets, is used for overriding, inherently means that all properties of the original stylesheet are lost. (e.g., media, scope, group, weight, etc.pp.)

  • The current stylesheets[] theme .info property allows to magically remove stylesheets, depending on whether the referenced CSS file name exists in the filesystem.

    This is a weird behavior for themers. It means to add something to remove something.

    It also requires us to perform a file_exists() on each and every CSS file that is loaded, which is bad for performance.

  • All of this magic logic is mixed into the stylesheets[] theme .info property, which is close to impossible to document and understand.

  • The current stylesheets[] theme .info property logic does not properly take base themes and sub themes into account.

  • Implementing a logic to properly override and remove CSS files in a theme requires advanced PHP skills and is anything but trivial to figure out.

Goal

  • Allow themes to properly override CSS files, retaining their original position and properties.
  • Allow themes to properly remove CSS files, respecting base theme and sub theme decisions.
  • Remove the entire magic from the stylesheets[] theme .info property and only make it add CSS files.
  • Improve performance by removing file_exists() checks.

Proposed solution

  1. Introduce the stylesheets-override[] theme .info property to reliably override CSS files that have been added by modules or base themes, retaining their original position, and retaining their original properties:

    stylesheets-override[] = system.theme.css
    

    Note: This exact example is where HEAD terribly fails — the overridden CSS file is appended to the CSS file stack instead of replacing the file at its original position, which inherently breaks all CSS on the entire site.

  2. Introduce the stylesheets-remove[] theme .info property to reliably remove CSS files that have been added by modules or base themes:

    stylesheets-remove[] = node.css
    

    Note: HEAD performs a file_exists() call on each and every CSS file, just to be able to support this. For a single page, this can easily mean 30+ filestats. For absolutely no good reason.

Override/Remove logic

Credits to @Jacine for putting this together:

Description Base theme Sub theme File used
Override combinations:
Base theme overrides a stylesheet, sub theme inherits it. stylesheets-override[] = file.css base-theme/file.css
Sub theme overrides a stylesheet. stylesheets-override[] = file.css sub-theme/file.css
Base theme adds a stylesheet, sub theme overrides it. stylesheets[all] = file.css stylesheets-override[] = file.css sub-theme/file.css
Base theme removes a stylesheet, sub theme overrides it. stylesheets-remove[] = file.css stylesheets-override[] = file.css sub-theme/file.css
Remove combinations:
Base theme removes a stylesheet; sub theme inherits it. stylesheets-remove[] = file.css None
Sub theme removes a stylesheet. stylesheets-remove[] = file.css None
Base theme adds a stylesheet; sub theme removes it. stylesheets[all] = file.css stylesheets-remove[] = file.css None
Base theme overrides a stylesheet, sub theme removes it. stylesheets-override[] = file.css stylesheets-remove[] = file.css None

Notes

  • These new theme .info properties have been implemented by Edge module for D7, and currently present the only reason for why that module is downloaded and installed by many themers.

Original summary

Themes like Seven (#575294: Add reset.css through drupal_add_css() instead of through the .info file) need to be able to conditionally override stylesheets only.

We have a MAGIC facility in theme .info files already that allows to just specify the stylesheets filename to override (or completely omit) it.

However, that is not sufficient, because it means to load all stylesheets on every page that need to be overridden.

Also: cross-referencing #443514: Stylesheet override logic prevents loading of stylesheets of third-party libraries, which is closely related to this issue, but not really the same bug.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Title: Conditionally override stylesheets » Conditionally override module stylesheets

This issue also needs to fix the problem that overridden stylesheets are appended to the theme stylesheet stack instead of replacing the original stylesheet's position in the stack.

A fairly simple approach would be to ditch the current automagical override

stylesheets[all][] = system.css

with

stylesheet_overrides[] = system.css

Doing so - and thereby removing the magic override from 'stylesheets' - would most probably also solve aforementioned #443514: Stylesheet override logic prevents loading of stylesheets of third-party libraries in one shot.

alanburke’s picture

This issue also needs to fix the problem that overridden stylesheets are appended to the theme stylesheet stack instead of replacing the original stylesheet's position in the stack.

I don't really see how that's an problem.
We've been doing this in Drupal6 for while now - it seems logical enough to me.
Have other themers been complaining about this?

alanburke’s picture

Priority: Critical » Normal

Oops.
I get it now.
Where a module css file gets overridden at theme level,
it gets included on every page load - not just when the original style would have been loaded.

Is it really critical? We've been getting by for a long time like this.

When css aggregation is used, doesn't every css file get loaded on every page anyway?

But overall, I agree, when an overriding css file is used, it should only be called when the original would have been called.
I don't see why we would need a new naming convention.
The overriding css file should simply only be loaded when needed.
Perhaps the logic in drupal_get_css simply needs to be improved?

More questions than answers.. sorry!

hass’s picture

+

sun’s picture

Any better suggestions than the one in #1 ?

If not, then let's do that. I'm facing this problem all over again.

RobLoach’s picture

Status: Active » Needs review
FileSize
4.16 KB

We ran into this with Seven's vertical-tabs.css being added all the time, and the solution was...

/**
 * Implements hook_css_alter().
 */
function seven_css_alter(&$css) {
  // Use Seven's vertical tabs style instead of the default one.
  if (isset($css['misc/vertical-tabs.css'])) {
    $css['misc/vertical-tabs.css']['data'] = drupal_get_path('theme', 'seven') . '/vertical-tabs.css';
  }
}

It would be great to remove that code though! Not sure stylesheet_overrides[] = system.css would work, as we'd need to know that full path of the file to override it. Although I'm not 100% sure on the implementation, this patch uses:

  stylesheet_overrides[misc/vertical-tabs.css] = vertical-tabs.css

Since themes can't know the entire path of modules or other themes, this patch also supports:

  stylesheet_overrides[module][system][system.css] = system.css
  stylesheet_overrides[theme][zen][layout.css] = myownlayout.css

I guess we could loop through $css, checking the basename of each CSS file added, but that might be slow. Thoughts?

casey’s picture

No need for paths. All stylesheet names are unique.

http://api.drupal.org/api/function/drupal_add_css/7 (see basename)

RobLoach’s picture

FileSize
4.99 KB

Why check the basenames on drupal_get_js() when we could do it during addition instead?

RobLoach’s picture

FileSize
5.47 KB

This might be a bit cleaner as it checks the CSS's weight before overriding basename collisions.

Status: Needs review » Needs work

The last submitted patch, 575298.patch, failed testing.

Jacine’s picture

Very happy to see this! :)

JohnAlbin’s picture

subscribe.

casey’s picture

It might be too late, but I think stylesheets defined in .info files shouldn't be added automaticly to all pages. The .info file just defines their existence (like files[]).

They still should be added through drupal_add_css or [#attached][css].

So stylesheet[] == stylesheet_overrides[]

Does that make sense?

RobLoach’s picture

In the theme's .info file, stylesheets defined via stylesheets[] are always added to the page. This proves to be not very helpful when we have CSS like vertical-tabs.css, which is only needed when Vertical Tabs are being displayed. We don't want Seven's Vertical Tabs CSS being added all the time, we only want it there when Drupal core's vertical-tabs.css is there. This is something the theme can do through hook_css_alter(), but it would be nice for themers to not have to write any code to do it.

sun’s picture

Issue tags: +API change

We should definitely make all stylesheet .info file properties use a common pattern, see #522006-76: Conditional Styles in .info files, since drupal_add_css has it

RobLoach’s picture

What happens when instead of "overriding" a CSS file, we just want to add it when the original stylesheet is there?

sun’s picture

@Rob Loach: I can't recall to ever had a use-case for that. I'd say you either put such styles into your theme's css, or the module's stylesheet(s) should be rewritten, so that common base styles are separate from theme-specific styles, so you can override the theme-specific file.

casey’s picture

Trying to clarify my comment in #13:

To me the .info file is (should be) more like a description of what a module contains rather than a configuration file (although I can't find them, I do remember some issues where Dries said he doesn't like using .info files for configuration purposes). Therefor I think stylesheet[] should be used to indicate which stylesheets a module/theme contains, not where/when to use them.

Unfortunately this is not the case; all stylesheets defined in stylesheet[]'s are added to all pages. I rather see them added through code (using drupal_add_css or [#attached][css]) or through a specificly-for-configuration-only file (which doesn't exist, but which should be ideal for themes).

What I am trying to say: I don't really like the idea of stylesheet_overrides[]

But I understand the usefulness, so I won't bother any more.

RobLoach’s picture

sun at #17:

I can't recall to ever had a use-case for that. I'd say you either put such styles into your theme's css, or the module's stylesheet(s) should be rewritten, so that common base styles are separate from theme-specific styles, so you can override the theme-specific file.

I can see wanting to change the Vertical Tabs look a bit, but not completely overriding it. You wouldn't want to duplicate the CSS file to make a couple small changes. Wouldn't it make more sense to have a CSS file of just your Vertical Tab changes, rather than copy/pasting the whole thing? I don't know, might just be me. We don't want duplicated CSS everywhere.

Seems like this would give us:

stylesheet_overrides[]
Completely replaces CSS files
stylesheet_additions[] ?
Added only when the base CSS is provided.
sun’s picture

Title: Conditionally override module stylesheets » Overridden module stylesheets are loaded unconditionally
Issue tags: +frontend performance

Sure, I can see that use-case. But I still wouldn't even imagine to do the overhead of creating and registering a new .css file just to extend or add a couple of CSS lines for some infrequently loaded file. My theme's style.css can happily hold that. If I'd override more, then I replace the entire file anyway.

I'd recommend that we fix the actual override problem at hand for D7 and consider the stylesheets-additional[] for D8. Fixing this bug is important for frontend performance, especially in light of #769226: Optimize JS/CSS aggregation for front-end performance and DX

Therefore, tagging + better issue title.

sun’s picture

Status: Needs work » Needs review
FileSize
4.95 KB

@Rob Loach: Reconsidered. See #777814: Allow modules to provide styles for themes (finally leverage structure/behavior split of CSS files) for an approach that sounds more sane to me regarding "additional" stuff. The essence of your question is that you do not want to override generic/structural/behavior styles, only the look/design of something. Aforementioned issue intends to tackle this.

Let's continue with stylesheet overrides here.

yayayayay! This also finally solves the problem that overriding a module stylesheet puts the override at the end of the stack. This already caused headaches when node.css or whatever was overridden, but module Foo tried to override certain styles of that stylesheet.

sun’s picture

hehe, ok, touch of a clusterfuck, but doable.

sun’s picture

And if we already need to make it a bit weird, then we can also streamline + optimize the code to save some cycles for performance...

This one should hopefully make the bot happy, too.

Status: Needs review » Needs work

The last submitted patch, drupal.stylesheets-override.23.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
5.14 KB

Partially back to the original approach. Like it or not, the current API assumes that subsequently added CSS files sharing the same basename override the previously added one.

The effective goal here is to make all tests pass without any changes to them.

Status: Needs review » Needs work

The last submitted patch, drupal.stylesheets-override.25.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
7.51 KB

Fixed the stylesheet tests + heavily sped them up. wow, our tests really could use some love.

I have no idea what's wrong with those Color tests. Does anyone have a clue?

Status: Needs review » Needs work

The last submitted patch, drupal.stylesheets-override.27.patch, failed testing.

sun’s picture

Assigned: Unassigned » sun
Status: Needs work » Needs review
FileSize
10.08 KB

Resolved it. This one should pass.

sun’s picture

Issue tags: +Needs tests

Any feedback?

+++ includes/common.inc	14 Aug 2010 19:41:06 -0000
@@ -2767,9 +2767,20 @@ function drupal_add_css($data = NULL, $o
+      case 'file':
+        // Local CSS files are keyed by basename; if a file with the same
+        // basename is added more than once, it gets overridden.
+        // By default, take over the filename as basename.
+        if (!isset($options['basename'])) {
+          $options['basename'] = basename($data);
+        }
+        $css[$options['basename']] = $options;
+        break;

As already mentioned before, the current CSS API in Drupal core assumes that a later added file having the same basename of an already added file overrides that previous file. That was also the reason for introducing $options['basename'] in D7, so modules are able to load CSS files that may have names they cannot influence (e.g., external libraries). So I'm not sure why local CSS files were keyed by full file path, but given the current logic of the CSS API, it only makes sense to key them by basename.

+++ includes/common.inc	14 Aug 2010 19:41:06 -0000
@@ -2810,17 +2823,19 @@ function drupal_get_css($css = NULL) {
+  // Allow themes to conditionally override module CSS files by basename.
+  drupal_theme_initialize();

I'm not sure whether we need to ensure that the theme is initialized in drupal_get_css(). In 99.9% of all cases, it should be initialized already, as drupal_get_css() is normally invoked from theme/template process callbacks only. The only exception may be AJAX requests.

+++ includes/common.inc	14 Aug 2010 19:41:06 -0000
@@ -2810,17 +2823,19 @@ function drupal_get_css($css = NULL) {
+  if (!empty($theme_info->stylesheet_overrides)) {
+    $basenames = array();
+    foreach ($css as $key => $options) {
+      if (isset($options['basename'])) {
+        $basenames[$options['basename']] = $key;
+      }
+    }
+    foreach ($theme_info->stylesheet_overrides as $basename => $theme_name) {
+      if (isset($basenames[$basename]) && isset($css[$basenames[$basename]])) {
+        $css[$basenames[$basename]]['data'] = drupal_get_path('theme', $theme_name) . '/' . $basename;
       }

Speaking of basenames above, it may be possible to skip the $basenames mapping here and directly iterate over $theme_info->stylesheet_overrides resp. $css.

Powered by Dreditor.

Jacine’s picture

I really think this needs to go in. This is simply awesome. It solves that a huge problem and simplifies the process of overriding module CSS files significantly for themers.

So I'm not sure why local CSS files were keyed by full file path, but given the current logic of the CSS API, it only makes sense to key them by basename.

Me neither. Your patch is a HUGE improvement. The vertical tabs example is one thing, but when doing this for modules, you need end up needing to use drupal_get_path() twice, for no good reason.

Speaking of basenames above, it may be possible to skip the $basenames mapping here and directly iterate over $theme_info->stylesheet_overrides resp. $css.

I don't know the answer to this. :(

After this, all we need is #522006: Conditional Styles in .info files, since drupal_add_css has it, and we can put an end to the HUGE WTF that is working with CSS files in D7 themes.

sun’s picture

Shorter, quicker, faster.

I think this is ready to fly. When #887870: Make system_list() return full module records and #328357: Allow module .info files to add CSS/JS get in, we might be able to move the code from list_themes() into _system_rebuild_theme_data(). However, that shouldn't hold off this patch.

reglogge’s picture

Status: Needs review » Reviewed & tested by the community

Awesome. This would make it so much easier for themers to override stylesheets. Some themers I know grow green in the face when confronted with the current way of doing this and end up copy-and-pasting huge chunks of CSS from core and contrib stylesheets into their theme's style.css. So a huge +++ for getting this in together with #328357: Allow module .info files to add CSS/JS (of course).

I reviewed and tested the patch with Seven theme and it works perfectly.

Jacine’s picture

Beautiful. This is the kind of thing that will show people we actually mean it when we say we are reaching out to the design community.

RTBC +1!

sun’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +API change, +frontend performance

The last submitted patch, drupal.stylesheets-override.32.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
10.04 KB

Re-rolled against HEAD.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal.stylesheets-override.37.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests
FileSize
10.14 KB

Sorry, that was the wrong file, hence failing tests.

Jeff Burnz’s picture

Oh yes this is fantastic, thank-you so much for the hard work Sun, +1.

effulgentsia’s picture

Subscribe. I'm not spending time looking at this, as it's already RTBC, but +1 to fixing this bug. Even if it doesn't qualify as "major" or "critical", it's very nice for theme DX and front-end performance to have this.

Jacine’s picture

Priority: Normal » Major
juan_g’s picture

It looks like this major RTBC bug, and the critical bug it's blocking (see #42), are really important for theme designers.

Since their fixes need API change, they should be committed before Drupal 7 beta, which seems to be coming in the next days (see for example Criteria for Drupal 7 beta?).

sun’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +API change, +frontend performance

The last submitted patch, drupal.stylesheets-override.39.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community

Unknown testbot hiccup in Poll upgrade path.

reglogge’s picture

sun’s picture

Priority: Major » Critical

Alright, so let's bump this to critical to draw some attention and hopefully unblock those other critical issues ASAP.

Dries’s picture

Priority: Critical » Major

I read up on the issue and I don't understand why this is critical. Everything you want to do can already be done using hook_css_alter(). This seems like a DX improvement that can wait to D8. If you think this is truly critical for D7, please make a better case for it because it is not really clear.

(This is exactly what I warned us for; .info files evolving into a programming language. Soon we'll call it Drupal-script and we'll have a book dedicated to .info files.)

Jacine’s picture

DX is a major failure, but that is not the only issue here.

Override != Load on every page.

The main issue here is that overriding CSS files in .info files is horribly broken. Any stylesheets defined in the .info file will load on every single page, whether it was meant to in the first place or not. This is a definite performance problem. It's also a recipe for annoying CSS bugs, given that the CSS in these files was not written to be loaded on every page and therefore is likely to introduce conflicts and bugs when used elsewhere.

Also, how can we possibly claim the current functionality allows you to "override" stylesheets if said file is only meant to load in the admin section (for example) in the first place. When we "override" node.tpl.php we don't print the node.tpl.php template file on every single page, whether we are actually dealing with a node or not, just because it exists in the theme. This is the same thing. If Drupal didn't throw a gazillion stylesheets at themers in every installation, maybe it wouldn't be such a big deal, but that's not that case.

hook_css_alter() is too much and too hard to throw at newbies for such a basic task

You really need PHP chops to work with CSS files in Drupal 7. This is just sad. A developer saying it can all be done with hook_css_alter() is one thing, but for a themer to actually get to a place where where they can implement it is another thing entirely. I worry about the designers that are scared of preprocess functions (there are lots of them), and those just want to deal with a few template files and write CSS. I also think it's pretty ridiculous that such basic functionality is not properly supported in .info files.

With CSS files there are 3 very common and basic tasks that need to happen with stylesheets:

  1. Add
  2. Override (fixed by this patch)
  3. Remove (which is also currently broken - #901062: Regression: Themes can no longer remove stylesheets by overriding them in .info files

In reality, this is not a lot to ask, but we basically give designers the finger, and tell them to learn PHP AND Drupal APIs when it comes to doing anything other than adding files (and even that is half-baked). By not fixing this issue we are saying: Deal with these files on every page, and the nasty performance issue it causes, along with possible CSS conflicts since the CSS was not written to load everywhere, or learn the following:

  1. Learn what an alter hook is and go find an example somewhere.
  2. Create a template.php, and paste an example in there.
  3. Figure out that you must change the function name to begin with your theme name and that you must clear the cache for Drupal to pick up that it's there.
  4. Learn how to print arrays to the screen to find out what you are dealing with and begin to understand how you can make changes to it.
  5. Get way up close and personal with all the parameters of drupal_add_css(), once you find it.
  6. Because the keys are full paths, figure out how to use drupal_get_path() (a function you have probably never seen before as a themer) or hard-code paths to contributed modules that will likely break your site when you push it live.
  7. Figure out how to unset() or change variables inside the array (and no, that's not easy or self-explanatory for someone who writes HTML/CSS for a living).

All of that, instead of:

stylesheets-override[] = node.css

Really? Sigh.

Is this really what we want? Do you really think non-PHP themers/designers/newbies will get through this? Or do you think they'll go find html.tpl.php and just delete <?php print $styles; ?>, hard-code their own stylesheets in, get themselves into a world of trouble and give up? I'm not exaggerating. I've seen this before.

I think this is critical and that's why.

Jeff Burnz’s picture

I dont think we're asking for a new scripting language or books on info files - what we are asking for are a few simple tools to make Drupal theming a hell of lot more simple.

We can already do many things in info files - set regions, hide regions, disable features, declare base themes, and yes, we can even add some stylesheets (but not all, if you want to support IE go learn PHP thank-you.). I actually can't see the difference between being able to hide a region or disable a feature than overriding a stylesheet. Why is this so much more worse?

And yes, we are "giving designers the finger", my guys are shit scared of D7 at the moment, I am not kidding you - they look D7 and just start crying. As far as they are concerned the learning curve just went vertical, they don't even want to touch it because they know they're going to have to spend 100 hours boning up and figuring all this new stuff out. And we want to make this even harder? Why?

reglogge’s picture

@Jacine + @Jeff: AMEN!

dreamleaf’s picture

@Dries As a designer/themer (in that order), I can say that while most of the technical conversation goes over my head, I can see the plot and understand that if we want (cos I do) adoption of D7 and onwards to increase expotentially, then we need a system that makes that possible.

The changes proposed to add this functionality within the .info files makes most sense to me. My reasoning is that the system (process) should support the user, and as stated well above, forcing someone to trawl, learn and comprehend various API calls when it can be done from within the .info file seems counter productive.

When new people (am talking the non php community) approach Drupal they expect a learning curve, but a large proportion just want a system "that works". This is partially unrealistic because the system is so powerful, but as already identified and evangelised, Drupal needs more designers and more links with the design community to bridge a real gap to get rid of the stigma of "ugly Drupal". By introducing small features such as this WILL help bridge the technical divide. For example, Wordpress attracts more designers.. why? Because the community produce lots of code snippets that designers can use easily without reading a manual.

juan_g’s picture

Jacine wrote (#31, #34):
> I really think this needs to go in. This is simply awesome. It solves that a huge problem and simplifies the process of overriding module CSS files significantly for themers. (...)
> Beautiful. This is the kind of thing that will show people we actually mean it when we say we are reaching out to the design community.

Jeff Burnz wrote (#40):
> Oh yes this is fantastic, thank-you so much for the hard work Sun, +1.

effulgentsia wrote (#41):
> +1 to fixing this bug. Even if it doesn't qualify as "major" or "critical", it's very nice for theme DX and front-end performance to have this.

Well, if all these remarkable Drupal contributors are supporting this sun's bug fix (see also Jacine's #50 and Jeff Burnz's #51, even JohnAlbin -Zen- is subscribed since #12), then it seems a significant issue for the Drupal designer community, and also for performance. And it's RTBC already, so it's not delaying D7 beta, on the contrary its fix would unblock other D7 critical issues.

BTW, thank you very much to Jacine for your Skinr module (for customizable themes, with styles in Drupal UI), used by some of the most popular base themes and their subthemes, such as TopNotchThemes' Fusion (Acquia Marina, Commons...), Jeff Burnz's AdaptiveTheme, etc. Even a Jacine & JohnAlbin's patch for integration with Skinr was committed to Zen. Thanks to all for their great contributions to the Drupal theming improvement.

sun’s picture

Next to the very good reasons above, let me add a technical reason:

PHP and hook_css_alter() can do anything. Through simple .info file properties, Drupal core can provide and ensure an expected default behavior that is guaranteed to be the same.

It's very easy to break the list of added CSS via PHP, mistakenly change weights, or mistakenly do other things. Only an .info file property is able to do a predefined, hard-coded manipulation of CSS files. Technically, it is the only way to ensure that every CSS override behaves identically and does not lead to unexpected consequences.

Given the built-in stylesheets-override .info property handling, Drupal core is able to adjust and tweak the default overriding behavior whenever necessary, if we happen to find a bug or want or need to support any kind of improvement.

sun’s picture

Status: Reviewed & tested by the community » Closed (won't fix)
Jeff Burnz’s picture

sun - can we please have some explanation as to why this is suddenly closed and won't fix, that seems rather sudden.

Dave Reid’s picture

Status: Closed (won't fix) » Reviewed & tested by the community

No reasoning to backup the change in status, so restoring status.

sun’s picture

Version: 7.x-dev » 8.x-dev

Although badly needed, this sounds like D8 material to me.

chx’s picture

Version: 8.x-dev » 7.x-dev

#50 is reasoning enough to consider for Drupal 7.

Mark Trapp’s picture

#50 rings hollow to me. I don't understand the need to be so hyperbolic in trying to get this feature in.

Every single theme at some point requires some amount of PHP knowledge, even it's in the form of copying and pasting snippets. Heck, all but a few of the theme howtos in the handbook involve PHP.

And in case one thinks Drupal is far behind in themeability and themers are going to leave Drupal in droves because they have to use hook_css_alter(), WordPress, arguably one of the most themer-friendly CMSes out there, is even more PHP-laden, and it's never been a problem for them.

In fact, we should be empowering users to build great themes using the entire theme system, not trying to shield people and convince them that Drupal's theme system is something it's not by exposing an arbitrary subset of the theme system's capability to info files.

Why couldn't snippets for overriding CSS with hook_css_alter() be added to the handbook like everything else?

juan_g’s picture

Mark, the development community and the design community are trying to meet in Drupal, however they are different points of view indeed. For instance, we can see module contributors using command line for CVS who can't understand why theme contributors use graphical user interfaces for the same task...

Jacine’s picture

@Mark Trapp

I could not disagree more with everything you wrote. I also do not think as a "Lead Developer" you have any grounds for accusing me of being "so hyperbolic" in my response, when I was clearly stating what's obvious to most designers/themers out there. I think it's very easy for any developer to say that, but it's wrong.

In fact, we should be empowering users to build great themes using the entire theme system, not trying to shield people and convince them that Drupal's theme system is something it's not by exposing an arbitrary subset of the theme system's capability to info files.

I suppose you feel the same way about using jQuery over vanilla JavaScript then? FTR, we DO encourage users to build great themes using the the entire theme system, but we cannot throw people into the deep end like this and expect them to succeed. There are entire books written on the theming system, and it is far from intuitive, and has many WTF's and inconsistencies along the way. You make it sound like something anyone should be able to jump into without issue.

This is such basic functionality that is horribly broken and it should be fixed, and that is the bottom line. If we are not interested in doing that, then lets just be honest and mark this "wont fix" because every day, these issues are seeming more and more like a waste of precious time.

Mark Trapp’s picture

@Jacine as a themer myself, I am well aware of many of the quirks of the Drupal theming system. And I'm all for features that improve the Drupal experience.

But you seem to be implying that a new themer shouldn't have to know PHP to get started in theming, and by requiring hook_css_alter(), Drupal breaks that covenant with its themers. But as you know, you can't actually write a Drupal theme without knowing some PHP.

So, even if everything is implemented to remove the requirement to use hook_css_alter() for some tasks involving CSS, a themer is still going to encounter PHP right from the get-go. So now there's an additional level of complexity (a pseudo-language implementation of a hook that already exists) that doesn't solve the initial problem: that a themer needs to dive into PHP in order to build a theme.

So why do we need to implement something that's suggesting one thing (you can get started with Drupal theming without being familiar with PHP or the Drupal API), when what's being suggested is really not true? It seems like, if themers having to know PHP is a game-breaking issue, it should be tackled in earnest for D8 or beyond as these two issues aren't going to solve the core problem.

But I'm suggesting it's not a game-breaker: adding and overriding CSS is a straight-forward, clear operation that provides a great way for a themer who might be apprehensive about getting into PHP to get his or her feet wet with Drupal's API. And from there, they can move on to all the other countless things they're going to need to do in PHP to build awesome themes with Drupal.

Jacine’s picture

But you seem to be implying that a new themer shouldn't have to know PHP to get started in theming, and by requiring hook_css_alter(), Drupal breaks that covenant with its themers. But as you know, you can't actually write a Drupal theme without knowing some PHP.

Huh? Of course you can.

Part of the reason Stark was added to Drupal 7 was to demonstrate Drupal's default markup and encourage CSS only themes. Let me quote webchick's slides from Drupalcon DC as part of what's new in Drupal 7 for designers: http://www.slideshare.net/guest31ca73/drupal-7 (slide 104-106)

Re-vamped page.tpl.php

  • Re-engineered XHTML structure to facilitate CSS-only design
  • Standard, semantic class/ID names (.section/#navigation, etc.)
  • Improved consistency throughout.
  • Enables designers to create beautiful, CSS-only themes without touching PHP

Unless this bug and and the other critical is fixed, this is a flat out lie, and that is what I am trying to get fixed here.

Jeff Burnz’s picture

With all due respect Mark your *average* designer/themer is going to faint at the site of hook_[whatever]_alter. We're trying to reduce barriers for designers and telling peeps to learn PHP has not been a winning formula for Drupal in the past (with regards to design), and that isn't going to change.

I agree that doing a custom Drupal theme does require a fair amount of PHP chops, however a big drive of D7 was to remove that barrier as much as possible - to make the CSS only theme a reality. I'm not completely hung up on doing stuff in the info file, but I do agree its at least a solution to a problem. If a better solution comes along I'd be very interested, however simply bashing designers with the PHP whacking stick is not the answer here.

catch’s picture

Issue tags: +Needs committer feedback

This either needs to go in, or get put out of its misery and pushed to D8 again.

juan_g’s picture

webchick's quote on D7:
> Enables designers to create beautiful, CSS-only themes without touching PHP

Excellent. I'm not a themer, but a former Perl programmer -learning PHP now- and science teacher, and also agree with the designers (Jacine, Jeff Burnz...) and most developers here (effulgentsia, chx...).

In addition, this RTBC would quickly unblock a critical D7 bug, #901062: Regression: Themes can no longer remove stylesheets by overriding them in .info files, a regression compared to D6, which in turn would help unblock Drupal 7 beta if we follow the criteria of upgrade+API before beta.

reglogge’s picture

Apart from the discussion about who would benefit here from a UX/TX perspective, I also think that sun makes an excellent technical argument for committing this in #55.

So getting this in would not only be a huge boon for themers but also harden Drupal's css-handling considerably.

juan_g’s picture

Yes, and this also fixes a performance bug. See comments #20, #41, #50.

catch’s picture

Priority: Major » Critical

If this blocks #901062: Regression: Themes can no longer remove stylesheets by overriding them in .info files, then it's critical, unless that issue gets bumped down to major. Haven't reviewed the patch here, but it's either a blocker or it's not.

webchick’s picture

Version: 7.x-dev » 8.x-dev
Category: bug » task
Priority: Critical » Major

Two things:

#1: "Enables designers to create beautiful, CSS-only themes without touching PHP" is not a flat-out lie. It's talking about the additional classes available in page.tpl.php which do indeed allow themers to do a whole bunch of stuff in style.css without touching PHP where previously they'd have had to use tons of preprocess rules (just look at Zen's template.php in D6).

I didn't say that they would be able to create pixel-perfect designs without having to override a bazillion rules or using a reset.css to do it. :P That would be a flat-out lie. :P

#2: This patch doesn't hold up #901062: Regression: Themes can no longer remove stylesheets by overriding them in .info files at all, from what I can tell. The minimal required to fix that issue is to restore the removal functionality from D6 which is a 766 byte patch. And even the "maximal" fix depends on nothing in this patch, apart from the two will conflict because they touch the same general area of the code, at least from my 10-second scan.

I've been weighing this patch heavily over the past week or so. I get that this is a HUGE TX improvement. Every single thing Jacine said in #50? +1. This hugely breaks down enormous barriers for designers who are comfortable in (X)HTML (5) and CSS. These are a very important part of our community, and we've made tremendous strides in D7 to make life easier for them, and this would be a wonderful finishing touch.

But, even aside from Dries's concerns about DrupalScript™, it's simply way, way, way too late to make these kinds of changes in D7. Unfortunately, it fails the "Was this situation bad enough to block previous releases of Drupal?" test. No, it wasn't. And therefore by definition, can't be bad enough to stop the release of D7, which means I can't justify the API change here, no matter how much I'd love to.

So, with heavy heart, moving to D8. :\

One thing that would be interesting to explore though is that we might just have all the ingredients in place (hook_css_alter(), hook_theme_settings() et al) to allow someone to make a utility module in contrib that provides this functionality in D7.

Jacine’s picture

Ok, I'm not happy about it, but I do appreciate knowing the fate of this issue, so thank you :)

Also, just including this here for reference, when we start on D8. This patch also fixes this sort of stupidity in hook_css_alter(), because you need to manually deal with RTL CSS files right now:

--- themes/seven/template.php	14 Aug 2010 00:43:24 -0000	1.20
+++ themes/seven/template.php	1 Oct 2010 09:00:47 -0000
@@ -97,6 +97,9 @@ function seven_css_alter(&$css) {
   if (isset($css['misc/vertical-tabs.css'])) {
     $css['misc/vertical-tabs.css']['data'] = drupal_get_path('theme', 'seven') . '/vertical-tabs.css';
   }
+  if (isset($css['misc/vertical-tabs-rtl.css'])) {
+    $css['misc/vertical-tabs-rtl.css']['data'] = drupal_get_path('theme', 'seven') . '/vertical-tabs-rtl.css';
+  }
juan_g’s picture

webchick wrote:
> One thing that would be interesting to explore though is that we might just have all the ingredients in place (hook_css_alter(), hook_theme_settings() et al) to allow someone to make a utility module in contrib that provides this functionality in D7.

That would be useful indeed. Is it possible with a D7 module? Maybe sun or Rob Loach can say...

sun’s picture

Project: Drupal core » Edge
Version: 8.x-dev » 7.x-1.x-dev
Component: theme system » Code
Status: Reviewed & tested by the community » Needs review

This issue will be moved back into the D8 queue, after it has been resolved and committed to Edge 7.x.

hass’s picture

Sounds like the wrong way... some may have forgotten

"Do not hack core!" This phrase is commonly heard in the Drupal community. There are even t-shirts made with this phrase and this cannot be said enough. No matter how easy it is to modify core files and make Drupal do what you want it to do... do not modify Drupal core files.

read http://drupal.org/node/144376 again, please.

juan_g’s picture

hass wrote:
> Do not hack core!

Not sure about the details of that very new project, Edge, but sun has said "is not a fork, just a module". So it seems an experimental module with cutting edge functionality before it goes to core, for impatient folks, or pioneers, or something. :)

catch’s picture

http://drupal.org/project/entitycache and http://drupal.org/project/performance_hacks are also projects that include some Drupal 7 patches that got pushed to D8 (and performance_hacks has stuff I would never dare to submit as a patch to D7 at this point, or likely ever). This is a good pattern to have. There's also http://drupal.org/project/simpletest 7.x-2.x

Hopefully this one will end up largely being a collection of backports from D8. It'd be less disruptive to the issue queue if separate issues were created for the patches being moved over (and then discussion like this wouldn't be happening in the middle too).

juan_g’s picture

catch wrote:
> Hopefully this one will end up largely being a collection of backports from D8.

Interesting...

sun’s picture

This one turned out to be not so easy. What's most annoying is that we're not even able to implement proper tests, as we cannot provide a testing theme that lives within the module.

Therefore, I resorted to testing with core's test_theme, and drupal_altering in our hypothetical .info file property. That's of course no valid real world scenario, and we are also not able to test the base theme behavior, but well, it works for now.

Of course, the patch has been written in preparation for the other .info file properties we are likely going to support (i.e., stylesheets-remove, stylesheets-conditional).

Jacine’s picture

Status: Needs review » Reviewed & tested by the community

This patch fixes yet another major (IMO critical actually core bug): #967166: Content rendered via AJAX does not respect stylesheets removed in .info files.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, edge-HEAD.stylesheets-override.81.patch, failed testing.

Jacine’s picture

Err, I thought stylesheets-remove was still in this patch. The issues linked above doesn't fix the problem (yet).

Here is a reroll.

Jacine’s picture

Status: Needs work » Reviewed & tested by the community

Override functionality is good, so let's get this in and deal with adding the stylesheets-remove functionality separately.

sun’s picture

Project: Edge » Drupal core
Version: 7.x-1.x-dev » 8.x-dev
Component: Code » theme system
Assigned: sun » Unassigned
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs committer feedback

Thanks for reporting, reviewing, and testing! Committed to Edge HEAD.

quicksketch’s picture

It would be nice to get this moving again. Enthusiasm looks like it's curbed since this was bumped into D8. I think this issue is in the right category/priority, but major tasks are now holding up new features per http://drupal.org/node/1201874.

sun’s picture

Status: Needs work » Needs review
sun’s picture

Re-rolled against HEAD.

Status: Needs review » Needs work

The last submitted patch, drupal8.stylesheets-override.89.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
9.91 KB

This makes #1388546: Adding CSS or JS files twice changes weight work for me, which makes sense.

Status: Needs review » Needs work

The last submitted patch, drupal-575298-91.patch, failed testing.

tim.plunkett’s picture

This isn't good.
http://api.drupal.org/api/drupal/core--modules--simpletest--tests--ajax....

The CSS file names stored in ['ajaxPageState']['css'] are just the file names, no paths, whereas the expected CSS has the path.

But shouldn't it know who is providing the file?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
11.67 KB

Ah, it uses array_diff_key and drupal_add_css to match everything up in ajax_render(). Nice.

Fixing the failure in the AJAX test.

sun’s picture

Category: task » bug

Thanks for re-rolling, @tim.plunkett!

We still need to extract the dedicated tests from the Edge patch in #84.

tim.plunkett’s picture

tim.plunkett’s picture

FileSize
11.65 KB

Rerolled after #278425: Using basename() is not locale safe.
Still needs the tests from #84, but I'm a little lost on that.

Status: Needs review » Needs work

The last submitted patch, drupal-575298-97.patch, failed testing.

sun’s picture

Just to give you the slightest idea of what amount advanced PHP code is required to resemble this functionality within a theme:

function mytheme_preprocess_page(&$vars) {
  // Override stylesheets.
  mytheme_style_overrides($vars, array(
    'system.css',
    'system-menus.css',
    'node.css',
  ));
}

/**
 * Overrides module stylesheets with theme versions, but keeping their position.
 */
function mytheme_style_overrides(&$vars, array $files) {
  $files = array_flip($files);
  $styles = drupal_add_css();
  $module_styles = array();
  foreach ($styles['all']['module'] as $file => $value) {
    $basename = basename($file);
    if (!isset($files[$basename])) {
      $module_styles[$file] = $value;
    }
    else {
      $module_styles[$vars['directory'] . '/' . $basename] = $value;
    }
  }
  $styles['all']['module'] = $module_styles;
  $vars['styles'] = drupal_get_css($styles);
}

You might have noticed, this code is for D6. It might still work for D7, I did not test. In case it does not, then it only gets more complex than that.

If we require that amount of PHP skills for themers, then I do think we're designing the theme system for the wrong audience. IIRC, it took me one hour to get this code right, taking the edge-cases of special CSS file $types into account.

I do not understand the resistance against a simple stylesheets-override[] = node.css property in .info files.

The only alternative to that would be an optional "hook" (callback) for themes, say, THEME_css_override() which is supposed to return a list of CSS files to override. But someone needs to explain why that difference in language/syntax makes any difference in the first place. Additionally, please take into account that we want to amend stylesheets-override with a similar stylesheets-remove .info property. Can't get any more trivial in .info files. But in PHP code...? Separate callbacks, separate APIs, separate docs to look up, etc.pp.

tim.plunkett’s picture

Title: Overridden module stylesheets are loaded unconditionally » Provide non-PHP way to reliably override CSS
Category: bug » task

Retitling. The patch in #97 needs tests, which are in the patch in #84.

KrisBulman’s picture

Attempted to re-roll the failed patch in #97, but simpletest seems to have changed considerably and I'm at a loss. ajax.test and common.test are no longer present in core (their own modules I see).

I really wish the solution that came out of this were as simple as an addition of stylesheets-override[] = node.css in the .info file.

chx’s picture

Kris, I am glad you are trying help! Let me assist you: Drupal 8 adheres to PSR-0 which means every class on its own file. In this case, core/modules/system/lib/Drupal/system/Tests/Ajax/FrameworkTest.php and core/modules/system/lib/Drupal/system/Tests/Common/CascadingStylesheetsTest.php are the files you are looking for. If you are running patch and not git apply you might even be able to supply those to patch when it errors out saying file not found, what did you want to patch? In the future, you can just search for strings contained in the files to find them quick and easy, I am using ack here but grep would do or an editor searching:

➜  d8 git:(8.x) ✗ ack 'age state now has the %css file' '
core/modules/system/lib/Drupal/system/Tests/Ajax/FrameworkTest.php
138:    $this->assertEqual($new_css, $original_css + array($expected['css'] => 1), t('Page state now has the %css file.', array('%css' => $expected['css'])));
➜  d8 git:(8.x) ✗ ack 'Test that stylesheets in module .info files are loaded.'
core/modules/system/lib/Drupal/system/Tests/Common/CascadingStylesheetsTest.php
46:   * Test that stylesheets in module .info files are loaded.
KrisBulman’s picture

Status: Needs work » Needs review
FileSize
12.18 KB

This is just a re-roll of #97 since things have been re-organized in core. Tests from #84 still need to be added.. maybe someone could provide more clarification on what needs to be done to pull these dedicated tests into this patch.

Status: Needs review » Needs work

The last submitted patch, drupal-575298-103.patch, failed testing.

KrisBulman’s picture

Status: Needs work » Needs review
FileSize
12.12 KB

Attempting to fix AJAX fail in test

Status: Needs review » Needs work
Issue tags: -API change, -frontend performance

The last submitted patch, drupal-575298-105.patch, failed testing.

KrisBulman’s picture

Status: Needs work » Needs review
Issue tags: +API change, +frontend performance

#105: drupal-575298-105.patch queued for re-testing.

David_Rothstein’s picture

Priority: Major » Normal

This is an important issue, but I don't believe it's a major task (see http://drupal.org/node/45111 and http://drupal.org/node/1181250). Specifically, the main reason this seems to have been marked "major" is that it was considered a blocker for #901062: Regression: Themes can no longer remove stylesheets by overriding them in .info files, but evidently it wasn't in the end since that issue was fixed a long time ago :)

If you disagree and feel this must still be marked major, please provide a very specific justification and keep in mind that every major issue prevents other features that people are working hard on from getting into Drupal core, due to the issue queue threshold policy (until/unless #1810428: [Policy, no patch] Adjust major and critical task thresholds during code thaw is addressed, at any rate). Note that I am not singling out this issue for particular attention but rather (slowly) trying to go through the issue queue and do this in many places so that other features still have a chance to get into Drupal core soon without the thresholds getting in the way. I am trying my best to be impartial, and of course, just because an issue isn't marked "major" certainly doesn't mean it's unimportant to work on. You can follow my progress via the tag I've just added to this issue :)

c4rl’s picture

Just getting up to speed with this issue. We came up with a similar idea at the BADCamp D8 theme sprint, not realizing that this present issue existed. FWIW, see #1833896: Provide syntax for better CSS & JS asset management in custom theme .info files, which I will consider marking as duplicate once I catch-up with some of the comments.

sun’s picture

Assigned: Unassigned » sun
Priority: Normal » Major
Issue tags: +mobile, +API clean-up
FileSize
9.9 KB

1) As mentioned in #1810428: [Policy, no patch] Adjust major and critical task thresholds during code thaw, it is not possible to provide reasoning for why a major task is major, because what constitutes a major/critical task is not defined in any sensible way currently. The only existing definition for major tasks we currently have is that there's sufficient consensus within the community that a certain issue presents a major architectural/functional problem that must be fixed. This is the case here — in its entire history this issue has only been questioned by developers who are not able to relate to the actual needs of themers, but themers of all skill levels confirmed that the current situation is unacceptable. I underlined this with some concrete PHP code snippets in #99, which require a PHP skill level that no one can reasonably expect from themers. (It took me one hour to get the required code logic right.) The functionality we're currently handing to themers is critically broken, and if there was any kind of definition for critical tasks, then I'm confident this issue would meet the requirements. Fact is, the only reason this issue is task and not a bug is that Drupal core doesn't treat systematic problems of themers in the same way as systematic problems of developers. Furthermore, the lack of a proper way to override or enhance module stylesheets presents a substantial front-end performance issue. Therefore, I'm bumping this back to major. I have huge respect for your deep and common sense, but let's also make sure to respect the needs of themers here. :)

2) I've re-rolled the latest patch against HEAD, removed unnecessary changes, and moved it as a branch into the Platform sandbox, but did not review the actual code in-depth yet.

Status: Needs review » Needs work
Issue tags: -mobile, -API change, -API clean-up, -frontend performance

The last submitted patch, theme.css_.110.patch, failed testing.

KrisBulman’s picture

Status: Needs work » Needs review

#110: theme.css_.110.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +mobile, +API change, +API clean-up, +frontend performance

The last submitted patch, theme.css_.110.patch, failed testing.

hass’s picture

Does this patch really keep the original css files order intact? Code wise it looks not.

David_Rothstein’s picture

OK, I agree with this being major. I hadn't read the issue too carefully the first time through - mostly just looked at the comments that had bumped it higher in priority and concluded that all of those comments mentioned specific reasons for bumping it that were no longer valid.

But I agree if something is causing big pain for a large segement of the community, that's a legitimate (and independent) reason for it to be classified as major also.

sun’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
11.22 KB

Fixed Views Cache Plugin Test.

re: #114: Yes, all overridden stylesheets are replacing the original stylesheets at their exact original position.

sun’s picture

Feedback, anyone? :)

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I like the approach taken here, and I'm going to mark it RTBC to see if there are more critiques to be made.

The only problem I can think of is the lack of documentation, but I'd imagine that it would live in the handbook anyway, not in code, since the online in code docs for the old approach were buried in theme_test.info.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue tags: +Performance
FileSize
11.96 KB
20.55 KB

I've rewritten the issue summary.

Additionally, I've removed the excessive file_exists() calls for each and every CSS file that is added. The only reason for all of those filestats is the other, wonky, magic built-in behavior of stylesheets[], which allows to remove a CSS file by adding it, but without providing the actual file. This insane logic is replaced by stylesheets-remove[]. The issue summary contains further details. Also tagging with Performance due to that.

Furthermore, this patch comes with full test coverage now, which asserts all expectations, and goes even one step further and verifies proper base theme + sub theme handling of all CSS file combinations.

Changes:

  1. Added tests based on #967266-1: stylesheets-remove theme .info file property
  2. Removed obsolete file_exists() from drupal_pre_render_styles().
  3. Changed test_subtheme to use a ./css subdirectory and still expect to be able to override by basename.
  4. Fixed stylesheets-override does not work if theme wants to put files into a subdirectory.
  5. Added required processing for stylesheets-remove.
tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review

Haven't reviewed the changes, so, un-RTBCing until I or someone else has looked at this.

Status: Needs review » Needs work

The last submitted patch, theme.css_.119.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
521 bytes
20.55 KB

Fixed test_theme does not intend to override but remove system.base.css.

This should come back green again.

Status: Needs review » Needs work

The last submitted patch, theme.css_.122.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
20.55 KB

Odd. I'm not able to reproduce that test failure. So, anyway, here's a safety merge/re-roll against HEAD.

sun’s picture

Issue summary: View changes

Updated issue summary.

DamienMcKenna’s picture

+1 from a developer who has ran into this problem numerous times.

ry5n’s picture

subscribing

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Code looks good and is really something we want, this definitely needs a change notice since it changes the behaviour from D7.

attiks’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

webchick’s picture

Assigned: sun » Dries
Category: task » feature

Moving to a feature request, which is more apt. We're up over thresholds atm so I can't commit this, and since Dries was the one pushing back on this the hardest in #49, it makes sense for him to sign off on it. Assigning.

FWIW though, I agree that hook_css_alter() isn't really a great answer for someone who doesn't know PHP, let alone Drupal APIs, so I'm conceptually on board with the general gist of this patch. "stylesheets-remove" I find weird, though. I'll thinker about it some more, but don't hold it up on me.

sun’s picture

It has been addressed in earlier comments already, but I want to clarify once more that this is not introducing an .info file scripting language. The resulting properties are still declarative definitions. They are still simple metadata to describe what CSS a theme wants to load. The definitions are compatible with every notion of project/package metadata system and format that exists (e.g., even Composer).

As themers, we want clearly defined and predictable properties in theme .info files. Something as simple as adding/overriding/removing CSS - which is the first and foremost job of a themer - should be straightforward and trivial to do.

Asking a pure front-end person to have to learn and use PHP, learn Drupal's APIs, introspect and learn internal data structures, determine the correct path names, and most importantly, to implement the proper, non-trivial PHP code, in order to perform a ridiculously simple task as overriding or removing CSS, is just simply too much. — Performing this task is even too complex and cumbersome for me, even though I know PHP inside out.

What we want and need is a proper, simple, and intuitive tool to perform our most frequently performed task. We already know theme .info files and are using them on a daily basis. The revised properties here are solving a full range of problems and are exactly what we want.

Dries’s picture

Assigned: Dries » Unassigned

Looking at this again, I'm now comfortable with this. http://drupal.org/node/575298#comment-6215068 was convincing. It doesn't look like the patch applies though so let me ask for a re-test.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Performance, +mobile, +API change, +API clean-up, +frontend performance

The last submitted patch, theme.css_.124.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
20.51 KB
2.04 KB

Rerolled, wait for the FAIL patch to fail and the PASS patch to pass, please :)

webchick’s picture

Awesome. :) Now we just need to be under thresholds (major bugs are completely out of hand atm), and then I'm happy to commit this. YAY!

sun’s picture

#133: drupal-575298-133-PASS.patch queued for re-testing.

sun’s picture

Assigned: Unassigned » sun
sun’s picture

Category: feature » task

FYI: There's a sister issue to this, which essentially removes/reverts the entire .info asset handling for modules:
#1861676: Remove stylesheets[] and scripts[] .info file property support for modules

These two issues go hand in hand and complement each other: That one forces module developers to use PHP to load their assets, because developers know PHP and should carefully consider where to load their assets to begin with. And this one here allows themers to finally make sense of the stylesheet declarations in their .info files.

Lastly, also reverting this back to task, since #128 did not provide any reason that would suddenly invalidate #0, #50, #110, and all prior argumentation that has been provided in this issue. Furthermore, the actual changes in this patch are refactoring existing (and partially broken) logic. As clarified before already, this issue is borderline a bug (and it's beyond me why we continue to brush away problems of themers).

sun’s picture

#133: drupal-575298-133-PASS.patch queued for re-testing.

sun’s picture

Hm. Does anyone know what blocks this patch from being committed?

ry5n’s picture

@sun Seems the only thing may be thresholds. If it helps at all, you’ve got a +1 from me :)

catch’s picture

Also that it no longer applies, sorry. Quick re-roll?

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Performance, +mobile, +API change, +API clean-up, +frontend performance

The last submitted patch, drupal-575298-133-PASS.patch, failed testing.

danillonunes’s picture

Status: Needs work » Needs review
FileSize
20.44 KB
2.04 KB

Quick reroll from #133.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! I manually diffed #144 against #124 and it's still the same.

catch’s picture

Title: Provide non-PHP way to reliably override CSS » Change notice: Provide non-PHP way to reliably override CSS
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

Thanks! Committed/pushed to 8.x.

Will need a change notice.

juan_g’s picture

Wow, a happy day for Drupal theme designers (and site builders).

sun’s picture

Title: Change notice: Provide non-PHP way to reliably override CSS » Provide non-PHP way to reliably override CSS
Priority: Critical » Major
Status: Active » Fixed

Status: Fixed » Closed (fixed)
Issue tags: -Performance, -mobile, -API change, -API clean-up, -frontend performance

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

LewisNyman’s picture

Issue tags: +frontend

In #2389735: Core and base theme CSS files in libraries override theme CSS files with the same name we are talking about some of the flaws with how stylesheet-override works and are suggesting removing it. Just a heads up for people to chime in.