From f074fd83fa161e559d6b45e1951571dd0eaa9556 Mon Sep 17 00:00:00 2001 From: mark_story Date: Fri, 16 Aug 2013 20:36:58 -0400 Subject: [PATCH] Expand abilities of Cache::config() Allow various types of configuration to be provided. The new variants allow for simpler dependency injection and still afford configuration files. Reconfiguring an adapter is now not allowed, as the configuration changes were never propagated to the adapter. This results in a silent failure that is hard to detect. Move/remove tests around to make sense with the new API's --- lib/Cake/Cache/Cache.php | 60 +++-- lib/Cake/Cache/CacheRegistry.php | 7 +- lib/Cake/Test/TestCase/Cache/CacheTest.php | 228 ++++++++---------- .../TestCase/Cache/Engine/FileEngineTest.php | 25 +- 4 files changed, 173 insertions(+), 147 deletions(-) diff --git a/lib/Cake/Cache/Cache.php b/lib/Cake/Cache/Cache.php index 133de719bc2..0ab340e8bc7 100644 --- a/lib/Cake/Cache/Cache.php +++ b/lib/Cake/Cache/Cache.php @@ -125,25 +125,61 @@ class Cache { /** * This method can be used to define cache adapters for an application - * during the bootstrapping process. You can use this method to add new cache adapters - * at runtime as well. New cache configurations will be constructed upon the next write. + * or read existing configuration. * * To change an adapter's configuration at runtime, first drop the adapter and then * reconfigure it. * * Adapters will not be constructed until the first operation is done. * + * ### Usage + * + * Reading config data back: + * + * `Cache::config('default');` + * + * Setting a cache engine up. + * + * `Cache::config('default', $settings);` + * + * Injecting a constructed adapter in: + * + * `Cache::config('default', $instance);` + * + * Using a factory function to get an adapter: + * + * `Cache::config('default', function () { return new FileEngine(); });` + * + * Configure multiple adapters at once: + * + * `Cache::config($arrayOfConfig);` + * * @param string|array $key The name of the cache config, or an array of multiple configs. * @param array $config An array of name => config data for adapter. - * @return void + * @return mixed null when adding configuration and an array of configuration data when reading. + * @throws Cake\Error\Exception When trying to modify an existing config. */ public static function config($key, $config = null) { - if ($config !== null && is_string($key)) { - static::$_config[$key] = $config; + // Read config. + if ($config === null && is_string($key)) { + return isset(static::$_config[$key]) ? static::$_config[$key] : null; + } + if ($config === null && is_array($key)) { + foreach ($key as $name => $settings) { + static::config($name, $settings); + } return; } - - static::$_config = array_merge(static::$_config, $key); + if (isset(static::$_config[$key])) { + throw new Error\Exception(__d('cake_dev', 'Cannot reconfigure existing adapter "%s"', $key)); + } + if (is_object($config)) { + $config = ['className' => $config]; + } + if (isset($config['engine']) && empty($config['className'])) { + $config['className'] = $config['engine']; + } + static::$_config[$key] = $config; } /** @@ -156,13 +192,11 @@ protected static function _buildEngine($name) { if (empty(static::$_registry)) { static::$_registry = new CacheRegistry(); } - if (empty(static::$_config[$name]['engine'])) { + if (empty(static::$_config[$name]['className'])) { throw new Error\Exception(__d('cake_dev', 'The "%s" cache configuration does not exist.', $name)); } $config = static::$_config[$name]; - $config['className'] = $config['engine']; - static::$_registry->load($name, $config); if (!empty($config['groups'])) { @@ -193,11 +227,9 @@ public static function configured() { * @return boolean success of the removal, returns false when the config does not exist. */ public static function drop($config) { - if (!isset(static::$_registry->{$config})) { - return false; + if (isset(static::$_registry->{$config})) { + static::$_registry->unload($config); } - - static::$_registry->unload($config); unset(static::$_config[$config], static::$_restore[$config]); return true; } diff --git a/lib/Cake/Cache/CacheRegistry.php b/lib/Cake/Cache/CacheRegistry.php index f3e730a7373..adeedd42fc4 100644 --- a/lib/Cake/Cache/CacheRegistry.php +++ b/lib/Cake/Cache/CacheRegistry.php @@ -65,11 +65,14 @@ protected function _throwMissingClassError($class, $plugin) { * the correct interface. */ protected function _create($class, $settings) { - if (is_object($class)) { + $isObject = is_object($class); + if ($isObject && $class instanceof \Closure) { + $instance = $class(); + } elseif ($isObject) { $instance = $class; } - unset($settings['engine'], $settings['className']); + unset($settings['className']); if (!isset($instance)) { $instance = new $class($settings); } diff --git a/lib/Cake/Test/TestCase/Cache/CacheTest.php b/lib/Cake/Test/TestCase/Cache/CacheTest.php index 0c54659564d..d4a6c93dffa 100644 --- a/lib/Cake/Test/TestCase/Cache/CacheTest.php +++ b/lib/Cake/Test/TestCase/Cache/CacheTest.php @@ -15,6 +15,7 @@ namespace Cake\Test\TestCase\Cache; use Cake\Cache\Cache; +use Cake\Cache\Engine\FileEngine; use Cake\Core\App; use Cake\Core\Configure; use Cake\Core\Plugin; @@ -35,40 +36,24 @@ class CacheTest extends TestCase { public function setUp() { parent::setUp(); Configure::write('Cache.disable', false); - - Cache::drop('tests'); - Cache::config('default', [ - 'engine' => 'File', - 'path' => TMP . 'tests' - ]); } /** - * testEngine method + * tearDown method * * @return void */ - public function testEngine() { - $settings = [ - 'engine' => 'File', - 'path' => TMP . 'tests', - 'prefix' => 'cake_test_' - ]; - Cache::config('tests', $settings); - $engine = Cache::engine('tests'); - $this->assertInstanceOf('Cake\Cache\Engine\FileEngine', $engine); + public function tearDown() { + parent::tearDown(); + Cache::drop('tests'); } -/** - * testConfigInvalidEngine method - * - * @expectedException Cake\Error\Exception - * @return void - */ - public function testConfigInvalidEngine() { - $settings = array('engine' => 'Imaginary'); - Cache::config('tests', $settings); - Cache::engine('tests'); + protected function _configCache() { + Cache::config('tests', [ + 'engine' => 'File', + 'path' => TMP, + 'prefix' => 'test_' + ]); } /** @@ -78,10 +63,7 @@ public function testConfigInvalidEngine() { */ public function testNonFatalErrorsWithCachedisable() { Configure::write('Cache.disable', true); - Cache::config('tests', [ - 'engine' => 'File', - 'path' => TMP, 'prefix' => 'error_test_' - ]); + $this->_configCache(); Cache::write('no_save', 'Noooo!', 'tests'); Cache::read('no_save', 'tests'); @@ -118,26 +100,6 @@ public function testConfigWithLibAndPluginEngines() { Plugin::unload(); } -/** - * Test reading from a config that is undefined. - * - * @expectedException PHPUnit_Framework_Error_Warning - * @return void - */ - public function testInvalidConfig() { - // In debug mode it would auto create the folder. - Configure::write('debug', 0); - - Cache::config('tests', array( - 'engine' => 'File', - 'duration' => '+1 year', - 'prefix' => 'testing_invalid_', - 'path' => 'data/', - 'serialize' => true, - )); - Cache::read('Test', 'tests'); - } - /** * Test write from a config that is undefined. * @@ -168,58 +130,97 @@ public function testDecrementNonExistingConfig() { $this->assertFalse(Cache::decrement('key', 1, 'totally fake')); } +/** + * Data provider for valid config data sets. + * + * @return array + */ + public static function configProvider() { + return [ + 'Array of data using engine key.' => [[ + 'engine' => 'File', + 'path' => TMP . 'tests', + 'prefix' => 'cake_test_' + ]], + 'Array of data using classname key.' => [[ + 'className' => 'File', + 'path' => TMP . 'tests', + 'prefix' => 'cake_test_' + ]], + 'Direct instance' => [new FileEngine()], + 'Closure factory' => [function () { + return new FileEngine(); + }], + ]; + } +/** + * testConfig method + * + * @dataProvider configProvider + * @return void + */ + public function testConfigVariants($settings) { + $this->assertNotContains('test', Cache::configured(), 'test config should not exist.'); + Cache::config('tests', $settings); + + $engine = Cache::engine('tests'); + $this->assertInstanceOf('Cake\Cache\Engine\FileEngine', $engine); + $this->assertContains('tests', Cache::configured()); + } + +/** + * testConfigInvalidEngine method + * + * @expectedException Cake\Error\Exception + * @return void + */ + public function testConfigInvalidEngine() { + $settings = array('engine' => 'Imaginary'); + Cache::config('test', $settings); + Cache::engine('test'); + } + /** * test that trying to configure classes that don't extend CacheEngine fail. * * @expectedException Cake\Error\Exception * @return void */ - public function testAttemptingToConfigureANonCacheEngineClass() { + public function testConfigInvalidObject() { $this->getMock('\StdClass', array(), array(), 'RubbishEngine'); - Cache::config('tests', array( + Cache::config('test', array( 'engine' => '\RubbishEngine' )); Cache::engine('tests'); } /** - * Test that engine() can be used to inject instances. + * Ensure you cannot reconfigure a cache adapter. * + * @expectedException Cake\Error\Exception * @return void */ - public function testSetEngineValid() { - $engine = $this->getMockForAbstractClass('\Cake\Cache\CacheEngine'); - Cache::config('test', ['engine' => $engine]); - $this->assertSame($engine, Cache::engine('test')); + public function testConfigErrorOnReconfigure() { + Cache::config('tests', ['engine' => 'File', 'path' => TMP]); + Cache::config('tests', ['engine' => 'Apc']); } /** - * test that calling config() sets the 'default' configuration up. + * Test reading configuration. * * @return void */ - public function testConfigSettingDefaultConfigKey() { - Cache::config('tests', [ + public function testConfigRead() { + $settings = [ 'engine' => 'File', - 'prefix' => 'tests_' - ]); - - Cache::write('value_one', 'I am cached', 'tests'); - $result = Cache::read('value_one', 'tests'); - $this->assertEquals('I am cached', $result); - - $result = Cache::read('value_one'); - $this->assertEquals(null, $result); - - Cache::write('value_one', 'I am in default config!'); - $result = Cache::read('value_one'); - $this->assertEquals('I am in default config!', $result); - - $result = Cache::read('value_one', 'tests'); - $this->assertEquals('I am cached', $result); - - Cache::delete('value_one', 'tests'); - Cache::delete('value_one', 'default'); + 'path' => TMP, + 'prefix' => 'cake_' + ]; + Cache::config('tests', $settings); + $expected = $settings; + $expected['className'] = $settings['engine']; + unset($settings['engine']); + $this->assertEquals($expected, Cache::config('tests')); } /** @@ -307,11 +308,7 @@ public function testDrop() { Configure::write('App.namespace', 'TestApp'); $result = Cache::drop('some_config_that_does_not_exist'); - $this->assertFalse($result); - - Cache::engine('default'); - $result = Cache::drop('default'); - $this->assertTrue($result, 'Built engines should be dropped'); + $this->assertTrue($result, 'Drop should succeed.'); Cache::config('unconfigTest', [ 'engine' => 'TestAppCache' @@ -320,56 +317,30 @@ public function testDrop() { 'TestApp\Cache\Engine\TestAppCacheEngine', Cache::engine('unconfigTest') ); - $this->assertTrue(Cache::drop('unconfigTest')); } -/** - * Test that dropping a cache config refreshes its configuration and - * creates a new instance. - * - * @return void - */ - public function testDropChangeConfig() { - Cache::config('tests', [ - 'engine' => 'File', - ]); - $result = Cache::engine('tests'); - $settings = Cache::settings('tests'); - - $this->assertEquals(CACHE, $settings['path']); - $id = spl_object_hash($result); - - Cache::drop('tests'); - - Cache::config('tests', [ - 'engine' => 'File', - 'extra' => 'value' - ]); - $result = Cache::engine('tests'); - $this->assertNotEquals($id, spl_object_hash($result)); - } - /** * testWriteEmptyValues method * * @return void */ public function testWriteEmptyValues() { - Cache::write('App.falseTest', false); - $this->assertSame(Cache::read('App.falseTest'), false); + $this->_configCache(); + Cache::write('App.falseTest', false, 'tests'); + $this->assertSame(Cache::read('App.falseTest', 'tests'), false); - Cache::write('App.trueTest', true); - $this->assertSame(Cache::read('App.trueTest'), true); + Cache::write('App.trueTest', true, 'tests'); + $this->assertSame(Cache::read('App.trueTest', 'tests'), true); - Cache::write('App.nullTest', null); - $this->assertSame(Cache::read('App.nullTest'), null); + Cache::write('App.nullTest', null, 'tests'); + $this->assertSame(Cache::read('App.nullTest', 'tests'), null); - Cache::write('App.zeroTest', 0); - $this->assertSame(Cache::read('App.zeroTest'), 0); + Cache::write('App.zeroTest', 0, 'tests'); + $this->assertSame(Cache::read('App.zeroTest', 'tests'), 0); - Cache::write('App.zeroTest2', '0'); - $this->assertSame(Cache::read('App.zeroTest2'), '0'); + Cache::write('App.zeroTest2', '0', 'tests'); + $this->assertSame(Cache::read('App.zeroTest2', 'tests'), '0'); } /** @@ -452,22 +423,21 @@ public function testCacheDisable() { * @return void */ public function testSet() { - $_cacheSet = Cache::set(); + $this->_configCache(); - Cache::set(array('duration' => '+1 year')); - $data = Cache::read('test_cache'); + Cache::set(array('duration' => '+1 year'), 'tests'); + $data = Cache::read('test_cache', 'tests'); $this->assertFalse($data); $data = 'this is just a simple test of the cache system'; - $write = Cache::write('test_cache', $data); + $write = Cache::write('test_cache', $data, 'tests'); $this->assertTrue($write); - Cache::set(array('duration' => '+1 year')); - $data = Cache::read('test_cache'); + Cache::set(array('duration' => '+1 year'), 'tests'); + $data = Cache::read('test_cache', 'tests'); $this->assertEquals('this is just a simple test of the cache system', $data); - Cache::delete('test_cache'); - Cache::set($_cacheSet); + Cache::delete('test_cache', 'tests'); } /** diff --git a/lib/Cake/Test/TestCase/Cache/Engine/FileEngineTest.php b/lib/Cake/Test/TestCase/Cache/Engine/FileEngineTest.php index f02e5d01f0f..68bd26b8853 100644 --- a/lib/Cake/Test/TestCase/Cache/Engine/FileEngineTest.php +++ b/lib/Cake/Test/TestCase/Cache/Engine/FileEngineTest.php @@ -371,12 +371,33 @@ public function testWriteQuotedString() { public function testPathDoesNotExist() { $this->skipIf(is_dir(TMP . 'tests' . DS . 'autocreate'), 'Cannot run if test directory exists.'); - Cache::config('autocreate', array( + Cache::config('test', array( 'engine' => 'File', 'path' => TMP . 'tests' . DS . 'autocreate' )); + Cache::read('Test', 'test'); - Cache::drop('autocreate'); + Cache::drop('test'); + unlink(TMP . 'tests' . DS . 'autocreate'); + } + +/** + * Test that under debug 0 directories do not get made. + * + * @expectedException PHPUnit_Framework_Error_Warning + * @return void + */ + public function testPathDoesNotExistDebugOff() { + $this->skipIf(is_dir(TMP . 'tests/autocreate'), 'Cannot run if test directory exists.'); + Configure::write('debug', 0); + + Cache::config('test', array( + 'engine' => 'File', + 'duration' => '+1 year', + 'prefix' => 'testing_invalid_', + 'path' => TMP . 'tests/autocreate', + )); + Cache::read('Test', 'test'); } /**