WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 3 weeks ago

#19867 new defect (bug)

wp_dropdown_users() still not scalable

Reported by: prettyboymp Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.3.1
Component: Users Keywords: has-patch
Focuses: ui, performance Cc:

Description

#14572 made huge improvements to the performance of wp_dropdown_users(), however, on certain sites that have an unusually large set of authors, wp_dropdown_users() still isn't usable. It either causes a memory error on the server, or potentially crashes the client browser due to content size.

Attachments (11)

user_autocomplete.diff (9.2 KB) - added by prettyboymp 5 years ago.
user_dropdown_filter.diff (962 bytes) - added by prettyboymp 5 years ago.
user_autocomplete.2.diff (11.6 KB) - added by prettyboymp 5 years ago.
wp_suggest_users.alpha.1.diff (7.4 KB) - added by mpvanwinkle77 4 years ago.
19867.diff (4.3 KB) - added by helen 2 years ago.
19867.2.diff (16.9 KB) - added by helen 2 years ago.
19867.3.diff (17.1 KB) - added by helen 2 years ago.
19867.patch (833 bytes) - added by BevanR 15 months ago.
Workaround
19867-04.diff (1.0 KB) - added by norcross 10 months ago.
abstracts the args being passed to wp_dropdown_user and filters them before being passed into the query
19867-05.diff (716 bytes) - added by norcross 10 months ago.
filters the args inside of wp_dropdown_users instead of just targeting the delete portion
19867.4.diff (1.2 KB) - added by DrewAPicture 10 months ago.
Add default arguments to wp_dropdown_users_args

Download all attachments as: .zip

Change History (79)

#1 @prettyboymp
5 years ago

attachment:ticket:19867:user_autocomplete.diff is a suggested solution using an autocomplete handling to replace the select box after a threshold of results is hit.

As an alternative, I'm working on a second patch that will just be a tweak to add a filter that will a plugin to add this functionality before the dropdown is generated, in case a built in solution isn't accepted.

#2 @prettyboymp
5 years ago

attachment:ticket:19867:user_dropdown_filter.diff is alternative patch to at least allow a plugin to override the user dropdown before any queries are run.

#3 @prettyboymp
5 years ago

Just realized that my first patch didn't include the supporting js. Added attachment:ticket:19867:user_autocomplete.2.diff with it included.

#4 @nacin
4 years ago

+1 on the filter.

Additional +1 on autocomplete after a certain number of users. Since we are doing autocomplete for sites/users in the network admin for 3.4, the bits can likely be shared quite nicely.

#6 @DrewAPicture
4 years ago

  • Cc xoodrew@… added

#7 @Japh
4 years ago

  • Cc Japh added

#8 @scribu
4 years ago

Closed #20769 as dup.

#9 @SergeyBiryukov
4 years ago

#21286 closed as a duplicate.

#10 @scribu
4 years ago

  • Milestone changed from Awaiting Review to Future Release

#11 @brokentone
4 years ago

  • Cc kenton.jacobsen@… added

#12 @SergeyBiryukov
4 years ago

#23129 was marked as a duplicate.

#13 @mpvanwinkle77
4 years ago

What's the status on this? Says targeted to 3.3.1 but it doesn't look like these changes ever made it through. I can help test if there's an approved patch for testing.

#14 follow-up: @helen
4 years ago

Version is when something was reported/requested or the earliest a bug can be reproduced. Milestone is for the target, as it were.

Could probably use a patch that implements autocomplete the way it's done now for the network admin after a certain number of users. There's wp_is_large_network( 'users' ) to check for a large number of users but the default of 10k is probably still way too many for a dropdown.

#15 in reply to: ↑ 14 @kovshenin
4 years ago

  • Cc kovshenin added
  • Keywords needs-refresh added

Replying to helen: I think wp_is_large_network is actually used to prevent autocomplete because such searches would be really slow on large databases. Here's site-users.php:

if ( ! wp_is_large_network( 'users' ) )
	wp_enqueue_script( 'user-suggest' );

We can probably just reuse the whole thing as is, with a different ajax callback.

#16 @mpvanwinkle77
4 years ago

  • Cc mpvanwinkle77 added

So here's a new approach. creates a new function wp_suggest_users ... allows for several criterion in wp_dropdown_users to initiate a bail to wp_suggest_users() ... minor updates to js and to wp-admin/includes/post.php required to make this work. There's still a lot to do to make this production ready, but wanted to get some general feed back on the approach before continuing.

#17 @SergeyBiryukov
3 years ago

#23439 was marked as a duplicate.

#18 @toscho
3 years ago

  • Cc info@… added

#19 @SergeyBiryukov
3 years ago

#24557 was marked as a duplicate.

#20 @mpvanwinkle77
3 years ago

Anyone ever get a chance to look at this? Is this the route we want to go? or is there a better strategy?

#21 @kevinlangleyjr
3 years ago

  • Cc kevinlangleyjr added

#23 @timfield
3 years ago

  • Cc tim@… added

