Make WordPress Core

Opened 13 years ago

Closed 19 months ago

Last modified 19 months ago

#17025 closed enhancement (fixed)

wp_list_authors() is not filterable

Reported by: kevinb's profile kevinB Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Query Keywords: needs-dev-note has-patch
Focuses: Cc:

Description

The template function wp_list_authors() is not filterable. Plugins may use existing WP_Query hooks to filter some authors' posts out of the result set, making the unfiltered authors list invalid.

The corresponding patch adds query filter 'list_authors_query'

Attachments (17)

list-authors_filter_3.2.patch (874 bytes) - added by kevinB 13 years ago.
query filter for wp_list_authors()
17025.diff (929 bytes) - added by wonderboymusic 11 years ago.
17025.2.diff (1.0 KB) - added by DrewAPicture 11 years ago.
filter docs
17025.3.diff (1.6 KB) - added by DrewAPicture 11 years ago.
Alternate approach
17025.4.diff (1.7 KB) - added by DrewAPicture 11 years ago.
'ids' fallback for false $hide_empty
17025.5.diff (1.5 KB) - added by DrewAPicture 10 years ago.
refresh
17025.6.diff (1.5 KB) - added by DrewAPicture 10 years ago.
refresh
17025.7.diff (1.1 KB) - added by Mte90 8 years ago.
patch refreshed for 4.6
17025.optimizeforeach.diff (834 bytes) - added by Mte90 8 years ago.
optimized foreach moving query outside
17025.8.diff (1.5 KB) - added by audrasjb 3 years ago.
Query: introduce a new hook to filter wp_list_authors
17025.9.diff (858 bytes) - added by rafiahmedd 2 years ago.
As in some countries they make fullname like "last name" + "first name", so I thought a filter hook to modify the fullname of an author could be helpful for the users
17025.10.diff (877 bytes) - added by rafiahmedd 2 years ago.
Thank @audrasjb I here is the new patch
17025.11.diff (860 bytes) - added by costdev 23 months ago.
Refresh of 17025.10.diff with docblock improvements and WPCS fixes.
17025.12.diff (9.2 KB) - added by SergeyBiryukov 22 months ago.
17025.12.wp_list_authors_args.diff (7.8 KB) - added by SergeyBiryukov 22 months ago.
17025.12.display_name.diff (3.8 KB) - added by SergeyBiryukov 22 months ago.
17025.13.diff (8.6 KB) - added by SergeyBiryukov 22 months ago.

Download all attachments as: .zip

Change History (94)

@kevinB
13 years ago

query filter for wp_list_authors()

#1 @scribu
13 years ago

Related: #17022

#2 @scribu
13 years ago

  • Summary changed from add hook for wp_list_authors() to wp_list_authors() is not filterable

#3 @wonderboymusic
11 years ago

  • Milestone changed from Awaiting Review to 3.7

The filter seems reasonable.

#4 @wonderboymusic
11 years ago

  • Keywords needs-docs added

There's a new filter

@DrewAPicture
11 years ago

filter docs

#5 @DrewAPicture
11 years ago

  • Keywords commit added; needs-docs removed

17025.2.diff adds a docblock for the list_authors_query filter hook.

#6 @nacin
11 years ago

I would prefer to not add a filter that directly operates on SQL. This is not very flexible or fun.

Can we instead filter A) $authors, and/or B) $author_count?

Also: Unless I am reading it wrong, this function also appears to be very, unnecessarily heavy. By only asking for 'ids' from get_users(), none of the users are getting cached, which means each individual call to get_usermeta() is triggering a new query.

It would make more sense to run the raw query first, then fetch only those user objects (see also cache_users(); get_users() can also do this), unless $hide_empty is false, in which case you still need to fetch all objects as we're doing now.

#7 @ocean90
11 years ago

  • Keywords commit removed

#8 @wonderboymusic
11 years ago

  • Keywords needs-refresh added

