Remove default values for stylesheet and scripts includes from system module
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | theme system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | Needs design review |
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.
| Attachment | Size |
|---|---|
| system.module_remove_js_and_stylesheet_defaults.patch | 469 bytes |
| Testbed results | ||
|---|---|---|
| system.module_remove_js_and_stylesheet_defaults.patch | failed | Failed: Failed to install HEAD. Detailed results |
#1
#2
The last submitted patch failed testing.
#3
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?
#4
Requesting retest
#5
I'd be in support of this patch (assuming testing succeeds).
#6
Requesting another re-test.
#7
The last submitted patch failed testing.
#8
New patch with path to system.module modified, also used CVS diff instead of just diff.
#9
OK, let's try this one.
#10
The last submitted patch failed testing.
#11
OK, let's try this again with correct characters in the patch name.
#12
#13
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.
#14
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?
#15
@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?
#16
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.
#17
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.
#18
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_dataso it doesn't throw notices if 'scripts' or 'stylesheets' aren't defined.#19
+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.cssandscript.jsdoesn't get included unless I specify them inmytheme.info, as expected!Nothing to comment about code style.
#20
I support this patch but we have to update the themes in core. With this patch, the screenshots disappear, for example.
#21
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.
#22
Ops, forgot new lines at end of all .info files. Fixed in this patch!
#23
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.
#24
+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?
#25
If you've given it a proper review, please RTBC! :-)
btw, D6->D7 theme upgrade docs are here: http://drupal.org/node/254940
#26
RTBC'ing then!
#27
Committed to CVS HEAD. Make sure to update the documentation. Thanks!
#28
@todo: Docs need updating at http://drupal.org/node/254940
#29
I've updated the documentation, so any further changes can be made on that page.
#30
The patch in #147000: Regression: Unify and 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.
#31
Bot says the patch is good. Marking this RTBC since its already been committed once before.
#32
Committed (again) to HEAD. :) Thanks!
#33
Automatically closed -- issue fixed for 2 weeks with no activity.