Fixes #4454

This fixes a problem where using duplicate aliases in the joinable
associations chain would end up confusing the ResultSet groupResult
method. The problem cause was that EagerLoader assumed that collision
would occur in certain order.

Instead of relying on the order, a collisions map is generated and
then traversed so that fetching strategies can be corrected. A downside
of this is that it relies on keeping references to arrays.

A future enhancement to this fix would be to convert the arrays to objects
so that the reference handling happen implicitly.

src/ORM/EagerLoader.php

@@ -160,14 +160,15 @@ public function normalized(Table $repository) {
 				$contain = (array)$this->_containments;
 				break;
 			}
-			$contain[$alias] = $this->_normalizeContain(
+			$contain[$alias] =& $this->_normalizeContain(
 				$repository,
 				$alias,
 				$options,
 				['root' => null]
 			);
 		}
 
+		$this->_fixStrategies();
 		return $this->_normalized = $contain;
 	}
 
@@ -303,7 +304,7 @@ public function externalAssociations(Table $repository) {
  * @return array normalized associations
  * @throws \InvalidArgumentException When containments refer to associations that do not exist.
  */
-	protected function _normalizeContain(Table $parent, $alias, $options, $paths) {
+	protected function &_normalizeContain(Table $parent, $alias, $options, $paths) {
 		$defaults = $this->_containOptions;
 		$instance = $parent->association($alias);
 		if (!$instance) {
@@ -327,35 +328,58 @@ protected function _normalizeContain(Table $parent, $alias, $options, $paths) {
 			'propertyPath' => trim($paths['propertyPath'], '.')
 		];
 		$config['canBeJoined'] = $instance->canBeJoined($config['config']);
-		$config = $this->_correctStrategy($alias, $config, $paths['root']);
 
 		if ($config['canBeJoined']) {
-			$this->_aliasList[$paths['root']][$alias] = true;
+			$this->_aliasList[$paths['root']][$alias][] =& $config;
 		} else {
 			$paths['root'] = $config['aliasPath'];
 		}
 
 		foreach ($extra as $t => $assoc) {
-			$config['associations'][$t] = $this->_normalizeContain($table, $t, $assoc, $paths);
+			$config['associations'][$t] =& $this->_normalizeContain($table, $t, $assoc, $paths);
 		}
 
 		return $config;
 	}
 
+/**
+ * Iterates over the joinable aliases list and corrects the fetching strategies
+ * in order to avoid aliases collision in the generated queries.
+ *
+ * This function operates on the array references that were generated by the
+ * _normalizeContain() function.
+ *
+ * @return void
+ */
+	protected function _fixStrategies() {
+		foreach ($this->_aliasList as &$aliases) {
+			foreach ($aliases as $alias => &$configs) {
+				if (count($configs) < 2) {
+					continue;
+				}
+				foreach ($configs as &$config) {
+					if (strpos($config['aliasPath'], '.')) {
+						$this->_correctStrategy($config, $alias);
+					}
+				}
+			}
+		}
+	}
+
 /**
  * Changes the association fetching strategy if required because of duplicate
  * under the same direct associations chain
  *
- * @param string $alias the name of the association to evaluate
+ * This function modifies the $config variable
+ *
  * @param array $config The association config
- * @param string $root A string representing the root association that started
- * the direct chain this alias is in
- * @return array The modified association config
+ * @param string $alias the name of the association to evaluate
+ * @return void
  * @throws \RuntimeException if a duplicate association in the same chain is detected
  * but is not possible to change the strategy due to conflicting settings
  */
-	protected function _correctStrategy($alias, $config, $root) {
-		if (!$config['canBeJoined'] || empty($this->_aliasList[$root][$alias])) {
+	protected function _correctStrategy(&$config, $alias) {
+		if (!$config['canBeJoined']) {
 			return $config;
 		}
 
@@ -368,8 +392,6 @@ protected function _correctStrategy($alias, $config, $root) {
 
 		$config['canBeJoined'] = false;
 		$config['config']['strategy'] = $config['instance']::STRATEGY_SELECT;
-
-		return $config;
 	}
 
 /**

tests/TestCase/ORM/QueryRegressionTest.php

@@ -401,8 +401,8 @@ public function testAssociationChainOrder() {
 			->first();
 
 		$this->assertEquals($resultA, $resultB);
-		$this->assertNotEmpty($resultA->user);
-		$this->assertNotEmpty($resultA->articles_tag->user);
+		$this->assertNotEmpty($resultA->author);
+		$this->assertNotEmpty($resultA->articles_tag->author);
 	}
 
 }