see @nacin's comment - needs alternate filter approach

@DrewAPicture
11 years ago

Alternate approach

#9 @DrewAPicture
11 years ago

  • Keywords dev-feedback added; needs-refresh removed

Would something like 17025.3.diff work better?

@DrewAPicture
11 years ago

'ids' fallback for false $hide_empty

#10 @DrewAPicture
11 years ago

  • Cc xoodrew@… added

#11 @kevinB
11 years ago

This request was part of a handful which I tossed out a couple years ago with an overall goal of allowing records to be filtered out of or into the result set. Filters applied in WP_Query and elsewhere make it possible to elevate the logged user to (or demote them from) author capabilities contextually based on criteria which WP core need not be aware of nor formally support. But some of the corresponding "user assignment" UI does not permit corresponding filtering.

wp_list_authors() is low-priority to me since it's not called by core. But inevitably some plugins and themes use it in a way that doesn't mesh with per-user inclusion/exclusion I do with WP_Query filters. Then I'll have to filter every query and use strpos() and preg_match() to determine if it is the wp_list_authors() query. Now that's not fun. So basically, the significance of this ticket is in setting a precedent for whether the API fully supports external modification of content and user queries, or whether it presumes to anticipate and regulate all implementations.

In this case, caching the "all objects" results of the default query is not sufficient since it is built on query clauses that do not account for post types other than 'post' nor visibility statuses other than 'private':

"SELECT DISTINCT post_author, COUNT(ID) AS count FROM $wpdb->posts WHERE post_type = 'post' AND " . get_private_posts_cap_sql( 'post' )

The 'pub_priv_sql_capability' filter in function get_posts_by_author_sql() would at least allow a different post type for the current user capability check, but even that is up for deprecation according to the inline comment. And then you still have this:

$sql .= " OR post_status = 'private'";

So a filter on the results would be better than nothing, but it has the disadvantage of forcing an additional query, which makes plugins look bad ;)

Last edited 11 years ago by kevinB (previous) (diff)

#12 @alex-ye
11 years ago

  • Cc nashwan.doaqan@… added

#13 @nacin
11 years ago

  • Keywords 3.8-early added
  • Milestone changed from 3.7 to Future Release

There's a lot going on here. Moving to 3.8.

#14 @wonderboymusic
10 years ago

  • Keywords needs-refresh added; dev-feedback removed
  • Milestone changed from Future Release to 4.0

@DrewAPicture probably wants to update his patch/docs

@DrewAPicture
10 years ago

refresh

#15 @DrewAPicture
10 years ago

  • Keywords needs-refresh removed

17025.5.diff refreshes the patch.

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


10 years ago

@DrewAPicture
10 years ago

refresh

#17 @DrewAPicture
10 years ago

  • Keywords 4.1-early added; 3.8-early removed
  • Milestone changed from 4.0 to Future Release

Let's work on this in 4.1. 17025.6.diff refreshes the patch at r29015. Unit tests might also need an update for $hide_empty.

#18 @chriscct7
8 years ago

  • Keywords needs-refresh added; 4.1-early removed

@Mte90
8 years ago

patch refreshed for 4.6

@Mte90
8 years ago

optimized foreach moving query outside

#19 @Mte90
8 years ago

  • Keywords dev-feedback added; needs-refresh removed

Patch refreshed, updated code and splitted in two patch.
17025.7.diff contain the new filter and 17025.optimizeforeach.diff contain the optimization of the foreach of the original patch.

#20 @jorbin
8 years ago

@DrewAPicture and @boonebgorges Can you two take a look? Would love to know your thoughts.

#21 @Mte90
8 years ago

any chance for the 4.7?

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


6 years ago

#23 @SergeyBiryukov
6 years ago

  • Milestone changed from Future Release to 5.0

#24 @afercia
6 years ago

  • Keywords needs-refresh added
  • Milestone changed from 5.0 to Future Release

