WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#10329 closed enhancement (fixed)

sort_by and count_limit options for wp_list_authors

Reported by: takaitra Owned by: scribu
Milestone: 3.1 Priority: normal
Severity: normal Version:
Component: Template Keywords: has-patch
Focuses: Cc:

Description

As author of the List Authors widget plugin, there are two options I suggest to add to the wp_list_authors template function: sort_by and count_limit. Both of these would address frequent feature requests for the author list.

sort_by options:
name - sorts by author name (or full name if show_fullname=1)
post_count - sorts by number of author posts in descending order

count_limit: when passed a positive integer, will limit the list to that number of authors.

I reviewed the function and believe that count_limit would be almost trivial to implement via adding a limit to the SELECT statement. The post_count sorting is not as trivial but could be done. I would be willing to code and submit a patch.

Attachments (7)

author-template.diff (4.2 KB) - added by takaitra 4 years ago.
Patch to add sort_by and count_limit options.
author-template.r12704.diff (4.8 KB) - added by takaitra 4 years ago.
Updated patch for latest trunk. Multi-blog logic. Options renamed "orderby," "order," and "number" to match the other template tags.
11017.2.2.patch (1.6 KB) - added by ramiy 4 years ago.
10329.diff (6.0 KB) - added by takaitra 4 years ago.
10329.2.diff (6.7 KB) - added by Denis-de-Bernardy 4 years ago.
10329.3.diff (7.0 KB) - added by takaitra 4 years ago.
10329.4.diff (7.0 KB) - added by takaitra 4 years ago.
Updated diff against r15567 of author-template.php

Download all attachments as: .zip

Change History (45)

comment:1 in reply to: ↑ description ; follow-up: Denis-de-Bernardy5 years ago

  • Milestone changed from Unassigned to Future Release

Replying to takaitra:

I reviewed the function and believe that count_limit would be almost trivial to implement via adding a limit to the SELECT statement. The post_count sorting is not as trivial but could be done. I would be willing to code and submit a patch.

Not so. if the first batch of authors are subscribers with no posts and you're using hide_empty, you'd get random results.

There's a separate ticket with very similar requests, btw, alongside optimization requests.

comment:2 in reply to: ↑ 1 takaitra5 years ago

Replying to Denis-de-Bernardy:

Replying to takaitra:

Not so. if the first batch of authors are subscribers with no posts and you're using hide_empty, you'd get random results.

There's a separate ticket with very similar requests, btw, alongside optimization requests.

I think you are referring to #5407? You are right, with the current query it won't work. In the JOIN query of the optimized suggestion I don't see a problem because the post count will be part of the first SELECT. Feel free to close this and I can follow the other discussion.

takaitra4 years ago

Patch to add sort_by and count_limit options.

comment:3 takaitra4 years ago

  • Keywords has-patch added; wp_list_authors removed

comment:4 takaitra4 years ago

  • Milestone changed from Future Release to 3.0

Based on input from #wordpress-dev, asking for consideration in the 3.0 milestone.

I tested the patch with many combinations of sort_by, count_limit and the other options. I also tested with and without an author with zero posts. All combinations worked correctly.

comment:5 westi4 years ago

  • Owner set to westi
  • Status changed from new to reviewing

takaitra4 years ago

Updated patch for latest trunk. Multi-blog logic. Options renamed "orderby," "order," and "number" to match the other template tags.

comment:6 voyagerfan57614 years ago

  • Cc WordPress@… added

comment:7 follow-up: nacin4 years ago

See #11017 for a potential duplicate.

comment:8 in reply to: ↑ 7 takaitra4 years ago

Replying to nacin:

See #11017 for a potential duplicate.

I agree, it's a duplicate. My patch implements the same two new options for wp_list_authors but also adds the "number" option. The latest SVN revision of author-template.php added multi-blog logic to this template tag and my patch supports this. My patch also implements the efficiency improvement suggested in #5407 which is a big deal for blogs with many authors.

comment:9 follow-up: ramiy4 years ago

if we set the #11017 as a duplicate, at least try to implement it's solution in this ticket.

# add the order parameter (ASC, DESC).
# validated the new parameters, $order, $orderby and $number.

comment:10 in reply to: ↑ 9 takaitra4 years ago

Replying to ramiy:

