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
-
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.
-
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.
Comment | File | Size | Author |
---|---|---|---|
#144 | drupal-575298-144-FAIL.patch | 2.04 KB | danillonunes |
#144 | drupal-575298-144-PASS.patch | 20.44 KB | danillonunes |
#133 | drupal-575298-133-FAIL.patch | 2.04 KB | tim.plunkett |
#133 | drupal-575298-133-PASS.patch | 20.51 KB | tim.plunkett |
#124 | theme.css_.124.patch | 20.55 KB | sun |
Comments
Comment #1
sunThis 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
with
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.
Comment #2
alanburke CreditAttribution: alanburke commentedI 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?
Comment #3
alanburke CreditAttribution: alanburke commentedOops.
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!
Comment #4
hass CreditAttribution: hass commented+
Comment #5
sunAny better suggestions than the one in #1 ?
If not, then let's do that. I'm facing this problem all over again.
Comment #6
RobLoachWe ran into this with Seven's vertical-tabs.css being added all the time, and the solution was...
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:Since themes can't know the entire path of modules or other themes, this patch also supports:
I guess we could loop through
$css
, checking the basename of each CSS file added, but that might be slow. Thoughts?Comment #7
casey CreditAttribution: casey commentedNo need for paths. All stylesheet names are unique.
http://api.drupal.org/api/function/drupal_add_css/7 (see basename)
Comment #8
RobLoachWhy check the basenames on drupal_get_js() when we could do it during addition instead?
Comment #9
RobLoachThis might be a bit cleaner as it checks the CSS's weight before overriding basename collisions.
Comment #11
JacineVery happy to see this! :)
Comment #12
JohnAlbinsubscribe.
Comment #13
casey CreditAttribution: casey commentedIt 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?
Comment #14
RobLoachIn 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.
Comment #15
sunWe 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
Comment #16
RobLoachWhat happens when instead of "overriding" a CSS file, we just want to add it when the original stylesheet is there?
Comment #17
sun@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.
Comment #18
casey CreditAttribution: casey commentedTrying 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.
Comment #19
RobLoachsun at #17:
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[]
stylesheet_additions[] ?
Comment #20
sunSure, 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.
Comment #21
sun@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.
Comment #22
sunhehe, ok, touch of a clusterfuck, but doable.
Comment #23
sunAnd 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.
Comment #25
sunPartially 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.
Comment #27
sunFixed 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?
Comment #29
sunResolved it. This one should pass.
Comment #30
sunAny feedback?
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.
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.
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.
Comment #31
JacineI 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.
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.
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.
Comment #32
sunShorter, 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.
Comment #33
reglogge CreditAttribution: reglogge commentedAwesome. 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.
Comment #34
JacineBeautiful. 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!
Comment #35
sun#32: drupal.stylesheets-override.32.patch queued for re-testing.
Comment #37
sunRe-rolled against HEAD.
Comment #39
sunSorry, that was the wrong file, hence failing tests.
Comment #40
Jeff Burnz CreditAttribution: Jeff Burnz commentedOh yes this is fantastic, thank-you so much for the hard work Sun, +1.
Comment #41
effulgentsia CreditAttribution: effulgentsia commentedSubscribe. 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.
Comment #42
JacineThis is currently holding up this critical #901062: Regression: Themes can no longer remove stylesheets by overriding them in .info files. : (
Comment #43
juan_g CreditAttribution: juan_g commentedIt 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?).
Comment #44
sun#39: drupal.stylesheets-override.39.patch queued for re-testing.
Comment #46
sunUnknown testbot hiccup in Poll upgrade path.
Comment #47
reglogge CreditAttribution: reglogge commentedThis is currently holding up two critical issues:
#901062: Regression: Themes can no longer remove stylesheets by overriding them in .info files
#925350: Vertical tabs broken
Comment #48
sunAlright, so let's bump this to critical to draw some attention and hopefully unblock those other critical issues ASAP.
Comment #49
Dries CreditAttribution: Dries commentedI 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.)
Comment #50
JacineDX 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:
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:
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.
Comment #51
Jeff Burnz CreditAttribution: Jeff Burnz commentedI 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?
Comment #52
reglogge CreditAttribution: reglogge commented@Jacine + @Jeff: AMEN!
Comment #53
dreamleaf@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.
Comment #54
juan_g CreditAttribution: juan_g commentedJacine 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.
Comment #55
sunNext 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.
Comment #56
sunComment #57
Jeff Burnz CreditAttribution: Jeff Burnz commentedsun - can we please have some explanation as to why this is suddenly closed and won't fix, that seems rather sudden.
Comment #58
Dave ReidNo reasoning to backup the change in status, so restoring status.
Comment #59
sunAlthough badly needed, this sounds like D8 material to me.
Comment #60
chx CreditAttribution: chx commented#50 is reasoning enough to consider for Drupal 7.
Comment #62
Mark Trapp#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?Comment #63
juan_g CreditAttribution: juan_g commentedMark, 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...
Comment #64
Jacine@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.
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.
Comment #65
Mark Trapp@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.
Comment #66
JacineHuh? 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)
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.
Comment #67
Jeff Burnz CreditAttribution: Jeff Burnz commentedWith 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.
Comment #68
catchThis either needs to go in, or get put out of its misery and pushed to D8 again.
Comment #69
juan_g CreditAttribution: juan_g commentedwebchick'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.
Comment #70
reglogge CreditAttribution: reglogge commentedApart 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.
Comment #71
juan_g CreditAttribution: juan_g commentedYes, and this also fixes a performance bug. See comments #20, #41, #50.
Comment #72
catchIf 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.
Comment #73
webchickTwo 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.
Comment #74
JacineOk, 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:
Comment #75
juan_g CreditAttribution: juan_g commentedwebchick 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...
Comment #76
sunThis issue will be moved back into the D8 queue, after it has been resolved and committed to Edge 7.x.
Comment #77
hass CreditAttribution: hass commentedSounds like the wrong way... some may have forgotten
read http://drupal.org/node/144376 again, please.
Comment #78
juan_g CreditAttribution: juan_g commentedhass 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. :)
Comment #79
catchhttp://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).
Comment #80
juan_g CreditAttribution: juan_g commentedcatch wrote:
> Hopefully this one will end up largely being a collection of backports from D8.
Interesting...
Comment #81
sunThis 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).
Comment #82
JacineThis patch fixes yet another major (IMO critical actually core bug): #967166: Content rendered via AJAX does not respect stylesheets removed in .info files.
Comment #84
JacineErr, I thought stylesheets-remove was still in this patch. The issues linked above doesn't fix the problem (yet).
Here is a reroll.
Comment #85
JacineOverride functionality is good, so let's get this in and deal with adding the stylesheets-remove functionality separately.
Comment #86
sunThanks for reporting, reviewing, and testing! Committed to Edge HEAD.
Comment #87
quicksketchIt 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.
Comment #88
sun#39: drupal.stylesheets-override.39.patch queued for re-testing.
Comment #89
sunRe-rolled against HEAD.
Comment #91
tim.plunkettThis makes #1388546: Adding CSS or JS files twice changes weight work for me, which makes sense.
Comment #93
tim.plunkettThis 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?
Comment #94
tim.plunkettAh, it uses array_diff_key and drupal_add_css to match everything up in ajax_render(). Nice.
Fixing the failure in the AJAX test.
Comment #95
sunThanks for re-rolling, @tim.plunkett!
We still need to extract the dedicated tests from the Edge patch in #84.
Comment #96
tim.plunkettShould this be postponed on #278425: Using basename() is not locale safe?
Comment #97
tim.plunkettRerolled after #278425: Using basename() is not locale safe.
Still needs the tests from #84, but I'm a little lost on that.
Comment #99
sunJust to give you the slightest idea of what amount advanced PHP code is required to resemble this functionality within a theme:
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 amendstylesheets-override
with a similarstylesheets-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.Comment #100
tim.plunkettRetitling. The patch in #97 needs tests, which are in the patch in #84.
Comment #101
KrisBulman CreditAttribution: KrisBulman commentedAttempted 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.Comment #102
chx CreditAttribution: chx commentedKris, 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:
Comment #103
KrisBulman CreditAttribution: KrisBulman commentedThis 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.
Comment #105
KrisBulman CreditAttribution: KrisBulman commentedAttempting to fix AJAX fail in test
Comment #107
KrisBulman CreditAttribution: KrisBulman commented#105: drupal-575298-105.patch queued for re-testing.
Comment #108
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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 :)
Comment #109
c4rl CreditAttribution: c4rl commentedJust 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.
Comment #110
sun1) 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.
Comment #112
KrisBulman CreditAttribution: KrisBulman commented#110: theme.css_.110.patch queued for re-testing.
Comment #114
hass CreditAttribution: hass commentedDoes this patch really keep the original css files order intact? Code wise it looks not.
Comment #115
David_Rothstein CreditAttribution: David_Rothstein commentedOK, 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.
Comment #116
sunFixed Views Cache Plugin Test.
re: #114: Yes, all overridden stylesheets are replacing the original stylesheets at their exact original position.
Comment #117
sunFeedback, anyone? :)
Comment #118
tim.plunkettI 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.
Comment #118.0
sunUpdated issue summary.
Comment #119
sunI'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 ofstylesheets[]
, which allows to remove a CSS file by adding it, but without providing the actual file. This insane logic is replaced bystylesheets-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:
Comment #120
tim.plunkettHaven't reviewed the changes, so, un-RTBCing until I or someone else has looked at this.
Comment #122
sunFixed test_theme does not intend to override but remove system.base.css.
This should come back green again.
Comment #124
sunOdd. I'm not able to reproduce that test failure. So, anyway, here's a safety merge/re-roll against HEAD.
Comment #124.0
sunUpdated issue summary.
Comment #125
DamienMcKenna+1 from a developer who has ran into this problem numerous times.
Comment #126
ry5n CreditAttribution: ry5n commentedsubscribing
Comment #127
attiks CreditAttribution: attiks commentedCode looks good and is really something we want, this definitely needs a change notice since it changes the behaviour from D7.
Comment #127.0
attiks CreditAttribution: attiks commentedUpdated issue summary.
Comment #127.1
sunUpdated issue summary.
Comment #128
webchickMoving 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.
Comment #129
sunIt 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.
Comment #130
Dries CreditAttribution: Dries commentedLooking 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.
Comment #131
Dries CreditAttribution: Dries commented#124: theme.css_.124.patch queued for re-testing.
Comment #133
tim.plunkettRerolled, wait for the FAIL patch to fail and the PASS patch to pass, please :)
Comment #134
webchickAwesome. :) 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!
Comment #135
sun#133: drupal-575298-133-PASS.patch queued for re-testing.
Comment #136
sunComment #137
sunFYI: 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).
Comment #138
sun#133: drupal-575298-133-PASS.patch queued for re-testing.
Comment #139
sunHm. Does anyone know what blocks this patch from being committed?
Comment #140
ry5n CreditAttribution: ry5n commented@sun Seems the only thing may be thresholds. If it helps at all, you’ve got a +1 from me :)
Comment #141
catchAlso that it no longer applies, sorry. Quick re-roll?
Comment #142
catch#133: drupal-575298-133-PASS.patch queued for re-testing.
Comment #144
danillonunes CreditAttribution: danillonunes commentedQuick reroll from #133.
Comment #145
sunThanks! I manually diffed #144 against #124 and it's still the same.
Comment #146
catchThanks! Committed/pushed to 8.x.
Will need a change notice.
Comment #147
juan_g CreditAttribution: juan_g commentedWow, a happy day for Drupal theme designers (and site builders).
Comment #148
sunChange notice: http://drupal.org/node/1876600
Comment #149.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #151
LewisNymanIn #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.