Fixed various remaining element query bugs

craftcms · Feb 7, 2024 · 7008e37 · 7008e37
1 parent db3de0f
commit 7008e37
Show file tree

Hide file tree

Showing 6 changed files with 106 additions and 200 deletions.
diff --git 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
diff --git a/src/db/CallbackExpressionBuilder.php b/src/db/CallbackExpressionBuilder.php
diff --git 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,124 +3055,67 @@ 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.
      */
     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
@@ -0,0 +1,23 @@
+<?php
+/**
+ * @link https://craftcms.com/
+ * @copyright Copyright (c) Pixel & Tonic, Inc.
+ * @license https://craftcms.github.io/license/
+ */
+
+namespace craft\elements\db;
+
+use yii\db\Expression;
+
+/**
+ * @author Pixel & Tonic, Inc. <[email protected]>
+ * @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
@@ -16,14 +16,14 @@
 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;
 use craft\elements\conditions\ElementConditionInterface;
 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]);