17025.optimizeforeach.diff needs some coding standards: spacing, empty lines should not have spaces, etc.

#25 @afercia
6 years ago

  • Milestone changed from Future Release to 5.0

#26 @johnbillion
5 years ago

  • Milestone changed from 5.0 to 5.1

#27 @pento
5 years ago

  • Milestone changed from 5.1 to Future Release

This ticket was mentioned in PR #1051 on WordPress/wordpress-develop by Mte90.


3 years ago
#28

  • Keywords needs-refresh removed

Mte90 commented on PR #1051:


3 years ago
#29

I think that the docker script to tests is crashing and not the tests itself.

#30 @Mte90
3 years ago

Updated also this ticket with code refreshed

#31 @SergeyBiryukov
3 years ago

  • Milestone changed from Future Release to 5.8

#32 @chetan200891
3 years ago

@Mte90

PR looks good. Just need to update version(@since) now to 5.8.0

#33 @Mte90
3 years ago

Usually this bump is done by the committer has you cannot know when the code will be approved.

This ticket was mentioned in Slack in #core by mte90. View the logs.


3 years ago

#35 @desrosj
3 years ago

  • Milestone changed from 5.8 to 5.9

Today is the 5.8 feature freeze. Since this one was not able to be reviewed and committed in time, I'm going to punt it. Since there has been good momentum and it seems very close, I'll punt to 5.9 instead of Future Release.

#36 @audrasjb
3 years ago

  • Keywords needs-refresh added; dev-feedback removed

The PR looks good to me, but it needs to be refreshed against trunk and also to add a proper @since mention.

@audrasjb
3 years ago

Query: introduce a new hook to filter wp_list_authors

#37 @audrasjb
3 years ago

  • Keywords needs-refresh removed

17025.8.diff refreshes the patch against trunk.

#38 @Mte90
2 years ago

I am wondering if the changes at https://core.trac.wordpress.org/ticket/16841#comment:80 can help on this ticket and maybe add more filters.

#39 @hellofromTonya
2 years ago

  • Keywords early added
  • Milestone changed from 5.9 to Future Release

Query changes by design need careful consideration and plenty of soak time to ensure nothing breaks. With 5.9 feature freeze ~2 days away, it's too late in the release cycle for a change to query. Punting this out of 5.9. As the next milestone is not yet available, moving to Future Release. Once available, feel free to move into the next release.

#40 @SergeyBiryukov
2 years ago

#55091 was marked as a duplicate.

#41 @SergeyBiryukov
2 years ago

#55091 was marked as a duplicate.

@rafiahmedd
2 years ago

As in some countries they make fullname like "last name" + "first name", so I thought a filter hook to modify the fullname of an author could be helpful for the users

#42 @audrasjb
2 years ago

  • Keywords needs-refresh added

Thanks for the patch @rafiahmedd.
However, there are a few details to address:

  • @since should be 6.0.0, 5.9 was released on January 25, 2022.
  • replace "Allows …" with "Filters …" in the hook description
  • missing trailing dots in params descriptions
  • there are a few indentation/coding standards errors in the params

@rafiahmedd
2 years ago

Thank @audrasjb I here is the new patch

#43 @Mte90
2 years ago

Just checking this ticket has two patch one to add the filter by the ticket name and another one to change the user name.

@costdev
23 months ago

Refresh of 17025.10.diff with docblock improvements and WPCS fixes.

#44 @audrasjb
23 months ago

Thanks for the refresh @costdev.
I think we could remove the empty line on L490 but otherwise it looks good to go.

(and this could be done during commit 😊)

#45 @costdev
23 months ago

  • Keywords needs-testing added; needs-refresh removed

Sounds good @audrasjb! I'll add needs-testing and remove needs-refresh.

What else do you think this ticket needs? Do you think it's landable for early 6.1?

#46 @audrasjb
23 months ago

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

