Merge pull request #5634 from cakephp/3.0-matching-fixes

Adding test to disprove #5584

src/ORM/EagerLoadable.php

@@ -0,0 +1,241 @@
+<?php
+/**
+ * CakePHP(tm) : Rapid Development Framework (http://cakephp.org)
+ * Copyright (c) Cake Software Foundation, Inc. (http://cakefoundation.org)
+ *
+ * Licensed under The MIT License
+ * For full copyright and license information, please see the LICENSE.txt
+ * Redistributions of files must retain the above copyright notice.
+ *
+ * @copyright     Copyright (c) Cake Software Foundation, Inc. (http://cakefoundation.org)
+ * @link          http://cakephp.org CakePHP(tm) Project
+ * @since         3.0.0
+ * @license       http://www.opensource.org/licenses/mit-license.php MIT License
+ */
+namespace Cake\ORM;
+
+/**
+ * Represents a single level in the associations tree to be eagerly loaded
+ * for a specific query. This contains all the information required to
+ * fetch the results from the database from an associations and all its children
+ * levels.
+ *
+ * @internal
+ */
+class EagerLoadable
+{
+
+    /**
+     * The name of the association to load.
+     *
+     * @var string
+     */
+    protected $_name;
+
+    /**
+     * A list of other associations to load from this level.
+     *
+     * @var array
+     */
+    protected $_associations = [];
+
+    /**
+     * The Association class instance to use for loading the records.
+     *
+     * @var \Cake\ORM\Association
+     */
+    protected $_instance;
+
+    /**
+     * A list of options to pass to the association object for loading
+     * the records.
+     *
+     * @var array
+     */
+    protected $_config = [];
+
+    /**
+     * A dotted separated string representing the path of associations
+     * that should be followed to fetch this level.
+     *
+     * @var string
+     */
+    protected $_aliasPath;
+
+    /**
+     * A dotted separated string representing the path of entity properties
+     * in which results for this level should be placed.
+     *
+     * @var string
+     */
+    protected $_propertyPath;
+
+    /**
+     * Whether or not this level can be fetched using a join.
+     *
+     * @var bool
+     */
+    protected $_canBeJoined = false;
+
+    /**
+     * Whether or not this level was meant for a "matching" fetch
+     * operation
+     *
+     * @var bool
+     */
+    protected $_forMatching;
+
+    /**
+     * Constructor. The $config parameted accepts the following array
+     * keys:
+     *
+     * - associations
+     * - instance
+     * - config
+     * - canBeJoined
+     * - aliasPath
+     * - propertyPath
+     * - forMatching
+     *
+     * The keys maps to the settable properties in this class.
+     *
+     * @param string $name The Association name.
+     * @param array $config The list of properties to set.
+     */
+    public function __construct($name, array $config = [])
+    {
+        $this->_name = $name;
+        $allowed = [
+            'associations', 'instance', 'config', 'canBeJoined',
+            'aliasPath', 'propertyPath', 'forMatching'
+        ];
+        foreach ($allowed as $property) {
+            if (isset($config[$property])) {
+                $this->{'_' . $property} = $config[$property];
+            }
+        }
+    }
+
+    /**
+     * Adds a new association to be loaded from this level.
+     *
+     * @param string $name The association name.
+     * @param \Cake\ORM\EagerLoadable $association The association to load.
+     * @return void
+     */
+    public function addAssociation($name, EagerLoadable $association)
+    {
+        $this->_associations[$name] = $association;
+    }
+
+    /**
+     * Returns the Association class instance to use for loading the records.
+     *
+     * @return array
+     */
+    public function associations()
+    {
+        return $this->_associations;
+    }
+
+    /**
+     * Gets the Association class instance to use for loading the records.
+     *
+     * @return \Cake\ORM\Association|null
+     */
+    public function instance()
+    {
+        return $this->_instance;
+    }
+
+    /**
+     * Gets a dot separated string representing the path of associations
+     * that should be followed to fetch this level.
+     *
+     * @return string|null
+     */
+    public function aliasPath()
+    {
+        return $this->_aliasPath;
+    }
+
+    /**
+     * Gets a dot separated string representing the path of entity properties
+     * in which results for this level should be placed.
+     *
+     * @return string|null
+     */
+    public function propertyPath()
+    {
+        return $this->_propertyPath;
+    }
+
+    /**
+     * Sets whether or not this level can be fetched using a join.
+     *
+     * If called with no arguments it returns the current value.
+     *
+     * @param bool|null $possible The value to set.
+     * @return bool|null
+     */
+    public function canBeJoined($possible = null)
+    {
+        if ($possible === null) {
+            return $this->_canBeJoined;
+        }
+        $this->_canBeJoined = $possible;
+    }
+
+    /**
+     * Sets the list of options to pass to the association object for loading
+     * the records.
+     *
+     * If called with no arguments it returns the current
+     * value.
+     *
+     * @param array|null $config The value to set.
+     * @return array|null
+     */
+    public function config(array $config = null)
+    {
+        if ($config === null) {
+            return $this->_config;
+        }
+        $this->_config = $config;
+    }
+
+    /**
+     * Gets whether or not this level was meant for a
+     * "matching" fetch operation.
+     *
+     * @return bool|null
+     */
+    public function forMatching()
+    {
+        return $this->_forMatching;
+    }
+
+    /**
+     * Returns a representation of this object that can be passed to
+     * Cake\ORM\EagerLoader::contain()
+     *
+     * @return array
+     */
+    public function asContainArray()
+    {
+        $associations = [];
+        foreach ($this->_associations as $assoc) {
+            $associations += $assoc->asContainArray();
+        }
+        $config = $this->_config;
+        if ($this->_forMatching !== null) {
+            $config = ['matching' => $this->_forMatching] + $config;
+        }
+        return [
+            $this->_name => [
+                'associations' => $associations,
+                'config' => $config
+            ]
+        ];
+    }
+}

