diff --git a/framework/CHANGELOG.md b/framework/CHANGELOG.md index 3fc33cde5e..aa0d5aacd9 100644 --- a/framework/CHANGELOG.md +++ b/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) diff --git a/framework/db/ActiveQuery.php b/framework/db/ActiveQuery.php index e68fe6d1c5..1ee840703e 100644 --- a/framework/db/ActiveQuery.php +++ b/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; } diff --git a/framework/db/BatchQueryResult.php b/framework/db/BatchQueryResult.php index e5a16064cc..f1583839c7 100644 --- a/framework/db/BatchQueryResult.php +++ b/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); } /** diff --git a/framework/db/Query.php b/framework/db/Query.php index 8ec8ccd6fb..41fbfe2fd5 100644 --- a/framework/db/Query.php +++ b/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, + ]); + } } diff --git a/framework/db/QueryBuilder.php b/framework/db/QueryBuilder.php index ea8ca3373f..fd9872750d 100644 --- a/framework/db/QueryBuilder.php +++ b/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); diff --git a/tests/unit/data/ar/Category.php b/tests/unit/data/ar/Category.php index 1ce4608695..10fd7bb300 100644 --- a/tests/unit/data/ar/Category.php +++ b/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]]); + } } diff --git a/tests/unit/framework/db/ActiveRecordTest.php b/tests/unit/framework/db/ActiveRecordTest.php index ca9b9e976f..2c8e21abe3 100644 --- a/tests/unit/framework/db/ActiveRecordTest.php +++ b/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()); + } }