diff --git a/cache/classes/helper.php b/cache/classes/helper.php index 5f42b24909c..b3a5d083d21 100644 --- a/cache/classes/helper.php +++ b/cache/classes/helper.php @@ -861,4 +861,16 @@ class cache_helper { } return $warnings; } + + /** + * A helper to determine whether a result was found. + * + * This has been deemed required after people have been confused by the fact that [] == false. + * + * @param mixed $value + * @return bool + */ + public static function result_found($value): bool { + return $value !== false; + } } diff --git a/cache/classes/loaders.php b/cache/classes/loaders.php index 8f32d6560da..a8ebef37503 100644 --- a/cache/classes/loaders.php +++ b/cache/classes/loaders.php @@ -470,7 +470,7 @@ class cache implements cache_loader { } } else { // If there's no result, obviously it doesn't meet the required version. - if (!$result) { + if (!cache_helper::result_found($result)) { return false; } if (!($result instanceof \core_cache\version_wrapper)) { @@ -503,7 +503,7 @@ class cache implements cache_loader { if ($usesstaticacceleration) { $result = $this->static_acceleration_get($key); - if ($result && self::check_version($result, $requiredversion)) { + if (cache_helper::result_found($result) && self::check_version($result, $requiredversion)) { if ($requiredversion === self::VERSION_NONE) { return $result; } else { @@ -518,7 +518,7 @@ class cache implements cache_loader { // 3. Get it from the store. Obviously wasn't in the static acceleration array. $result = $this->store->get($parsedkey); - if ($result) { + if (cache_helper::result_found($result)) { // Check the result has at least the required version. try { $validversion = self::check_version($result, $requiredversion); @@ -548,7 +548,7 @@ class cache implements cache_loader { $this->store->delete($parsedkey); } } - if ($result !== false) { + if (cache_helper::result_found($result)) { // Look to see if there's a TTL wrapper. It might be inside a version wrapper. if ($requiredversion !== self::VERSION_NONE) { $ttlconsider = $result->data; @@ -582,7 +582,7 @@ class cache implements cache_loader { // 4. Load if from the loader/datasource if we don't already have it. $setaftervalidation = false; - if ($result === false) { + if (!cache_helper::result_found($result)) { if ($this->perfdebug) { cache_helper::record_cache_miss($this->store, $this->definition); } @@ -608,13 +608,13 @@ class cache implements cache_loader { } } } - $setaftervalidation = ($result !== false); + $setaftervalidation = (cache_helper::result_found($result)); } else if ($this->perfdebug) { $readbytes = $this->store->get_last_io_bytes(); cache_helper::record_cache_hit($this->store, $this->definition, 1, $readbytes); } // 5. Validate strictness. - if ($strictness === MUST_EXIST && $result === false) { + if ($strictness === MUST_EXIST && !cache_helper::result_found($result)) { throw new coding_exception('Requested key did not exist in any cache stores and could not be loaded.'); } // 6. Set it to the store if we got it from the loader/datasource. Only set to this direct @@ -1372,7 +1372,7 @@ class cache implements cache_loader { $result = $data; } } - if ($result !== false) { + if (cache_helper::result_found($result)) { if ($this->perfdebug) { cache_helper::record_cache_hit(cache_store::STATIC_ACCEL, $this->definition); } diff --git a/cache/tests/cache_helper_test.php b/cache/tests/cache_helper_test.php new file mode 100644 index 00000000000..fb8dc3b1f7e --- /dev/null +++ b/cache/tests/cache_helper_test.php @@ -0,0 +1,64 @@ +. + +namespace core_cache; + +/** + * PHPunit tests for the cache_helper class. + * + * @package core + * @category cache + * @copyright 2023 Andrew Lyons + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @coversDefaultClass \cache_helper + */ +class cache_helper_test extends \advanced_testcase { + /** + * Test the result_found method. + * + * @param mixed $value + * @param bool $expected + * @dataProvider result_found_provider + * @covers ::result_found + */ + public function test_result_found($value, bool $expected): void { + $this->assertEquals($expected, \cache_helper::result_found($value)); + } + + /** + * Data provider for result_found tests. + * + * @return array + */ + public function result_found_provider(): array { + return [ + // Only false values are considered as not found. + [false, false], + + // The rest are considered valid values. + [null, true], + [0, true], + ['', true], + [[], true], + [new \stdClass(), true], + [true, true], + [1, true], + ['a', true], + [[1], true], + [new \stdClass(), true], + ]; + } +} diff --git a/cache/tests/cache_test.php b/cache/tests/cache_test.php index 5a660c34cd9..ae4f8d28e49 100644 --- a/cache/tests/cache_test.php +++ b/cache/tests/cache_test.php @@ -50,6 +50,7 @@ use cacheable_object_array; * @copyright 2012 Sam Hemelryk * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later * @coversDefaultClass \cache + * @covers \cache */ class cache_test extends \advanced_testcase { @@ -2827,6 +2828,162 @@ class cache_test extends \advanced_testcase { $startstats[$requestid]['stores']['default_request']['sets']); } + /** + * Data provider for static acceleration performance tests. + * + * @return array + */ + public function static_acceleration_performance_provider(): array { + // Note: These are the delta values, not the absolute values. + // Also note that the set will actually store the valuein the static cache immediately. + $validfirst = [ + 'default_application' => [ + 'hits' => 1, + 'misses' => 0, + ], + cache_store::STATIC_ACCEL => [ + 'hits' => 0, + 'misses' => 1, + ], + ]; + + $validsecond = [ + 'default_application' => [ + 'hits' => 0, + 'misses' => 0, + ], + cache_store::STATIC_ACCEL => [ + 'hits' => 1, + 'misses' => 0, + ], + ]; + + $invalidfirst = [ + 'default_application' => [ + 'hits' => 0, + 'misses' => 1, + ], + cache_store::STATIC_ACCEL => [ + 'hits' => 0, + 'misses' => 1, + ], + ]; + $invalidsecond = [ + 'default_application' => [ + 'hits' => 0, + 'misses' => 1, + ], + cache_store::STATIC_ACCEL => [ + 'hits' => 0, + 'misses' => 1, + ], + ];; + + return [ + 'Truthy' => [ + true, + $validfirst, + $validsecond, + ], + 'Null' => [ + null, + $validfirst, + $validsecond, + ], + 'Empty Array' => [ + [], + $validfirst, + $validsecond, + ], + 'Empty String' => [ + '', + $validfirst, + $validsecond, + ], + 'False' => [ + false, + $invalidfirst, + $invalidsecond, + ], + ]; + } + + /** + * Test performance of static acceleration caches with values which are frequently confused with missing values. + * + * @dataProvider static_acceleration_performance_provider + * @param mixed $value The value to test + * @param array $firstfetchstats The expected stats on the first fetch + * @param array $secondfetchstats The expected stats on the subsequent fetch + */ + public function test_static_acceleration_values_performance( + $value, + array $firstfetchstats, + array $secondfetchstats, + ): void { + // Note: We need to modify perfdebug to test this. + global $CFG; + $this->resetAfterTest(true); + $CFG->perfdebug = 15; + + $instance = cache_config_testing::instance(); + $instance->phpunit_add_definition('phpunit/accelerated', [ + 'mode' => cache_store::MODE_APPLICATION, + 'component' => 'phpunit', + 'area' => 'accelerated', + 'staticacceleration' => true, + 'staticaccelerationsize' => 1, + ]); + + $cache = cache::make('phpunit', 'accelerated'); + $this->assertInstanceOf(cache_phpunit_application::class, $cache); + + $this->assertTrue($cache->set('value', $value)); + + $checkstats = function( + array $start, + array $expectedstats, + ): array { + $applicationid = 'phpunit/accelerated'; + $endstats = cache_helper::get_stats(); + + $start = $start[$applicationid]['stores']; + $end = $endstats[$applicationid]['stores']; + + foreach ($expectedstats as $cachename => $expected) { + foreach ($expected as $type => $value) { + $startvalue = array_key_exists($cachename, $start) ? $start[$cachename][$type] : 0; + $endvalue = array_key_exists($cachename, $end) ? $end[$cachename][$type] : 0; + $diff = $endvalue - $startvalue; + $this->assertEquals( + $value, + $diff, + "Expected $cachename $type to be $value, got $diff", + ); + } + } + + return $endstats; + }; + + // Reset the cache factory so that we can get the stats from a fresh instance. + $factory = cache_factory::instance(); + $factory->reset_cache_instances(); + $cache = cache::make('phpunit', 'accelerated'); + + // Get the initial stats. + $startstats = cache_helper::get_stats(); + + // Fetching the value the first time should seed the static cache from the application cache. + $this->assertEquals($value, $cache->get('value')); + $startstats = $checkstats($startstats, $firstfetchstats); + + // Fetching the value should only hit the static cache. + $this->assertEquals($value, $cache->get('value')); + $checkstats($startstats, $secondfetchstats); + } + + public function test_static_cache() { global $CFG; $this->resetAfterTest(true); diff --git a/cache/upgrade.txt b/cache/upgrade.txt index 3a63c137790..d2a072e3ffa 100644 --- a/cache/upgrade.txt +++ b/cache/upgrade.txt @@ -1,5 +1,9 @@ This files describes API changes in /cache/stores/* - cache store plugins. Information provided here is intended especially for developers. + +=== 4.2.2 === +* A new cache_helper::result_found() helper has been added to assist with cache value validation + === 4.2 === * The memcached cachestore has been removed. * The mongodb cachestore has been removed.