Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Theme .info files can add styles and scripts to the page with styles[] and scripts[] respectively. Modules should be able to do the same thing and that's what this issue is to do.
Comment | File | Size | Author |
---|---|---|---|
#79 | drupal.module-assets.79.patch | 20.19 KB | sun |
#76 | drupal.module-assets.76.patch | 19.54 KB | sun |
#64 | drupal.module-assets.64.patch | 19.37 KB | tstoeckler |
#60 | drupal.module-assets.60.patch | 20.51 KB | fgm |
#58 | drupal.module-assets.58.patch | 19.71 KB | sun |
Comments
Comment #1
RobLoachThis would be nice: #338002: Allow modules to cache module information
Then we could just use:
..... Because at the moment, the module information isn't cached anywhere.
Comment #2
webchickI've always been taught that you should only include CSS/JS files on the pages you actually need them, so for modules, the inclusion of scripts/styles should always be done in the theme function of whatever's being displayed.
Themes are a bit different since they ALWAYS need style.css and layout.css in order to display properly.
Is that true? If so, is this a good idea? For example, jQuery UI module loading all of its scripts on every single page, regardless of whether it was actually used, would cause a tremendous amount of code to be downloaded each page request...
Comment #3
ultimateboy CreditAttribution: ultimateboy commentedI agree with webchick. There are some modules that do need to add css and js to every page, but these are few and far between. I think that giving module developers this ability would cause misuse very rapidly.
Comment #4
mfer CreditAttribution: mfer commentedThere are two schools of thought behind loading the js:
1) Load a minimal size script and only a few scripts per page. On each page load you download a new package of scripts but it's small. The plus side to this method is small file size. This is better for slower connections (e.g., dialup). The negative is scripts needs to be downloaded on every page.
2) Download all the javascript in one shot and let the browser cache it. Then, on each followup pageload the js doesn't need to be downloaded. The plus side of this is it's a one shot download. On the negative, slower connections will see the hit on that first page load.
Modules like sf cache do something in between. Instead of one cached file there are a few and they are more common bundles of files. So, you download them once and you're done. It's a cross between the ideas and can depend on who is browsing your site. Personally, I'm a fan of #1 for core and then using a plugable framework so others can replace the caching method with their own.
But, the reason I added this issue was a little less about these 2 methods. Sometimes you want to add a js file to every page. For example, I'm giving typeface.js a spin. If I add this in as a configurable module it would be nice to add it to every page. This is just another method.
If this isn't a route we want to take I'm ok marking this 'won't fix'.
Comment #5
RobLoachYou are absolutely correct about styles and scripts only being loaded when they are used. But, if you look at some of the implementations of hook_init(), you see that we're calling drupal_add_css() a lot. So, it seems that it is reasonable to have a module's .info file load styles so that the call to hook_init() is not needed. JavaScript is a different story and I think it could be managed in a separate issue.
Adding styles[] to module.info would allow us to remove aggregator_init, book_init, forum_init, node_init, poll_init and system_init.
Comment #6
webchickWell, *or* is the answer to re-work aggregator, book, forum, node, poll, system, etc. so that they are doing drupal_add_css() at the proper time? I have a feeling them having code in hook_init() is not out of necessity, it's part of the new menu system conversion that made these no longer have an if (!$may_cache) to throw logic.
I'm not in favour of adding one property and not both. And adding both causes the scripts problem of dooooom. I'm willing to consider it though if other people think this is really a good idea...
Comment #7
webchickDigging into this more in aggregator module... This module is using *.tpl.php files rather than traditional theme() functions to handle output, which is probably why this ends up in hook_init(). Everything appears to be run through theme('aggregator_wrapper'). There's a function here where we can introduce logic specific to that call:
Not sure... can we drupal_add_css() there, or not because it's cached? Or do we need a hook_css_alter, analogous to our hook_js_alter?
Comment #8
RobLoachOne of the reasons CSS is added in hook_init() is because calls to drupal_add_css() don't work for blocks when block caching is enabled. #214856: CSS and JS for Cached Blocks (drupal_add_css incompatible with blocks) or #257032: Split block $ops into separate functions arn't going in anytime soon though...
Comment #9
mfer CreditAttribution: mfer commented@webchick I expect when we refactor the css system the same way we are on the js system we will introduce a hook_css_alter.
Comment #10
sunComment #11
sun.core CreditAttribution: sun.core commentedComment #12
RobLoachI'm actually inclined to set this to "by design". With the addition of ['#attached']['css'], we can get very contextual with what CSS we can add to the page, and when its added. With styles[], the CSS would just end up being added all the time, even if the page isn't actually using it.
Comment #13
sunAgreed.
Comment #14
sunRe-opening due to latest discussion in #769226: Optimize JS/CSS aggregation for front-end performance and DX
Comment #15
RobLoachHaving #338002: Allow modules to cache module information would allow us to not have to read the .info file every page load for the styles[] data.
Comment #16
skilip CreditAttribution: skilip commentedComment #17
alanburke CreditAttribution: alanburke commentedPatch applies with offset.
Error displayed on screen
cache_get returns False if no cache data is set already.
Adjusted patch to allow for that.
Comment #18
sunre #15: Please note that #887870: Make system_list() return full module records is trying to improve what is being cached by system_list(), so this code could simply re-use it.
Comment #20
jenlamptonCan we use a file_exists check before the system includes the files? I added the new lines to my info file and both the the stylesheet and js file were included in my header even though I had not provided an actual files.
Comment #21
alanburke CreditAttribution: alanburke commentedDoesn't work.
Tested with node module.
Removed drupal_add_css to add the node.css file.
Added
stylesheets[all] = node.css
to node.info
Cleared cache with Drush.
Spotted this error.
The node.css file was not included on page load.
Comment #22
alanburke CreditAttribution: alanburke commentedComment #23
bleen CreditAttribution: bleen commentedThis patch is from #769226-143: Optimize JS/CSS aggregation for front-end performance and DX ...
And it addresses this same issue ... I think the duplicate work is moving to this issue although thats not 100% clear
Comment #25
jenlampton@alanburke you used the wrong syntax in your .info file, it should be
stylesheets[all][] = node.css
@bleen18 is this a duplicate issue? Where should we consolidate work?
Comment #26
sunLet's keep this patch moving here. I've purposively re-opened this issue to not turn the other issue into a giant monster beast of changes.
re #20: No file_exists(), please. We can demand from module developers to properly define their CSS and JS files. Drupal does not babysit broken code. It's much better to see that you're getting bogus 404s through JS/CSS, instead of hiding the bug infinitely, slowing down the system due to broken code that no one ever recognizes, because Drupal auto-corrects it on every request.
Comment #27
bleen CreditAttribution: bleen commentedI'm not sure which is the duplicate of which ...
In the mean time, chasing HEAD
Comment #28
moshe weitzman CreditAttribution: moshe weitzman commentedmodule_init_all() looks like a confusing function name to me. Can we say drupal_add_styles_scripts()? Not perfect, but a bit better IMO Also, I don't see where we add scripts (i.e. no drupal_add_js() call).
i agree with no file_exists call. also, it would be good to be able to add external stylesheets/scripts. can themes do this already?
Comment #29
jenlamptonHm, the last patch got the module path correctly, this time It's adding my module's stylesheet as though it's at the site root. Also agree with @moshe weitzman that no js file is added at all this time.
Comment #30
moshe weitzman CreditAttribution: moshe weitzman commentedI tested and my fake blog.css was included but not a fake blog.js. You need to add scripts[] support it seems.
Comment #31
moshe weitzman CreditAttribution: moshe weitzman commentedHmm. my blog.css did get added with proper path.
Comment #32
jenlamptonSorry, cleared cache and confirmed correct stylesheet path.
Comment #33
moshe weitzman CreditAttribution: moshe weitzman commentedSomeone needs to review the core css/js files and move as many as possible to the .info file. The only ones that we should not move there are large files (usually ones that depend on libraries) that fewer users need (or few pages).
Comment #34
webchickCan we please do #33 in a follow-up? I'd like to keep these issues small, manageable, and (most importantly) reviewable. :)
Comment #35
webchickMmm. Also, thanks to our suck-covered version control system, moving those files is going to require someone with shell access on cvs.d.o. Sigh.
Comment #36
Owen Barton CreditAttribution: Owen Barton commentedNote that #33 was basically already done in #769226: Optimize JS/CSS aggregation for front-end performance and DX (that was the main part of that patch, in fact) - there may be a few tweaks (which should be a separate issue), but I think it is pretty close. We just need to move all the hook_init calls to drupal_add_* to the .info files, which should be done in this patch.
Comment #37
sunI'm not sure what files would have to be moved - didn't understand those follow-ups.
I don't think that's the appropriate place to (potentially rebuild and) load module CSS/JS files. Technically, I'd say it belongs into template_preprocess_page(), no?
1) The function parameters need to be documented.
2) Shouldn't we pass $info['stylesheets'] resp. $info['scripts'] directly? I.e., let the caller figure out whether to call the helper function.
3) It looks like the caller could already pass dirname($uri) instead of $uri.
4) Technically, both functions are doing the same -- prefixing the deepest nested array values with the extension's path. I currently don't see anything else these helper functions could do, so I wonder whether we shouldn't have a helper that just does that, i.e., system_info_prefix_values_with_path($info['somekey'], $prefix)
Powered by Dreditor.
Comment #38
jenlamptonrerolled patch to include scripts[] support and rename the function drupal_add_styles_scripts. Should we move hook_init back out of that function?
*edit* just saw @sun's comment... will work more on this later.
Comment #39
bleen CreditAttribution: bleen commentedI couldnt get the patch in #38 to apply. It was created one level up and it has all sorts of extra stuff in it that kept borking when I tried to apply (all the changes to .info files and I think some tests)...
Anyway, this patch fixes the missing support for styles and it takes into account most of the comments buy sun in #37. It
drupal_add_styles_scripts
to template_preprocess_page (good call)_system_rebuild_info_stylesheets
andsystem_rebuild_info_scripts
Comment #41
sunTechnically, something like the attached should work, but it somehow just killed my server... not sure why.
Comment #43
sunLess references work, too.
Comment #44
bleen CreditAttribution: bleen commented#41 causes MAMP to crash for me
Comment #45
bleen CreditAttribution: bleen commented#43 works much better
why not
$path = dirname($module->uri) . '/';
Powered by Dreditor.
Comment #47
bleen CreditAttribution: bleen commentedI dont know how/why, but Bartik's .info file is wrong:
should be :
And that is causing the exceptions...
This patch fixes bartik.info (and $path = dirname($module->uri) . '/'; I mentioned in #45) although I'm sure that should be fixed elsewhere. Let's keep this going
Comment #48
bleen CreditAttribution: bleen commentedooops .. last patch left in a couple debug() calls
Comment #50
bleen CreditAttribution: bleen commentedOK ... I did a completely fresh checkout form HEAD and bartik.info is still wrong because now there are no js files at all in bartik/scripts
This patch adjusts for that
Comment #51
sunAbove exception mess is why we do not use file_exists() anywhere here. Developers (and themers) would never notice that their .info files are bogus otherwise. Nice to directly have a real world example ;)
I'm not yet sure about these, as they were originally added with a certain (lower) weight. Perhaps we should revert that. What do others think?
Normally, all directory paths in Drupal do not contain a trailing slash.
The intention was to keep this code as identical as possible for both modules and themes. For themes, dirname($theme->uri) is also needed a few lines later, so the goal was to prepare $path without slash, pass $path . '/' to the helper, and use $path . '/' . $screenshot further down below.
An alternative would be to rename the helper to system_info_add_path($info, $path), and make that inject a '/' between $path and $value.
Powered by Dreditor.
Comment #52
manarth CreditAttribution: manarth commentedThe attached patch adds tests to check that stylesheets can be added in module .info files.
Comment #53
thehong CreditAttribution: thehong commentedIs it possible to support path checking?
Comment #54
webchickNO :)
We need to get D7 out the door. We'll carry over the behaviour of theme styles/scripts lines directly.
But feel free to file a feature request for that if there isn't one already, since it sounds like a useful enhancement for D8.
Comment #56
manarth CreditAttribution: manarth commentedThe test-bot failure is expected: the patch adds 3 tests which check that stylesheets can be added via module.info.
Until the patch which enables this feature is added, these tests will report test-failure, which is expected behaviour.
Comment #57
sunIncorporated everything from #51 as well as the tests (a bit revised though) from #52.
Comment #58
sunFixed that fatal error.
Comment #59
fgmI think the API info about hook_init needs to be updated too when this patch gets in, otherwise it will still have the sentence about "For example, this hook is a typical place for modules to add CSS or JS that should be present on every page.", which should no longer be the case.
Comment #60
fgmWording proposal.
Comment #61
manarth CreditAttribution: manarth commented@sun should the tests also check that #20 and #26 work as expected - i.e. if a css file is added via module.info, but that file doesn't exist, it should still be included?
Comment #62
sunNo, we cannot test that, because it unconditionally leads to PHP warnings/exceptions, as visible in #48, and that's good + intentional.
This looks ready to fly for me. However, it decreases page request performance for every full bootstrap request until #887870: Make system_list() return full module records gets in.
Comment #63
sunNo, wait -- I thought I reverted those system.info/system_init() changes...?
Comment #64
tstoecklerHmmm... Is that something that should go into hook_init documentation or simply in the upgrade guide? I'm actually uncertain.
Anyway, rerolled without the system.module hunk, which was controversial. I hope I got everything.
Powered by Dreditor.
Comment #65
tstoeckler(needs review)
Comment #66
sunThank you!
Comment #67
skilip CreditAttribution: skilip commentedawesome!
Comment #68
fgmRe: documentation, I think it needs to go here because if the wording does not change, contrib devs will see the doc being identical word for word with earlier versions while the suggested use case is really no longer suggested, although it will still work, but be suboptimal.
Comment #69
tstoeckler@fgm: My question was not whether to leave the documentation as is, but whether we should mention the info file method at all. Again, it could actually be good to do that, I'm simply uncertain.
Comment #70
catchThis can't go in without benchmarks for the same reason that #887870: Make system_list() return full module records can't. I'm disappointed that the original css/js performance issue is now being executed in such a way that it could affect PHP and mysql performance, this doesn't seem necessary at all. I understand the desire to unify the behaviour of modules and themes but there should not be a negative tradeoff for back end performance, which has taken a serious hit in Drupal 7 already (especially the out of the box experience).
So someone please post benchmarks and cachegrind data, also please don't commit this until I've had a chance to verify those.
Comment #71
sunThis obviously means we have to postpone this issue on #887870: Make system_list() return full module records
Comment #72
sun#887870: Make system_list() return full module records is in.
Comment #73
sun#64: drupal.module-assets.64.patch queued for re-testing.
Comment #74
sunThis patch was already written with #887870: Make system_list() return full module records in mind, was reviewed multiple times and RTBC already, so let's get it in, so we can move forward with the overall JS/CSS aggregation issue.
Comment #75
effulgentsia CreditAttribution: effulgentsia commentedWhy is this being done in template_preprocess_page()? Shouldn't it be done in _drupal_bootstrap_full() just before calling module_invoke_all('init')? While in #769226: Optimize JS/CSS aggregation for front-end performance and DX we may end up improving grouping behavior such that it doesn't matter, at least for now, the timing of when drupal_add_*() are called in a page request does matter, so doing it near hook_init() is what is most compatible with HEAD, and consistent with the order in which THEME.info / THEME_init() run.
Powered by Dreditor.
Comment #76
sunNot sure whether that is the right location either. I've moved the actual code into a new system_add_module_assets() helper function. Went with the system namespace, as it relies on system_get_info(). However, I also played with the idea of putting this into module.inc, within the module namespace, but it doesn't really belong to the module system, so that's probably not really the right place.
Moved the invocation into system_init().
Comment #77
effulgentsia CreditAttribution: effulgentsia commentedThanks. That addresses my concerns, so back to RTBC as per #74
Comment #78
Dries CreditAttribution: Dries commentedThis looks good. Only one thing came to mind:
Looks more like a helper function than an API, not? Should it be prefixed with an underscore?
It would also be good if the PHPdoc described why/when this function should be used.
Comment #79
sunIncorporated the requested changes.
Comment #81
effulgentsia CreditAttribution: effulgentsia commented#79: drupal.module-assets.79.patch queued for re-testing.
Comment #82
Dries CreditAttribution: Dries commentedReviewed once more, and committed to CVS HEAD.
Comment #83
catchComment #84
catchJust to clarify, status change was because this still needs documentation.
Comment #85
rfaySaw catch saying in IRC that this needed an announcement.
Could we have an issue summary please, and an API change summary. Does it break BC? I saw the Sept 4 commit (#82) and have already happily used this feature.
Comment #86
catchI think this is a new feature, so it may not need an announcement. However it should definitely go in the module upgrade documentation.
Comment #87
rfayLooks to me like this hasn't even been updated in the D7 Module .info file page. What's up with that?
Comment #88
jhodgdonLooks like this one needs a Handbook edit as well as an addition to the 6/7 module update guide
Comment #89
rfayI'll give this a shot for documentation.
Comment #90
rfayPlease review this update in the 6/7 module upgrade guide.
I also added about loading in .info files in the D7 Managing JS page.
Comment #91
jhodgdonThis issue number is not mentioned in that section. Is that OK?
Comment #92
rfay@jhodgdon: Since this update doc is a combination of about 5 issues and the key one is definitely not grokkable, I decided to forego the issue. If you think otherwise we can add it.
Comment #94
sunTo be potentially reverted for D8: #1861676: Remove stylesheets[] and scripts[] .info file property support for modules