CacheClearCase::testClearArray() contains the following code.

// Set the cache clear threshold to 2 to confirm that the full bin is cleared
// when the threshold is exceeded.
variable_set('cache_clear_threshold', 2);

That persistent variable is not used from Drupal code. With git grep --fixed-strings 'cache_clear_threshold', I get the following output.

modules/simpletest/tests/cache.test: variable_set('cache_clear_threshold', 2);

 

Those lines need to be removed.

Issue fork drupal-3464097

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

avpaderno created an issue. See original summary.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue summary: View changes
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue summary: View changes
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Category: Task » Bug report
Priority: Normal » Minor

(This is a bug, even if minor.)

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Title: CacheClearCase::testClearArray() set a persistent variable that has no effect » CacheClearCase::testClearArray() sets a persistent variable that has no effect

avpaderno opened merge request !8948

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Active » Needs review

Not even Drupal 6.x nor Drupal 5.x uses such value. I was thinking of left-over code from previous versions, but it is not so.

poker10’s picture

poker10 commented

Thanks for reporting and working on this.

I did some search and found out that the changes in test were committed in #333171: Optimize the field cache. If we read the discussion in that issue, we can see that it was proposed to add the new variable cache_clear_threshold and it was in patch in #18. But then it was removed, but was left in the test (seems like unintentionally). So I agree we can remove this safely.

I think there is one question left here - do we need the rest of the test, or can we delete all other line from cache_clear_threshold to the end of the function? I do not see any difference between:

    // Clear two entries using an array.
    cache_clear_all(array('test_cid_clear1', 'test_cid_clear2'), $this->default_bin);
    $this->assertFalse($this->checkCacheExists('test_cid_clear1', $this->default_value)
                       || $this->checkCacheExists('test_cid_clear2', $this->default_value),
                       'Two cache entries removed after clearing with an array.');

and 

    cache_clear_all(array('test_cid_clear1', 'test_cid_clear2', 'test_cid_clear3'), $this->default_bin);
    $this->assertFalse($this->checkCacheExists('test_cid_clear1', $this->default_value)
                       || $this->checkCacheExists('test_cid_clear2', $this->default_value)
                       || $this->checkCacheExists('test_cid_clear3', $this->default_value),
                       'All cache entries removed when the array exceeded the cache clear threshold.');

Just that the first is deleting two entries and second three entries. If I have not missed anything, I think we can safely remove the remaining part of the test in this cleanup as well. What do you think?

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

I was a bit confused by the following lines in the test.

// Set the cache clear threshold to 2 to confirm that the full bin is cleared
// when the threshold is exceeded.
variable_set('cache_clear_threshold', 2);
cache_set('test_cid_clear1', $this->default_value, $this->default_bin);
cache_set('test_cid_clear2', $this->default_value, $this->default_bin);
$this->assertTrue($this->checkCacheExists('test_cid_clear1', $this->default_value)

Reading them after reading your comment, I think the last three lines are merely testing the effect of variable_set('cache_clear_threshold', 2); and I agree they should be removed.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

Actually, all these lines can be removed.

    cache_set('test_cid_clear1', $this->default_value, $this->default_bin);
    cache_set('test_cid_clear2', $this->default_value, $this->default_bin);
    $this->assertTrue($this->checkCacheExists('test_cid_clear1', $this->default_value)
                      && $this->checkCacheExists('test_cid_clear2', $this->default_value),
                      'Two cache entries were created.');
    cache_clear_all(array('test_cid_clear1', 'test_cid_clear2', 'test_cid_clear3'), $this->default_bin);
    $this->assertFalse($this->checkCacheExists('test_cid_clear1', $this->default_value)
                       || $this->checkCacheExists('test_cid_clear2', $this->default_value)
                       || $this->checkCacheExists('test_cid_clear3', $this->default_value),
                       'All cache entries removed when the array exceeded the cache clear threshold.');

The last line makes clear what their purpose is: All cache entries removed when the array exceeded the cache clear threshold.
Since there is no longer a cache clear threshold, they are not useful.

poker10’s picture

poker10 commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Pending Drupal 7 commit

Yes, that is what I meant (sorry if I was not clear enough). The clear cache behavior with an array is still being tested on lines which are left in that function. I think this change looks good now. Thanks!

  • mcdruid committed 72bcab83 on 7.x 
    Issue #3464097 by avpaderno, poker10: CacheClearCase::testClearArray()...
mcdruid’s picture

mcdruid commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Less code = more good.

Thanks!

Status: Fixed » Closed (fixed)

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