Fixes #4938: When yii\db\ActiveQuery is used to build sub-queries, its WHERE clause is not correctly generated

framework/CHANGELOG.md

@@ -90,6 +90,7 @@ Yii Framework 2 Change Log
 - Bug #4813: Fixed MSSQL schema that was getting incorrect info about constraints (samdark, SerjRamone, o-rey)
 - Bug #4880: Return value of yii\web\Request::getPrefferedLanguage() was a normalized value instead of a valid language value from the input array (cebe)
 - Bug #4920: `yii\filters\auth\CompositeAuth` should not trigger error as long as one of the methods succeeds (qiangxue)
+- Bug #4938: When `yii\db\ActiveQuery` is used to build sub-queries, its WHERE clause is not correctly generated (qiangxue)
 - Bug #4954: MSSQL column comments are not retrieved correctly (SerjRamone)
 - Bug #4970: `joinWith()` called by a relation was ignored by `yii\db\ActiveQuery` (stepanselyuk)
 - Bug #5001: `yii\rest\CreateAction`, `yii\rest\UpdateAction` and `yii\rest\DeleteAction` should throw 500 error if the model operation returns false without validation errors (qiangxue)

framework/db/ActiveQuery.php

@@ -133,8 +133,12 @@ class ActiveQuery extends Query implements ActiveQueryInterface
     /**
      * @inheritdoc
      */
-    public function prepareBuild($builder)
+    public function prepare($builder)
     {
+        // NOTE: because the same ActiveQuery may be used to build different SQL statements
+        // (e.g. by ActiveDataProvider, one for count query, the other for row data query,
+        // it is important to make sure the same ActiveQuery can be used to build SQL statements
+        // multiple times.
         if (!empty($this->joinWith)) {
             $this->buildJoinWith();
             $this->joinWith = null;    // clean it up to avoid issue https://github.com/yiisoft/yii2/issues/2687
@@ -162,12 +166,50 @@ class ActiveQuery extends Query implements ActiveQueryInterface
                 break;
             }
         }
+
+        if ($this->primaryModel === null) {
+            // eager loading
+            $query = Query::create($this);
+        } else {
+            // lazy loading of a relation
+            $where = $this->where;
+
+            if ($this->via instanceof self) {
+                // via pivot table
+                $viaModels = $this->via->findPivotRows([$this->primaryModel]);
+                $this->filterByModels($viaModels);
+            } elseif (is_array($this->via)) {
+                // via relation
+                /* @var $viaQuery ActiveQuery */
+                list($viaName, $viaQuery) = $this->via;
+                if ($viaQuery->multiple) {
+                    $viaModels = $viaQuery->all();
+                    $this->primaryModel->populateRelation($viaName, $viaModels);
+                } else {
+                    $model = $viaQuery->one();
+                    $this->primaryModel->populateRelation($viaName, $model);
+                    $viaModels = $model === null ? [] : [$model];
+                }
+                $this->filterByModels($viaModels);
+            } else {
+                $this->filterByModels([$this->primaryModel]);
+            }
+
+            $query = Query::create($this);
+            $this->where = $where;
+        }
+
+        if (!empty($this->on)) {
+            $query->andWhere($this->on);
+        }
+
+        return $query;
     }
 
     /**
      * @inheritdoc
      */