if we set the #11017 as a duplicate, at least try to implement it's solution in this ticket.

# add the order parameter (ASC, DESC).
# validated the new parameters, $order, $orderby and $number.

Sure. I will update the patch tomorrow to include the order parameter.

comment:11 nacin4 years ago

  • Milestone changed from 3.0 to Future Release

comment:12 ramiy4 years ago

Replying to nacin:

Nacim, why Future Release?
We postpone this enhancement since 2.8.

comment:13 follow-up: nacin4 years ago

The patch doesn't seem up for inclusion -- it appears to generate an unnecessarily complex query -- but more importantly it hasn't been reviewed, benchmarked, or tested.

comment:14 Denis-de-Bernardy4 years ago

this:

if ( $orderby == 'name' ) { 
  $author_sort = "ORDER BY author_name $order"; 
} else { 
  $author_sort = "ORDER BY author_count $order"; 
} 

should be something like:

if ( $order == 'count' ) { 
  $author_sort = "ORDER BY author_count $order"; 
} else { 
  $author_sort = "ORDER BY author_name $order"; 
} 

comment:15 Denis-de-Bernardy4 years ago

  • Keywords needs-patch added; has-patch removed

there's also an error in the query: the $wpdb->posts table used in the count() statement isn't always defined.

I think that the count()/group by statements should only be present when the count information is requested.

likewise, LEFT JOIN $wpdb->usermeta AS meta_capabilities doesn't seem to be used. shouldn't that be more like an inner join, which would get tossed in if and only if non-empty authors are not requested?

comment:16 scribu4 years ago

Related: #11914

comment:17 Denis-de-Bernardy4 years ago

btw, re the count() statement... another screaming option is a min_count. to achieve that using sql, add a having clause:

having author_count >= $min_count

Re: nacin -- "it appears to generate an unnecessarily complex query" that hasn't been benchmarked

if the keys mentioned in #11213 get checked in, it'll be a *much* faster query than the zillions of "simpler" queries that we're doing to achieve the same result.

comment:18 in reply to: ↑ 13 ramiy4 years ago

Replying to nacin:

The patch doesn't seem up for inclusion -- it appears to generate an unnecessarily complex query -- but more importantly it hasn't been reviewed, benchmarked, or tested.

ok nacin, if this patch is too coplex, lets use #11017 patch, its a simpler solotion.

ramiy4 years ago

takaitra4 years ago

comment:19 takaitra4 years ago

Created 10329.diff based on today's discussion:

  • Added inline comments
  • Added min_count option
  • meta_capabilities changed to an inner join. It is used limit to authors from the current blog.
  • Sort by author name is the catch-all in the if/else statement.

Denis-de-Bernardy4 years ago

comment:20 Denis-de-Bernardy4 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 3.0

amended patch fixes conditional insertion of fields, joins and having clauses based on the various arguments, which I nailed by testing this by turning them on and off in all sorts of combinations while reviewing takaitra's patch.

comment:21 Denis-de-Bernardy4 years ago

quick additional note: I took the liberty to rehash the sql a bit. namely to exclude the weird-looking NULL checks, where our tables don't accept NULL values. I also excluded private posts from the counts, as it doesn't seem to make any sense for those to be public.

posting some status in a bit.

comment:22 Denis-de-Bernardy4 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 3.0 to 3.1

before patch (each adds a whole bunch of extra queries in addition to the signature):

wp_list_authors(array(
	'optioncount' => false, 'exclude_admin' => false,
	'show_fullname' => false, 'hide_empty' => false,
	));

SELECT DISTINCT post_author, COUNT(ID) AS count FROM www_posts WHERE post_type = 'post' AND (post_status = 'publish' OR post_status = 'private') GROUP BY post_author

7.8ms

SELECT * FROM users WHERE ID = 12 LIMIT 1

0.2ms times as many authors

SELECT meta_key, meta_value FROM usermeta WHERE user_id = 12

0.3ms times as many authors

wp_list_authors(array(
	'optioncount' => true, 'exclude_admin' => false,
	'show_fullname' => true, 'hide_empty' => true,
	));

SELECT user_id, user_id AS ID, user_login, display_name, user_email, meta_value FROM users, usermeta WHERE users.ID = usermeta.user_id AND meta_key = 'www_capabilities' ORDER BY usermeta.user_id

1.0ms

