FIX ConfigManifest regenerating every request if variantKeySpec is an empty array()

core/manifest/ConfigManifest.php

@@ -23,7 +23,7 @@ class SS_ConfigManifest {
 	  * the current environment.
 	  * @var array
 	  */
-	protected $variantKeySpec = array();
+	protected $variantKeySpec = false;
 
 	/**
 	 * All the _config.php files. Need to be included every request & can't be cached. Not variant specific.
@@ -88,10 +88,7 @@ class SS_ConfigManifest {
 		$this->includeTests = $includeTests;
 
 		// Get the Zend Cache to load/store cache into
-		$this->cache = SS_Cache::factory('SS_Configuration', 'Core', array(
+		$this->cache = $this->getCache();
-			'automatic_serialization' => true,
-			'lifetime' => null
-		));
 
 		// Unless we're forcing regen, try loading from cache
 		if (!$forceRegen) {
@@ -102,7 +99,7 @@ class SS_ConfigManifest {
 		}
 
 		// If we don't have a variantKeySpec (because we're forcing regen, or it just wasn't in the cache), generate it
-		if (!$this->variantKeySpec) {
+		if (false === $this->variantKeySpec) {
 			$this->regenerate($includeTests);
 		}
 
@@ -110,6 +107,18 @@ class SS_ConfigManifest {
 		$this->buildYamlConfigVariant();
 	}
 
+	/**
+	 * Provides a hook for mock unit tests despite no DI
+	 * @return Zend_Cache_Frontend
+	 */
+	protected function getCache()
+	{
+		return SS_Cache::factory('SS_Configuration', 'Core', array(
+			'automatic_serialization' => true,
+			'lifetime' => null
+		));
+	}
+
 	/**
 	 * Register a callback to be called whenever the calculated merged config changes
 	 *

tests/core/manifest/ConfigManifestTest.php

@@ -8,6 +8,11 @@ class ConfigManifestTest_ConfigManifestAccess extends SS_ConfigManifest {
 
 class ConfigManifestTest extends SapphireTest {
 
+	/**
+	 * This is a helper method for getting a new manifest
+	 * @param $name
+	 * @return any
+	 */
 	protected function getConfigFixtureValue($name) {
 		$manifest = new SS_ConfigManifest(dirname(__FILE__).'/fixtures/configmanifest', true, true);
 		return $manifest->get('ConfigManifestTest', $name);
@@ -20,16 +25,140 @@ class ConfigManifestTest extends SapphireTest {
 		return sprintf('Reference path "%s" failed to parse correctly', $path);
 	}
 
+	/**
+	 * A helper method to return a mock of the cache in order to test expectations and reduce dependency
+	 * @return Zend_Cache_Core
+	 */
+	protected function getCacheMock() {
+		return $this->getMock(
+			'Zend_Cache_Core',
+			array('load', 'save'),
+			array(),
+			'',
+			false
+		);
+	}
+
+	/**
+	 * A helper method to return a mock of the manifest in order to test expectations and reduce dependency
+	 * @param $methods
+	 * @return SS_ConfigManifest
+	 */
+	protected function getManifestMock($methods) {
+		return $this->getMock(
+			'SS_ConfigManifest',
+			$methods,
+			array(), // no constructor arguments
+			'', // default
+			false // don't call the constructor
+		);
+	}
+
+	/**
+	 * Test the caching functionality when we are forcing regeneration
+	 *
+	 * 1. Test that regenerate is called in the default case and that cache->load isn't
+	 * 2. Test that save is called correctly after the regeneration
+	 */
+	public function testCachingForceRegeneration() {
+		// Test that regenerate is called correctly.
+		$manifest = $this->getManifestMock(array('getCache', 'regenerate', 'buildYamlConfigVariant'));
+
+		$manifest->expects($this->once()) // regenerate should be called once
+			->method('regenerate')
+			->with($this->equalTo(true)); // includeTests = true
+
+		// Set up a cache where we expect load to never be called
+		$cache = $this->getCacheMock();
+		$cache->expects($this->never())
+			->method('load');
+
+		$manifest->expects($this->any())
+			->method('getCache')
+			->will($this->returnValue($cache));
+
+		$manifest->__construct(dirname(__FILE__).'/fixtures/configmanifest', true, true);
+
+		// Test that save is called correctly
+		$manifest = $this->getManifestMock(array('getCache'));
+
+		$cache = $this->getCacheMock();
+		$cache->expects($this->atLeastOnce())
+			->method('save');
+
+		$manifest->expects($this->any())
+			->method('getCache')
+			->will($this->returnValue($cache));
+
+		$manifest->__construct(dirname(__FILE__).'/fixtures/configmanifest', true, true);
+	}
+
+	/**
+	 * Test the caching functionality when we are not forcing regeneration
+	 *
+	 * 1. Test that load is called
+	 * 2. Test the regenerate is called when the cache is unprimed
+	 * 3. Test that when there is a value in the cache regenerate isn't called
+	 */
+	public function testCachingNotForceRegeneration() {
+		// Test that load is called
+		$manifest = $this->getManifestMock(array('getCache', 'regenerate', 'buildYamlConfigVariant'));
+
+		// Load should be called twice
+		$cache = $this->getCacheMock();
+		$cache->expects($this->exactly(2))
+			->method('load');
+
+		$manifest->expects($this->any())
+			->method('getCache')
+			->will($this->returnValue($cache));
+
+		$manifest->__construct(dirname(__FILE__).'/fixtures/configmanifest', true, false);
+
+
+		// Now test that regenerate is called because the cache is unprimed
+		$manifest = $this->getManifestMock(array('getCache', 'regenerate', 'buildYamlConfigVariant'));
+
+		$cache = $this->getCacheMock();
+		$cache->expects($this->exactly(2))
+			->method('load')
+			->will($this->onConsecutiveCalls(false, false));
+
+		$manifest->expects($this->any())
+			->method('getCache')
+			->will($this->returnValue($cache));
+
+		$manifest->expects($this->once())
+			->method('regenerate')
+			->with($this->equalTo(false)); //includeTests = false
+
+		$manifest->__construct(dirname(__FILE__).'/fixtures/configmanifest', false, false);
+
+		// Now test that when there is a value in the cache that regenerate isn't called
+		$manifest = $this->getManifestMock(array('getCache', 'regenerate', 'buildYamlConfigVariant'));
+
+		$cache = $this->getCacheMock();
+		$cache->expects($this->exactly(2))
+			->method('load')
+			->will($this->onConsecutiveCalls(array(), array()));
+
+		$manifest->expects($this->any())
+			->method('getCache')
+			->will($this->returnValue($cache));
+
+		$manifest->expects($this->never())
+			->method('regenerate');
+
+		$manifest->__construct(dirname(__FILE__).'/fixtures/configmanifest', false, false);
+	}
+
 	/**
 	 * This test checks the processing of before and after reference paths (module-name/filename#fragment)
 	 * This method uses fixture/configmanifest/mysite/_config/addyamlconfigfile.yml as a fixture
 	 */
 	public function testAddYAMLConfigFileReferencePathParsing() {
 		// Use a mock to avoid testing unrelated functionality
-		$manifest = $this->getMockBuilder('SS_ConfigManifest')
+		$manifest = $this->getManifestMock(array('addModule'));
-			->disableOriginalConstructor()
-			->setMethods(array('addModule'))
-			->getMock();
 
 		// This tests that the addModule method is called with the correct value
 		$manifest->expects($this->once())