[PP-1] Fix block cache to use Attachment Collectors to prevent brokenness with drupal_add_* calls

This looks like a clever and functional approach, though obviously not what one would call elegant.

Seems like if one was desperate one could achieve the same thing within an override of the cache class, but that would be a rather dire abuse of the cache API.

Given that using #attached is not fully adopted or enforced, it seems like this is more of a bug than a feature request.

It would be helpful to have a test case that demonstrates the bug, especially for a core module.

some possible candidates:

http://api.drupal.org/api/drupal/modules!locale!locale.module/function/l...

any block that calls theme('table') might?

Just some docs cleanup; no functional changes.

The test looks good to me. Two minor cleanups:

• Function summary should begin with a third-person verb. (Reference: http://drupal.org/node/1354#functions)
• No t() on assertion message texts. (Reference: http://drupal.org/simpletest-tutorial-drupal7#t and #500866: [META] remove t() from assert message)

I've also provided a separate patch with only the test to expose the failure on testbot for other reviewers.

Crossposted. Similarly without #917490: Performance patch for block_list() (D6) and _block_render_blocks() (D7) functions, for real this time. Test is not affected by this change.

Using drupal_add_js() inside a block callback is a bug, that's why we allow blocks to return renderable arrays. It's not a core bug that lots of modules aren't properly updated to Drupal 7 yet.

The alternative is to fix core block module? The problem is that hook_block_view_alter() doesn't reliably get a render array.

Well, whether we call it a bug or a feature request or an antelope, there's a patch!

so, either this patch is right, because core block module returns a strong instead of a render array:
http://api.drupal.org/api/drupal/modules!block!block.module/function/blo...

or we need to dig around a lot deeper and wider to fix this.

Given that the patch is basically a fairly simple work-around of an inconsistent core behavior and API, I think that's the better approach for 7.

Has tests.

@pwolanin: Er, which is the better approach? :)

I think the current patch is a better option than trying to fix broad parts of D7 core.

The function Peter links to above is the view operation for custom blocks. Those return a string and thats fine since an admin can't exactly create a complex render structure using of a textarea on the web.

I still don't understand what what 'fix broad parts of drupal core' refers to.

+++ b/modules/block/tests/block_test.module
@@ -32,5 +37,11 @@ function block_test_block_info() {
+  case "test_cache_with_assets":
+    drupal_add_js('misc/collapse.js');
+    return array('content' => variable_get('block_test_content', ''));

Is there a core module that calls drupal_add_*() in a way that breaks with block cache? If so, that's a core bug we need to fix. Otherwise, this issue is about coddling broken contrib modules. And should we be adding something like that to D7 core at this point? Can this be reformulated as a contrib module instead?

Sorry. Didn't mean to change status.

Is there a core module that calls drupal_add_*() in a way that breaks with block cache? If so, that's a core bug we need to fix.

Yep, that's why I changed this to a feature request.

I've not tried it, but hook_block_list_alter() very likely allows you to swap out the block cache for your own version - since you can set the block content in that hook which prevents block module's own code from running later.

As mentioned, we do have #attached for precisely this reason. I don't see a reason to break encapsulation and promote bad practices, especially now that we promote rigid object-oriented designs.

This is won't fix for me. At any rate, this belongs in 8.x anyway.

Per the summary, the issue is resolved in 8.x because blocks are already using the render cache, unless I've misunderstood?

Block caching is based on the render cache, but blocks in Drupal 8 can still return a string for $block['content'], http://api.drupal.org/api/drupal/core%21modules%21block%21block.module/f...

so, fixing block_block_view to make him return a render and mark this won't fix could work?

I don't think we should change the return value of block_block_view() in Drupal 7. It's not a bug that it's using a string rather than a render array, and changing it would affect hook_block_view_alter() implementations.

If a contrib module needs to add #attached to it, I think it could turn it into an array (and add #attached at the same time) in hook_block_view_alter() itself.

I don't think we should change the return value of block_block_view() in Drupal 7.

For D8, I could envision a separate issue for ensuring that all page and block callbacks return render arrays instead of strings. But purely on consistency grounds, not something that's sufficiently related to this issue. However, if we did open such an issue for D8, I don't think it would get much traction, given that D8 will at some point possibly begin a process of converting page and block callbacks to some other WSCCI/Layouts initiative construct.

I'm marking this "won't fix", since I don't really see a path forward here. If someone wants to reopen it though, go ahead. Do we have a list of which popular contrib modules are incompatible with block/render cache? Perhaps the way forward is to post issues in their queues to use #attached correctly?

I'm guessing Mark meant to re-open this? Clearly if it breaks Views to use the block cache, this should be fixed in core.

Here's a re-roll since the patch doesn't apply to 7.24. I'm pretty sure it's been resolved in other ways in 8.x, so leaving this issue for 7.x.

I just wanted to check to make sure I was understanding correctly, and that i am in the right place.

When I enable block caching (in any way) the JS within that block fails. This occurs with Masonry and Flexslider. For flexslider, rather than slides it produces a list and masonry doesn't fill it's allotted area.

Setting 'do not cache' fixes the problem, but then, obviously the view is not cached.

Is the consensus that it is the way the JS is built that is causing this issue or is it an issue in views/block? Or do changes have to be applied to both?

Sorry for the simplistic question, I am not much of a developer and trying to get my head round where the issue lies and how to start understanding how to fix it.

Re-roll and auto-merged successfully.

This and #1534490: Make block cache cids alterable would be awesome additions to Core.

Just tested this patch (#34) myself and a previously messed up Views Slideshow now displays correctly with the patch and block caching enabled. Nice work. Thanks.

1.         $array = module_invoke($block->module, 'block_view', $block->delta);
   ....
   +          foreach ($after_render as $key => $values) {
   +            // If there are any differences between the old and the new CSS or
   +            // JavaScript, store them to be added later.
   +            $array[$key] = $added = array_diff_assoc($after_render[$key], $before_render[$key]);
   +          }
   

   With this patch applied I get all sorts of PHP notices about array-to-string conversion when running tests. (I am not sure why the testbot didn't give them, probably a PHP 5.3 vs 5.4 thing.)

   It's valid since hook_block_view() does not always return an array (example) although it's documented as doing so. But other code in the same function checks for this.

2.  function block_test_block_view($delta = 0) {
   -  return array('content' => variable_get('block_test_content', ''));
   +  switch ($delta) {
   +  case "test_cache_with_assets":
   +    drupal_add_js('misc/collapse.js');
   +    return array('content' => variable_get('block_test_content', ''));
   

   I suspect this test will still pass/fail as expected if it returns a render array and uses #attached for the JS? (A lot of the discussion above of render arrays + #attached as a solution to this issue was based on Drupal 8, I think, not Drupal 7.)

   If so, it should change to use that, since it's the recommended way to return data from hook_block_view() so it's better to be testing that scenario. (Also it would prove that this issue is definitely valid for Drupal 7.)

3. +function drupal_restore_html_head($head) {
   +  drupal_add_html_head($head);
   +}
   

   Is this wrapper really worth defining?

4. +          foreach ($after_render as $key => $values) {
   +            // If there are any differences between the old and the new CSS or
   +            // JavaScript, store them to be added later.
   +            $array[$key] = $added = array_diff_assoc($after_render[$key], $before_render[$key]);
   +          }
   +
   +          // Special-case the settings key and get the difference of the data.
   +          $array['js']['settings'] = array_diff_assoc($after_render['js']['settings']['data'], $before_render['js']['settings']['data']);
   

   Aren't these multidimensional arrays? If so, array_diff_assoc() is unlikely to be the right thing here (see #1850798: Add a recursive version of array_diff_assoc()).

5.  function block_test_block_view($delta = 0) {
   -  return array('content' => variable_get('block_test_content', ''));
   +  switch ($delta) {
   +  case "test_cache_with_assets":
   +    drupal_add_js('misc/collapse.js');
   +    return array('content' => variable_get('block_test_content', ''));
   +  default:
   +    return array('content' => variable_get('block_test_content', ''));
   +  }
   

   (minor) Incorrect indentation.

Maybe this code can help:
http://cgit.drupalcode.org/cachedblock/tree/cachedblock.module#n68

I have tested the latest patch (34) and it works, but I get some notices. I replaced array_diff_assoc with drupal_array_diff_assoc_recursive and it worked perfectly.

The patch in #38 solves my block caching issue (javascript not getting added correctly) and I'm not seeing any PHP Notices.

Noting that the patch uses $key in the foreach it adds to _block_render_blocks, overwriting the $key defined by the outer loop.

  foreach ($region_blocks as $key => $block) {
...
          foreach ($after_render as $key => $values) {

$key isn't used by the outer loop afterward, so it technically works, but the new foreach should probably use a safer variable name (e.g $k, which sibling loops in that function are using). Updated patch attached.

#40 did load all the scripts and css files but there were times that it changed their order, which could lead eg to js errors. I added some lines that reset the weight of these files/snippets/etc to their original value before being saved in the cache. I also added a check before doing the array_diff for js settings so as not to unnecessarily bloat the cached blocks.

In 42, I wrongly assumed that drupal_add_html_head() initializes a #weight even if the original block had no such key, as drupal_add_js and drupal_add_css do. I was wrong and the path in 42 resulted in some warnings. Here is an updated patch with a simple check.

This is a partial duplicate of #2754245: [META] Explore how to backport render context and #cache properties from Drupal 8 to empower contrib, which solves the same problem space in a more generic way (same way as in D8).

Enabling the new drupal_render_use_render_context() option should make it possible to fix this in an easier way, however the blocks need to use the #pre_render / #cache pattern.

The problem with asset diff'ing is that the stores in drupal_add_js() / drupal_add_css() are not idempotent, which means e.g if blockA already uses foo.css, then it will never be detected for blockB - as core would think it is already loaded.

The views diff'ing ran into that problem again and again ... And it is very hard to track down as it depends on the rows being present and even the caching state of the items ...

I will discuss with stefan, if we want to add a quick follow-up to fix the block cache for the new option already in 7.50 (or wait for the next bug fix release) as it should be a two-liner with #2754245: [META] Explore how to backport render context and #cache properties from Drupal 8 to empower contrib applied now ...

It would definitely provide great value!

Tentatively marking as a Drupal 7.50 target - even if we only would fix it for the new experimental option.

Here is a patch rolled on top of #2754245: [META] Explore how to backport render context and #cache properties from Drupal 8 to empower contrib and a single patch, which just is the interdiff.

And even if catch said 4 years ago that modules just should fix their code to use #attached and not drupal_add_*, now in 2016 this still has not been fully changed and drupal_add_* is a supported API in Drupal 7.

Therefore marking major and approving as Drupal 7 framework manager.

Random test failure (CI error). We do have a race condition in the D7 tests setup - it seems. I presume two tests get the same prefix somehow OR a prefix that still has tables gets re-used.

Testbot really does not like this patch ...

And even if catch said 4 years ago that modules just should fix their code to use #attached and not drupal_add_*, now in 2016 this still has not been fully changed and drupal_add_* is a supported API in Drupal 7.

Therefore marking major and approving as Drupal 7 framework manager.

Except this patch will then only fix it for people who modify their settings.php to opt in to the new behavior. See #2754245-19: [META] Explore how to backport render context and #cache properties from Drupal 8 to empower contrib and -20 for more details about my concerns.

#52: That is correct, but we can have a handbook page for it.

If we change the default behavior, someone might have relied on the wrong behavior (e.g. an asset not being added as the block was cached most of the time).

e.g. a site displaying a JS error, but just on cold cache, combine with something to prevent the cron cache clearing of blocks (happens in the wild) and the site "mostly works", but not when the fix would be applied. Then it would fail 100% of the time (same as if the setting was enabled).

Except this patch will then only fix it for people who modify their settings.php to opt in to the new behavior. See #2754245-19: Backport render context and #cache properties from Drupal 8 to empower contrib and -20 for more details about my concerns.

I wouldn't be against eventually removing experimental status (in 7.60 or such) and exposing this as a checkbox on the performance screen (and recommending advanced users to enable it in release notes).

For 7.x we'll have to make certain bugfixes opt-in anyway, so as to not risk breaking sites.

Making this a bug and it probably is a good idea to get feedback from the people using the experimental option first.

The patch is small enough that I would put it on a production site myself.

Hey,

I had the problem with blocks created by Display Suite. It's described here https://www.drupal.org/node/1820942.

Once I applied the patch from the comment #45, the problem's gone. So, I can confirm that this patch works for me.
Tested on Drupal 7.44 and Display suite 7.x-2.13

#56: Thank you for testing. I will probably have an updated patch within the next week - or so.

Postponed on #2820757: Add Attachment Collector to ensure #attached assets are not lost during render caching.

Hello Fabianx

Can you clarify why this patch has been postponed ? is because if we get to commit #2820757 this patch should be rewritten to use the Attachment Collector API right ?