Content overview page: make Author column sortable by default on new installations

Make Author column sortable

pls review the patch attached

Such a simple patch. Now that you mention it. It IS rather odd that the author is the only field in the default view that isn't sortable. I wonder if that was intentional or an outcome of converting the default admin_view for the content page to Drupal 8.

I think making this small change will dodge a source of confusion to content managers who visit this page on their shiny new Drupal 8 sites.

RTBC.

I believe this patch won't work as soon as one implementation of hook_user_format_name_alteralters the user name since the view will sort by the username stored in the database and not by the one displayed

Thanks for submitting this patch!

As a feature request, this change should now be postponed to 8.1.x since Drupal 8.0.x is in its release candidate phase. Reference: https://www.drupal.org/core/d8-allowed-changes

Well at least it is a simple thing to change

What's to review, it's a simple configuration change. Are you saying that it should be manually tested? I could do that.

Just checking if the patch still applies :)

@capuleto's point is worth discussing a bit more I think. Not sure if that's why it isn't sortable now.

Also sorting on username is only very helpful if you're looking for something authored by A or Z - otherwise you're clicking through pages and could use a filter instead.

Reviewed patch #3 in 8.2.x-dev and it works correctly.

can we add tests here for testing this functionality ?
do we need an update hook here for views update ?

@naveenvalecha I don;t think we need to change existing sites - the functionality is not that important - plus they could have manually changed this view.

There's still not a decent response to @capuleto's point in #7 - about what happens when the user name is changed completely by the hook. For example by the realname module.

What makes hook_user_format_name_alter() so special? There are lots of other hooks that may break the view. My point is that the developer who changes the name format owes fixing the view sorting to himself.

There is the realname module which implements this for example. It is more common to have this hook implemented than other hooks, which might break the view. In general though I agree with you, it should not prevent us from this here.

I tested this manually. It might still need automated tests, but I think this is a good feature to have, so helping it along.

Without the patch, the author is not sortable/clickable:

With the patch:

Ordered by updated date:

Click on "Author" ascending:

Click on "Author" again, descending:

I also tried changing usernames, and still all worked.

#1986606: Convert the comments administration screen to a view might be a related issue. Most patches there makes uid, hence author, sortable. And right now the comment overview form has "Author" sortable.

Thanks @arshadcn for looking for other precedents in core; that is helpful. Thanks also @ronchica for the thorough manual testing; that's exactly the sort of review this issue needed to make progress.

I am changing the title so that everyone will keep in mind this is just a small change to new installations and will not affect existing ones.

1. To me sorting on username did not seem that valuable... sites will have hundreds, thousands, or millions of pieces of content, so it would not be very usable to sort on username usually. On the other hand, sorting by published status makes far less sense, and the table lets you do that already in HEAD.
2. I think the discussion about the value of the sort when username display is often altered (#7 and on) is valuable. It looks like @dawehner has signed off as a subsystem maintainer in #21, so I think that is addressed now.
3. However, the sorting was removed on purpose in D7 critical issue #373897: admin/content still has sortable author column resulting in SQL errors for performance/scalability reasons. I don't see anyone addressing that yet after @Chi pointed it out. Maybe Views' caching etc. mitigates this performance concern in D8, but I have no idea. This view is a massive one on most sites so we need to actually profile it and confirm that the sorting doesn't cause a performance/scalability regression.
4. Finally, if the performance turns out to not be a problem, this patch does still need automated tests. This view is very important, and is tested thoroughly in core/modules/node/src/Tests/NodeAdminTest.php (although it's maybe not... the most modern automated test out there). Other sorting for the view is tested in that class, so let's add tests there for this feature as well. However, I would not do the work to add tests until we confirm the performance is acceptable.

NW for #3 especially.

Given the extent of my issue update in #27 and the fact that it was over seven years ago, this issue probably isn't a good novice contribution candidate. Thanks!

Rolled into an MR.

I think the decision on whether this makes sense is valid when sites implement hook_user_format_name_alter, however those sites could simply turn it off?

I don't think we need tests here as we're just toggling on sorting - this should be tested elsewhere.

I don't think we need profiling here as the db query is simple.

Seems pretty straight forward.

This is the only column in that view that's not on the node base table, and given there can be filters on published status etc., it seems quite possible we could end up with a temporary table and filesort on the query that there otherwise wouldn't be when it has to filter on one table and sort on another, so I think this does need an EXPLAIN per #27.

Agreed it doesn't need automated tests though, views should cover this in its own test coverage.

May be the right status?

Here's the explain on the query from loading the view and clicking the tablesort.

MySQL [local]> explain SELECT node_field_data.nid AS nid, users_field_data_node_field_data.uid AS users_field_data_node_field_data_uid
    -> FROM
    -> node_field_data node_field_data
    -> INNER JOIN users_field_data users_field_data_node_field_data ON node_field_data.uid = users_field_data_node_field_data.uid
    -> WHERE (node_field_data.status = 1 OR (node_field_data.uid = 1 AND 1 <> 0 AND 1 = 1) OR 1 = 1)
    -> ORDER BY users_field_data_node_field_data.name ASC
    -> LIMIT 50 OFFSET 0
    -> ;
+----+-------------+----------------------------------+------------+------+----------------------------------------------+---------+---------+---------------------------+------+----------+---------------------------------+
| id | select_type | table                            | partitions | type | possible_keys                                | key     | key_len | ref                       | rows | filtered | Extra                           |
+----+-------------+----------------------------------+------------+------+----------------------------------------------+---------+---------+---------------------------+------+----------+---------------------------------+
|  1 | SIMPLE      | node_field_data                  | NULL       | ALL  | node_field__uid__target_id                   | NULL    | NULL    | NULL                      |    1 |   100.00 | Using temporary; Using filesort |
|  1 | SIMPLE      | users_field_data_node_field_data | NULL       | ref  | PRIMARY,user__id__default_langcode__langcode | PRIMARY | 4       | local.node_field_data.uid |    1 |   100.00 | NULL                            |
+----+-------------+----------------------------------+------------+------+----------------------------------------------+---------+---------+---------------------------+------+----------+---------------------------------+
2 rows in set, 1 warning (0.000 sec)

So yeah we're using temp tables.

Setting to PMNMI for the decision on whether we want to do this. Personally I'm happy to close.

Given all the points raised in #27, especially the potential performance impact and the fact that this is so easy to toggle on for a site I think it's best that we leave it off by default.

Updating credits.