src/ORM/EagerLoader.php

@@ -17,6 +17,7 @@ namespace Cake\ORM;
 use Cake\Database\Statement\BufferedStatement;
 use Cake\Database\Statement\CallbackStatement;
 use Cake\ORM\Association;
+use Cake\ORM\EagerLoadable;
 use Cake\ORM\Query;
 use Cake\ORM\Table;
 use Closure;
@@ -113,6 +114,7 @@ class EagerLoader
      * - matching: Whether to inform the association class that it should filter the
      *  main query by the results fetched by that class.
      * - joinType: For joinable associations, the SQL join type to use.
+     * - strategy: The loading strategy to use (join, select, subquery)
      *
      * @param array|string $associations list of table aliases to be queried.
      * When this method is called multiple times it will merge previous list with
@@ -176,6 +178,10 @@ class EagerLoader
      * loaded for a table. The normalized array will restructure the original array
      * by sorting all associations under one key and special options under another.
      *
+     * Each of the levels of the associations tree will converted to a Cake\ORM\EagerLoadable
+     * object, that contains all the information required for the association objects
+     * to load the information from the database.
+     *
      * Additionally it will set an 'instance' key per association containing the
      * association instance from the corresponding source table
      *
@@ -195,7 +201,7 @@ class EagerLoader
                 $contain = (array)$this->_containments;
                 break;
             }
-            $contain[$alias] =& $this->_normalizeContain(
+            $contain[$alias] = $this->_normalizeContain(
                 $repository,
                 $alias,
                 $options,
@@ -203,7 +209,6 @@ class EagerLoader
             );
         }
 
-        $this->_fixStrategies();
         return $this->_normalized = $contain;
     }
 
@@ -228,6 +233,12 @@ class EagerLoader
                 $options = [];
             }
 
+            if ($options instanceof EagerLoadable) {
+                $options = $options->asContainArray();
+                $table = key($options);
+                $options = current($options);
+            }
+
             if (isset($this->_containOptions[$table])) {
                 $pointer[$table] = $options;
                 continue;
@@ -279,23 +290,20 @@ class EagerLoader
             return;
         }
 
