Views exposed sort identifiers are not configurable

This is great! Code looks good and works as advertised!

Adding this to get this in line with exposed filters makes sense.

+++ b/core/modules/views/src/Plugin/views/sort/SortPluginBase.php
@@ -49,6 +49,7 @@ protected function defineOptions() {
+        'identifier' => ['default' => ''],

the additional option needs to be added to the config schema, hence the fails. This also means that this would need an upgrade path.

Also, this needs tests.

Changed the patch to apply to core 8.8.2

Fixed patch for core 8.8.2

Hope I got it right this time. :p

Update for 8.8.4 - Removed the label escaping per #2956722: Exposed sort label is double-escaping special characters (apostrophe)

+++ b/core/modules/views/src/Plugin/views/sort/SortPluginBase.php
@@ -225,6 +278,7 @@ public static function trustedCallbacks() {
+      'identifier' => $this->options['id'],

This is a new config key and needs a schema. It might be the source of so many failures.

Assigning to try a fix.

Here's a schema fix.

Added a tested upgrade path.

This is weird, I cannot replicate locally some of the failures. Still fixed one.

1. We need to fix also the default configuration.

2. +++ b/core/modules/views/src/Plugin/views/exposed_form/ExposedFormPluginBase.php
   @@ -157,19 +157,17 @@ public function query() {
   +        elseif (!empty($sort->options['expose']['identifier']) && $sort->options['expose']['identifier'] == $sort_by) {
   +          if (isset($exposed_data['sort_order']) && in_array($exposed_data['sort_order'], ['ASC', 'DESC'])) {
   

   Let's be strict.

3. +++ b/core/modules/views/src/Plugin/views/sort/SortPluginBase.php
   @@ -207,7 +208,59 @@ public function buildExposeForm(&$form, FormStateInterface $form_state) {
   +  public function validateExposeForm($form, FormStateInterface $form_state) {
   +    $identifier = $form_state->getValue(['options', 'expose', 'identifier']);
   +    $this->validateIdentifier($identifier, $form_state, $form['expose']['identifier']);
   +  }
   ...
   +  protected function validateIdentifier($identifier, FormStateInterface $form_state = NULL, &$form_group = []) {
   +    $error = '';
   +    if (empty($identifier)) {
   +      $error = $this->t('The identifier is required if the sort is exposed.');
   +    }
   +    elseif ($identifier == 'value') {
   +      $error = $this->t('This identifier is not allowed.');
   +    }
   +    elseif (preg_match('/[^a-zA-z0-9_~\.\-]/', $identifier)) {
   +      $error = $this->t('This identifier has illegal characters.');
   +    }
   +
   +    if ($form_state && !$this->view->display_handler->isIdentifierUnique($form_state->get('id'), $identifier)) {
   +      $error = $this->t('This identifier is used by another handler.');
   +    }
   +
   +    if (!empty($form_state) && !empty($error)) {
   +      $form_state->setError($form_group, $error);
   +    }
   +    return $error;
      }
   

   This code is almost the same with the one from filter. Let's avoid code duplication.

4. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -477,4 +480,44 @@ protected function mapOperatorFromSingleToMultiple($single_operator) {
   +    if ($handler_type === 'sort' && !empty($handler['exposed'])) {
   
   +++ b/core/modules/views/tests/fixtures/update/update_2897638_sort_identifier.php
   @@ -0,0 +1,23 @@
   +$config['display']['default']['display_options']['sorts']['created']['exposed'] = TRUE;
   
   +++ b/core/modules/views/tests/src/Functional/Update/ViewsSortIdentifiersUpdate.php
   @@ -0,0 +1,42 @@
   +    $this->assertTrue($view->get("{$trail}.exposed"));
   ...
   +    $this->assertTrue($sort['exposed']);
   

   In fact we need to add the default value also for non-exposed sorts.

Keep assigned to fix the above.

Fixed #23, still needs tests.

Forgot the @group tag for the new test.

Fixing title, IS.

A patch for 8.9.x for whom may concern.

More default config to be fixed...

• Reverted the identifier validation code reusability. The sort identifier is different.
• Fixing views config updater.
• [CSS] The new textfield should have the same indent as the label.
• Add tests for for the admin UI part. Still needs tests with exposed form.

Adding tests for custom sort identifier functionality.

• Added screenshot for Views UI changes.
• Added change notice draft: https://www.drupal.org/node/3169366

• Fixing the release note snippet.
• Small docblock fix.

It was a random failure. Back to NR.

This looks great.

The one thing I would like to see added to the test coverage is that the 'uniqueness' validation also works cross handler, so we are sure that any collisions between exposed filter identifiers and sort identifiers is also detected. Looking at the code in \Drupal\views\Plugin\views\display\DisplayPluginBase::isIdentifierUnique, it should work as it stands now, but since currently this code only needs to work for filters I think it would be good to add test coverage for that.

Related to #8, @Lendude and me, we had a discussion on Slack #contribute channel:

clau
I wonder why a filter identifier should not be the same as a sort identifier. Isn’t that safe? Why?

clau
I think a filter and a sort identifier could be the same (edited)

clau
I would add a new parameter to method:

public function isIdentifierUnique($id, $identifier, $handler_type = NULL) {
  if (!$handler_type) {
    $handler_type = 'filter';
    @trigger_error(...deprecation message);
  }
  ... search only in the scope of $handler_type
}

clau
And, course, fix all the calls to isIdentifierUnique() to pass the type

lendude
If they are the same would that give you a weird query string? since then they would be using the same query parameter?

lendude
ah no, because order always uses the ‘order’ get parameter

clau
actually. sort_by=

lendude
Ah ok, was looking at click sorting :)

lendude
Ok, then your scenario makes sense. You should be able to use the same exposed identifier between sorts and filters, and right now, that will not pass validation

Fixed #39. Updated IS & change record.

Thinking more on #40. This change is a little risky if a module created a handler, other than filter which adds its query string params. Also it changes the behaviour of the method, We cannot anticipate if the method is used for other reasons in contrib/custom code. So, maybe is safer to keep it as it is and use a custom code in SortPluginBasethat only checks for sort handlers.

Reroll

+++ b/core/modules/views/tests/src/Functional/Update/ViewsSortIdentifiersUpdateTest.php
@@ -0,0 +1,44 @@
+declare(strict_types = 1);

Don't think we do this in core?

+++ b/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
@@ -2553,6 +2553,13 @@ public function newDisplay() {
   public function isIdentifierUnique($id, $identifier) {
     foreach (ViewExecutable::getHandlerTypes() as $type => $info) {
+      if ($type === 'sort') {
+        // The exposed sort identifier is the value of 'sort_by' query string
+        // parameter and cannot collide with $identifier as the later is used as
+        // query string parameter key.
+        continue;
+      }

As pinged on Slack, should we consider not naming the sort identifier 'identifier' in config, so we prevent the logic in isIdentifierUnique() from triggering?

I think #44 is a great idea as long as exposed sort identifier are a different kind of fish than the filter identifiers. I've renamed identifier to field_identifier as it refers to the field being sorted on.

Re-rolled the patch for 9.1.6 - first time I do for a so long patch, I hope I've done right but wouldn't hurt if someone could check it.

Reroll patch for 9.2.x.

Fixed https://www.drupal.org/project/drupal/issues/2897638#mr54-note4976

@KapilV, the 9.2 (official) change is in https://git.drupalcode.org/project/drupal/-/merge_requests/54 merge request

Hard one to review with all the views getting changed, but tried to focus on the non-config changes, and I think this looks good now. Nice work!

MR !54 needs a rebase against 9.3.x. Only the original creator of the MR can do so.

Rebased.

How can I apply the latest version to my project?

I know how to apply a patch with composer, but I don't understand how to do with this merge request.

This looks good to me, but it needs another rebase.

@Giuseppe87 you can use https://git.drupalcode.org/project/drupal/-/merge_requests/54.diff - although to get a stable patch to apply, it's best to download this to a /patches directory in your codebase, and then add the local patch path to composer.

Rerolled against latest 9.3.x.
Back to RTBC per #54.

Committed/pushed to 9.3.x, thanks!

Tagging for highlights since this is a UX improvement.

Now with even more accurate tags! 😇

Publish the CR