Allow module .info files to add CSS/JS

This would be nice: #338002: Allow modules to cache module information

Then we could just use:

$info = module_info($module);
foreach($info->styles as $style) {
  drupal_add_css($style);
}

..... Because at the moment, the module information isn't cached anywhere.

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

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

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

Well, *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...

Digging 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:

function template_preprocess_aggregator_wrapper(&$variables) {
  $variables['pager'] = theme('pager', NULL, 20, 0);
}

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?

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

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

Agreed.

Re-opening due to latest discussion in #769226: Optimize JS/CSS aggregation for front-end performance and DX

Having #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.

Patch applies with offset.
Error displayed on screen

Error message
Notice: Trying to get property of non-object in _module_load_attach_cache() (line 945 of /home/ubuntu/projects/testlocal/drupal-7-git/includes/module.inc).

cache_get returns False if no cache data is set already.
Adjusted patch to allow for that.

re #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.

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

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

Invalid argument supplied for foreach() in                           [warning]
_system_rebuild_module_data() (line 2310 of
/home/ubuntu/projects/testlocal/drupal-7-git/modules/system/system.module

The node.css file was not included on page load.

This 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

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

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

I'm not sure which is the duplicate of which ...

In the mean time, chasing HEAD

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

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

I tested and my fake blog.css was included but not a fake blog.js. You need to add scripts[] support it seems.

Hmm. my blog.css did get added with proper path.

Sorry, cleared cache and confirmed correct stylesheet path.

Someone 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).

Can we please do #33 in a follow-up? I'd like to keep these issues small, manageable, and (most importantly) reviewable. :)

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

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

I'm not sure what files would have to be moved - didn't understand those follow-ups.

