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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

wonder95’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

wonder95’s picture

Hmmm, I was able to install this patch without a problem on my local install. Any chance this was a problem with the test bot?

dmitrig01’s picture

Status: Needs work » Needs review

Requesting retest

Dries’s picture

I'd be in support of this patch (assuming testing succeeds).

wonder95’s picture

Requesting another re-test.

Status: Needs review » Needs work

The last submitted patch failed testing.

wonder95’s picture

New patch with path to system.module modified, also used CVS diff instead of just diff.

wonder95’s picture

Status: Needs work » Needs review
FileSize
631 bytes

OK, let's try this one.

Status: Needs review » Needs work

The last submitted patch failed testing.

wonder95’s picture

OK, let's try this again with correct characters in the patch name.

lilou’s picture

Status: Needs work » Needs review
mfer’s picture

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

flickerfly’s picture

I'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?

mfer’s picture

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

wonder95’s picture

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

flickerfly’s picture

Issue tags: +Needs design review

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

deviantintegral’s picture

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

dixon_’s picture

Status: Needs review » Reviewed & tested by the community

+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 and script.js doesn't get included unless I specify them in mytheme.info, as expected!

Nothing to comment about code style.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

I support this patch but we have to update the themes in core. With this patch, the screenshots disappear, for example.

dixon_’s picture

Status: Needs work » Needs review
FileSize
3.52 KB

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.

dixon_’s picture

Ops, forgot new lines at end of all .info files. Fixed in this patch!

JohnAlbin’s picture

Title: Remove defaults values for stylesheet and scripts includes from system module » Remove default values for stylesheet and scripts includes from system module
FileSize
1.93 KB

Actually, 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.

deviantintegral’s picture

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

JohnAlbin’s picture

+1 on the patch at #23

If you've given it a proper review, please RTBC! :-)

btw, D6->D7 theme upgrade docs are here: http://drupal.org/node/254940

deviantintegral’s picture

Status: Needs review » Reviewed & tested by the community

RTBC'ing then!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Make sure to update the documentation. Thanks!

JohnAlbin’s picture

Status: Fixed » Needs work

@todo: Docs need updating at http://drupal.org/node/254940

deviantintegral’s picture

Status: Needs work » Fixed

I've updated the documentation, so any further changes can be made on that page.

JohnAlbin’s picture

Status: Fixed » Needs review
FileSize
670 bytes

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

JohnAlbin’s picture

Status: Needs review » Reviewed & tested by the community

Bot says the patch is good. Marking this RTBC since its already been committed once before.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed (again) to HEAD. :) Thanks!

Status: Fixed » Closed (fixed)

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

Jacine’s picture

Status: Closed (fixed) » Needs work

IMO, 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.

gazwal’s picture

subscribing to jacine's request

mfer’s picture

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

Jacine’s picture

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

ultimateboy’s picture

jacine, 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.

Jacine’s picture

Status: Needs work » Fixed

Oh 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 :(

Status: Fixed » Closed (fixed)
Issue tags: -Needs design review

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