#24 follow-up: @nacin
3 years ago

  • Component changed from Performance to Users
  • Focuses performance added
  • Milestone changed from Future Release to 3.9

Could we use the same user autocomplete we used in the network admin?

#25 in reply to: ↑ 24 @helen
2 years ago

Replying to nacin:

Could we use the same user autocomplete we used in the network admin?

I think so - two things occur to me that are probably different, though. It needs the ID as the value, as opposed to user_login in the current instances or user_email as in the patch on #25348, and the UI needs to show something more friendly than the ID and should prevent freeform entry.

Also, the patch on #25348 whitelists the field against an enumerated list (in_array) - should we keep expanding on that list, make it filterable, something else?

#26 @nacin
2 years ago

#8648 was marked as a duplicate.

#27 @helen
2 years ago

  • Focuses ui added
  • Keywords ui-feedback removed

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


2 years ago

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


2 years ago

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


2 years ago

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


2 years ago

@helen
2 years ago

@helen
2 years ago

@helen
2 years ago

#32 @helen
2 years ago

  • Milestone changed from 3.9 to Future Release

I dove way down, and didn't come back up in time for 3.9. Here's where I landed (with help from gcorne). tl;dr: Autocomplete needs to enhance the right UI for the given data, whether that's a text input, a select element, or something else.