Yeah, I think it is clearly doable early.
Self-assigning for test/review

#47 @audrasjb
22 months ago

  • Milestone changed from Future Release to 6.1

Moving this ticket for 6.1 consideration.

#49 @audrasjb
22 months ago

  • Keywords needs-testing removed

@costdev @Mte90 I put together a new PR for this. Are you okay with that changeset?

#50 @Mte90
22 months ago

Yes no problem :-)

#51 @costdev
22 months ago

@audrasjb LGTM 👍

#52 @audrasjb
22 months ago

  • Keywords commit added

Thanks, I'll commit this asap :)

#53 @audrasjb
22 months ago

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

In 53486:

Query: Add a hook to filter author full name from wp_list_authors().

This changeset introduces the wp_list_author_full_name hook to allows developers to filter author full name, which by default is a combinaison of the author first and last name, separated by a space.

Props kevinB, wonderboymusic, DrewAPicture, nacin, Mte90, jorbin, afercia, rafiahmedd.
Fixes #17025.

#55 @TimothyBlynJacobs
22 months ago

Can we type $author as a WP_User instance instead of the generic object? It looks like it comes directly from get_userdata

#56 @audrasjb
22 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopened to handle the above comment.

#57 @audrasjb
22 months ago

  • Keywords needs-dev-note added

Adding needs-dev-note workflow keyword to make sure it is at least mentioned in the Misc Changes Dev note.

#58 @SergeyBiryukov
22 months ago

  • Keywords 2nd-opinion added; commit removed
  • It seems that the ticket went at some point from filtering the wp_list_authors() query or results to just filtering the full names of authors, which seems unrelated to the initial request and does not really address it.
  • 17025.8.diff should probably be reconsidered:
    • It needs a new filter name, since it filters not the actual result of the function, but rather the found users. Maybe wp_list_authors_found_users?
    • To avoid a second query, should the function arguments be filtered instead, as suggested in #55091? Maybe wp_list_authors_args then?
    • Should wp_list_users() get a similar filter for consistency?
    • WHERE post_type = 'post' should not be re-added there, it was removed in [31653] / #30354 and appears to be a remnant of the earlier patches.
  • There is a translatable string in wp_insert_user() for the same purpose:
    /* translators: 1: User's first name, 2: Last name. */
    $display_name = sprintf( _x( '%1$s %2$s', 'Display name based on first name and last name' ), $meta['first_name'], $meta['last_name'] );
    
    Perhaps it could be reused here, instead of the full names filter? I'm not sure the need for a new filter has been demonstrated yet. If some locales prefer to put the last name first, it seems like the string would be the way to go. It could then also be reused for the same pattern in other areas of core: As it stands, it seems a bit inconsistent to only add a filter for wp_list_authors() but not those other instances.
  • If there is a strong reason for the full names filter to stay, should it be renamed to wp_list_authors_full_name (note authors instead of author), to match the function name?
Last edited 22 months ago by SergeyBiryukov (previous) (diff)

#59 @SergeyBiryukov
22 months ago

To summarize the above, I think we should:

  • Add a wp_list_authors_args filter for $query_args. See wp_dropdown_users_args for example.
  • Add a wp_list_users_args filter to wp_list_users() too, for consistency.
  • Replace the wp_list_author_full_name filter with this string for now, added in [21876] / #20637:
    _x( '%1$s %2$s', 'Display name based on first name and last name' )
    

#60 @SergeyBiryukov
22 months ago

There is also a performance improvement suggestion in comment:6, but that should be explored in a new ticket.

#61 @SergeyBiryukov
22 months ago

I believe 17025.12.diff should address the comments above.

#62 @SergeyBiryukov
22 months ago

