Merge branch 'MDL-45584_master' of git://github.com/markn86/moodle

cache/classes/definition.php

@@ -796,10 +796,12 @@ class cache_definition {
      * @throws coding_exception
      */
     public function set_identifiers(array $identifiers = array()) {
-        // If we are setting the exact same identifiers then just return as nothing really changed.
+        if ($this->identifiers !== null) {
-        // We don't care about order as cache::make will use the same definition order all the time.
+            throw new coding_exception("You can only set identifiers on initial definition creation." .
-        if ($identifiers === $this->identifiers) {
+                " Define a new cache to set different identifiers.");
-            return false;
+        }
+        if (!empty($identifiers) && !empty($this->invalidationevents)) {
+            throw new coding_exception("You cannot use event invalidation and identifiers at the same time.");
         }
 
         foreach ($this->requireidentifiers as $identifier) {

cache/classes/factory.php

@@ -191,15 +191,18 @@ class cache_factory {
      * @return cache_application|cache_session|cache_request
      */
     public function create_cache_from_definition($component, $area, array $identifiers = array(), $unused = null) {
-        $definitionname = $component.'/'.$area;
+        $identifierstring = empty($identifiers) ? '' : '/'.http_build_query($identifiers);
+        $definitionname = $component.'/'.$area.$identifierstring;
         if (isset($this->cachesfromdefinitions[$definitionname])) {
             $cache = $this->cachesfromdefinitions[$definitionname];
-            $cache->set_identifiers($identifiers);
             return $cache;
         }
         $definition = $this->create_definition($component, $area);
-        $definition->set_identifiers($identifiers);
+        // Identifiers are cached as part of the cache creation, so we store a cloned version of the cache.
-        $cache = $this->create_cache($definition);
+        $cacheddefinition = clone($definition);
+        $cacheddefinition->set_identifiers($identifiers);
+        $cache = $this->create_cache($cacheddefinition);
+
         // Loaders are always held onto to speed up subsequent requests.
         $this->cachesfromdefinitions[$definitionname] = $cache;
         return $cache;
@@ -222,10 +225,14 @@ class cache_factory {
      * @return cache_application|cache_session|cache_request
      */
     public function create_cache_from_params($mode, $component, $area, array $identifiers = array(), array $options = array()) {
-        $key = "{$mode}_{$component}_{$area}";
+        $identifierstring = empty($identifiers) ? '' : '_'.http_build_query($identifiers);
-        if (array_key_exists($key, $this->cachesfromparams)) {
+        $key = "{$mode}_{$component}_{$area}{$identifierstring}";
+        if (isset($this->cachesfromparams[$key])) {
             return $this->cachesfromparams[$key];
         }
+        // Regular cache definitions are cached inside create_definition().  This is not the case for Adhoc definitions
+        // using load_adhoc().  They are built as a new object on each call.
+        // We do not need to clone the definition because we know it's new.
         $definition = cache_definition::load_adhoc($mode, $component, $area, $options);
         $definition->set_identifiers($identifiers);
         $cache = $this->create_cache($definition);

cache/classes/helper.php

@@ -229,7 +229,8 @@ class cache_helper {
     /**
      * Invalidates a given set of keys by means of an event.
      *
-     * @todo add support for identifiers to be supplied and utilised.
+     * Events cannot determine what identifiers might need to be cleared. Event based purge and invalidation
+     * are only supported on caches without identifiers.
      *
      * @param string $event
      * @param array $keys
@@ -292,8 +293,9 @@ class cache_helper {
         if ($cache instanceof cache_store) {
             $factory = cache_factory::instance();
             $definition = $factory->create_definition($component, $area, null);
-            $definition->set_identifiers($identifiers);
+            $cacheddefinition = clone $definition;
-            $cache->initialise($definition);
+            $cacheddefinition->set_identifiers($identifiers);
+            $cache->initialise($cacheddefinition);
         }
         // Purge baby, purge.
         $cache->purge();
@@ -303,6 +305,9 @@ class cache_helper {
     /**
      * Purges a cache of all information on a given event.
      *
+     * Events cannot determine what identifiers might need to be cleared. Event based purge and invalidation
+     * are only supported on caches without identifiers.
+     *
      * @param string $event
      */
     public static function purge_by_event($event) {

cache/classes/loaders.php

@@ -275,6 +275,65 @@ class cache implements cache_loader {
         }
     }
 
+    /**
+     * Process any outstanding invalidation events for the cache we are registering,
+     *
+     * Identifiers and event invalidation are not compatible with each other at this time.
+     * As a result the cache does not need to consider identifiers when working out what to invalidate.
+     */
+    protected function handle_invalidation_events() {
+        if (!$this->definition->has_invalidation_events()) {
+            return;
+        }
+
+        $lastinvalidation = $this->get('lastinvalidation');
+        if ($lastinvalidation === false) {
+            // This is a new cache or purged globally, there won't be anything to invalidate.
+            // Set the time of the last invalidation and move on.
+            $this->set('lastinvalidation', self::now());
+            return;
+        } else if ($lastinvalidation == self::now()) {
+            // We've already invalidated during this request.
+            return;
+        }
+
+        // Get the event invalidation cache.
+        $cache = self::make('core', 'eventinvalidation');
+        $events = $cache->get_many($this->definition->get_invalidation_events());
+        $todelete = array();
+        $purgeall = false;
+        // Iterate the returned data for the events.
+        foreach ($events as $event => $keys) {
+            if ($keys === false) {
+                // No data to be invalidated yet.
+                continue;
+            }
+            // Look at each key and check the timestamp.
+            foreach ($keys as $key => $timestamp) {
+                // If the timestamp of the event is more than or equal to the last invalidation (happened between the last
+                // invalidation and now)then we need to invaliate the key.
+                if ($timestamp >= $lastinvalidation) {
+                    if ($key === 'purged') {
+                        $purgeall = true;
+                        break;
+                    } else {
+                        $todelete[] = $key;
+                    }
+                }
+            }
+        }
+        if ($purgeall) {
+            $this->purge();
+        } else if (!empty($todelete)) {
+            $todelete = array_unique($todelete);
+            $this->delete_many($todelete);
+        }
+        // Set the time of the last invalidation.
+        if ($purgeall || !empty($todelete)) {
+            $this->set('lastinvalidation', self::now());
+        }
+    }
+
     /**
      * Retrieves the value for the given key from the cache.
      *
@@ -1214,54 +1273,7 @@ class cache_application extends cache implements cache_loader_with_locking {
             $this->requirelockingwrite = $definition->require_locking_write();
         }
 
-        if ($definition->has_invalidation_events()) {
+        $this->handle_invalidation_events();
-            $lastinvalidation = $this->get('lastinvalidation');
-            if ($lastinvalidation === false) {
-                // This is a new session, there won't be anything to invalidate. Set the time of the last invalidation and
-                // move on.
-                $this->set('lastinvalidation', cache::now());
-                return;
-            } else if ($lastinvalidation == cache::now()) {
-                // We've already invalidated during this request.
-                return;
-            }
-
-            // Get the event invalidation cache.
-            $cache = cache::make('core', 'eventinvalidation');
-            $events = $cache->get_many($definition->get_invalidation_events());
-            $todelete = array();
-            $purgeall = false;
-            // Iterate the returned data for the events.
-            foreach ($events as $event => $keys) {
-                if ($keys === false) {
-                    // No data to be invalidated yet.
-                    continue;
-                }
-                // Look at each key and check the timestamp.
-                foreach ($keys as $key => $timestamp) {
-                    // If the timestamp of the event is more than or equal to the last invalidation (happened between the last
-                    // invalidation and now)then we need to invaliate the key.
-                    if ($timestamp >= $lastinvalidation) {
-                        if ($key === 'purged') {
-                            $purgeall = true;
-                            break;
-                        } else {
-                            $todelete[] = $key;
-                        }
-                    }
-                }
-            }
-            if ($purgeall) {
-                $this->purge();
-            } else if (!empty($todelete)) {
-                $todelete = array_unique($todelete);
-                $this->delete_many($todelete);
-            }
-            // Set the time of the last invalidation.
-            if ($purgeall || !empty($todelete)) {
-                $this->set('lastinvalidation', cache::now());
-            }
-        }
     }
 
     /**
@@ -1615,54 +1627,7 @@ class cache_session extends cache {
         // This will trigger check tracked user. If this gets removed a call to that will need to be added here in its place.
         $this->set(self::LASTACCESS, cache::now());
 
-        if ($definition->has_invalidation_events()) {
+        $this->handle_invalidation_events();
-            $lastinvalidation = $this->get('lastsessioninvalidation');
-            if ($lastinvalidation === false) {
-                // This is a new session, there won't be anything to invalidate. Set the time of the last invalidation and
-                // move on.
-                $this->set('lastsessioninvalidation', cache::now());
-                return;
-            } else if ($lastinvalidation == cache::now()) {
-                // We've already invalidated during this request.
-                return;
-            }
-
-            // Get the event invalidation cache.
-            $cache = cache::make('core', 'eventinvalidation');
-            $events = $cache->get_many($definition->get_invalidation_events());
-            $todelete = array();
-            $purgeall = false;
-            // Iterate the returned data for the events.
-            foreach ($events as $event => $keys) {
-                if ($keys === false) {
-                    // No data to be invalidated yet.
-                    continue;
-                }
-                // Look at each key and check the timestamp.
-                foreach ($keys as $key => $timestamp) {
-                    // If the timestamp of the event is more than or equal to the last invalidation (happened between the last
-                    // invalidation and now)then we need to invaliate the key.
-                    if ($timestamp >= $lastinvalidation) {
-                        if ($key === 'purged') {
-                            $purgeall = true;
-                            break;
-                        } else {
-                            $todelete[] = $key;
-                        }
-                    }
-                }
-            }
-            if ($purgeall) {
-                $this->purge();
-            } else if (!empty($todelete)) {
-                $todelete = array_unique($todelete);
-                $this->delete_many($todelete);
-            }
-            // Set the time of the last invalidation.
-            if ($purgeall || !empty($todelete)) {
-                $this->set('lastsessioninvalidation', cache::now());
-            }
-        }
     }
 
     /**

cache/disabledlib.php

@@ -212,6 +212,9 @@ class cache_factory_disabled extends cache_factory {
      * @return cache_application|cache_session|cache_request
      */
     public function create_cache_from_definition($component, $area, array $identifiers = array(), $unused = null) {
+        // Regular cache definitions are cached inside create_definition().  This is not the case for disabledlib.php
+        // definitions as they use load_adhoc().  They are built as a new object on each call.
+        // We do not need to clone the definition because we know it's new.
         $definition = $this->create_definition($component, $area);
         $definition->set_identifiers($identifiers);
         $cache = $this->create_cache($definition);
@@ -233,7 +236,10 @@ class cache_factory_disabled extends cache_factory {
      * @return cache_application|cache_session|cache_request
      */
     public function create_cache_from_params($mode, $component, $area, array $identifiers = array(), array $options = array()) {
-        $definition = cache_definition::load_adhoc($mode, $component, $area);
+        // Regular cache definitions are cached inside create_definition().  This is not the case for disabledlib.php
+        // definitions as they use load_adhoc().  They are built as a new object on each call.
+        // We do not need to clone the definition because we know it's new.
+        $definition = cache_definition::load_adhoc($mode, $component, $area, $options);
         $definition->set_identifiers($identifiers);
         $cache = $this->create_cache($definition);
         return $cache;

cache/tests/cache_test.php

@@ -188,7 +188,9 @@ class core_cache_testcase extends advanced_testcase {
     }
 
     /**
-     * Tests set_identifiers resets identifiers and static cache
+     * Tests set_identifiers fails post cache creation.
+     *
+     * set_identifiers cannot be called after initial cache instantiation, as you need to create a difference cache.
      */
     public function test_set_identifiers() {
         $instance = cache_config_testing::instance();
@@ -204,16 +206,8 @@ class core_cache_testcase extends advanced_testcase {
         $this->assertTrue($cache->set('contest', 'test data 1'));
         $this->assertEquals('test data 1', $cache->get('contest'));
 
+        $this->expectException('coding_exception');
         $cache->set_identifiers(array());
-        $this->assertFalse($cache->get('contest'));
-        $this->assertTrue($cache->set('contest', 'empty ident'));
-        $this->assertEquals('empty ident', $cache->get('contest'));
-
-        $cache->set_identifiers(array('area'));
-        $this->assertEquals('test data 1', $cache->get('contest'));
-
-        $cache->set_identifiers(array());
-        $this->assertEquals('empty ident', $cache->get('contest'));
     }
 
     /**
@@ -2022,6 +2016,16 @@ class core_cache_testcase extends advanced_testcase {
         $this->assertEquals('b', $returnedinstance2->name);
     }
 
+    public function test_identifiers_have_separate_caches() {
+        $cachepg = cache::make('core', 'databasemeta', array('dbfamily' => 'pgsql'));
+        $cachepg->set(1, 'here');
+        $cachemy = cache::make('core', 'databasemeta', array('dbfamily' => 'mysql'));
+        $cachemy->set(2, 'there');
+        $this->assertEquals('here', $cachepg->get(1));
+        $this->assertEquals('there', $cachemy->get(2));
+        $this->assertFalse($cachemy->get(1));
+    }
+
     public function test_performance_debug() {
         global $CFG;
         $this->resetAfterTest(true);

cache/upgrade.txt

@@ -1,6 +1,14 @@
 This files describes API changes in /cache/stores/* - cache store plugins.
 Information provided here is intended especially for developers.
 
+=== 3.3 ===
+* Identifiers and invalidation events have been explictly been marked as incompatible and will
+  throw a coding exception. Unexpected results would have occurred if the previous behaviour was attempted.
+* Identifiers are now part of loaded caches, so identifiers can only be set at cache::make()
+  a coding_exception will be thrown if attempts are made at other times.
+  Multiple calls to cache::make with different identifiers will produce 2 caches instead of changing the
+  keyspace of a single cache.
+
 === 3.2 ===
 * The following methods have been finally deprecated and should no longer be used.
   - cache_definition::should_be_persistent()

lib/dml/moodle_database.php

@@ -124,6 +124,9 @@ abstract class moodle_database {
     /** @var cache_application for column info */
     protected $metacache;
 
+    /** @var cache_request for column info on temp tables */
+    protected $metacachetemp;
+
     /** @var bool flag marking database instance as disposed */
     protected $disposed;
 
@@ -332,13 +335,14 @@ abstract class moodle_database {
     /**
      * Handle the creation and caching of the databasemeta information for all databases.
      *
-     * TODO MDL-53267 impelement caching of cache::make() results when it's safe to do so.
-     *
      * @return cache_application The databasemeta cachestore to complete operations on.
      */
     protected function get_metacache() {
-        $properties = array('dbfamily' => $this->get_dbfamily(), 'settings' => $this->get_settings_hash());
+        if (!isset($this->metacache)) {
-        return cache::make('core', 'databasemeta', $properties);
+            $properties = array('dbfamily' => $this->get_dbfamily(), 'settings' => $this->get_settings_hash());
+            $this->metacache = cache::make('core', 'databasemeta', $properties);
+        }
+        return $this->metacache;
     }
 
     /**
@@ -347,9 +351,12 @@ abstract class moodle_database {
      * @return cache_application The temp_tables cachestore to complete operations on.
      */
     protected function get_temp_tables_cache() {
-        // Using connection data to prevent collisions when using the same temp table name with different db connections.
+        if (!isset($this->metacachetemp)) {
-        $properties = array('dbfamily' => $this->get_dbfamily(), 'settings' => $this->get_settings_hash());
+            // Using connection data to prevent collisions when using the same temp table name with different db connections.
-        return cache::make('core', 'temp_tables', $properties);
+            $properties = array('dbfamily' => $this->get_dbfamily(), 'settings' => $this->get_settings_hash());
+            $this->metacachetemp = cache::make('core', 'temp_tables', $properties);
+        }
+        return $this->metacachetemp;
     }
 
     /**