Problem/Motivation

When using table display, any fields (numeric, date, etc) that should be click sortable are not.

To reproduce

  1. Add a number field to the article type
  2. Create a view of articles and add this field to the list of fields
  3. Change display format to 'Table'
  4. The new field cannot be selected to sort on

Proposed resolution

Most field types should be click-sortable.

Remaining tasks

Fix the issue.

User interface changes

Fields will once again be click sortable.

API changes

None anticipated.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category You can't click sort fieldapi fields, this is a bug
Issue priority Major, because in can involve pretty much every table view
Disruption Just a bug fix
CommentFileSizeAuthor
#13 click-sort-2395763-13.patch2.62 KBjhedstrom
#13 interdiff.txt1.92 KBjhedstrom
#10 click-sort-2395763-09.patch2.31 KBjhedstrom
#9 click-sort-2395763-09.patch0 bytesjhedstrom
#9 interdiff.txt2.73 KBjhedstrom
#1 click-sort-2395763-01.patch1.62 KBjhedstrom
#1 click-sort-2395763-01-TEST-ONLY.patch819 bytesjhedstrom
Table_sort_view__Content____Site-Install.png173.21 KBjhedstrom

Comments

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Active » Needs review
StatusFileSize
new click-sort-2395763-01-TEST-ONLY.patch819 bytes
new click-sort-2395763-01.patch1.62 KB

The last submitted patch, 1: click-sort-2395763-01-TEST-ONLY.patch, failed testing.

sam152’s picture

sam152 commented

Functionality works as advertised. +1 to RTBC.

olli’s picture

olli commented
Related issues: +#1891036: Make 'click sortable' default to TRUE

Why not make all fields that use sort also "click sortable"?

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

Re: #4 I added this logic:

+++ b/core/modules/field/field.views.inc
@@ -421,6 +421,13 @@ function field_views_field_default_views_data(FieldStorageConfigInterface $field
+        // Allow click sortable on table displays if this column matches the
+        // real field.
+        if (isset($data[$table_alias][$field_name]['field']['real field'])
+          && $column_real_name == $data[$table_alias][$field_name]['field']['real field']) {
+          $data[$table_alias][$field_name]['field']['click sortable'] = TRUE;
+        }
       }

for a reason that I can't remember now. Simply making all fields here click-sortable resulted in SQL errors for certain fields.

idebr’s picture

idebr commented
Status: Needs review » Needs work

The original issue #1891036: Make 'click sortable' default to TRUE as well as the change record #1895020: 'click sortable' = TRUE is now standard in views data suggest that click sortable defaults to TRUE. However Field::clickSortable() is still disables click sorting by default:

  /**
   * Determine if this field is click sortable.
   */
  public function clickSortable() {
    // Not click sortable in any case.
    if (empty($this->definition['click sortable'])) {
      return FALSE;
    }

Should this patch not be the other way around?

  • Default Field::clickSortable() to return TRUE
  • Disable click sortable for incompatible fields

This probably belongs in the field.module component.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

Interesting, I hadn't seen the logic mentioned in #6 while working on this. I'll work on reversing the logic here and make a slight adjustment to the Field::clickSortable() method.

Given #2399931: Generic entity api field handler should live in views module not in field module, I think the views.module component still makes sense for this issue.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

Ah! Re #4. Working on this now I remembered why this was needed:

+        // Allow click sortable on table displays if this column matches the
+        // real field.
+        if (isset($data[$table_alias][$field_name]['field']['real field'])
+          && $column_real_name == $data[$table_alias][$field_name]['field']['real field']) {

Adding 'click sorting' for all columns that are deemed sortable doesn't work here because the 'field' array is only defined for the $field_name (this function is incredible complex/confusing).

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt2.73 KB
new click-sort-2395763-09.patch0 bytes

This defaults Field::clickSortable to only return false if click sorting is explicitly disabled.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
StatusFileSize
new click-sort-2395763-09.patch2.31 KB

And a non-empty patch.

damiankloip’s picture

damiankloip commented
Status: Needs review » Needs work

Overall, looks fine. Setting

  1. +++ b/core/modules/views/src/Plugin/views/field/Field.php
@@ -345,7 +345,7 @@ function add_field_table($use_groupby) {
+    if (isset($this->definition['click sortable']) && $this->definition['click sortable'] === FALSE) {

    Can we just make this simpler while we are here?

    if (isset($this->definition['click sortable'])) {
  return (bool) $this->definition['click sortable'];
}

    and just return it if it's set? what do you think? 

  2. +++ b/core/modules/views/views.views.inc
@@ -583,6 +583,13 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
+      // Set click sortable on table displays if this column matches the
+      // real field.

    This comment seems weird, maybe "Set click sortable for.." or remove the part about table displays.

damiankloip’s picture

damiankloip commented 
+++ b/core/modules/views/views.views.inc
@@ -583,6 +583,13 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
+      if (isset($data[$table_alias][$field_name]['field']['real field'])
+        && $column_real_name == $data[$table_alias][$field_name]['field']['real field']) {
+        $data[$table_alias][$field_name]['field']['click sortable'] = $allow_sort;
+      }

Also, in this context, isn't 'real field' always going to get set? So this could just live with the rest of the definition?

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.92 KB
new click-sort-2395763-13.patch2.62 KB

This should address #11.

Regarding #12, I don't think it is always set, or if it is, there isn't always a field definition at that level. I've changed the patch to what I think is actually supposed to happen here (eg, check if there is a field definition).

dawehner’s picture

dawehner
Primary language German
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

+1 The new patch makes more sense.

Moved to major, as it will be even more important in the future (#2342045: Standard views base fields need to use same rendering as Field UI fields, for formatting, access checking, and translation consistency)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Priority: Normal » Major
dawehner’s picture

dawehner
Primary language German
commented

good spot @tim.plunkett !

  • webchick committed 61f2a71 on 8.0.x 
    Issue #2395763 by jhedstrom: Fields are not 'click sortable' in views
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

liquidcms’s picture

liquidcms commented

Not sure if anyone sees closed issues; but trying to sift through this to see why it breaks Link fields from being click sortable (even though the field config allows you to pick which part of the field this is sorted on). Hard to believe this has been missed all these years.

I posted the bug here: #3239705: Link fields are not click-sortable in Views tables