SELECT ID, user_nicename from users WHERE ID IN(1,6,10,11,12) ORDER BY display_name

0.3ms can be much larger with many authors

and then the same queries as above

after patch:

wp_list_authors(array(
	'optioncount' => false, 'exclude_admin' => false,
	'show_fullname' => false, 'hide_empty' => false,
	'orderby' => 'name', 'order' => 'ASC', 'min_count' => false
	));

		SELECT users.ID, users.user_nicename, users.display_name as author_name
		FROM users
		
		
		JOIN usermeta AS meta_capabilities ON meta_capabilities.user_id = users.ID AND meta_capabilities.meta_key = 'www_capabilities'
		
		
		
		ORDER BY author_name ASC

1.0ms


wp_list_authors(array(
	'optioncount' => true, 'exclude_admin' => false,
	'show_fullname' => false, 'hide_empty' => false,
	'orderby' => 'name', 'order' => 'ASC', 'min_count' => false
	));


		SELECT users.ID, users.user_nicename, users.display_name as author_name, COUNT(www_posts.ID) as author_count
		FROM users
		LEFT JOIN www_posts ON www_posts.post_author = users.ID
		
		JOIN usermeta AS meta_capabilities ON meta_capabilities.user_id = users.ID AND meta_capabilities.meta_key = 'www_capabilities'
		WHERE www_posts.post_type = 'post' AND www_posts.post_status = 'publish'
		GROUP BY users.ID
		
		ORDER BY author_name ASC

33.0ms

wp_list_authors(array(
	'optioncount' => true, 'exclude_admin' => false,
	'show_fullname' => false, 'hide_empty' => true,
	'orderby' => 'name', 'order' => 'ASC', 'min_count' => false
	));

		SELECT users.ID, users.user_nicename, users.display_name as author_name, COUNT(www_posts.ID) as author_count
		FROM users
		JOIN www_posts ON www_posts.post_author = users.ID
		
		
		WHERE www_posts.post_type = 'post' AND www_posts.post_status = 'publish'
		GROUP BY users.ID
		HAVING author_count >= 1
		ORDER BY author_name ASC

8.5ms

wp_list_authors(array(
	'optioncount' => true, 'exclude_admin' => false,
	'show_fullname' => false, 'hide_empty' => false,
	'orderby' => 'count', 'order' => 'DESC', 'min_count' => false
	));

		SELECT users.ID, users.user_nicename, users.display_name as author_name, COUNT(www_posts.ID) as author_count
		FROM users
		LEFT JOIN www_posts ON www_posts.post_author = users.ID
		
		JOIN usermeta AS meta_capabilities ON meta_capabilities.user_id = users.ID AND meta_capabilities.meta_key = 'www_capabilities'
		WHERE www_posts.post_type = 'post' AND www_posts.post_status = 'publish'
		GROUP BY users.ID
		
		ORDER BY author_count DESC, author_name

33.0ms

wp_list_authors(array(
	'optioncount' => true, 'exclude_admin' => false,
	'show_fullname' => false, 'hide_empty' => false,
	'orderby' => 'count', 'order' => 'DESC', 'min_count' => 1
	));

		SELECT users.ID, users.user_nicename, users.display_name as author_name, COUNT(www_posts.ID) as author_count
		FROM users
		JOIN www_posts ON www_posts.post_author = users.ID
		
		
		WHERE www_posts.post_type = 'post' AND www_posts.post_status = 'publish'
		GROUP BY users.ID
		HAVING author_count >= 1
		ORDER BY author_count DESC, author_name

8.1ms

so basically... punting back to future pending more massaging. further tests reveal that:

  • the group by tends to hinder performance because it introduces a unnecessary sort in MySQL; it can be removed entirely
  • the author name logic would actually be better handled by mass-querying the usermeta table using the functions from miqrogroove in a separate ticket

comment:23 follow-up: takaitra4 years ago

Removing the NULL check on the post_type causes authors with zero posts to be incorrectly excluded. I realize there is no NULL post_type but the check is necessary due to the joins.

How do you suggest the GROUP BY be removed while retaining the "optioncount" and "number" argument functionality?

I read through miqrogroove's patch in #11914. I see how User objects could be queried for first and last name. If displaying full name, we would then need to sort in PHP as opposed to in SQL.

