Sort option refactoring

Fixes #2975

CHANGELOG-v3.md

@@ -2,9 +2,14 @@
 
 ## 3.0.10.3 - 2018-06-07
 
+### Changed
+- Sort options defined by element types’ `sortOptions()` / `defineSortOptoins()` methods can now be specified as sub-arrays with `label`, `orderBy`, and `attribute` keys.
+
 ### Fixed
 - Fixed a bug where the “New Entry” menu on the Entries index page would not contain any options on single-site installs, running MySQL. ([#2961](https://github.com/craftcms/cms/issues/2961))
 - Fixed a bug where the `siteName` config setting wasn’t working as expected when set to an array. ([#2968](https://github.com/craftcms/cms/issues/2968))
+- Fixed a bug where it was not possible to order search results by search score, if the element type didn’t specify any sort options.
+- Fixed a bug where clicking on “Date Created” and “Date Updated” column headers on element indexes wouldn’t update the sort order. ([#2975](https://github.com/craftcms/cms/issues/2975))
 
 ## 3.0.10.2 - 2018-06-07
 

src/base/Element.php

@@ -43,6 +43,7 @@
 use yii\base\Event;
 use yii\base\Exception;
 use yii\base\InvalidConfigException;
+use yii\base\InvalidValueException;
 use yii\validators\NumberValidator;
 use yii\validators\Validator;
 
@@ -408,23 +409,9 @@ public static function indexHtml(ElementQueryInterface $elementQuery, array $dis
                 unset($viewState['order']);
             }
         } else {
-            $sortOptions = static::sortOptions();
-
-            if (!empty($sortOptions)) {
-                $sortOptions = array_keys($sortOptions);
-                $sortOptions[] = 'score';
-                $order = (!empty($viewState['order']) && in_array($viewState['order'], $sortOptions, true)) ? $viewState['order'] : reset($sortOptions);
-                $sort = (!empty($viewState['sort']) && in_array($viewState['sort'], ['asc', 'desc'], true)) ? $viewState['sort'] : 'asc';
-
-                // Combine them, accounting for the possibility that $order could contain multiple values,
-                // and be defensive about the possibility that the first value actually has "asc" or "desc"
-
-                // typeId             => typeId [sort]
-                // typeId, title      => typeId [sort], title
-                // typeId, title desc => typeId [sort], title desc
-                // typeId desc        => typeId [sort]
-
-                $elementQuery->orderBy(preg_replace('/^(.*?)(?:\s+(?:asc|desc))?(,.*)?$/i', "$1 {$sort}$2", $order));
+            $orderBy = self::_indexOrderBy($viewState);
+            if ($orderBy !== false) {
+                $elementQuery->orderBy($orderBy);
             }
         }
 
@@ -647,6 +634,55 @@ protected static function prepElementQueryForTableAttribute(ElementQueryInterfac
         }
     }
 
+    /**
+     * Returns the orderBy value for element indexes
+     *
+     * @param array $viewState
+     * @return array|false
+     */
+    private static function _indexOrderBy(array $viewState)
+    {
+        // Define the available sort attribute/option pairs
+        $sortOptions = [];
+        foreach (static::sortOptions() as $key => $sortOption) {
+            if (is_string($key)) {
+                // Shorthand syntax
+                $sortOptions[$key] = $key;
+            } else {
+                if (!isset($sortOption['orderBy'])) {
+                    throw new InvalidValueException('Sort options must specify an orderBy value');
+                }
+                $attribute = $sortOption['attribute'] ?? $sortOption['orderBy'];
+                $sortOptions[$attribute] = $sortOption['orderBy'];
+            }
+        }
+        $sortOptions['score'] = 'score';
+
+        if (!empty($viewState['order']) && isset($sortOptions[$viewState['order']])) {
+            $columns = $sortOptions[$viewState['order']];
+        } else if (count($sortOptions) > 1) {
+            $columns = reset($sortOptions);
+        } else {
+            return false;
+        }
+
+        // Borrowed from QueryTrait::normalizeOrderBy()
+        $columns = preg_split('/\s*,\s*/', trim($columns), -1, PREG_SPLIT_NO_EMPTY);
+        $result = [];
+        foreach ($columns as $i => $column) {
+            if ($i === 0) {
+                // The first column's sort direction is always user-defined
+                $result[$column] = !empty($viewState['sort']) && strcasecmp($viewState['sort'], 'desc') ? SORT_ASC : SORT_DESC;
+            } else if (preg_match('/^(.*?)\s+(asc|desc)$/i', $column, $matches)) {
+                $result[$matches[1]] = strcasecmp($matches[2], 'desc') ? SORT_ASC : SORT_DESC;
+            } else {
+                $result[$column] = SORT_ASC;
+            }
+        }
+
+        return $result;
+    }
+
     // Properties
     // =========================================================================
 

src/base/ElementInterface.php

@@ -289,28 +289,31 @@ public static function indexHtml(ElementQueryInterface $elementQuery, array $dis
     /**
      * Returns the sort options for the element type.
      *
-     * This method should return an array, where the keys reference database column names that should be sorted on,
-     * and where the values define the user-facing labels.
+     * This method should return an array, where each item is a sub-array with the following keys:
+     *
+     * - `label` – The sort option label
+     * - `orderBy` – A comma-delimited string of columns to order the query by
+     * - `attribute` _(optional)_ – The [[tableAttributes()|table attribute]] name that this option is associated with
      *
      * ```php
      * return [
-     *     'columnName1' => Craft::t('app', 'Attribute Label 1'),
-     *     'columnName2' => Craft::t('app', 'Attribute Label 2'),
+     *     [
+     *         'label' => Craft::t('app', 'Attribute Label'),
+     *         'orderBy' => 'columnName',
+     *         'attribute' => 'attributeName'
+     *     ],
      * ];
      * ```
      *
-     * If you want to sort by multilple columns simultaneously, you can specify multiple column names in the key,
-     * separated by commas.
+     * A shorthand syntax is also supported, if there is no corresponding table attribute, or the table attribute
+     * has the exact same name as the column.
      *
      * ```php
      * return [
-     *     'columnName1, columnName2 asc' => Craft::t('app', 'Attribute Label 1'),
-     *     'columnName3'                  => Craft::t('app', 'Attribute Label 2'),
+     *     'columnName' => Craft::t('app', 'Attribute Label'),
      * ];
      * ```
      *
-     * If you do that, you can specify the sort direction for the subsequent columns (`asc` or `desc`. There is no point
-     * in specifying the sort direction for the first column, though, since the end user has full control over that.
      * Note that this method will only get called once for the entire index; not each time that a new source is
      * selected.
      *

src/elements/Asset.php

@@ -283,8 +283,16 @@ protected static function defineSortOptions(): array
             'filename' => Craft::t('app', 'Filename'),
             'size' => Craft::t('app', 'File Size'),
             'dateModified' => Craft::t('app', 'File Modification Date'),
-            'elements.dateCreated' => Craft::t('app', 'Date Uploaded'),
-            'elements.dateUpdated' => Craft::t('app', 'Date Updated'),
+            [
+                'label' => Craft::t('app', 'Date Uploaded'),
+                'orderBy' => 'elements.dateCreated',
+                'attribute' => 'dateCreated'
+            ],
+            [
+                'label' => Craft::t('app', 'Date Updated'),
+                'orderBy' => 'elements.dateUpdated',
+                'attribute' => 'dateUpdated'
+            ],
         ];
     }
 

src/elements/Category.php

@@ -195,8 +195,16 @@ protected static function defineSortOptions(): array
         return [
             'title' => Craft::t('app', 'Title'),
             'uri' => Craft::t('app', 'URI'),
-            'elements.dateCreated' => Craft::t('app', 'Date Created'),
-            'elements.dateUpdated' => Craft::t('app', 'Date Updated'),
+            [
+                'label' => Craft::t('app', 'Date Created'),
+                'orderBy' => 'elements.dateCreated',
+                'attribute' => 'dateCreated'
+            ],
+            [
+                'label' => Craft::t('app', 'Date Updated'),
+                'orderBy' => 'elements.dateUpdated',
+                'attribute' => 'dateUpdated'
+            ],
         ];
     }
 

src/elements/Entry.php

@@ -353,8 +353,16 @@ protected static function defineSortOptions(): array
             'uri' => Craft::t('app', 'URI'),
             'postDate' => Craft::t('app', 'Post Date'),
             'expiryDate' => Craft::t('app', 'Expiry Date'),
-            'elements.dateCreated' => Craft::t('app', 'Date Created'),
-            'elements.dateUpdated' => Craft::t('app', 'Date Updated'),
+            [
+                'label' => Craft::t('app', 'Date Created'),
+                'orderBy' => 'elements.dateCreated',
+                'attribute' => 'dateCreated'
+            ],
+            [
+                'label' => Craft::t('app', 'Date Updated'),
+                'orderBy' => 'elements.dateUpdated',
+                'attribute' => 'dateUpdated'
+            ],
         ];
     }
 

src/elements/User.php

@@ -242,8 +242,16 @@ protected static function defineSortOptions(): array
                 'firstName' => Craft::t('app', 'First Name'),
                 'lastName' => Craft::t('app', 'Last Name'),
                 'lastLoginDate' => Craft::t('app', 'Last Login'),
-                'elements.dateCreated' => Craft::t('app', 'Date Created'),
-                'elements.dateUpdated' => Craft::t('app', 'Date Updated'),
+                [
+                    'label' => Craft::t('app', 'Date Created'),
+                    'orderBy' => 'elements.dateCreated',
+                    'attribute' => 'dateCreated'
+                ],
+                [
+                    'label' => Craft::t('app', 'Date Updated'),
+                    'orderBy' => 'elements.dateUpdated',
+                    'attribute' => 'dateUpdated'
+                ],
             ];
         } else {
             $attributes = [
@@ -252,8 +260,16 @@ protected static function defineSortOptions(): array
                 'lastName' => Craft::t('app', 'Last Name'),
                 'email' => Craft::t('app', 'Email'),
                 'lastLoginDate' => Craft::t('app', 'Last Login'),
-                'elements.dateCreated' => Craft::t('app', 'Date Created'),
-                'elements.dateUpdated' => Craft::t('app', 'Date Updated'),
+                [
+                    'label' => Craft::t('app', 'Date Created'),
+                    'orderBy' => 'elements.dateCreated',
+                    'attribute' => 'dateCreated'
+                ],
+                [
+                    'label' => Craft::t('app', 'Date Updated'),
+                    'orderBy' => 'elements.dateUpdated',
+                    'attribute' => 'dateUpdated'
+                ],
             ];
         }
 

src/templates/_elements/indexcontainer.html

@@ -18,6 +18,11 @@
 {% endif %}
 {% set sortOptions = elementInstance.sortOptions() %}
 
+{% macro sortOptionLabel(sortOption) %}
+    {{ sortOption.label ?? sortOption  }}
+{% endmacro %}
+{% from _self import sortOptionLabel %}
+
 <div class="main">
     <div class="toolbar">
         <div class="flex">
@@ -46,11 +51,11 @@
                 <div class="clear hidden" title="{{ 'Clear'|t('app') }}"></div>
             </div>
             <div>
-                <div class="btn menubtn sortmenubtn"{% if sortOptions %} title="{{ 'Sort by {attribute}'|t('app', { attribute: sortOptions|first }) }}"{% endif %} data-icon="asc">{{ sortOptions ? sortOptions|first }}</div>
+                <div class="btn menubtn sortmenubtn"{% if sortOptions %} title="{{ 'Sort by {attribute}'|t('app', { attribute: sortOptionLabel(sortOptions|first) }) }}"{% endif %} data-icon="asc">{{ sortOptions ? sortOptionLabel(sortOptions|first) }}</div>
                 <div class="menu">
                     <ul class="padded sort-attributes">
-                        {% for key, label in sortOptions %}
-                            <li><a{% if loop.first %} class="sel"{% endif %} data-attr="{{ key }}">{{ label }}</a></li>
+                        {% for key, sortOption in sortOptions %}
+                            <li><a{% if loop.first %} class="sel"{% endif %} data-attr="{{ sortOption.attribute ?? sortOption.orderBy ?? key }}">{{ sortOptionLabel(sortOption) }}</a></li>
                         {% endfor %}
                     </ul>
                     <hr>