Remove the default values for the scripts, stylesheets, and screenshot settings from system_theme_data in system.module. Per Crell:
The declaring of any default for styles and scripts is itself the bug that
needs to be fixed. We didn't realize until after feature freeze that a side
effect of that behavior is that a theme that declares no scripts, for instance,
will therefore implicitly define scripts.js, which removes the scripts.js from
the parent theme. Hilarity ensues.Instead if the default is to declare nothing, everything works as expected at
the cost of themes needing to be explicit about all of their styles and
scripts. I'd say that's a preferred behavior anyway.
The theme would be required to list default CSS, script, and screenshot files explicitly in the theme.info file.
Comments
Comment #1
wonder95 CreditAttribution: wonder95 commentedComment #3
wonder95 CreditAttribution: wonder95 commentedHmmm, I was able to install this patch without a problem on my local install. Any chance this was a problem with the test bot?
Comment #4
dmitrig01 CreditAttribution: dmitrig01 commentedRequesting retest
Comment #5
Dries CreditAttribution: Dries commentedI'd be in support of this patch (assuming testing succeeds).
Comment #6
wonder95 CreditAttribution: wonder95 commentedRequesting another re-test.
Comment #8
wonder95 CreditAttribution: wonder95 commentedNew patch with path to system.module modified, also used CVS diff instead of just diff.
Comment #9
wonder95 CreditAttribution: wonder95 commentedOK, let's try this one.
Comment #11
wonder95 CreditAttribution: wonder95 commentedOK, let's try this again with correct characters in the patch name.
Comment #12
lilou CreditAttribution: lilou commentedComment #13
mfer CreditAttribution: mfer commentedCan someone explain to me by having defaults here is a bug that needs to have the defaults removed?
What I'm seeing, and please correct me if I'm wrong, is that sub themes that do not declare scripts in their .info file have a scripts.js automatically added and overide the scripts.js file included by the parent theme. This seems like it's the bug.
Wouldn't it be more appropriate to leave the defaults and account for them in _system_theme_data() so this issue doesn't pop up? Therefore leaving the defaults for themers and removing this bug.
I bet if a bunch of themers (not the hard core ones) were polled this would be decreasing the themer experience.
Comment #14
flickerfly CreditAttribution: flickerfly commentedI'm thinking fixing this such that defaults remain will save several duplicate bugs in the issue queue. It will certainly be perceived as a loss of feature, not a gain from the themer perspective. It was even mentioned in theme training I attended just this past weekend so it is a known and used feature among themers.
Of course tossing the responsibility of adaptation on the themer isn't a big deal, but why aggravate an entire segment of the drupal community?
Comment #15
mfer CreditAttribution: mfer commented@flickerfly better yet, why aggravate the themer part of the drupal community when it's a part of the community we are really attempting to court and grow?
Comment #16
wonder95 CreditAttribution: wonder95 commentedI'm not understanding why this would be such a huge aggravation. All the theme creator would have to do is add one line (stylesheets[all][] = style.css) to the .info file. The same is true with scripts.js (scripts[] = scripts.js). That doesn't seem to be too difficult, and it's something the theme creator would have to add, not each person that's using the theme.
See this devel list thread for a discussion on why this is better.
Comment #17
flickerfly CreditAttribution: flickerfly commentedIt may be simple in practice, but it is an unexpected change and a retraining of the user. Someone who has expected and used that as a feature for a length of time could easily spend a good bit of time figuring out why that doesn't work in 7.x. The people who will deal with this don't float in the issue queue's very often. They aren't programmers and so those are foreign and hard to understand.
I think the options are a marketing campaign to note the change and help people to know about it before it happens or just code it such that it meets everyone's needs. The latter strikes me as the saner of the two. It is a question of community interface, not technology. When you're dealing with end users, unexpected change is always a bad thing and should only be done when it must be done.
It does occur to me that the premise that this matters to themers has yet to be proven. At the risk of splitting the discussion, I've tossed a poll up at http://groups.drupal.org/design-drupal to see if we can get some feedback from the active themers and designers in that group. Maybe I'm making a fuss for nothing. If so, I'll withdraw my objection.
Comment #18
deviantintegral CreditAttribution: deviantintegral commentedIt doesn't look like that poll has received much attention. IMO having default files included in this manner is actually more confusing since it's not incredibly obvious what's included by default and why. For someone new looking at a .info file with many scripts or stylesheets, it would be confusing as to why some files aren't listed.
Here is an updated patch against HEAD, which also fixes
_system_theme_data
so it doesn't throw notices if 'scripts' or 'stylesheets' aren't defined.Comment #19
dixon_+1 for this patch as it will make the theme system more consistent as previously said.
The patch applies perfectly and works fine. I have tested it with a very simple custom theme.
style.css
andscript.js
doesn't get included unless I specify them inmytheme.info
, as expected!Nothing to comment about code style.
Comment #20
Dries CreditAttribution: Dries commentedI support this patch but we have to update the themes in core. With this patch, the screenshots disappear, for example.
Comment #21
dixon_Okey, here comes an attempt to update the core themes. Don't know if I covered all that had to be changed. Screeshots now appear. That was the only thing I had to change in out three core themes.
Comment #22
dixon_Ops, forgot new lines at end of all .info files. Fixed in this patch!
Comment #23
JohnAlbinActually, per crell's original quote, he doesn't mention screenshot as a problem. And its not. Sub-themes currently inherit screenshots without issue, so we don't need to modify that default setting.
For new themers, we need to explain how to add or override stylesheets by adding lines to the theme’s .info file. Having a default (and hidden) line in the .info file adds clutter to explaining this idea to them.
Also, given Drupal’s usage growth, we will have more new themers than themers who are upgrading from D6 to D7. Plus, our upgrade docs are pretty damn good, IMO. And this patch will necessitate a note in the D7 theme upgrade docs.
This new patch removes the screenshot change.
Comment #24
deviantintegral CreditAttribution: deviantintegral commented+1 on the patch at #23. I can't seem to find anything in the API docs about info files, so I'm not sure what the procedure for upgrading the docs is. Should http://drupal.org/node/231036 be copied into the D7 handbook and changed as needed?
Comment #25
JohnAlbinIf you've given it a proper review, please RTBC! :-)
btw, D6->D7 theme upgrade docs are here: http://drupal.org/node/254940
Comment #26
deviantintegral CreditAttribution: deviantintegral commentedRTBC'ing then!
Comment #27
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Make sure to update the documentation. Thanks!
Comment #28
JohnAlbin@todo: Docs need updating at http://drupal.org/node/254940
Comment #29
deviantintegral CreditAttribution: deviantintegral commentedI've updated the documentation, so any further changes can be made on that page.
Comment #30
JohnAlbinThe patch in #147000: Rewrite module_rebuild_cache() and system_theme_data() was not re-rolled well after this issue was committed. And it reverted part of this patch.
Here's a new patch which re-applies the changes.
Comment #31
JohnAlbinBot says the patch is good. Marking this RTBC since its already been committed once before.
Comment #32
webchickCommitted (again) to HEAD. :) Thanks!
Comment #34
JacineIMO, this is a huge leap backwards and I would appreciate it if some sort of alternative could be considered.
Because of this, I'll have to:
(1) Minor: Load a bunch of admin-only CSS for everyone, even though it's not needed (same applies for drupal_add_js())
(2) Major: Remove the CSS (and functionality) I'm custom generating to provide user configurable layout, fonts, color, etc.
This is a big problem for any contrib theme that attempts to load CSS/JS based on logic in theme settings... A perfect high-profile example of this is TNT's new Fusion theme. They are providing multiple layouts, and loading the appropriate CSS file based on the chosen setting.
You just can't do this sort of thing in the .info file.
Comment #35
gazwal CreditAttribution: gazwal commentedsubscribing to jacine's request
Comment #36
mfer CreditAttribution: mfer commented@Jacine - this update removed the defaults for a style.css and script.js file. You can still have them you just need to specify them in your .info file. All this did was remove the defaults.
Comment #37
Jacine@mfer, I'm saying that some files can't be defined in the .info file, especially ones that are called based on settings a user chooses in theme settings.
No instance of drupal_add_css() is working for me, no matter where I put it.
Comment #38
ultimateboy CreditAttribution: ultimateboy commentedjacine, I dont quite understand what you are saying. If I am understanding things correctly, this patch really doesnt do anything in terms of functionality, it simply removes the defaults. By default, D6 loads style.css and script.js. This patch simply makes the theme explicitly state it it wants those files loaded instead of drupal doing it automatically. drupal_add_css and drupal_add_js can still be implemented the same way. So I believe your tweet is not quite right this patch doesnt really change anything along those lines.
I think the confusion is coming from the last line of the original post. "The theme would be required to list default CSS, script, and screenshot files explicitly in the theme.info file." That, is not quite correct as stylesheets and scripts can still be added via drupal_add_css and drupal_add_js in the theme's template.php.
[edit] I just tried drupal_add_css in preprocess_page and everything works as expected.
Comment #39
JacineOh my gosh... Sigh & Phew...
I guess having the right syntax would help. It is working. I was pretty panicked when I couldn't get this working, and could swear I checked the syntax, but I guess not.
Sorry, everyone :(