Merge branch 'MDL-78466-401' of https://github.com/andrewnicols/moodle into MOODLE_401_STABLE

cache/classes/helper.php

@@ -860,4 +860,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;
+    }
 }

cache/classes/loaders.php

@@ -457,7 +457,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)) {
@@ -490,7 +490,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 {
@@ -505,7 +505,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);
@@ -535,7 +535,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;
@@ -569,7 +569,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);
             }
@@ -595,13 +595,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
@@ -1359,7 +1359,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);
             }

cache/tests/cache_helper_test.php

@@ -0,0 +1,64 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+namespace core_cache;
+
+/**
+ * PHPunit tests for the cache_helper class.
+ *
+ * @package    core
+ * @category   cache
+ * @copyright  2023 Andrew Lyons <andrew@nicols.co.uk>
+ * @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],
+        ];
+    }
+}

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);

cache/upgrade.txt

@@ -1,6 +1,9 @@
 This files describes API changes in /cache/stores/* - cache store plugins.
 Information provided here is intended especially for developers.
 
+=== 4.1.5 ===
+* A new cache_helper::result_found() helper has been added to assist with cache value validation
+
 === 4.1 ===
 * Added new `requirelockingbeforewrite` option for cache definitions. This will check that a lock for a given cache key already
   exists before it will perform a `set()` on that key. A `coding_exception` is thrown if the lock has not been acquired.