Opened 15 years ago
Closed 14 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)
Change History (45)
#1
in reply to:
↑ description
;
follow-up:
↓ 2
@
15 years ago
- Milestone changed from Unassigned to Future Release
#2
in reply to:
↑ 1
@
15 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.
#4
@
15 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.
@
15 years ago
Updated patch for latest trunk. Multi-blog logic. Options renamed "orderby," "order," and "number" to match the other template tags.
#8
in reply to:
↑ 7
@
15 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.
#9
follow-up:
↓ 10
@
15 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.
#12
@
15 years ago
Replying to nacin:
Nacim, why Future Release?
We postpone this enhancement since 2.8.
#13
follow-up:
↓ 18
@
15 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.
#14
@
15 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"; }
#15
@
15 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?
#17
@
15 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.
#19
@
15 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.
#20
@
15 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.
#21
@
15 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.
#22
@
15 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
#23
follow-up:
↓ 24
@
15 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.
#24
in reply to:
↑ 23
@
15 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.
#25
@
15 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.
#26
@
15 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.
#27
follow-up:
↓ 29
@
14 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.. :)
#28
@
14 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.
#29
in reply to:
↑ 27
@
14 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.
#30
follow-up:
↓ 31
@
14 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.
#31
in reply to:
↑ 30
@
14 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.
#32
@
14 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.
#33
@
14 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.
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.