Render cache shouldn't treat empty strings as a cache miss

looks good to me.

This is a bit yoda: FALSE !== $cached_output = drupal_render_cache_get($elements)

Could we do it another way?

Could also use a test for this one.

New version with less yoda and a test!

+++ b/core/modules/simpletest/tests/common.testundefined
@@ -1832,6 +1832,33 @@ class DrupalRenderTestCase extends DrupalWebTestCase {
+  /**
+   * Test caching of an empty render item.
+   */

Should be "Tests."

Attached fixes that. Test also uploaded separately to expose the failure.

+++ b/core/modules/simpletest/tests/common.test
@@ -1832,6 +1832,33 @@ class DrupalRenderTestCase extends DrupalWebTestCase {
+    // Make sure we did go via the render process.

This made me stop and wonder. Would it be better to say: "Make sure the render process got the cache for the empty render item." ...or have I misunderstood?

Rest looks good :)

Thanks @Tor Arne Thune! I'll polish the inline docs a little. Edit: But, um, not at 3a. That could end badly. :)

Clarified the inline comments a bit.

Looks good and sane.

chx in IRC:

$this->assertTrue(!isset($element['#printed']), t('Cache hit')); this should be assertFalse

Fixed here.

I am very happy to see render cache enhancements given how much me (and my client, thanks ITV for sponsoring my time on this review) rely on it.

+    // Force a request via GET.
+    $_SERVER['REQUEST_METHOD'] = 'GET';

Should this be set back to its previous value at the end of the test? (Offhand, I'm not sure how much it matters.)

I'm not sure how much it matters either, but I also don't want to find out in three months that it does, so could we do a quick re-roll for that?

So, uh. Like this?

Yep.

Committed/pushed to 8.x, thanks folks.

Moving back to 7.x for backport.

Fast port for D7.11 including consideration.

Straight forward port if the bot comes back green this looks good.

Looks like a straight-up bug fix to me. Thanks for the tests!

Committed and pushed to 7.x.