Block caching per-role, per-user, per-page is broken

Is this related or duplicate to/of: #235673: Changes to block caching mode not caught?

$this->assertNoText(variable_get('block_test_content', ''), t('Anonymous user does not see content cached per-role for normal user.'));

I don't see how that is going to work. Since you're getting the text right from the database, the text you're asserting on is always going to be the non-cached one, so I don't see why this is a "assertNoText". An "assertText" wouldn't make sense here either, you should assert on $old_content, except then you should move the cache_clear_all() to after that.
I think.
Haven't tried it out. Please ignore if that was complete nonsense.

Cross-post.
Fixing title

subscribe

Nice catch. I'd be very interested to see how that affects our benchmarks.

Got it. Committed to CVS HEAD. Thanks for writing tests.

Sorry about that. I rolled it back for now.

resetting status

+++ modules/block/block.test	23 Jan 2010 06:05:13 -0000
@@ -318,3 +318,180 @@
+    $this->normal_user_alt->roles = $this->normal_user->roles;

This won't work as it only alters the in-memory copy of the account.

Attached updates the patch so the role synchronization is persisted, all tests now pass. I have not reviewed the rest of the patch.

Powered by Dreditor.

+++ modules/block/block.test	1 Feb 2010 21:08:00 -0000
@@ -318,3 +318,181 @@ class BlockAdminThemeTestCase extends Dr
+    $this->admin_user = $this->drupalCreateUser(array('administer blocks', 'administer filters', 'access administration pages'));

Minor: Is there any reason this user needs 'administer filters'?

All in all, the tests look good and it seems like they cover everything. They pass locally and I didn't really know what to test manually.
So, from my view, RTBC if the test bot agrees, but I'll let someone more knowledgeable than me, make the decision.

Powered by Dreditor.

Looks pretty good to me... Some minor nitpicks:

+++ modules/block/block.test	15 Feb 2010 17:26:50 -0000
@@ -318,3 +318,181 @@
+    cache_clear_all();
+    $this->drupalGet('');
+    $this->assertNoText($old_content, t('Block cache clear removes stale cache data.'));

I would add an additional positive assertion here to make sure the new, non-cached, content *is* displayed.

+++ modules/block/tests/block_test.module	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,24 @@
+    'info' => 'Test block caching',

Should be 'info' => t('Test block caching'),

+++ modules/block/tests/block_test.info	1 Jan 1970 00:00:00 -0000
@@ -0,0 +1,8 @@
+package = Core

Should be package = Testing similar to how other test modules are set up.

Powered by Dreditor.

Ok, one final question from me and then I think this is RTBC (and I might be totally missing something here):

+++ modules/block/block.test	15 Feb 2010 19:20:26 -0000
@@ -318,3 +318,182 @@
+    $this->drupalLogin($this->admin_user);
+    $this->drupalGet('');
+    $this->assertNoText($old_content, t('Admin user does not see content cached per-role for normal user.'));

User 1 is always excluded from block caching:

function _block_get_cache_id($block) {
  global $theme, $base_root, $user;

  // User 1 being out of the regular 'roles define permissions' schema,
  // it brings too many chances of having unwanted output get in the cache
  // and later be served to other users. We therefore exclude user 1 from
  // block caching.
  if (variable_get('block_cache', 0) && $block->cache != BLOCK_NO_CACHE && $user->uid != 1) {

This means it should not be used to check caching per role (as it will never see cached content, irregardless of role). I think it's ok to just drop those three lines alltogether, since I think we have full coverage between the "anonymous user" role and the normal_user/normal_user_alt role, right?

On the other hand, we probably want to add a couple of lines to each test to verify that admin user never gets to see cached content, even though for example caching is set to DRUPAL_CACHE_PAGE and the admin user visits a page which has cached blocks on it.

Does that make sense?

In light of the above, I don't understand why the following test case succeeds:

  /**
  * Test global caching.
  */
  function testCacheGlobal() {
    $this->setCacheMode(DRUPAL_CACHE_GLOBAL);
    variable_set('block_test_content', $this->randomName());

    $this->drupalGet('');
    $this->assertText(variable_get('block_test_content', ''), t('Block content displays.'));

    $old_content = variable_get('block_test_content', '');
    variable_set('block_test_content', $this->randomName());

    $this->drupalLogout();
    $this->drupalGet('user');
    $this->assertText($old_content, t('Block content served from global cache.'));
  }

Since the first page is requested as "admin" (set in setUp), no blocks should be cached, and the second page request should serve fresh content instead of cached content. No?

Powered by Dreditor.

I knew I was missing something obvious, thanks for clearing that up!

A one line fix and an impressive 200 lines of tests to prove that the fix works and to make sure this doesn't get broken again, RTBC!

Nice work! Great tests. Some feedback -- nothing major:

+++ modules/block/block.test	15 Feb 2010 19:20:26 -0000
@@ -318,3 +318,182 @@
+    // Sync the roles, since drupalCreateUser() creates separate roles for
+    // the same permission sets.
+    user_save($this->normal_user_alt, array('roles' => $this->normal_user->roles));
+    $this->normal_user_alt->roles = $this->normal_user->roles;
+

Wishlist: would be nice if drupalCreateUser() would take a role as input, instead of permissions. That would avoid these kind of hacks and would align with how Drupal works. Not for this issue though.

+++ modules/block/block.test	15 Feb 2010 19:20:26 -0000
@@ -318,3 +318,182 @@
+    // Enable block caching.
+    variable_set('block_cache', 1);

Mmm, it looks like we're mixing booleans and integers.

$ grep -r block_cache . | grep variable
./modules/block/block.module:  if (variable_get('block_cache', 0) && !in_array($block->cache, array(DRUPAL_NO_CACHE, DRUPAL_CACHE_CUSTOM)) && $user->uid != 1) {
./modules/block/block.module:    '#default_value' => variable_get('block_cache', FALSE),

Maybe we can tidy that up too?

+++ modules/block/block.test	15 Feb 2010 19:20:26 -0000
@@ -318,3 +318,182 @@
+    $old_content = variable_get('block_test_content', '');

Small code optimization: this could be moved up as it would avoid the previous variable_get(). This pattern occurs a few times.

+++ modules/block/block.test	15 Feb 2010 19:20:26 -0000
@@ -318,3 +318,182 @@
+   * Test DRUPAL_NO_CACHE.

Inconsistent documentation. You don't use the defines in the other phpdoc comments.

Wishlist: would be nice if drupalCreateUser() would take a role as input, instead of permissions. That would avoid these kind of hacks and would align with how Drupal works. Not for this issue though.

Maybe worthwile to revisit #463354: Support custom role name in drupalCreateUser()?

Looks like this needs a quick re-roll for the rest.

Committed to CVS HEAD. Thanks carlos8f and mr.baileys.

Seems like HEAD is broken:

Block caching (BlockCacheTestCase) [Block] 192 passes, 30 fails, 0 exceptions

HEAD is broken because the helper test module has not been committed:

modules/block/tests/block_test.info
modules/block/tests/block_test.module

Now fixed. Thanks, Dries.