Make WordPress Core

Opened 12 years ago

Last modified 12 months 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's profile raphaabreu Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Future Release 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 11 years ago.
23498.2.diff (1.9 KB) - added by MikeHansenMe 10 years ago.
Refreshed because of the extract ticket and with unit tests
23498.3.diff (1.9 KB) - added by MikeHansenMe 9 years ago.
23498.4.diff (2.1 KB) - added by donmhico 5 years ago.
Refreshed the patch for 5.3.
23498.5.diff (2.5 KB) - added by SergeyBiryukov 5 years ago.
23498.6.diff (3.7 KB) - added by SergeyBiryukov 5 years ago.

Download all attachments as: .zip

Change History (30)

#1 @knutsp
12 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
12 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 11 years ago by SergeyBiryukov (previous) (diff)

#3 @SergeyBiryukov
12 years ago

In 23448:

Fix typo in phpdoc. see #23498.

#4 @raphaabreu
12 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
12 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
11 years ago

#26544 was marked as a duplicate.

#7 in reply to: ↑ 2 @SergeyBiryukov
11 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
11 years ago

  • Cc mdhansen@… added

#9 @MikeHansenMe
11 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 11 years ago by MikeHansenMe (previous) (diff)

@MikeHansenMe
11 years ago

#10 @nacin
11 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
10 years ago

Refreshed because of the extract ticket and with unit tests

#11 @MikeHansenMe
10 years ago

  • Keywords has-patch added

@MikeHansenMe
9 years ago

@donmhico
5 years ago

Refreshed the patch for 5.3.

#12 @donmhico
5 years 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 years ago

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

#14 @SergeyBiryukov
5 years ago

In 45795:

Users: Make wp_list_authors() unit tests more readable.

See #23498.

#15 follow-ups: @SergeyBiryukov
5 years 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 years 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 years 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?

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


5 years ago

#19 @talldanwp
5 years ago

@SergeyBiryukov This ticket was discussed during a triage session (link above this comment).

The feedback was generally in agreement with your last comment that it'd be less than ideal if the behaviour of the function changes considerably for those who've implemented it.

One thought that I had is that given the arguments are in an options array, it might be that exclude_admin can be deprecated and replaced with exclude_administrator_role which has the new behaviour proposed in the patch. That would ensure that anyone using the function in its existing capacity doesn't see any/much change.

#20 @davidbaumwald
5 years ago

@SergeyBiryukov With 5.3 Beta 3 shipping today, how do you feel about this ticket?

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


5 years ago

#22 @kirasong
5 years ago

  • Milestone changed from 5.3 to Future Release

Thanks everyone for your work and feedback on this!

Since we're nearing the 5.3 RC and it's been a couple of weeks since this ticket has seen movement, I'm going to go ahead and move it to Future Release.

Please feel free to move it back into the milestone if consensus is formed/an appropriate patch becomes available before RC!

#23 @knutsp
5 years ago

Suggestion:

  1. Add exclude_administrators argument to exclude any with administrator role alone (unusual, but in this case it may open for some administrators to be included)
  1. Soft deprecate exclude_admin (no deprecation warning emitted)

Update docs to reflect this.

#24 @SergeyBiryukov
12 months ago

#60071 was marked as a duplicate.

Note: See TracTickets for help on using tickets.