Remove drupal_add_js/css from seven.theme

I didn't quite get the reason of this, but patch works well for me, didn't break anything in ie8. +1

Uh. That's an awful lot of custom, difficult-to-write PHP code for something so simple as "I want to pull in some JS libraries and use them with my theme."

If themes need to declare script dependencies it feels like that should be in the .info file, not in PHP.

Ok, so from themer perspective:
If it's more documented - it's actually not so bad. I would just copy an example code and change/remove the pieces I need.
For example this code in not so readable, and if you break it to multiple lines, then you just loose the simple thing as 'drupal_add_js' and get something more diffuclt:
drupal_add_css(path_to_theme() . '/ie.css', array('group' => CSS_AGGREGATE_THEME, 'browsers' => array('IE' => 'lte IE 8', '!IE' => FALSE), 'weight' => 999, 'preprocess' => FALSE));
And this is actually an example for just one file. I think it's more readable and maintainable, when you have more files to add.

If there's a way to simplify it - i vote for it. If not - totally ok with that.
Hope my description makes sense.

Accidently removed. Need opinion from other themers.

Analyzing this code, though, I don't see why if the .info file said:

css:
- ie.css
js:
- js/mobile.install.js

...whatever makes hook_libraries_info "go" couldn't be smart enough to fill in the gaps of:

+  $libraries[$filename] = array(
+    'version' => VERSION,
+    'js' => array(
+      drupal_get_path('theme', $themename) . $filename => array('group' => JS_THEME),
+    ),
+  );

and:

+  $libraries[$filename] = array(
+    'version' => VERSION,
+    'css' => array(
+      path_to_theme() . $filename => array(
+        'group' => CSS_AGGREGATE_THEME,
+        'weight' => 999,
+        'preprocess' => FALSE,
+      ),
+    ),
+  );

...thus removing the need of at least 10-15 lines of boilerplate code from every theme.

The 'browsers' => array('IE' => 'lte IE 8', '!IE' => FALSE), bit is tricky, admittedly, but I also thought there was already .info file syntax for that?

Re-tagging, cos it'd be good to get some more perspectives on this.

I'm using drupal_add_css() in my themes for all CSS files.

A library files has no benefit as I check conditions based on setting in PHP code and add css files than e.g. Menu A configured; add a.menu.css and Menu B configured; add b.menu.css. Other CSS files are also attached or added via inline CSS based on page width and other conditions.

If #1996238: Replace hook_library_info() by *.libraries.yml file we can refactor this. This is a straight bug fix and shouldn't be blocked on it though.

This is a straight bug fix

What bug is this fixing?

#14: it's fixing the fact that we should move to using #attached. See #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff.

Reroll.

Note: the reason we must still use drupal_add_library() and not #attached is because themes can't implement hook_page_build() yet. It is still a big step forward, because it allows us to get rid of drupal_add_(css|js)(). (I double-checked this with nod_.)

Patch applies and Drupal installs with patch applied.

Couldn't Seven implement hook_page_alter() and use #attached there?

it's fixing the fact that we should move to using #attached.

But the patch doesn't fix that. #17 would though.

#17: themes can't use hook_page_build() nor hook_page_alter(), unless I'm mistaken.

#18: see the above, and the note I wrote in #15.

@Wim, themes can implement alter hooks since Drupal 7.

#20: I must've done something wrong then. I'll work on a reroll.

I have read hook_page_alter() goes away in D8...?

I have read hook_page_alter() goes away in D8...?

That was a goal for the Blocks-Layouts initiative, but it didn't happen, and we're now on the other side of the API freeze deadline. I would still like us to remove hook_page_build() and hook_page_alter() implementations from core (even if it's too late to remove the hooks themselves), because that would make modules like Panels in contrib more robust. But that's a job for a separate issue and requires figuring out what we can move those implementations to. In the meantime, setting #attached in hook_page_build() or hook_page_alter() is preferable to calling drupal_add_*() in that it replaces a global side effect with something attached to a known context, even if in a future issue we'll need to figure out a better context for it.

Hey... Not both... I need them for google analytics and piwik. I'm not aware of any alternatives.

I'm not aware of any alternatives.

Indeed. That's why they haven't been removed yet and are legitimate to use in this issue and others. They can only be removed when there's an alternative. Whether an alternative is figured out in a way that D8 maintainers approve of remains to be seen, but the odds of it shrink with each passing month at this point.

I should reroll this.

Though it would be nice if somebody picked this up! :)

Added as a task to drupalmentoring.org:

http://drupalmentoring.org/task/4916

Yay! Go Tim! :) I'll review your patch! Thanks for working on this!

First patch!

Second patch!

+++ b/core/themes/seven/seven.theme
@@ -143,10 +166,9 @@ function seven_tablesort_indicator($variables) {
+  drupal_add_library('seven', 'install-page');

Let's use $variables['#attached']['library'] ...

Third times the charm!

I think the patch on comment #32 will work.

I think #32 is as good as we're going to get at this time.

We can't do #attached in the preprocess function. We also can't do hook_page_alter(), theme.maintenance.inc says the following and I can't get seven_page_alter() to do anything during installation or in any maintenance page.

_drupal_maintenance_theme():

  // These CSS files are normally added by system_page_build(), except
  // system.maintenance.css. When the database is inactive, it's not called so
  // we add them here.
  drupal_add_library('system', 'drupal.base');

#38,#39, but how are we then supposed to get rid of drupal_add_library()?

Yes this needs more review.

I think hypothetically for #attached to work here, we'd still need to have a render array in seven_preprocess_install_page() so we can add #attached to it. Right now we see $variables['content'] is just a string of HTML.

Crosspost…

This is the technique that template_preprocess_maintenance_page() currently uses. Rather ugly but seems to work fine when manually testing and gets rid of drupal_add_library().

See #2008270: Remove drupal_add_css() from template_preprocess_maintenance_page() — use #attached

That's not a good option either - drupal_render() internally calls drupal_add_*().

However maintenance page theming isn't really the target audience for drupal_add_*() removal so I'm thinking we could go with #44 then open a follow-up. We do need to fix the inability to add js/css in preprocess nicely - either by figuring out a more appropriate spot or making it actually possible.

Indeed, +1 to #44. It's not ideal, but it's the best we can do today, and it's what we did for #2008270: Remove drupal_add_css() from template_preprocess_maintenance_page() — use #attached, which catch even RTBC'd :)

Hence: RTBC.

reword

Yeah OK. I opened an issue for direct calls to drupal_render() already, which I can't find, but that covers this for follow-up.

Committed/pushed to 8.x, thanks!