From 7008e3700c9bd68899581e5e83c30808b330f402 Mon Sep 17 00:00:00 2001 From: brandonkelly Date: Wed, 7 Feb 2024 08:33:14 -0800 Subject: [PATCH] Fixed various remaining element query bugs --- CHANGELOG.md | 2 - src/db/CallbackExpression.php | 44 ---- src/db/CallbackExpressionBuilder.php | 36 ---- src/elements/db/ElementQuery.php | 197 +++++++----------- .../db/OrderByPlaceholderExpression.php | 23 ++ src/fields/BaseRelationField.php | 4 +- 6 files changed, 106 insertions(+), 200 deletions(-) delete mode 100644 src/db/CallbackExpression.php delete mode 100644 src/db/CallbackExpressionBuilder.php create mode 100644 src/elements/db/OrderByPlaceholderExpression.php diff --git a/CHANGELOG.md b/CHANGELOG.md index e47ae7f7a21..7714b7f1b73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,6 @@ ## Unreleased -- Added `craft\db\CallbackExpressionBuilder`. -- Added `craft\db\CallbackExpression`. - Fixed a bug where querying elements with `eagerly()` wasn’t working across element types. - Fixed a SQL error that could occur when querying relations with `eagerly()`. - Fixed a bug where element actions weren’t working for elements that were added to a relational field. diff --git a/src/db/CallbackExpression.php b/src/db/CallbackExpression.php deleted file mode 100644 index f84b19f1eec..00000000000 --- a/src/db/CallbackExpression.php +++ /dev/null @@ -1,44 +0,0 @@ -andWhere( - * new CallbackExpression(function(array &$params): string { - * $params['foo'] = 'bar'; - * return '[[column_name]] = :foo'; - * }) - * ); - * ``` - * - * @author Pixel & Tonic, Inc. - * @since 5.0.0 - */ -class CallbackExpression extends BaseObject implements ExpressionInterface -{ - /** - * Constructor - * - * @param callable $callback the DB expression callback - * @param array $config name-value pairs that will be used to initialize the object properties - */ - public function __construct( - public $callback, - array $config = [], - ) { - parent::__construct($config); - } -} diff --git a/src/db/CallbackExpressionBuilder.php b/src/db/CallbackExpressionBuilder.php deleted file mode 100644 index ed4856f0ecc..00000000000 --- a/src/db/CallbackExpressionBuilder.php +++ /dev/null @@ -1,36 +0,0 @@ - - * @since 5.0.0 - */ -class CallbackExpressionBuilder implements ExpressionBuilderInterface -{ - use ExpressionBuilderTrait; - - /** - * @inheritdoc - */ - public function build(ExpressionInterface $expression, array &$params = []) - { - if (!$expression instanceof CallbackExpression) { - throw new NotSupportedException('$expression must be an instance of CallbackExpression.'); - } - - return call_user_func($expression->callback, $this->queryBuilder, $params); - } -} diff --git a/src/elements/db/ElementQuery.php b/src/elements/db/ElementQuery.php index feac69212e5..79aa2efdb95 100644 --- a/src/elements/db/ElementQuery.php +++ b/src/elements/db/ElementQuery.php @@ -16,12 +16,8 @@ use craft\behaviors\DraftBehavior; use craft\behaviors\RevisionBehavior; use craft\cache\ElementQueryTagDependency; -use craft\db\CallbackExpression; -use craft\db\CallbackExpressionBuilder; use craft\db\Connection; use craft\db\FixedOrderExpression; -use craft\db\mysql\QueryBuilder as MysqlQueryBuilder; -use craft\db\pgsql\QueryBuilder as PgsqlQueryBuilder; use craft\db\Query; use craft\db\QueryAbortedException; use craft\db\Table; @@ -564,13 +560,12 @@ public function __construct(string $elementType, array $config = []) { $this->elementType = $elementType; - // Set the default `select` and `orderBy` params to callback expressions, - // so we can determine the SQL dynamically only once the query is being executed - if (!isset($this->select)) { - $this->select(new CallbackExpression(fn(QueryBuilder $builder) => $this->defaultSelectSql($builder))); - } + // Use ** as a placeholder for "all the default columns" + $config['select'] = $config['select'] ?? ['**' => '**']; + + // Set a placeholder for the default `orderBy` param if (!isset($this->orderBy)) { - $this->orderBy(new CallbackExpression(fn(QueryBuilder $builder) => $this->defaultOrderBySql($builder))); + $this->orderBy(new OrderByPlaceholderExpression()); } parent::__construct($config); @@ -1379,11 +1374,6 @@ public function cache($duration = true, $dependency = null): \yii\db\Query|Eleme */ public function prepare($builder): Query { - // Set our callback expression builder - $builder->setExpressionBuilders([ - CallbackExpression::class => CallbackExpressionBuilder::class, - ]); - // Log a warning if the app isn't fully initialized yet if (!Craft::$app->getIsInitialized()) { Craft::warning( @@ -2967,10 +2957,42 @@ private function _applyOrderByParams(YiiConnection $db): void return; } + $orderBy = array_merge($this->orderBy ?: []); + + // Only set to the default order if `orderBy` is still set to the placeholder + if ( + count($orderBy) === 1 && + ($orderBy[0] ?? null) instanceof OrderByPlaceholderExpression + ) { + if ($this->fixedOrder) { + if (empty($this->id)) { + throw new QueryAbortedException(); + } + + $ids = $this->id; + if (!is_array($ids)) { + $ids = is_string($ids) ? StringHelper::split($ids) : [$ids]; + } + + if (!$db instanceof Connection) { + throw new Exception('The database connection doesn’t support fixed ordering.'); + } + $orderBy = [new FixedOrderExpression('elements.id', $ids, $db)]; + } elseif ($this->revisions) { + $orderBy = ['num' => SORT_DESC]; + } elseif ($this->_shouldJoinStructureData()) { + $orderBy = ['structureelements.lft' => SORT_ASC] + $this->defaultOrderBy; + } elseif (!empty($this->defaultOrderBy)) { + $orderBy = $this->defaultOrderBy; + } else { + return; + } + } else { + $orderBy = array_filter($orderBy, fn($value) => !$value instanceof OrderByPlaceholderExpression); + } + // Rename orderBy keys based on the real column name mapping // (yes this is awkward but we need to preserve the order of the keys!) - /** @var array $orderBy */ - $orderBy = array_merge($this->orderBy); $orderByColumns = array_keys($orderBy); foreach ($this->_columnMap as $orderValue => $columnName) { @@ -3033,57 +3055,6 @@ private function _applyOrderByParams(YiiConnection $db): void $this->subQuery->orderBy($orderBy); } - private function defaultOrderBySql(QueryBuilder $builder): string - { - $db = $builder->db; - - if ($this->fixedOrder) { - if (empty($this->id)) { - throw new QueryAbortedException(); - } - - $ids = $this->id; - if (!is_array($ids)) { - $ids = is_string($ids) ? StringHelper::split($ids) : [$ids]; - } - - if (!($builder instanceof MysqlQueryBuilder || $builder instanceof PgsqlQueryBuilder)) { - throw new Exception('The database connection doesn’t support fixed ordering.'); - } - - if ($this->inReverse) { - $ids = array_reverse($ids); - } - - return $builder->fixedOrder('elements.id', $ids); - } - - if ($this->revisions) { - $columns = ['num' => SORT_DESC]; - } elseif ($this->_shouldJoinStructureData()) { - $columns = ['structureelements.lft' => SORT_ASC] + $this->defaultOrderBy; - } elseif (!empty($this->defaultOrderBy)) { - $columns = $this->defaultOrderBy; - } else { - return ''; - } - - $orders = []; - foreach ($columns as $name => $direction) { - if ($direction instanceof ExpressionInterface) { - $orders[] = $builder->buildExpression($direction); - } else { - if ($this->inReverse) { - $direction = $direction === SORT_DESC ? SORT_ASC : SORT_DESC; - } - - $orders[] = $db->quoteColumnName($name) . ($direction === SORT_DESC ? ' DESC' : ''); - } - } - - return implode(', ', $orders); - } - /** * Applies the 'select' param to the query being prepared. */ @@ -3091,66 +3062,60 @@ private function _applySelectParam(): void { // Select all columns defined by [[select]], swapping out any mapped column names $select = []; + $includeDefaults = false; foreach ((array)$this->select as $alias => $column) { - // Is this a mapped column name (without a custom alias)? - if ($alias === $column && isset($this->_columnMap[$alias])) { - $column = $this->_columnMap[$alias]; + if ($alias === '**') { + $includeDefaults = true; + } else { + // Is this a mapped column name (without a custom alias)? + if ($alias === $column && isset($this->_columnMap[$alias])) { + $column = $this->_columnMap[$alias]; - // Completely ditch the mapped name if instantiated elements are going to be returned - if (!$this->asArray) { - $alias = $this->_columnMap[$alias]; + // Completely ditch the mapped name if instantiated elements are going to be returned + if (!$this->asArray) { + $alias = $this->_columnMap[$alias]; + } } - } - - $select[$alias] = $column; - } - // If the query already specifies any columns, merge those in too - if (!empty($this->query->select)) { - $select = array_merge($select, $this->query->select); + $select[$alias] = $column; + } } - $this->query->select = $select; - } - - private function defaultSelectSql(QueryBuilder $builder): string - { - $columns = [ - 'elements.id' => 'elements.id', - 'elements.canonicalId' => 'elements.canonicalId', - 'elements.fieldLayoutId' => 'elements.fieldLayoutId', - 'elements.uid' => 'elements.uid', - 'elements.enabled' => 'elements.enabled', - 'elements.archived' => 'elements.archived', - 'elements.dateLastMerged' => 'elements.dateLastMerged', - 'elements.dateCreated' => 'elements.dateCreated', - 'elements.dateUpdated' => 'elements.dateUpdated', - 'siteSettingsId' => 'elements_sites.id', - 'elements_sites.siteId' => 'elements_sites.siteId', - 'elements_sites.title' => 'elements_sites.title', - 'elements_sites.slug' => 'elements_sites.slug', - 'elements_sites.uri' => 'elements_sites.uri', - 'elements_sites.content' => 'elements_sites.content', - 'enabledForSite' => 'elements_sites.enabled', - ]; - - // If the query includes soft-deleted elements, include the date deleted - if ($this->trashed !== false) { - $columns['elements.dateDeleted'] = 'elements.dateDeleted'; - } + // Is there still a ** placeholder param? + if ($includeDefaults) { + // Merge in the default columns + $select = array_merge($select, [ + 'elements.id' => 'elements.id', + 'elements.canonicalId' => 'elements.canonicalId', + 'elements.fieldLayoutId' => 'elements.fieldLayoutId', + 'elements.uid' => 'elements.uid', + 'elements.enabled' => 'elements.enabled', + 'elements.archived' => 'elements.archived', + 'elements.dateLastMerged' => 'elements.dateLastMerged', + 'elements.dateCreated' => 'elements.dateCreated', + 'elements.dateUpdated' => 'elements.dateUpdated', + 'siteSettingsId' => 'elements_sites.id', + 'elements_sites.siteId' => 'elements_sites.siteId', + 'elements_sites.title' => 'elements_sites.title', + 'elements_sites.slug' => 'elements_sites.slug', + 'elements_sites.uri' => 'elements_sites.uri', + 'elements_sites.content' => 'elements_sites.content', + 'enabledForSite' => 'elements_sites.enabled', + ]); - $db = $builder->db; + // If the query includes soft-deleted elements, include the date deleted + if ($this->trashed !== false) { + $select['elements.dateDeleted'] = 'elements.dateDeleted'; + } - foreach ($columns as $alias => &$column) { - if ($alias === $column) { - $column = $db->quoteColumnName($column); - } else { - $column = sprintf('%s AS %s', $db->quoteColumnName($column), $db->quoteColumnName($alias)); + // If the query already specifies any columns, merge those in too + if (!empty($this->query->select)) { + $select = array_merge($select, $this->query->select); } } - return implode(', ', $columns); + $this->query->select = $select; } /** diff --git a/src/elements/db/OrderByPlaceholderExpression.php b/src/elements/db/OrderByPlaceholderExpression.php new file mode 100644 index 00000000000..4fda8811c61 --- /dev/null +++ b/src/elements/db/OrderByPlaceholderExpression.php @@ -0,0 +1,23 @@ + + * @since 5.0.0 + * @internal + */ +class OrderByPlaceholderExpression extends Expression +{ + public function __construct($params = [], $config = []) + { + parent::__construct('', $params, $config); + } +} diff --git a/src/fields/BaseRelationField.php b/src/fields/BaseRelationField.php index d4d9cc1e576..ae1d3c6c07f 100644 --- a/src/fields/BaseRelationField.php +++ b/src/fields/BaseRelationField.php @@ -16,7 +16,6 @@ use craft\base\InlineEditableFieldInterface; use craft\base\NestedElementInterface; use craft\behaviors\EventBehavior; -use craft\db\CallbackExpression; use craft\db\Query; use craft\db\Table as DbTable; use craft\elements\conditions\ElementCondition; @@ -24,6 +23,7 @@ use craft\elements\db\ElementQuery; use craft\elements\db\ElementQueryInterface; use craft\elements\db\ElementRelationParamParser; +use craft\elements\db\OrderByPlaceholderExpression; use craft\elements\ElementCollection; use craft\errors\SiteNotFoundException; use craft\events\CancelableEvent; @@ -704,7 +704,7 @@ public function normalizeValue(mixed $value, ?ElementInterface $element): mixed !$this->maintainHierarchy && ( !$query->orderBy || - (count($query->orderBy) === 1) && ($query->orderBy[0] ?? null) instanceof CallbackExpression + (count($query->orderBy) === 1) && ($query->orderBy[0] ?? null) instanceof OrderByPlaceholderExpression ) ) { $q->orderBy(["$relationsAlias.sortOrder" => SORT_ASC]);