comment:24 in reply to: ↑ 23 Denis-de-Bernardy4 years ago

Replying to takaitra:

Removing the NULL check on the post_type causes authors with zero posts to be incorrectly excluded. I realize there is no NULL post_type but the check is necessary due to the joins.

I think that's covered by the $min_count = min(1, $min_count) statement that I added

How do you suggest the GROUP BY be removed while retaining the "optioncount" and "number" argument functionality?

mysql is quite loose on the SQL spec and doesn't actually require a group by. (it groups by automatically.) when there *is* an explicit a group by, it also sorts, which introduces overhead.

I read through miqrogroove's patch in #11914. I see how User objects could be queried for first and last name. If displaying full name, we would then need to sort in PHP as opposed to in SQL.

right. so let's get #11914 in first, and revisit this once it's checked in.

comment:25 takaitra4 years ago

Denis, I see that microgroove's code was checked in. I'm going to code the following pseudocode and submit a new patch. Let me know if you see any logic problems. Thanks.

First, query a list of authors and post counts. This would be a join but either leave out the GROUP BY or include ORDER BY NULL to avoid the sort overhead. It would be a LEFT JOIN or INNER JOIN conditional on whether authors with zero posts need to be displayed. It would also include an appropriate HAVING and LIMIT clause based on the min_count and number options.

Next, acquire the associated User objects based on miqrogroove's new code. This would only be done if show_fullname is true. A loop through the authors will be needed to associate them with their post counts.

Sort the list of authors based on post count or name, depending on the orderby option.

takaitra4 years ago

comment:26 takaitra4 years ago

  • Keywords has-patch added; needs-patch removed

I completed the new patch. Can you take a look Denis? I already tested with a variety of inputs.

I confirmed that the removal of the NULL checks causes authors with zero posts to be incorrectly excluded.

A small side-effect of the conditional removal of the post count join is that the function can no longer know whether or not to include a link to the author posts (see line 401). I defaulted to including all links in this case.

comment:27 follow-up: t31os_4 years ago

I do find one thing a little strange, and that is, that a function designed to list authors actually lists users with any role.

Although i see why that is, there's no way to query for users with a particular role(s), because the capabilities of the user are serialized(so there's no room to write this into the SQL query).

So while we can set hide_empty to false and get users without posts, it does not actually distinguish between those that can actually post content(has posting capability) and those that can't. I'm not sure how that could be worked around, personally i'd see the hide_empty parameter dropped if there's no distinction between users that have the ability to post, and those that don't.

I tested the last two patches and they're definately better, but do both suffer from a few minor bugs when using particular args in conjunction with one another, mincount does not seem to work for me.

Try this.

wp_list_authors('hide_empty=0&optioncount=1&show_fullname=1&feed=feed&number=10&mincount=2');

Even without the hide_empty, mincount is still ignored with the above. I've been varying the args, but in honesty it's not much fun sitting there fiddling with args and the results differ slightly between the last 2 patches.

One area that could be a problem or even an oversight.

if ( $hide_empty || $min_count || $optioncount || $orderby == 'count' ) {

Then 6 lines down, *inside that condition*.

if ( !$min_count && !$hide_empty )

.. the second condition in some cases can't be met.

I like the patch by Denis(one query total - no offense intended takaitra), because it removes any need to call get_userdata which eats up two queries per iteration.

I did actually start writing my own patch following a thread discussion that linked to this ticket, but i figured before i go any further, that i should examine and test the patches available.

I've actually done pretty well up till now, had it down to two queries, but the patch by Denis put my work to shame(thought i was doing excellent at two queries, damn). So i'm going to use the patch Denis submitted as a framework and work in some changes. I'd like to submit it here for your critique, assuming i've not offended you by now.. :)

comment:28 t31os_4 years ago

Spoke to soon, ok perhaps not one query in the patch by Denis, but it's certainly producing less queries then your most recent patch takaitra, which i think is simply down to the get_userdata call.

comment:29 in reply to: ↑ 27 takaitra4 years ago

Replying to t31os_:

I tested the last two patches and they're definately better, but do both suffer from a few minor bugs when using particular args in conjunction with one another, mincount does not seem to work for me.

Try this.

wp_list_authors('hide_empty=0&optioncount=1&show_fullname=1&feed=feed&number=10&mincount=2');

