#cache does not record the #attached declared in child elements

subscribing.

Ran into this again with #986084: Add cache for media_gallery_block_view(). I'm upping this to major, since any module using render caching, where something down the line might use #attached, is going to be broken by this - it's equivalent to drupal_add_js() plus the block cache in D6, except that part of introducing render caching was specifically to avoid that bug.

Uploading a patch that works for the main case, where you set #cache on an element, and the child element implements #attached.

It doesn't account for the case where a child element has #pre_render set, and the pre_render callback adds #attached. At this point it starts getting very tricky, since a child element might also implement drupal_render() caching, and if so we'll need to track attached for all the children, at each level where #cache is set, and we only get #attached when the pre_render callback has been executed. I don't have time to work through that fully this afternoon, so uploading progress so far.

Definitely needs tests as well.

This should now handle handle elements that have children that set #pre_render as well.

No tests yet still but marking needs review since lack of tests shouldn't stop people looking at this.

For testing there are three code paths, only the first is necessary for the original bug, there second two are edge cases which might be nice to test:

1. #cache is set on an element, a child element sets #attached - the css/js should still appear when that element is cached.
2. #cache is set on an element, a child element has #pre_render, the pre_render callback adds #attached somewhere.
3. #cache is set on an element, as well as #theme, elements that get passed through theme() have #attached set on them somewhere.

Removing an unused variable, no other changes.

What about doing the #attached flattening process in drupal_render_cache_set?

+++ includes/common.inc	23 Dec 2010 17:12:55 -0000
@@ -5804,6 +5823,52 @@ function drupal_render_cache_set(&$marku
+  foreach ($element as $key => $value) {
+    if (is_array($value)) {
+      $element[$key]['#collect_attached'] = TRUE;
+      drupal_render_collect_attached($value);
+    }

This needs to use element_children or similar, to avoid adding #collect_attached unnecessarily.

Doing it in drupal_render_cache_set() is a lot simpler, I'd initially been concerned about elements that had already been cached etc. here, but there's no reason since we have attached available for those at this point too (because this patch fixes that bug, doh!), and everything has run at this point. This patch should deal with both of those. Didn't look into the forum test, but if this fixes it will do simpletests here next.

The forum test was breaking earlier due to the use of is_array instead of element_children.

Tests:
I've drafted up a testing routine which implements the first suggested test in #4:

#cache is set on an element, a child element sets #attached - the css/js should still appear when that element is cached.

It's not the prettiest but does elucidate the issue. Caveats in writing the simpletest:

1. Sets $_SERVER['REQUEST_METHOD'] = 'GET' so the cache system turns on.
2. Uses #attached javascript only, and uses drupal_static_reset('drupal_add_js'); to verify that the javascript is still attached after the element is reloaded from cache.

+++ includes/common.inc	24 Dec 2010 04:05:52 -0000
@@ -5625,6 +5626,51 @@ function drupal_render_cache_set(&$marku
+    foreach (array('library', 'css', 'js') as $key) {
+      if (isset($elements['#attached'][$key])) {
+        if (!isset($attached[$key])) {
+          $attached[$key] = array();
+        }
+        $attached[$key] = array_merge($attached[$key], $elements['#attached'][$key]);
+      }

Why limit this to 'library', 'css', and 'js', instead of doing something like:

foreach ($elements['#attached'] as $key => $value) {
  if (!isset($attached[$key])) {
    $attached[$key] = array();
  }
  $attached[$key] = array_merge($attached[$key], $value);
}

Test looks good, thanks!

The 2nd and 3rd test suggestions were for edge cases which don't really apply with the most recent patch, since we know that everything is available by the time we reach drupal_render_cache_set() itself.

Trying to remember why I limited this to the three keys, while drupal_process_attached() has a good reason, there's no reason to anywhere else, so made the suggested change.

Not strictly related to this bug, but drupal_render() #states support likely suffers from a similar bug, we should open a new issue for that I think - will need a similar approach but doesn't affect the code here.

Beautified the test routine a bit, a fixed some spelling mistakes. I think this is pretty much ready to go.

Regarding #states, I read quickly through the code and it seems to be tossing everything into #attached which would then get picked up and cached with this patch. Thus we might have solved that problem as well.

+++ includes/common.inc
@@ -5804,6 +5805,49 @@ function drupal_render_cache_set(&$markup, $elements) {
+    $attached[$key] = array_merge($attached[$key], $value);
+    }

Wrong indentation?

Powered by Dreditor.

Thanks sun. I've fixed the indentation.

#18: 859602-18.patch queued for re-testing.

The one failed test, somewhere in the Locale testing routines, passed locally on my machine.

I have also tested this patch, while testing http://drupal.org/node/986084#comment-3779090. The code looks good.

Subscribe. I haven't reviewed the implementation, but definitely +1 to getting this fixed in D7 (hopefully 7.1). The render cache is pretty much unusable otherwise. From a quick skim, nothing jumps out at me as breaking any BC.

Looked through the latest changes to the test, all looks fine, I fixed Javascript > JavaScript in several code comments. Since this was the only change, leaving RTBC.

Also just to confirm there's no BC breakage here, all we do is look through the entire element for #attached instead of only the top level.

And yes this makes render caching, and by extension #attached itself with any kind of caching (since #attached won't work for custom cache_get()/cache_set() or the normal block cache either) entirely useless without the patch applied.

This looks like a good change; improves performance and shouldn't affect existing code. Committed to CVS HEAD. Thanks.

AFAICT, there is no DRUPAL-7 branch yet, so I think HEAD is still 7.x.