Clean up condition filtering.

Cache the two kinds of conditions so we can avoid recomputing them on
each query.

Refs #7994

src/ORM/Association/BelongsToMany.php

@@ -14,6 +14,7 @@
  */
 namespace Cake\ORM\Association;
 
+use Cake\Database\ExpressionInterface;
 use Cake\Datasource\EntityInterface;
 use Cake\ORM\Association;
 use Cake\ORM\Query;
@@ -134,6 +135,20 @@ class BelongsToMany extends Association
      */
     protected $_dependent = true;
 
+    /**
+     * Filtered conditions that reference the target table.
+     *
+     * @var null|array
+     */
+    protected $_targetConditions;
+
+    /**
+     * Filtered conditions that reference the junction table.
+     *
+     * @var null|array
+     */
+    protected $_junctionConditions;
+
     /**
      * Sets the name of the field representing the foreign key to the target table.
      * If no parameters are passed current field is returned
@@ -323,17 +338,16 @@ public function attachTo(Query $query, array $options = [])
             $includeFields = $options['includeFields'];
         }
 
-        // TODO see if this can be removed and replaced with eagerly splitting
-        // up conditions when defining associations.
+        // Attach the junction table as well we need it to populate _joinData.
         $assoc = $this->_targetTable->association($junction->alias());
         $query->removeJoin($assoc->name());
-
-        unset($options['queryBuilder']);
-        // TODO clean up the definition of $options
-        $type = array_intersect_key($options, ['joinType' => 1, 'fields' => 1]);
-        $options = ['conditions' => $cond] + compact('includeFields');
-        $options['foreignKey'] = $this->targetForeignKey();
-        $assoc->attachTo($query, $options + $type);
+        $options = array_intersect_key($options, ['joinType' => 1, 'fields' => 1]);
+        $options += [
+            'conditions' => $cond,
+            'includeFields' => $includeFields,
+            'foreignKey' => $this->targetForeignKey(),
+        ];
+        $assoc->attachTo($query, $options);
         $query->eagerLoader()->addToJoinsMap($junction->alias(), $assoc, true);
     }
 
@@ -778,30 +792,72 @@ function () use ($sourceEntity, $targetEntities, $options) {
         $sourceEntity->dirty($property, false);
     }
 
+    /**
+     * {@inheritDoc}
+     */
+    public function conditions($conditions = null)
+    {
+        if ($conditions !== null) {
+            $this->_conditions = $conditions;
+            $this->_targetConditions = $this->_junctionConditions = [];
+        }
+        return $this->_conditions;
+    }
+
+    /**
+     * Returns filtered conditions that reference the target table.
+     *
+     * Any string expressions, or expression objects will
+     * also be returned in this list.
+     *
+     * @return mixed Generally an array. If the conditions
+     *   are not an array, the association conditions will be
+     *   returned unmodified.
+     */
     protected function targetConditions()
     {
-        $alias = $this->alias() . '.';
+        if ($this->_targetConditions !== null) {
+            return $this->_targetConditions;
+        }
+        $conditions = $this->conditions();
+        if (!is_array($conditions)) {
+            return $conditions;
+        }
         $matching = [];
-        // TODO add handling for strings, expression objects.
-        foreach ($this->conditions() as $field => $value) {
-            if (strpos($field, $alias) === 0) {
+        $alias = $this->alias() . '.';
+        foreach ($conditions as $field => $value) {
+            if (is_string($field) && strpos($field, $alias) === 0) {
+                $matching[$field] = $value;
+            } elseif (is_int($field) || $value instanceof ExpressionInterface) {
                 $matching[$field] = $value;
             }
         }
-        return $matching;
+        return $this->_targetConditions = $matching;
     }
 
+    /**
+     * Returns filtered conditions that specifically reference
+     * the junction table.
+     *
+     * @return array
+     */
     protected function junctionConditions()
     {
-        $alias = $this->_junctionAssociationName() . '.';
+        if ($this->_junctionConditions !== null) {
+            return $this->_junctionConditions;
+        }
         $matching = [];
-        foreach ($this->conditions() as $field => $value) {
-            // TODO add handling for strings, expression objects.
-            if (strpos($field, $alias) === 0) {
+        $conditions = $this->conditions();
+        if (!is_array($conditions)) {
+            return $matching;
+        }
+        $alias = $this->_junctionAssociationName() . '.';
+        foreach ($conditions as $field => $value) {
+            if (is_string($field) && strpos($field, $alias) === 0) {
                 $matching[$field] = $value;
             }
         }
-        return $matching;
+        return $this->_junctionConditions = $matching;
     }
 
     /**
@@ -820,17 +876,6 @@ protected function junctionConditions()
      */
     public function find($type = null, array $options = [])
     {
-        // The parent method applies the conditions.
-        // The application of the conditions causes issues as often the conditions
-        // reference the junction table.
-        // Ideally we'd get a query like:
-        //
-        // select * from articles
-        // inner join tags on (matching cond AND relvant assoc cond)
-        // inner join articles_tags on (tags.id = a_t.tag_id and articles.id = a_t.article_id and relevant assoc cond)
-        //
-        // Overriding conditions() and adding junctionConditions() might help here. Or filtering
-        // the conditions when defining the junction association.
         $type = $type ?: $this->finder();
         list($type, $opts) = $this->_extractFinder($type);
         $query = $this->target()

tests/TestCase/ORM/Association/BelongsToManyTest.php

@@ -16,7 +16,6 @@
 
 use Cake\Database\Expression\IdentifierExpression;
 use Cake\Database\Expression\QueryExpression;
-use Cake\Database\Expression\TupleComparison;
 use Cake\Database\TypeMap;
 use Cake\Datasource\ConnectionManager;
 use Cake\ORM\Association\BelongsToMany;
@@ -1005,7 +1004,7 @@ public function testAssociationProxyFindWithConditions()
      *
      * @return void
      */
-    public function testBelongsToManyAssociationWithConditions()
+    public function testBelongsToManyAssociationWithArrayConditions()
     {
         $table = TableRegistry::get('Articles');
         $table->belongsToMany('Tags', [
@@ -1022,6 +1021,28 @@ public function testBelongsToManyAssociationWithConditions()
         $this->assertNotEmpty($results[0]->_matchingData);
     }
 
+    /**
+     * Test that matching() works on belongsToMany associations.
+     *
+     * @return void
+     */
+    public function testBelongsToManyAssociationWithExpressionConditions()
+    {
+        $table = TableRegistry::get('Articles');
+        $table->belongsToMany('Tags', [
+            'foreignKey' => 'article_id',
+            'associationForeignKey' => 'tag_id',
+            'conditions' => [new QueryExpression('Tags.name LIKE "tag%"')],
+            'through' => 'SpecialTags'
+        ]);
+        $query = $table->find()->matching('Tags', function ($q) {
+            return $q->where(['Tags.name' => 'tag1']);
+        });
+        $results = $query->toArray();
+        $this->assertCount(1, $results);
+        $this->assertNotEmpty($results[0]->_matchingData);
+    }
+
     /**
      * Test that association proxy find() with matching resolves joins correctly
      *