I have split this into two patches for easier review, I think they should be committed separately:

  • 17025.12.wp_list_authors_args.diff introduces the wp_list_authors_args and wp_list_users_args filters, which should address the initial request here. Note that wp_list_users() was only added recently in [52064] / #15145, which is why it has not been mentioned in this ticket before.
  • 17025.12.display_name.diff uses a translatable string in every instance where we display a concatenated first name and last name, instead of adding a filter in just one of these instances. That allows locales to switch the order, should they prefer to do so.

#63 @SergeyBiryukov
22 months ago

In 53501:

I18N: Use a translatable string for displaying a user's first name and last name.

That allows locales to switch the order of the first name and last name, should they prefer to do so.

The string was previously used in wp_insert_user() and is now reused in other places for consistency:

  • WP_MS_Users_List_Table::column_name()
  • WP_Users_List_Table::column_name()​
  • wp_list_authors()
  • wp_list_users()

Note: This also removes the wp_list_author_full_name filter, introduced for the same purpose in wp_list_authors(), as redundant for now.

Follow-up to [53486].

See #17025.

#64 @SergeyBiryukov
22 months ago

It looks like a wp_list_authors_args filter by itself is not enough, as one would also need to override the raw SQL query for post counts, e.g. to account for custom post types, see comment:11.

17025.13.diff also adds a pre_wp_list_authors_post_counts_query filter for that, modeled after pre_months_dropdown_query added in [50163] / #51660.

This ticket was mentioned in Slack in #core by sergey. View the logs.


22 months ago

This ticket was mentioned in Slack in #core by chaion07. View the logs.


21 months ago

#67 follow-up: @chaion07
21 months ago

Thanks @kevinB for reporting this. We reviewed this ticket during a recent bug-scrub session. Based on the feedback received we summarized with the following:

  1. The filter requested for this is added. In short it serves the target of the ticket. However as

@sergey mentioned in the ticket that, an extra filter can be added to be able to override the raw SQL that fetches posts counts, etc.

Props to @cu121

#68 in reply to: ↑ 67 @SergeyBiryukov
21 months ago

Replying to chaion07:

The filter requested for this is added. In short it serves the target of the ticket.

Just to clarify, the wp_list_authors_args filter that was requested here has not been added yet, so the ticket is not addressed. Another (unrelated) filter was added in [53486] and removed in [53501] in favor of a translatable string.

I think 17025.13.diff should address this ticket. I can work on it after 6.0.1 RC1 ships later today :)

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


21 months ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


21 months ago

This ticket was mentioned in Slack in #core by chaion07. View the logs.


20 months ago

This ticket was mentioned in Slack in #core by chaion07. View the logs.


20 months ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


20 months ago

#74 @audrasjb
20 months ago

  • Keywords early 2nd-opinion removed

As per today's bugscrub, removing early keyword as it was already committed, then reopened to address further concerns.

#75 @audrasjb
19 months ago

  • Milestone changed from 6.1 to 6.2

With WP 6.1 beta 1 to be released today, let's move this ticket to the next milestone, as it has a patch which just needs refresh and testing.

#76 @SergeyBiryukov
19 months ago

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

In 54262:

Users: Make wp_list_authors() and wp_list_users() filterable.

This commit adds three filters to customize the wp_list_authors() and wp_list_users() output:

  • wp_list_authors_args: Filters the query arguments for the list of all authors of the site.
  • pre_wp_list_authors_post_counts_query: Filters whether to short-circuit performing the query for author post counts. This may be useful to account for custom post types or post statuses.
  • wp_list_users_args: Filters the query arguments for the list of all users of the site.

Follow-up to [979], [3848], [5135], [5727], [31653], [52064], [53486], [53501].

Props kevinB, wonderboymusic, DrewAPicture, Mte90, audrasjb, rafiahmedd, costdev, nacin, afercia, chetan200891, hellofromTonya, TimothyBlynJacobs, chaion07, SergeyBiryukov.
Fixes #17025.

#77 @SergeyBiryukov
19 months ago

  • Milestone changed from 6.2 to 6.1
Note: See TracTickets for help on using tickets.