WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 5 weeks ago

#23498 reviewing defect (bug)

wp_list_authors ignores 'exclude_admin' arg when admin account has a display_name other then 'admin'

Reported by: raphaabreu Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 3.1
Component: Users Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Line 293 of author-template.php should be changed from:

if ( $exclude_admin && 'admin' == $author->display_name )

to:

if ( $exclude_admin && 'admin' == $author->user_login )

Thanks.

Attachments (6)

23498.diff (937 bytes) - added by MikeHansenMe 6 years ago.
23498.2.diff (1.9 KB) - added by MikeHansenMe 5 years ago.
Refreshed because of the extract ticket and with unit tests
23498.3.diff (1.9 KB) - added by MikeHansenMe 4 years ago.
23498.4.diff (2.1 KB) - added by donmhico 5 weeks ago.
Refreshed the patch for 5.3.
23498.5.diff (2.5 KB) - added by SergeyBiryukov 5 weeks ago.
23498.6.diff (3.7 KB) - added by SergeyBiryukov 5 weeks ago.

Download all attachments as: .zip

Change History (23)

#1 @knutsp
7 years ago

  • Cc knut@… added

'admin' should be avoided as user_login anyway. On a single site there is usually a site owner, identified by the user who has the same email as the one who installed it and may be still be the receiver of admin emails for the site.

if ( $exclude_admin && get_option('admin_email') == $author->user_email )

This will exclude one, or sometimes no users. Or just deprecate the whole $exclude_admin thing in favor of something more modern in terms of today's WordPress roles and capabilities.

#2 follow-up: @SergeyBiryukov
7 years ago

  • Description modified (diff)
  • Keywords has-patch removed
  • Version changed from 3.5.1 to 3.1

Related: #9165, #9902

Introduced in [15620], previously user_login field was used for comparison.

The parameter itself was added in [2632]. We should probably deprecate it in favor of exclude (#9902).

Still, fixing it makes sense to me. The description refers to "the 'admin' user that is installed by default":
http://core.trac.wordpress.org/browser/tags/3.5.1/wp-includes/author-template.php#L246

Codex specifically refers to the login: "Exclude the 'admin' (login is admin) account from the list":
http://codex.wordpress.org/Function_Reference/wp_list_authors#Parameters

The default username, however, can be manually picked now (#10396).

I guess we have the following options to fix this:

  • 'admin' == $author->user_login.
  • 1 == $author->ID (from #9165).
  • get_option('admin_email') == $author->user_email (from comment:1).
Last edited 6 years ago by SergeyBiryukov (previous) (diff)

#3 @SergeyBiryukov
7 years ago

In 23448:

Fix typo in phpdoc. see #23498.

#4 @raphaabreu
7 years ago

I am fine with deprecating it in favor of exclude.

However, I am updating several old installs, some of them have multiple accounts with the same user e-mail. The third option would create another issue where several authors would not be shown.

Either option 1 or 2 would work perfectly in my humble opinion.

#5 @jonna
6 years ago

Would it be possible for someone to post the code that would allow me to exclude admin, as well as other users (I think this is the correct way to go).

I really need to get this done and this bug has been up for 3 months with no response.

#6 @SergeyBiryukov
6 years ago

#26544 was marked as a duplicate.

#7 in reply to: ↑ 2 @SergeyBiryukov
6 years ago

Replying to SergeyBiryukov:

I guess we have the following options to fix this:

ticket:26544:wp-list-author-number-exclude-admin.diff implements a fourth option: populating $query_args['exclude'] with the list of all administrator IDs.

#8 @MikeHansenMe
6 years ago

  • Cc mdhansen@… added

#9 @MikeHansenMe
6 years ago

I think excluding the admin before/during get users is important because otherwise it breaks the number arg. For example if I request 2 authors and admin is returned as one of them then gets excluded, it only returns 1 author even thought I requested 2. I have been looking at my previous patch and think I need to look at it a bit more but I do think it is a better approach because of the number issue.

Last edited 6 years ago by MikeHansenMe (previous) (diff)

@MikeHansenMe
6 years ago

#10 @nacin
6 years ago

Unit test, from #26538 (also by MikeHansenMe):

	/**
	 * @ticket 23498
	 */
	function test_wp_list_authors_number() {
		$expected['number'] = '<li><a href="http://example.org/?author=' . $this->users[1] . '" title="Posts by bob">bob</a></li><li><a href="http://example.org/?author=' . $this->users[2] . '" title="Posts by paul">paul</a></li>';
		$this->AssertEquals( $expected['number'], wp_list_authors( array( 'echo' => false, 'number' => 2 ) ) );
	}

@MikeHansenMe
5 years ago

Refreshed because of the extract ticket and with unit tests

#11 @MikeHansenMe
5 years ago

  • Keywords has-patch added

@MikeHansenMe
4 years ago

@donmhico
5 weeks ago

Refreshed the patch for 5.3.

#12 @donmhico
5 weeks ago

The bug still persist in 5.3. I've attached a refreshed patch - 23498.4.diff - and I did some updates in the unit test.

#13 @SergeyBiryukov
5 weeks ago

  • Milestone set to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#14 @SergeyBiryukov
5 weeks ago

In 45795:

Users: Make wp_list_authors() unit tests more readable.

See #23498.

#15 follow-ups: @SergeyBiryukov
5 weeks ago

  • Keywords commit added

Found a related issue while looking into this.

$query_args['fields'] = 'ids' added in [17100] works here, but only because $wpdb->users.ID is the default fallback in WP_User_Query::prepare_query().

$query_args['fields'] = 'ID' should be used instead.

23498.5.diff fixes that, uses the same approach for the get_users() call in the exclude_admin branch, and updates documentation.

The proposed unit test doesn't seem to be any different from test_wp_list_authors_default(). We already have a test for test_wp_list_authors_exclude_admin(), that seems enough here, but maybe could be expanded for multiple administrators.

Given that exclude_admin is true by default, I'm not 100% sure excluding all administrators would be the expected behavior for wp_list_authors(), but I can see it both ways. Marking for commit, unless there are any objections.

#16 in reply to: ↑ 15 @SergeyBiryukov
5 weeks ago

Replying to SergeyBiryukov:

We already have a test for test_wp_list_authors_exclude_admin(), that seems enough here, but maybe could be expanded for multiple administrators.

23498.6.diff adds a second administrator to the tests.

#17 in reply to: ↑ 15 @SergeyBiryukov
5 weeks ago

  • Keywords 2nd-opinion added; commit removed

Replying to SergeyBiryukov:

Given that exclude_admin is true by default, I'm not 100% sure excluding all administrators would be the expected behavior for wp_list_authors(), but I can see it both ways.

Thought about this some more. Given that:

  • exclude_admin parameter was limited to admin display name for the last 9 years, since [15620] / #10329.
  • admin is no longer a default username on install for the last 6 years, since [24998] / #24078.

the effect of exclude_admin was pretty limited. The suggested change might result in unexpected behavior, where wp_list_authors() would no longer include users who are post authors, but also administrators.

Note: neither core nor bundled themes use the function, so third party themes are the main concern here.

Maybe changing the default value to false and adjusting the tests and documentation accordingly would make the most sense here?

Note: See TracTickets for help on using tickets.