+++ includes/common.inc	23 Aug 2010 14:36:59 -0000
@@ -4605,7 +4605,7 @@ function _drupal_bootstrap_full() {
-    module_invoke_all('init');
+    module_init_all();

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?

+++ modules/system/system.module	23 Aug 2010 14:37:00 -0000
@@ -2290,6 +2290,10 @@ function _system_rebuild_module_data() {
+    $modules[$key]->info['stylesheets'] = _system_rebuild_info_stylesheets($modules[$key]->info, $modules[$key]->uri);
+    $modules[$key]->info['scripts'] = _system_rebuild_info_scripts($modules[$key]->info, $modules[$key]->uri);

@@ -2482,6 +2471,42 @@ function system_rebuild_theme_data() {
+function _system_rebuild_info_stylesheets($info, $uri) {
...
+  if (isset($info['stylesheets'])) {
+    foreach ($info['stylesheets'] as $media => $stylesheets) {
...
+function _system_rebuild_info_scripts($info, $uri) {
...
+  if (isset($info['scripts'])) {
+    foreach ($info['scripts'] as $script) {

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.

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

I 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

• supports styles & js
• moves the function call for (what is now called) drupal_add_styles_scripts to template_preprocess_page (good call)
• documents the parameters in _system_rebuild_info_stylesheets and system_rebuild_info_scripts
• Does both 2) & 3) from #37. IMO opinion this makes the code fuglier. I usually find out later that there was a really good reason for this kind of change that I'm not seeing, so I did it despite the aesthetics
• Does NOT do 4) from #37 - because of the "media" grouping of css this got messy when I tried to combine the function for css & js so I left them separate. If someone else wants to take a swing at combining those functions, please do

Technically, something like the attached should work, but it somehow just killed my server... not sure why.

Less references work, too.

#41 causes MAMP to crash for me

#43 works much better

+++ modules/system/system.module	23 Aug 2010 23:04:26 -0000
@@ -2290,6 +2285,15 @@ function _system_rebuild_module_data() {
+    $path = dirname($module->uri);

why not
$path = dirname($module->uri) . '/';

Powered by Dreditor.

I dont know how/why, but Bartik's .info file is wrong:

scripts[] = scripts/search.js

should be :

scripts[] = scripts/bartik.js

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

ooops .. last patch left in a couple debug() calls

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

+++ includes/theme.inc	24 Aug 2010 02:22:32 -0000
@@ -605,9 +605,7 @@ function list_themes($refresh = FALSE) {
-        if (file_exists($path)) {
-          $theme->scripts[$script] = $path;
-        }
+        $theme->scripts[$script] = $path;

Above 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 ;)

+++ modules/system/system.info	24 Aug 2010 02:22:33 -0000
@@ -17,3 +17,7 @@ files[] = system.updater.inc
+stylesheets[all][] = system.css
+stylesheets[all][] = system-behavior.css
+stylesheets[all][] = system-menus.css
+stylesheets[all][] = system-messages.css

+++ modules/system/system.module	24 Aug 2010 02:22:33 -0000
@@ -1832,11 +1832,6 @@ function system_init() {
-  drupal_add_css($path . '/system.css', array('weight' => CSS_SYSTEM, 'preprocess' => TRUE));
-  drupal_add_css($path . '/system-behavior.css', array('weight' => CSS_SYSTEM, 'preprocess' => TRUE));
-  drupal_add_css($path . '/system-menus.css', array('weight' => CSS_SYSTEM, 'preprocess' => TRUE));
-  drupal_add_css($path . '/system-messages.css', array('weight' => CSS_SYSTEM, 'preprocess' => TRUE));

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?

+++ modules/system/system.module	24 Aug 2010 02:22:33 -0000
@@ -2290,6 +2285,15 @@ function _system_rebuild_module_data() {
+    // Prefix stylesheets and scripts with module path.
+    $path = dirname($module->uri) . '/';
+    if (isset($module->info['stylesheets'])) {
+      $module->info['stylesheets'] = system_info_add_prefix($module->info['stylesheets'], $path);
+    }
+    if (isset($module->info['scripts'])) {
+      $module->info['scripts'] = system_info_add_prefix($module->info['scripts'], $path);
+    }

@@ -2413,28 +2419,14 @@ function _system_rebuild_theme_data() {
+    // Prefix stylesheets and scripts with module path.
+    $path = dirname($theme->uri) . '/';
+    $theme->info['stylesheets'] = system_info_add_prefix($theme->info['stylesheets'], $path);
+    $theme->info['scripts'] = system_info_add_prefix($theme->info['scripts'], $path);
...
-      $themes[$key]->info['screenshot'] = dirname($themes[$key]->uri) . '/' . $themes[$key]->info['screenshot'];
+      $themes[$key]->info['screenshot'] = $path . $themes[$key]->info['screenshot'];

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.

The attached patch adds tests to check that stylesheets can be added in module .info files.

Is it possible to support path checking?

scripts[search*][] = scripts/search.js
styles[node/*][] = __MODULE_/node.css

NO :)

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.

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

Incorporated everything from #51 as well as the tests (a bit revised though) from #52.

Fixed that fatal error.

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

Wording proposal.

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

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

No, wait -- I thought I reverted those system.info/system_init() changes...?

+++ modules/system/system.api.php	24 Aug 2010 16:51:27 -0000
@@ -1432,9 +1432,10 @@ function hook_boot() {
+ * To add CSS or JS that should be present on all pages, modules should not
+ * implement this hook, but declare these files in their .info file.

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

(needs review)

Thank you!

awesome!

Re: 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.

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

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

This obviously means we have to postpone this issue on #887870: Make system_list() return full module records

#887870: Make system_list() return full module records is in.

#64: drupal.module-assets.64.patch queued for re-testing.

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

+++ includes/theme.inc	24 Aug 2010 23:26:52 -0000
@@ -2274,6 +2272,22 @@ function template_preprocess_page(&$vari
+
+  // Add CSS and JS declared in module .info files.
+  foreach (system_get_info('module') as $module => $info) {
+    if (!empty($info['stylesheets'])) {
+      foreach ($info['stylesheets'] as $media => $stylesheets) {
+        foreach ($stylesheets as $stylesheet) {
+          drupal_add_css($stylesheet, array('media' => $media, 'preprocess' => TRUE));
+        }
+      }
+    }
+    if (!empty($info['scripts'])) {
+      foreach ($info['scripts'] as $script) {
+        drupal_add_js($script, array('preprocess' => TRUE));
+      }
+    }
+  }

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

Not 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().

Thanks. That addresses my concerns, so back to RTBC as per #74

This looks good. Only one thing came to mind:

+++ modules/system/system.module	4 Sep 2010 14:55:04 -0000
@@ -2497,6 +2516,27 @@ function system_rebuild_theme_data() {
+function system_info_add_path($info, $path) {

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.

Incorporated the requested changes.

#79: drupal.module-assets.79.patch queued for re-testing.

Reviewed once more, and committed to CVS HEAD.

Just to clarify, status change was because this still needs documentation.

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

I think this is a new feature, so it may not need an announcement. However it should definitely go in the module upgrade documentation.

Looks to me like this hasn't even been updated in the D7 Module .info file page. What's up with that?

Looks like this one needs a Handbook edit as well as an addition to the 6/7 module update guide

I'll give this a shot for documentation.

Please review this update in the 6/7 module upgrade guide.

I also added about loading in .info files in the D7 Managing JS page.

This issue number is not mentioned in that section. Is that OK?

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

To be potentially reverted for D8: #1861676: Remove stylesheets[] and scripts[] .info file property support for modules