Link fields are not click-sortable in Views tables

Seems it just needs a minor coding standards fix (two lines at end of file while one expected).

Verified and tested patch #7.
Patch applied successfully and looks good to me.

Testing Steps:
# Goto: Appearance -> Apply Seven Theme
# Goto: Structure -> /admin/structure/types/manage/article/fields
# Add "Link" field
# Save it
# Goto: /admin/structure/views/view/content
# Click on "Settings" under Format
# User will able to see added "test link"
# Check the results

Expected Results:
# After applying the patch, the User is able to see the "Sortable Checkbox" for added test link.

Actual Results:
# Currently, the User is not able to see the "Sortable Checkbox" for added test link.

Please refer attached screenshots for the same.
Looks good to me.
Can be a move to RTBC.

I think this will need a test, adding tag. Also the issue summary needs an update, it should state the proposed resolution and the remaining tasks,

Applied Patch #47 working fine
After patch we can sort link field
Thanks for the patch
for ref sharing screenshot ...

Removing credit for unnecessary screenshots in #12. Screenshots were already provided in #9. This would be best confirmed by tests instead of screenshots anyway (hence the "needs tests" tag in #10)

Hey everyone! I tried my best to create a new test for this issue, I'm uploading three files, the first one is a test-only patch, hoping this fail to validate the issue, the second file is the patch itself and the third one is an interdiff.

I hope you can validate this, and if you have any questions, please let me know.

Thanks!

@quietone, I'll remove the tags, thanks to @liquidcms for updating the issue summary, and about the tests, I'll keep this to needs review to see what you guys think about #16.

Thanks!

After applying the patch I'm not seeing the sortable checkbox next to a link field in the view.

I attempted to apply this patch using composer and the file wasn't created.

When I manually created the file based upon the patch I could indeed click sortable and sort the table based upon the link.

But one issue I noticed is that it sorts based upon the URL rather than the display name which probably isn't ideal for everyone.

I'm also not sure if the best way to accomplish this fix is through using a hook_alter for views rather than fixing the links module directly, but that's going to require more research on my part to determine how other fields tell views they are sortable.

Applied patch from #16 and it works in both drupal 10 and 11 versions.

@robbit, Link field can be sorted based on title by changing Column used for click sorting from URL to title.

I also checked core to see how other fields have enabled click sorting
Modules such as statistics, dblog etc use hook_views_data() to define fields and add 'click sortable' parameter from there.

Node module uses views_data handler "views_data" = "Drupal\node\NodeViewsData" for node entity and 'click sortable' added to fields from NodeViewsData.

taxonomy uses hook_views_data_alter() to add some fields and click sorting is added from there. So I think current implementation would be fine.

Used by Style: Table to determine the actual column to click sort the field on. The default is usually fine.

This is the default description added to click sort column field.

    // No need to ask the user anything if the field has only one column.
    if (count($field->getColumns()) == 1) {
      $form['click_sort_column'] = [
        '#type' => 'value',
        '#value' => $column_names[0] ?? '',
      ];
    }
    else {
      $form['click_sort_column'] = [
        '#type' => 'select',
        '#title' => $this->t('Column used for click sorting'),
        '#options' => array_combine($column_names, $column_names),
        '#default_value' => $this->options['click_sort_column'],
        '#description' => $this->t('Used by Style: Table to determine the actual column to click sort the field on. The default is usually fine.'),
      ];
    }

Maybe we can exclude "Used by Style:" and add only "Table to determine the actual column to click sort the field on. The default is usually fine" as the description?

Moving to needs review for #24. If description change is not needed, issue can be moved to RTBC

So I was able to get this to patch to apply. I realized I needed to set patch level to 2 as described here - https://www.drupal.org/docs/develop/using-composer/manage-dependencies#s... in order to apply a patch to core using composer-patches.