-        foreach ($this->attachableAssociations($repository) as $options) {
-            $config = $options['config'] + [
-                'aliasPath' => $options['aliasPath'],
-                'propertyPath' => $options['propertyPath'],
+        foreach ($this->attachableAssociations($repository) as $loadable) {
+            $config = $loadable->config() + [
+                'aliasPath' => $loadable->aliasPath(),
+                'propertyPath' => $loadable->propertyPath(),
                 'includeFields' => $includeFields
             ];
-            $options['instance']->attachTo($query, $config);
+            $loadable->instance()->attachTo($query, $config);
         }
     }
 
     /**
      * Returns an array with the associations that can be fetched using a single query,
      * the array keys are the association aliases and the values will contain an array
-     * with the following keys:
-     *
-     * - instance: the association object instance
-     * - config: the options set for fetching such association
+     * with Cake\ORM\EagerLoadable objects.
      *
      * @param \Cake\ORM\Table $repository The table containing the associations to be
      * attached
@@ -305,15 +313,13 @@ class EagerLoader
     {
         $contain = $this->normalized($repository);
         $matching = $this->_matching ? $this->_matching->normalized($repository) : [];
+        $this->_fixStrategies();
         return $this->_resolveJoins($contain, $matching);
     }
 
     /**
      * Returns an array with the associations that need to be fetched using a
-     * separate query, each array value will contain the following keys:
-     *
-     * - instance: the association object instance
-     * - config: the options set for fetching such association
+     * separate query, each array value will contain a Cake\ORM\EagerLoadable object.
      *
      * @param \Cake\ORM\Table $repository The table containing the associations
      * to be loaded
@@ -325,8 +331,7 @@ class EagerLoader
             return $this->_loadExternal;
         }
 
-        $contain = $this->normalized($repository);
-        $this->_resolveJoins($contain, []);
+        $this->attachableAssociations($repository);
         return $this->_loadExternal;
     }
 
@@ -344,7 +349,7 @@ class EagerLoader
      * @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);
@@ -369,18 +374,22 @@ class EagerLoader
             'propertyPath' => trim($paths['propertyPath'], '.')
         ];
         $config['canBeJoined'] = $instance->canBeJoined($config['config']);
+        $eagerLoadable = new EagerLoadable($alias, $config);
 
         if ($config['canBeJoined']) {
-            $this->_aliasList[$paths['root']][$alias][] =& $config;
+            $this->_aliasList[$paths['root']][$alias][] = $eagerLoadable;
         } else {
             $paths['root'] = $config['aliasPath'];
         }
 
         foreach ($extra as $t => $assoc) {
-            $config['associations'][$t] =& $this->_normalizeContain($table, $t, $assoc, $paths);
+            $eagerLoadable->addAssociation(
+                $t,
+                $this->_normalizeContain($table, $t, $assoc, $paths)
+            );
         }
 
-        return $config;
+        return $eagerLoadable;
     }
 
     /**
@@ -394,14 +403,14 @@ class EagerLoader
      */
     protected function _fixStrategies()
     {
-        foreach ($this->_aliasList as &$aliases) {
-            foreach ($aliases as $alias => &$configs) {
+        foreach ($this->_aliasList as $aliases) {
+            foreach ($aliases as $configs) {
                 if (count($configs) < 2) {
                     continue;
                 }
-                foreach ($configs as &$config) {
-                    if (strpos($config['aliasPath'], '.')) {
-                        $this->_correctStrategy($config, $alias);
+                foreach ($configs as $loadable) {
+                    if (strpos($loadable->aliasPath(), '.')) {
+                        $this->_correctStrategy($loadable);
                     }
                 }
             }
@@ -412,26 +421,23 @@ class EagerLoader
      * Changes the association fetching strategy if required because of duplicate
      * under the same direct associations chain
      *
-     * This function modifies the $config variable
-     *
-     * @param array &$config The association config
-     * @param string $alias the name of the association to evaluate
-     * @return void|array
-     * @throws \RuntimeException if a duplicate association in the same chain is detected
-     * but is not possible to change the strategy due to conflicting settings
+     * @param \Cake\ORM\EagerLoadable $loadable The association config
+     * @return void
      */
-    protected function _correctStrategy(&$config, $alias)
+    protected function _correctStrategy($loadable)
     {
-        $currentStrategy = isset($config['config']['strategy']) ?
-            $config['config']['strategy'] :
+        $config = $loadable->config();
+        $currentStrategy = isset($config['strategy']) ?
+            $config['strategy'] :
             'join';
 
-        if (!$config['canBeJoined'] || $currentStrategy !== 'join') {
-            return $config;
+        if (!$loadable->canBeJoined() || $currentStrategy !== 'join') {
+            return;
         }
 
-        $config['canBeJoined'] = false;
-        $config['config']['strategy'] = $config['instance']::STRATEGY_SELECT;
+        $config['strategy'] = Association::STRATEGY_SELECT;
+        $loadable->config($config);
+        $loadable->canBeJoined(false);
     }
 
     /**
@@ -445,19 +451,24 @@ class EagerLoader
     protected function _resolveJoins($associations, $matching = [])
     {
         $result = [];
-        foreach ($matching as $table => $options) {
-            $result[$table] = $options;
-            $result += $this->_resolveJoins($options['associations'], []);
+        foreach ($matching as $table => $loadable) {
+            $result[$table] = $loadable;
+            $result += $this->_resolveJoins($loadable->associations(), []);
         }
-        foreach ($associations as $table => $options) {
+        foreach ($associations as $table => $loadable) {
             $inMatching = isset($matching[$table]);
-            if (!$inMatching && $options['canBeJoined']) {
-                $result[$table] = $options;
-                $result += $this->_resolveJoins($options['associations'], $inMatching ? $mathching[$table] : []);
-            } else {
-                $options['canBeJoined'] = false;
-                $this->_loadExternal[] = $options;
+            if (!$inMatching && $loadable->canBeJoined()) {
+                $result[$table] = $loadable;
+                $result += $this->_resolveJoins($loadable->associations(), []);
+                continue;
             }
+
+            if ($inMatching) {
+                $this->_correctStrategy($loadable);
+            }
+
+            $loadable->canBeJoined(false);
+            $this->_loadExternal[] = $loadable;
         }
         return $result;
     }
@@ -481,21 +492,23 @@ class EagerLoader
         $driver = $query->connection()->driver();
         list($collected, $statement) = $this->_collectKeys($external, $query, $statement);
         foreach ($external as $meta) {
-            $contain = $meta['associations'];
-            $alias = $meta['instance']->source()->alias();
+            $contain = $meta->associations();
+            $instance = $meta->instance();
+            $config = $meta->config();
+            $alias = $instance->source()->alias();
 
-            $requiresKeys = $meta['instance']->requiresKeys($meta['config']);
+            $requiresKeys = $instance->requiresKeys($config);
             if ($requiresKeys && empty($collected[$alias])) {
                 continue;
             }
 
             $keys = isset($collected[$alias]) ? $collected[$alias] : null;
-            $f = $meta['instance']->eagerLoader(
-                $meta['config'] + [
+            $f = $instance->eagerLoader(
+                $config + [
                     'query' => $query,
                     'contain' => $contain,
                     'keys' => $keys,
-                    'nestKey' => $meta['aliasPath']
+                    'nestKey' => $meta->aliasPath()
                 ]
             );
             $statement = new CallbackStatement($statement, $driver, $f);
@@ -528,16 +541,20 @@ class EagerLoader
 
         $visitor = function ($level, $matching = false) use (&$visitor, &$map) {
             foreach ($level as $assoc => $meta) {
+                $canBeJoined = $meta->canBeJoined();
+                $instance = $meta->instance();
+                $associations = $meta->associations();
+                $forMatching = $meta->forMatching();
                 $map[] = [
                     'alias' => $assoc,
-                    'instance' => $meta['instance'],
-                    'canBeJoined' => $meta['canBeJoined'],
-                    'entityClass' => $meta['instance']->target()->entityClass(),
-                    'nestKey' => $meta['canBeJoined'] ? $assoc : $meta['aliasPath'],
-                    'matching' => isset($meta['matching']) ? $meta['matching'] : $matching
+                    'instance' => $instance,
+                    'canBeJoined' => $canBeJoined,
+                    'entityClass' => $instance->target()->entityClass(),
+                    'nestKey' => $canBeJoined ? $assoc : $meta->aliasPath(),
+                    'matching' => $forMatching !== null ? $forMatching : $matching
                 ];
-                if ($meta['canBeJoined'] && !empty($meta['associations'])) {
-                    $visitor($meta['associations'], $matching);
+                if ($canBeJoined && $associations) {
+                    $visitor($associations, $matching);
                 }
             }
         };
@@ -561,13 +578,12 @@ class EagerLoader
      */
     public function addToJoinsMap($alias, Association $assoc, $asMatching = false)
     {
-        $this->_joinsMap[$alias] = [
+        $this->_joinsMap[$alias] = new EagerLoadable($alias, [
             'aliasPath' => $alias,
             'instance' => $assoc,
             'canBeJoined' => true,
-            'matching' => $asMatching,
-            'associations' => []
-        ];
+            'forMatching' => $asMatching,
+        ]);
     }
 
     /**
@@ -583,13 +599,14 @@ class EagerLoader
     {
         $collectKeys = [];
         foreach ($external as $meta) {
-            if (!$meta['instance']->requiresKeys($meta['config'])) {
+            $instance = $meta->instance();
+            if (!$instance->requiresKeys($meta->config())) {
                 continue;
             }
 
-            $source = $meta['instance']->source();
-            $keys = $meta['instance']->type() === $meta['instance']::MANY_TO_ONE ?
-                (array)$meta['instance']->foreignKey() :
+            $source = $instance->source();
+            $keys = $instance->type() === Association::MANY_TO_ONE ?
+                (array)$instance->foreignKey() :
                 (array)$source->primaryKey();
 
             $alias = $source->alias();

tests/TestCase/ORM/EagerLoaderTest.php

@@ -404,27 +404,27 @@ class EagerLoaderTest extends TestCase
         $loader = new EagerLoader;
         $loader->contain($contains);
         $normalized = $loader->normalized($this->table);
-        $this->assertEquals('clients', $normalized['clients']['aliasPath']);
-        $this->assertEquals('client', $normalized['clients']['propertyPath']);
+        $this->assertEquals('clients', $normalized['clients']->aliasPath());
+        $this->assertEquals('client', $normalized['clients']->propertyPath());
 
-        $assocs = $normalized['clients']['associations'];
-        $this->assertEquals('clients.orders', $assocs['orders']['aliasPath']);
-        $this->assertEquals('client.order', $assocs['orders']['propertyPath']);
+        $assocs = $normalized['clients']->associations();
+        $this->assertEquals('clients.orders', $assocs['orders']->aliasPath());
+        $this->assertEquals('client.order', $assocs['orders']->propertyPath());
 
-        $assocs = $assocs['orders']['associations'];
-        $this->assertEquals('clients.orders.orderTypes', $assocs['orderTypes']['aliasPath']);
-        $this->assertEquals('client.order.order_type', $assocs['orderTypes']['propertyPath']);
-        $this->assertEquals('clients.orders.stuff', $assocs['stuff']['aliasPath']);
-        $this->assertEquals('client.order.stuff', $assocs['stuff']['propertyPath']);
+        $assocs = $assocs['orders']->associations();
+        $this->assertEquals('clients.orders.orderTypes', $assocs['orderTypes']->aliasPath());
+        $this->assertEquals('client.order.order_type', $assocs['orderTypes']->propertyPath());
+        $this->assertEquals('clients.orders.stuff', $assocs['stuff']->aliasPath());
+        $this->assertEquals('client.order.stuff', $assocs['stuff']->propertyPath());
 
-        $assocs = $assocs['stuff']['associations'];
+        $assocs = $assocs['stuff']->associations();
         $this->assertEquals(
             'clients.orders.stuff.stuffTypes',
-            $assocs['stuffTypes']['aliasPath']
+            $assocs['stuffTypes']->aliasPath()
         );
         $this->assertEquals(
             'client.order.stuff.stuff_type',
-            $assocs['stuffTypes']['propertyPath']
+            $assocs['stuffTypes']->propertyPath()
         );
     }
 

tests/TestCase/ORM/QueryRegressionTest.php

@@ -641,4 +641,59 @@ class QueryRegressionTest extends TestCase
 
         $this->assertCount(2, $result->first()->articles);
     }
+
+    /**
+     * Tests that matching does not overwrite associations in contain
+     *
+     * @see https://github.com/cakephp/cakephp/issues/5584
+     * @return void
+     */
+    public function testFindMatchingOverwrite()
+    {
+        $comments = TableRegistry::get('Comments');
+        $comments->belongsTo('Articles');
+
+        $articles = TableRegistry::get('Articles');
+        $articles->belongsToMany('Tags');
+
+        $result = $comments
+            ->find()
+            ->matching('Articles.Tags', function ($q) {
+                return $q->where(['Tags.id' => 2]);
+            })
+            ->contain('Articles')
+            ->first();
+
+        $this->assertEquals(1, $result->id);
+        $this->assertEquals(1, $result->_matchingData['Articles']->id);
+        $this->assertEquals(2, $result->_matchingData['Tags']->id);
+        $this->assertNotNull($result->article);
+        $this->assertEquals($result->article, $result->_matchingData['Articles']);
+    }
+
+    /**
+     * Tests that matching does not overwrite associations in contain
+     *
+     * @see https://github.com/cakephp/cakephp/issues/5584
+     * @return void
+     */
+    public function testFindMatchingOverwrite2()
+    {
+        $comments = TableRegistry::get('Comments');
+        $comments->belongsTo('Articles');
+
+        $articles = TableRegistry::get('Articles');
+        $articles->belongsTo('Authors');
+        $articles->belongsToMany('Tags');
+
+        $result = $comments
+            ->find()
+            ->matching('Articles.Tags', function ($q) {
+                return $q->where(['Tags.id' => 2]);
+            })
+            ->contain('Articles.Authors')
+            ->first();
+
+        $this->assertNotNull($result->article->author);
+    }
 }