We have two instances of jQuery UI autocomplete in core right now, both in multisite. One is for adding existing users to a site (either from the network admin or the individual site admin) and the other, added this cycle, is for the new site admin email (#25348).

In exploring what it would take to allow wp_dropdown_users() (and wp_dropdown_pages() in #9864) to use this same method in order to scale, I realized that a freeform text input as the non-JS part of the UI was not actually ideal, and in fact, incorrect. If you test some of the patches I've been periodically putting onto the ticket for progress tracking, you'll see that I implemented a rough alternate UI, but too late in the cycle to do proper design and testing.

What I also realized is that the existing usages are actually two different things, and one of them uses a misleading UI. For a new site admin email, you can enter any email address and it will create a new user if that user doesn't already exist. The "autocomplete" in that case is more of a "prompt" or "suggest" - much like tags. However, when adding an existing user to a site, the UI really should only allow you to select an existing user, and not allow for arbitrary input. Essentially, this latter case should operate much as a select element would. Server-side catches it, of course, but we should have better UI.

I still want to push forward on approach and aim for early next release cycle. I'm thinking there are a few aspects:

  • What we use as the JS helper for autocomplete. jQuery UI autocomplete with a custom set of skins, something else? Either way, we should definitely use our own JS wrapper so that switching libraries or maintaining shims is a little more future-proofed (see: Masonry).
  • The UX and design of said helper.
  • Being smart about offering up some initial options to speed things up (more of a nice-to-have enhancement than a necessity), e.g. 10 users with the most posts for author selection dropdowns.

#33 @SergeyBiryukov
2 years ago

#28078 was marked as a duplicate.

#34 @docjohn
2 years ago

Yes, having initial options would be nice. Solution needs to work on large-author sites (we have a site with 30,000 authors, and some of the backend screens for admin are now unstable in Chrome, constantly crashing the browser). Thanks.

#35 @dd32
23 months ago

#29665 was marked as a duplicate.

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


22 months ago

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


22 months ago

#38 @chriscct7
22 months ago

#12014 was marked as a duplicate.

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


22 months ago

#40 @chriscct7
22 months ago

Related (and could be implemented in #19867) #13269

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


21 months ago

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


21 months ago

#43 @tharsheblows
21 months ago

Hi - I know this is well underway but thought I'd put in a quick request just in case.

Would it be possible to hook the wp_dropdown_users select and eventually the autosuggest to an action that we could remove or add to? It would be really helpful to be able to completely remove that action and add my own - not just filter the wp_dropdown_users query, although that would be cool, too.

What I've done for users who want to be deleted is assign their content to a dummy user creatively named "_deleted account" (the underscore was to pop it to the top of the dropdown list). However, now I can't use the list (85k users) although it still loads and only breaks if I touch it.

To work around this, I've put a checkbox on the new and edit users pages that allows me to include them in a list of users to whom it's ok to reassign content. I keep this list as an array in wp_options so I don't have to hunt through either of the users tables piecing it together. I'm currently using this array to replace the options in the wp_dropdown_users select using jQuery.

Having a small curated list really works for me. Putting users on it might take a tiny bit more time but then once I do, I never have to remember again who I am using for deleted user accounts. And I don't have to worry about me or anyone else accidentally assigning content to the wrong person (disastrous! or maybe hilarious, it would depend...)

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


20 months ago

This ticket was mentioned in Slack in #design by helen. View the logs.


15 months ago

@BevanR
15 months ago

Workaround

#46 @BevanR
15 months ago

19867.patch is a solution that assigns the deleted user's content to user ID 0. It is a suitable workaround, but not widely tested, so not suitable as a full solution without more testing.

#47 @helen
13 months ago

Related / relies on: #31696

#48 @lumpysimon
13 months ago

I've just run up against this issue on a site where I've imported ~250,000 users. In general all is working fine except I cannot delete a user due to the "Attribute all content to:" wp_dropdown_users running out of memory.

All these users have a custom role which only has the read capability, so it seems to me that they shouldn't really be listed in the dropdown anyway?

Last edited 13 months ago by lumpysimon (previous) (diff)

#49 @helen
12 months ago

#33108 was marked as a duplicate.

#50 @SergeyBiryukov
12 months ago

#33150 was marked as a duplicate.

#51 @tomaston
11 months ago

I have run into this issue on a site with approx. 210,000 users. Only 30 or so non-subscribers, but delete page is running out of memory and the dropdown and 'Confirm Deletion' button do not appear. Is this marked to be looked at (and hopefully fixed) in a future release?

This ticket was mentioned in Slack in #design by tharsheblows. View the logs.


11 months ago

#53 @wonderboymusic
11 months ago

  • Milestone changed from Future Release to 4.4

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


11 months ago

@norcross
10 months ago

abstracts the args being passed to wp_dropdown_user and filters them before being passed into the query

#55 @norcross
10 months ago

As a stopgap / workaround, I've added a patch that will set the args outside of the query and filter before being passed into the wp_dropdown_users function itself. this will allow site users to define things such as specific user roles or total counts (and other things, for that matter)

#56 follow-up: @paulwilde
10 months ago

Such filter would be best inside of wp_dropdown_users. There is currently a filter called wp_dropdown_users which filters the HTML output. A new one wp_dropdown_users_args which filters the arguments would make sense.

#57 in reply to: ↑ 56 @norcross
10 months ago

Replying to paulwilde:

Such filter would be best inside of wp_dropdown_users. There is currently a filter called wp_dropdown_users which filters the HTML output. A new one wp_dropdown_users_args which filters the arguments would make sense.

I see both sides of it. I've added an additional patch that filters the args inside of wp_dropdown_users instead, I'm fine with either being implemented.

@norcross
10 months ago

filters the args inside of wp_dropdown_users instead of just targeting the delete portion

#58 @tharsheblows
10 months ago

19867-04.diff would work brilliantly for me as it makes targeting just that instance of wp_dropdown_users easier and clearer. I use a different set of users in the reassign dropdown on the delete page than for eg the author select in posts.

But either would work, so please yes to one of them.

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


10 months ago

#60 @DrewAPicture
10 months ago

In 34692:

Users: Introduce the wp_dropdown_users_args filter, making it possible to filter the arguments for wp_dropdown_users() before the query is run.

The 'name' argument (or the WP_Screen object if in the admin) can be used to help target specific instances of wp_dropdown_users() via this hook.

Props norcross.
See #19867.

#61 @DrewAPicture
10 months ago

  • Keywords needs-refresh removed

Following [34692], it was pointed out that the 'name' argument isn't actually available for targeting specific instances of wp_dropdown_users(). This is because we're currently only passing $query_args to the hook. Access to 'name' means we also need to pass the actual default arguments.

19867.4.diff adds $r (the default arguments) to the hook.

@DrewAPicture
10 months ago

Add default arguments to wp_dropdown_users_args

#62 @helen
10 months ago

Filtering the args for the query seems correct to me - it just needs the context of the original args that were passed to the function.

#63 @DrewAPicture
10 months ago

In 34705:

Users: Add the default arguments array as a second parameter to the wp_dropdown_users_args filter, introduced in [34692].

Adjust hook doc descriptions accordingly.

See #19867.

#64 follow-up: @DrewAPicture
10 months ago

  • Milestone changed from 4.4 to Future Release

The addition of the wp_dropdown_users_args filter is a good start, but I don't see us necessarily solving this in 4.4. Moving back to future release.

#65 in reply to: ↑ 64 ; follow-up: @norcross
10 months ago

Replying to DrewAPicture:

The addition of the wp_dropdown_users_args filter is a good start, but I don't see us necessarily solving this in 4.4. Moving back to future release.

The new filter solves the immediate problem that site owners face. While a deep dive into the query behind the scenes could be warranted, I still don't see how having this filter is a bad thing at all, especially given that we already filter the HTML output inside the same function.

#66 in reply to: ↑ 65 @DrewAPicture
10 months ago

Replying to norcross:

Replying to DrewAPicture:

The addition of the wp_dropdown_users_args filter is a good start, but I don't see us necessarily solving this in 4.4. Moving back to future release.

The new filter solves the immediate problem that site owners face. While a deep dive into the query behind the scenes could be warranted, I still don't see how having this filter is a bad thing at all, especially given that we already filter the HTML output inside the same function.

It's not a bad thing. It's just not a complete fix :-)

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


7 months ago

#68 @eclare
3 weeks ago

Any reason why @prettyboymp's fix with autocomplete wasn't included in any realease for 4 years? This issue is literally breaking large sites (30k+ users) and is present in non-network installs as well...

Note: See TracTickets for help on using tickets.