With that working I do think it makes sense to alter the description to say Used by Format: Table rather than Used by Style: Table which I think was from a previous version of Views.

I've created a new patch based upon 11.x that makes this change as well, so if anyone can test and review it then I think we can change it to RTBC as @akhil_babu suggests. My change merely updates the description and is based otherwise entirely on the patch on #16

The patch in #27 was failing drupal coding standard tests so I've added a new one that should at least pass the issue it was failing on (a lack of a <?php tag.

Otherwise if this passes the automated tests it should be ready for review.

From what I can tell the tests are check that the checkbox is there but doesn't test the sort?

Think we could also extend the tests to include the different column sort.

I think that testing the sort is a good idea. I haven't written any tests for a Drupal core contribution yet but I'm willing to give it a shot.
In looking at the code it appears that web/core/modules/views/tests/src/Functional/Handler/FieldWebTest.php has a test for testing click sorting that could be used as the basis for a new test of this functionality.

Also I see web/core/modules/views/tests/src/FunctionalJavascript/ClickSortingAJAXTest.php which tests the same thing via AJAX.

In theory I think it would make sense to also test the functionality of sorting based upon the title (if it is enabled) and also based upon the uri (the default). I'm not sure how to mock this test up exactly but I'll be digging into other tests and reading the documentation to try to figure it out but any help or pointers in the right direction would be appreciated.

From a UI/UX perspective I would think that if a link field uses a title/display that would be the default sort chosen since that is what usually displays but that would require further changes to this code and probably views (beyond updating the description). I also don't know what sorting by the options would mean and if this even makes sense to include.

I'm here at DrupalCon Atlanta in the mentored contribute @partyka and we looked under the core directory and don't see any other fields that are have their 'click sortable' tested. I believe that whether a field gets sorted or not is actually handled by Views code and so adding the ability to sort links shouldn't require a test of the sort. Going to push the code and update this to a MR.

This is currently not working because it uses the deprecated .views.inc approach that was removed from D11. It needs to be refactored to use ModuleViewsData.php to extend EntityViewsData in order to set click sortable to true. Although typically click sortable is enabled by default.

It looks like the appropriate thing to do for D11 compatibility is to change this to a views hook - see the commit https://git.drupalcode.org/project/drupal/-/commit/1da8aa097417905a9772d... and this issue https://www.drupal.org/project/drupal/issues/3489415

Essentially just need to rewrite the .views.inc to be compatible with the specific hook type.

Like for instance https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/taxon... replaced https://git.drupalcode.org/project/drupal/-/blob/10.5.x/core/modules/tax...

I'm reclassifying this as a feature request. It may seem like a bug because of "Column used for click sorting" setting, but Views adds that setting anytime a field has multiple columns. The inclusion of that setting element for fields that can't be click-sorted may be a bug and a follow-up issue could be opened for it. But in the end, this issue is asking for a feature to be added that wasn't present before now rather than fixing something that was broken.

I converted the patch from #28 to an MR, making several modifications along the way. Mainly, the changes were upgraded to the most current APIs, including the change to a Hook class as noted in #33. I also removed the text change in EntityField. Not only do I think it's out-of-scope for this issue, a change to a translatable string in a separate module may make it more difficult to get this change in. If you want to see the text changed, then open a follow-up issue.

From what I can tell the tests are check that the checkbox is there but doesn't test the sort?

Think we could also extend the tests to include the different column sort.

Personally, this feels like a "Shouldn't we trust the Views API to be tested and do its job?" kind of thing. I'm not sure writing a Functional or FunctionalJavascript test to test the functionality of a separate module adds much of value to this one. Hence, I'm setting this back to Needs Review.

Test coverage can be found here https://git.drupalcode.org/issue/drupal-3239705/-/jobs/7436803

I did verify this on a standard profile install
Added a test_link field to the Article content type
Used the admin content view for testing and yup can't sort by link
Applying MR I can now check the box

Left some comments in the MR.