Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Needs review
FileSize
950 bytes
andypost’s picture

+++ b/core/includes/theme.inc
@@ -215,7 +215,18 @@ function _drupal_theme_initialize($theme, $base_theme = array(), $registry_callb
   foreach ($final_stylesheets as $media => $stylesheets) {
     foreach ($stylesheets as $stylesheet) {
-      drupal_add_css($stylesheet, array('group' => CSS_AGGREGATE_THEME, 'every_page' => TRUE, 'media' => $media));
+      $attached = array(
...
+      drupal_render($attached);

I think it would be good to move drupal_render() out of loop

vijaycs85’s picture

Issue summary: View changes
FileSize
908 bytes
1.07 KB

Addressing #2

The last submitted patch, 3: 2096591-theme-css-3.patch, failed testing.

mr.baileys’s picture

The patch in #3 is causing a fatal error:

Fatal error: Cannot pass parameter 1 by reference in /home/sa66b5f73c7d291c/www/core/includes/theme.inc on line 227

Tweaked the patch to avoid the error, also removed the check on empty() since drupal_render takes care of this.

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

Sorry for the fatal... above patch looks good to go...

Damien Tournoud’s picture

This patch really doesn't fix *anything*. It doesn't brings us closer to removing drupal_add_css(), because #attach + drupal_render() is actually relying on the same global store for CSS information.

I'm not sure why we would pursue a kick-the-can-down-the-road approach like this.

We want to remove the global store for CSS information, which means that the information needs to be stored in a data object (like a Page object) that gets passed around.

catch’s picture

Status: Reviewed & tested by the community » Fixed

@Damien, no it's an improvement as it is, just not much of one.

drupal_render() could implement it's own internal storage, which then gets passed up properly whenever it has the opportunity to. That would help with situations like preprocess where there's nothing to #attach to, but where the theme call is actually inside a drupal_render() call that's handling #attached and caching properly. It's very much less than perfect, but at least allows that interim step to be taken.

This particular function never gets called in a place that's particularly affected by global state - it's all 'page-level' functions anyway. But we need to kill off drupal_add_css() as an API function for the places where, then circle back and look at all the direct calls to drupal_render() too. Most of the patches here have been doing #attached properly, but yes there's still plenty of places where there's nothing to #attach to and it does need sorting out.

Wim Leers’s picture

Indeed, baby steps.

Damien Tournoud’s picture

It's not baby step, it's kicking the can down the road. It would be more helpful to make sure that we have something to #attach those CSS files to in there.

Status: Fixed » Closed (fixed)

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