Merge pull request #4750 from cakephp/issue-4693-duplicate-load

Fix duplicate load in registrys

src/Core/ObjectRegistry.php

@@ -14,6 +14,8 @@
  */
 namespace Cake\Core;
 
+use RuntimeException;
+
 /**
  * Acts as a registry/factory for objects.
  *
@@ -65,9 +67,14 @@ abstract class ObjectRegistry {
  */
 	public function load($objectName, $config = []) {
 		list($plugin, $name) = pluginSplit($objectName);
-		if (isset($this->_loaded[$name])) {
+		$loaded = isset($this->_loaded[$name]);
+		if ($loaded && !empty($config)) {
+			$this->_checkDuplicate($name, $config);
+		}
+		if ($loaded) {
 			return $this->_loaded[$name];
 		}
+
 		if (is_array($config) && isset($config['className'])) {
 			$objectName = $config['className'];
 		}
@@ -82,6 +89,41 @@ abstract class ObjectRegistry {
 	}
 
 /**
+ * Check for duplicate object loading.
+ *
+ * If a duplicate is being loaded and has different configuration, that is
+ * bad and an exception will be raised.
+ *
+ * An exception is raised, as replacing the object will not update any
+ * references other objects may have. Additionally, simply updating the runtime
+ * configuration is not a good option as we may be missing important constructor
+ * logic dependent on the configuration.
+ *
+ * @param string $name The name of the alias in the registry.
+ * @param array $config The config data for the new instance.
+ * @return void
+ * @throws \RuntimeException When a duplicate is found.
+ */
+	protected function _checkDuplicate($name, $config) {
+		$existing = $this->_loaded[$name];
+		$msg = sprintf('The "%s" alias has already been loaded', $name);
+		$hasConfig = false;
+		if (method_exists($existing, 'config')) {
+			$hasConfig = true;
+		}
+		if (!$hasConfig) {
+			throw new RuntimeException($msg);
+		}
+		unset($config['enabled']);
+		if ($hasConfig && array_diff_assoc($config, $existing->config()) != []) {
+			$msg .= ' with the following config: ';
+			$msg .= var_export($this->{$name}->config(), true);
+			$msg .= ' which differs from ' . var_export($config, true);
+			throw new RuntimeException($msg);
+		}
+	}
+
+/**
  * Should resolve the classname for a given object type.
  *
  * @param string $class The class to resolve.

src/Core/StaticConfigTrait.php

@@ -107,7 +107,7 @@ trait StaticConfigTrait {
 		if (!isset(static::$_config[$config])) {
 			return false;
 		}
-		if (isset(static::$_registry) && isset(static::$_registry->{$config})) {
+		if (isset(static::$_registry)) {
 			static::$_registry->unload($config);
 		}
 		unset(static::$_config[$config]);

src/Log/Log.php

@@ -180,7 +180,9 @@ class Log {
 			if (isset($properties['engine'])) {
 				$properties['className'] = $properties['engine'];
 			}
-			static::$_registry->load($name, $properties);
+			if (!static::$_registry->loaded($name)) {
+				static::$_registry->load($name, $properties);
+			}
 		}
 	}
 

src/ORM/Table.php

@@ -34,6 +34,7 @@ use Cake\ORM\Exception\RecordNotFoundException;
 use Cake\ORM\Marshaller;
 use Cake\Utility\Inflector;
 use Cake\Validation\Validator;
+use RuntimeException;
 
 /**
  * Represents a single database table.
@@ -498,6 +499,7 @@ class Table implements RepositoryInterface, EventListener {
  * @param string $name The name of the behavior. Can be a short class reference.
  * @param array $options The options for the behavior to use.
  * @return void
+ * @throws \RuntimeException If a behavior is being reloaded.
  * @see \Cake\ORM\Behavior
  */
 	public function addBehavior($name, array $options = []) {

src/View/View.php

@@ -771,14 +771,15 @@ class View {
 /**
  * Loads a helper. Delegates to the `HelperRegistry::load()` to load the helper
  *
- * @param string $helperName Name of the helper to load.
+ * @param string $name Name of the helper to load.
  * @param array $config Settings for the helper
  * @return Helper a constructed helper object.
  * @see HelperRegistry::load()
  */
-	public function loadHelper($helperName, array $config = []) {
-		list(, $class) = pluginSplit($helperName);
-		return $this->{$class} = $this->helpers()->load($helperName, $config);
+	public function loadHelper($name, array $config = []) {
+		list(, $class) = pluginSplit($name);
+		$helpers = $this->helpers();
+		return $this->{$class} = $helpers->load($name, $config);
 	}
 
 /**

tests/TestCase/Controller/ControllerTest.php

@@ -812,7 +812,7 @@ class ControllerTest extends TestCase {
  *
  * @return void
  */
-	public function testAddComponent() {
+	public function testLoadComponent() {
 		$request = new Request('/');
 		$response = $this->getMock('Cake\Network\Response');
 
@@ -825,4 +825,24 @@ class ControllerTest extends TestCase {
 		$this->assertTrue(isset($registry->Paginator));
 	}
 
+/**
+ * Test adding a component that is a duplicate.
+ *
+ * @return void
+ */
+	public function testLoadComponentDuplicate() {
+		$request = new Request('/');
+		$response = $this->getMock('Cake\Network\Response');
+
+		$controller = new TestController($request, $response);
+		$this->assertNotEmpty($controller->loadComponent('Paginator'));
+		$this->assertNotEmpty($controller->loadComponent('Paginator'));
+		try {
+			$controller->loadComponent('Paginator', ['bad' => 'settings']);
+			$this->fail('No exception');
+		} catch (\RuntimeException $e) {
+			$this->assertContains('The "Paginator" alias has already been loaded', $e->getMessage());
+		}
+	}
+
 }

tests/TestCase/ORM/TableTest.php

@@ -1070,6 +1070,23 @@ class TableTest extends \Cake\TestSuite\TestCase {
 	}
 
 /**
+ * Test adding a behavior that is a duplicate.
+ *
+ * @return void
+ */
+	public function testAddBehaviorDuplicate() {
+		$table = new Table(['table' => 'articles']);
+		$this->assertNull($table->addBehavior('Sluggable', ['test' => 'value']));
+		$this->assertNull($table->addBehavior('Sluggable', ['test' => 'value']));
+		try {
+			$table->addBehavior('Sluggable', ['thing' => 'thing']);
+			$this->fail('No exception raised');
+		} catch (\RuntimeException $e) {
+			$this->assertContains('The "Sluggable" alias has already been loaded', $e->getMessage());
+		}
+	}
+
+/**
  * Test removing a behavior from a table.
  *
  * @return void

tests/TestCase/View/ViewTest.php

@@ -921,6 +921,23 @@ class ViewTest extends TestCase {
 	}
 
 /**
+ * Test loading helper when duplicate.
+ *
+ * @return void
+ */
+	public function testLoadHelperDuplicate() {
+		$View = new View();
+
+		$this->assertNotEmpty($View->loadHelper('Html', ['foo' => 'bar']));
+		try {
+			$View->loadHelper('Html', ['test' => 'value']);
+			$this->fail('No exception');
+		} catch (\RuntimeException $e) {
+			$this->assertContains('The "Html" alias has already been loaded', $e->getMessage());
+		}
+	}
+
+/**
  * Test loadHelpers method
  *
  * @return void