-    public function prepareResult($rows)
+    public function populate($rows)
     {
         if (empty($rows)) {
             return [];
@@ -242,7 +284,7 @@ class ActiveQuery extends Query implements ActiveQueryInterface
     {
         $row = parent::one($db);
         if ($row !== false) {
-            $models = $this->prepareResult([$row]);
+            $models = $this->populate([$row]);
             return reset($models) ?: null;
         } else {
             return null;
@@ -256,32 +298,6 @@ class ActiveQuery extends Query implements ActiveQueryInterface
      * @return Command the created DB command instance.
      */
     public function createCommand($db = null)
-    {
-        if ($this->primaryModel === null) {
-            // not a relational context or eager loading
-            if (!empty($this->on)) {
-                $where = $this->where;
-                $this->andWhere($this->on);
-                $command = $this->createCommandInternal($db);
-                $this->where = $where;
-
-                return $command;
-            } else {
-                return $this->createCommandInternal($db);
-            }
-        } else {
-            // lazy loading of a relation
-            return $this->createRelationalCommand($db);
-        }
-    }
-
-    /**
-     * Creates a DB command that can be used to execute this query.
-     * @param Connection|null $db the DB connection used to create the DB command.
-     * If null, the DB connection returned by [[modelClass]] will be used.
-     * @return Command the created DB command instance.
-     */
-    protected function createCommandInternal($db)
     {
         /* @var $modelClass ActiveRecord */
         $modelClass = $this->modelClass;
@@ -299,47 +315,6 @@ class ActiveQuery extends Query implements ActiveQueryInterface
         return $db->createCommand($sql, $params);
     }
 
-    /**
-     * Creates a command for lazy loading of a relation.
-     * @param Connection|null $db the DB connection used to create the DB command.
-     * @return Command the created DB command instance.
-     */
-    private function createRelationalCommand($db = null)
-    {
-        $where = $this->where;
-
-        if ($this->via instanceof self) {
-            // via pivot table
-            $viaModels = $this->via->findPivotRows([$this->primaryModel]);
-            $this->filterByModels($viaModels);
-        } elseif (is_array($this->via)) {
-            // via relation
-            /* @var $viaQuery ActiveQuery */
-            list($viaName, $viaQuery) = $this->via;
-            if ($viaQuery->multiple) {
-                $viaModels = $viaQuery->all();
-                $this->primaryModel->populateRelation($viaName, $viaModels);
-            } else {
-                $model = $viaQuery->one();
-                $this->primaryModel->populateRelation($viaName, $model);
-                $viaModels = $model === null ? [] : [$model];
-            }
-            $this->filterByModels($viaModels);
-        } else {
-            $this->filterByModels([$this->primaryModel]);
-        }
-
-        if (!empty($this->on)) {
-            $this->andWhere($this->on);
-        }
-
-        $command = $this->createCommandInternal($db);
-
-        $this->where = $where;
-
-        return $command;
-    }
-
     /**
      * Joins with the specified relations.
      *
@@ -552,13 +527,11 @@ class ActiveQuery extends Query implements ActiveQueryInterface
             // via table
             $this->joinWithRelation($parent, $via, $joinType);
             $this->joinWithRelation($via, $child, $joinType);
-
             return;
         } elseif (is_array($via)) {
             // via relation
             $this->joinWithRelation($parent, $via[1], $joinType);
             $this->joinWithRelation($via[1], $child, $joinType);
-
             return;
         }
 

framework/db/BatchQueryResult.php

@@ -144,7 +144,7 @@ class BatchQueryResult extends Object implements \Iterator
             $rows[] = $row;
         }
 
-        return $this->query->prepareResult($rows);
+        return $this->query->populate($rows);
     }
 
     /**

framework/db/Query.php

@@ -129,9 +129,11 @@ class Query extends Component implements QueryInterface
      * This method is called by [[QueryBuilder]] when it starts to build SQL from a query object.
      * You may override this method to do some final preparation work when converting a query into a SQL statement.
      * @param QueryBuilder $builder
+     * @return Query a prepared query instance which will be used by [[QueryBuilder]] to build the SQL
      */
-    public function prepareBuild($builder)
+    public function prepare($builder)
     {
+        return $this;
     }
 
     /**
@@ -202,7 +204,7 @@ class Query extends Component implements QueryInterface
     public function all($db = null)
     {
         $rows = $this->createCommand($db)->queryAll();
-        return $this->prepareResult($rows);
+        return $this->populate($rows);
     }
 
     /**
@@ -212,7 +214,7 @@ class Query extends Component implements QueryInterface
      * @param array $rows the raw query result from database
      * @return array the converted query result
      */
-    public function prepareResult($rows)
+    public function populate($rows)
     {
         if ($this->indexBy === null) {
             return $rows;
@@ -372,7 +374,7 @@ class Query extends Component implements QueryInterface
         } else {
             return (new Query)->select([$selectExpression])
                 ->from(['c' => $this])
-                ->createCommand($db)
+                ->createCommand($command->db)
                 ->queryScalar();
         }
     }
@@ -833,4 +835,30 @@ class Query extends Component implements QueryInterface
         }
         return $this;
     }
+
+    /**
+     * Creates a new Query object and copies its property values from an existing one.
+     * The properties being copies are the ones to be used by query builders.
+     * @param Query $from the source query object
+     * @return Query the new Query object
+     */
+    public static function create($from)
+    {
+        return new self([
+            'where' => $from->where,
+            'limit' => $from->limit,
+            'offset' => $from->offset,
+            'orderBy' => $from->orderBy,
+            'indexBy' => $from->indexBy,
+            'select' => $from->select,
+            'selectOption' => $from->selectOption,
+            'distinct' => $from->distinct,
+            'from' => $from->from,
+            'groupBy' => $from->groupBy,
+            'join' => $from->join,
+            'having' => $from->having,
+            'union' => $from->union,
+            'params' => $from->params,
+        ]);
+    }
 }

framework/db/QueryBuilder.php

@@ -85,7 +85,7 @@ class QueryBuilder extends \yii\base\Object
      */
     public function build($query, $params = [])
     {
-        $query->prepareBuild($this);
+        $query = $query->prepare($this);
 
         $params = empty($params) ? $query->params : array_merge($params, $query->params);
 

tests/unit/data/ar/Category.php

@@ -24,4 +24,10 @@ class Category extends ActiveRecord
     {
         return $this->hasMany(Item::className(), ['category_id' => 'id']);
     }
+
+    public function getLimitedItems()
+    {
+        return $this->hasMany(Item::className(), ['category_id' => 'id'])
+            ->onCondition(['item.id' => [1, 2, 3]]);
+    }
 }

tests/unit/framework/db/ActiveRecordTest.php

@@ -2,6 +2,7 @@
 namespace yiiunit\framework\db;
 
 use yiiunit\data\ar\ActiveRecord;
+use yiiunit\data\ar\Category;
 use yiiunit\data\ar\Customer;
 use yiiunit\data\ar\NullValues;
 use yiiunit\data\ar\OrderItem;
@@ -620,4 +621,14 @@ class ActiveRecordTest extends DatabaseTestCase
 //        $this->assertSame(true, $model->bool_col);
 //        $this->assertSame(false, $model->bool_col2);
     }
+
+    public function testIssues()
+    {
+        // https://github.com/yiisoft/yii2/issues/4938
+        $category = Category::findOne(2);
+        $this->assertTrue($category instanceof Category);
+        $this->assertEquals(3, $category->getItems()->count());
+        $this->assertEquals(1, $category->getLimitedItems()->count());
+        $this->assertEquals(1, $category->getLimitedItems()->distinct(true)->count());
+    }
 }