The option is "min_count", not "mincount." The above call works for me when using the correct option name.

One area that could be a problem or even an oversight.

if ( $hide_empty || $min_count || $optioncount || $orderby == 'count' ) {

Then 6 lines down, *inside that condition*.

if ( !$min_count && !$hide_empty )

.. the second condition in some cases can't be met.

The nested "if" statement you mention switches the inner join to a left join in the case that we need to include authors with zero posts. Can you explain why this is a problem?

I like the patch by Denis(one query total - no offense intended takaitra), because it removes any need to call get_userdata which eats up two queries per iteration.

I also liked the idea of having as few SQL queries as possible and if you look at the earlier patches, you will see that this is the approach I took. After performance testing that patch, however, Denis had these comments:

  • the group by tends to hinder performance because it introduces a unnecessary sort in MySQL; it can be removed entirely
  • the author name logic would actually be better handled by mass-querying the usermeta table using the functions from miqrogroove in a separate ticket

So, yes, the latest patch causes more SQL queries initially but 1) it is more readable and 2) the get_userdata method caches data so subsequent runs of wp_list_authors will be more efficient. I welcome any more feedback but I still feel that my patch is ready for testing by Denis or another WP dev.

comment:30 follow-up: t31os_4 years ago

Yes apologies i think the parameter issue was my mistake.

I'm not sure about the group by problem, perhaps you could remove the group by and use a distinct select instead, would that help at all?

I want to go back to this authors with zero posts thing though, don't you actually mean any user? As like i said further up, there's nothing in the queries to distinguish between a user that can create posts and one that cannot, so when you say authors with zero posts, i believe you're actually referring to all users, or is there something i'm overlooking?

The get_userdata call isn't just called once per iteration it appears(been looking at these functions over the last few hours) it's also called by get_author_posts_url, and then you have get_author_feed_link, which in turn calls get_author_posts_url, and again in turn that's another get_userdata lookup. In total you end up with 3 calls to get_userdata, to grab data you don't even need, because as far as i can see, all the data you're extracting from get_userdata is already available from the SQL query further up (ID, nicename, first_name, last_name, etc).

Taking a look at the two above mentioned functions, i can't see that it would be a huge problem to do something similar directly in the wp_list_authors function(i've been doing that to cut down the queries).

In regards to caching, i can't really comment as i know very little about the WordPress caching, but i thought caching was pretty much off by default(not sure where i got that impression from), but like i said, i know little about it.

takaitra4 years ago

Updated diff against r15567 of author-template.php

comment:31 in reply to: ↑ 30 takaitra4 years ago

Replying to t31os_:
What specific actions would you like me to take t31os? I added the get_userdata call in response to feedback from Denis-de-Bernardy in order to make use of the caching added to that function. From his earlier comment:

the author name logic would actually be better handled by mass-querying the usermeta table using the functions from miqrogroove in a separate ticket.

Are you suggesting I again remove the calls to get_userdata? I am using this patch in the latest version of my plugin, it is working well, and users are happy with the new features. Most importantly, it allows a limit on the number of authors listed instead of displaying all 10,000 in the case of large blog. For that reason, I think this prolonged discussion of which query is fastest is mostly moot. The existing version of wp_list_authors is known for using up an enormous amount of memory when displaying large numbers of authors. This patch finally allows a user to limit the number of authors displayed thereby increasing blog performance.

comment:32 t31os_4 years ago

5 months since i responded to this ticket, and not really tracking the progress/state of this function any more.

Any patch that introduces optimization gets my support.

+1 for dev feedback.

comment:33 scribu4 years ago

  • Milestone changed from Awaiting Triage to 3.1
  • Type changed from feature request to enhancement

wp_list_authors() should use the new get_users() function.

comment:34 scribu4 years ago

  • Owner changed from westi to scribu
  • Status changed from reviewing to accepted

comment:35 scribu4 years ago

(In [15620]) Clean up wp_list_authors(). See #10329

comment:36 scribu4 years ago

[15620]:

  • pass 'orderby', 'order' and 'number' to get_users().
  • reduce number of if clauses

Only thing not addressed is min_count.

comment:37 scribu4 years ago

Related: #14572

comment:38 scribu4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

min_count was an afterthought and we don't have it anywhere else, so a separate ticket should be opened for that.

Note: See TracTickets for help on using tickets.