Views "Global: Custom text" field handler should not be sortable

Moving to the right queue.

Updating the title and Issue summary to reflect the actual bug.

On a side note, the original post was for entity references not being sortable, but that works now.

I've also just run into this error.

Drupal\Core\Database\DatabaseExceptionWrapper: Exception in Projects[projects]: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'unknown' in 'order clause': SELECT node_field_revision.vid AS vid, node_field_data_node_field_revision.nid AS node_field_data_node_field_revision_nid, users_field_data_node_field_revision.uid AS users_field_data_node_field_revision_uid, content_moderation_state_field_data_node_field_data.id AS content_moderation_state_field_data_node_field_data_id, content_moderation_state_field_revision_node_field_revision.revision_id AS content_moderation_state_field_revision_node_field_revision_ FROM {node_field_revision} node_field_revision LEFT JOIN {node_field_data} node_field_data_node_field_revision ON node_field_revision.nid = node_field_data_node_field_revision.nid AND node_field_data_node_field_revision.langcode = node_field_revision.langcode LEFT JOIN {users_field_data} users_field_data_node_field_revision ON node_field_revision.uid = users_field_data_node_field_revision.uid LEFT JOIN {content_moderation_state_field_data} content_moderation_state_field_data_node_field_data ON node_field_data_node_field_revision.nid = content_moderation_state_field_data_node_field_data.content_entity_id AND content_moderation_state_field_data_node_field_data.content_entity_type_id = :views_join_condition_1 LEFT JOIN {content_moderation_state_field_revision} content_moderation_state_field_revision_node_field_revision ON node_field_revision.vid = content_moderation_state_field_revision_node_field_revision.content_entity_revision_id AND content_moderation_state_field_revision_node_field_revision.content_entity_type_id = :views_join_condition_2 LEFT JOIN {node_field_revision} node_field_revision2 ON node_field_revision.nid = node_field_revision2.nid AND node_field_revision2.vid > node_field_revision.vid INNER JOIN {node__field_project_director_area} node_field_data_node_field_revision__node__field_project_director_area ON node_field_data_node_field_revision.nid = node_field_data_node_field_revision__node__field_project_director_area.entity_id AND node_field_data_node_field_revision__node__field_project_director_area.deleted = :views_join_condition_4 WHERE ((node_field_data_node_field_revision__node__field_project_director_area.field_project_director_area_target_id = :node__field_project_director_area_field_project_director_area_target_id)) AND ((node_field_revision2.nid IS NULL) AND (node_field_data_node_field_revision.type IN (:db_condition_placeholder_5))) ORDER BY unknown ASC LIMIT 50 OFFSET 0; Array ( [:node__field_project_director_area_field_project_director_area_target_id] => 494 [:db_condition_placeholder_5] => project [:views_join_condition_1] => node [:views_join_condition_2] => node [:views_join_condition_4] => 0 ) in Drupal\views\Plugin\views\query\Sql->execute() (line 1543 of /code/web/core/modules/views/src/Plugin/views/query/Sql.php).

Anybody got an idea for a fix?

@artmarks7 was that also with a Custom text field?

Just looking at it, I'd say the custom text field shouldn't be click sortable at all, it is not part of the query so it won't work.

Really not sure if this needs a test, we are just changing a setting after all, not updating any logic

Also, doing an upgrade path for this feels very excessive, since the upgrade would do the same as a manual fix, just remove the click sorting. It's not like it would suddenly start to work.

So, just some before and after screenshots for now

Before

After

Agree, not sure on how to test this anyway, we'd be just testing the views_data (kernel) or the UI

This is a logical change so I don't think we need tests here. The sortable setting option is already has tests in \Drupal\Tests\views\Kernel\Handler\FieldKernelTest::testClickSortable(). Given that I'm going to set it to RTBC. Also updated IS and added screenshots to IS as well.

The fix looks fine but I think we need to update existing views - no? And we also need a post update function to clear the cache.

Added post update function to clear the cache.

This updates the existing Views in core, only 4 use this plugin and only 1 of those uses the table style, so updated that.

Since we still have #2917814: Views table style info never gets updated when changing/adding fields, I don't think we should bother writing an update hook to do this in project config, the data in that part of the configuration can be hopelessly out of sync anyway, so we would probably be updating garbage. But that is just my 2ct...

@Lendude we still need the empty post update to clear the cache so the change is available.

@anmolgoyal74 added the empty post update in #16, that should do the trick right?

@Lendude @anmolgoyal74 oops I missed that. Because we need an update this is 9.2.x only. #2917814: Views table style info never gets updated when changing/adding fields looks nice and tricky but I agree if we're already out of sync then this is already a mess.

Hi, the patch in #17 didn't apply on 9.2.x-dev, so I've rerolled it.

Please review.

Setting back to RTBC, I'm not sure if @alexpott's comment in #21 is agreeing with #17, i.e. that no update hook is fine - deferring to @alexpott to confirm that's the case

Reroll in #22 looks good.
I agree we do not need an update hook given how complicated / messy #2917814: Views table style info never gets updated when changing/adding fields makes this.
No code style problems.
Also agree there's nothing really to add a test for here, and all existing tests pass.

+1 to RTBC. Thanks, everyone!

Committed/pushed to 9.2.x, thanks!

This doesn't cherry-pick cleanly to 9.1.x.

Re-rolled the patch against 9.1.x. Plaese review

Agreed that we can add the update hook in #2917814: Views table style info never gets updated when changing/adding fields - it'll cover the change made here but more generically, and no point trying to update the data without that issue being fixed. If the order had been different we'd need to re-run similar logic here too though.

For patches that are only going to land in a minor, I think we can skip empty post updates since there's an extremely high likelihood that there'll be another core update or something to clear the cache. However since this is going into 9.1.x too the empty post update is good.

If we tried to backport this to 8.9.x we'd have out-of-sync post updates with 9.0.x to worry about, but since this is 'normal' we can skip the backport, and hence not worry about that.

The 9.1.x patch looks good so have committed/pushed to 9.1.x too. Thanks everyone.