Make WordPress Core

Opened 12 years ago

Last modified 2 months ago

#19867 assigned enhancement

wp_dropdown_users() still not scalable

Reported by: prettyboymp's profile prettyboymp Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.3.1
Component: Users Keywords: needs-patch needs-screenshots
Focuses: ui, accessibility, 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 12 years ago.
user_dropdown_filter.diff (962 bytes) - added by prettyboymp 12 years ago.
user_autocomplete.2.diff (11.6 KB) - added by prettyboymp 12 years ago.
wp_suggest_users.alpha.1.diff (7.4 KB) - added by mpvanwinkle77 11 years ago.
19867.diff (4.3 KB) - added by helen 10 years ago.
19867.2.diff (16.9 KB) - added by helen 10 years ago.
19867.3.diff (17.1 KB) - added by helen 10 years ago.
19867.patch (833 bytes) - added by BevanR 9 years ago.
Workaround
19867-04.diff (1.0 KB) - added by norcross 9 years 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 9 years 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 9 years ago.
Add default arguments to wp_dropdown_users_args

Download all attachments as: .zip

Change History (120)

#1 @prettyboymp
12 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
12 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
12 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
12 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
12 years ago

  • Cc xoodrew@… added

#7 @Japh
12 years ago

  • Cc Japh added

#8 @scribu
12 years ago

Closed #20769 as dup.

#9 @SergeyBiryukov
12 years ago

#21286 closed as a duplicate.

#10 @scribu
12 years ago

  • Milestone changed from Awaiting Review to Future Release

#11 @brokentone
11 years ago

  • Cc kenton.jacobsen@… added

#12 @SergeyBiryukov
11 years ago

#23129 was marked as a duplicate.

#13 @mpvanwinkle77
11 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
11 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
11 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
11 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
11 years ago

#23439 was marked as a duplicate.

#18 @toscho
11 years ago

  • Cc info@… added

#19 @SergeyBiryukov
11 years ago

#24557 was marked as a duplicate.

#20 @mpvanwinkle77
11 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
11 years ago

  • Cc kevinlangleyjr added

#23 @timfield
10 years ago

  • Cc tim@… added

#24 follow-up: @nacin
10 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
10 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
10 years ago

#8648 was marked as a duplicate.

#27 @helen
10 years ago

  • Focuses ui added
  • Keywords ui-feedback removed

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


10 years ago

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


10 years ago

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


10 years ago

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


10 years ago

@helen
10 years ago

@helen
10 years ago

@helen
10 years ago

#32 @helen
10 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
10 years ago

#28078 was marked as a duplicate.

#34 @docjohn
10 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
10 years ago

#29665 was marked as a duplicate.

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


10 years ago

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


10 years ago

#38 @chriscct7
10 years ago

#12014 was marked as a duplicate.

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


10 years ago

#40 @chriscct7
10 years ago

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

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


10 years ago

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


10 years ago

#43 @tharsheblows
10 years 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.


10 years ago

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


9 years ago

@BevanR
9 years ago

Workaround

#46 @BevanR
9 years 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
9 years ago

Related / relies on: #31696

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

#49 @helen
9 years ago

#33108 was marked as a duplicate.

#50 @SergeyBiryukov
9 years ago

#33150 was marked as a duplicate.

#51 @tomaston
9 years 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.


9 years ago

#53 @wonderboymusic
9 years ago

  • Milestone changed from Future Release to 4.4

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


9 years ago

@norcross
9 years ago

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

#55 @norcross
9 years 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
9 years 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
9 years 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
9 years ago

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

#58 @tharsheblows
9 years 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.


9 years ago

#60 @DrewAPicture
9 years 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
9 years 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
9 years ago

Add default arguments to wp_dropdown_users_args

#62 @helen
9 years 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
9 years 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
9 years 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
9 years 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
9 years 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.


8 years ago

#68 @eclare
8 years 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...

#69 @wilrevehl
8 years ago

  • Type changed from defect (bug) to enhancement

This is still a problem as of 4.6 and will become more and more of a problem as WooCommerce proliferates and uses Users the same way for Customers. Sure, we might only have a dozen authors for marketing or whatever but it doesn't take long to rack up 10s of thousands of customers and crush that Select dropdown. It almost certainly needs switched to AJAX.

Thanks guys, you're heroes.

This ticket was mentioned in Slack in #bbpress by netweb. View the logs.


7 years ago

#71 @rmens
7 years ago

This is still a problem as of 4.7.

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


7 years ago

#73 @SergeyBiryukov
6 years ago

#43529 was marked as a duplicate.

#74 @iandunn
6 years ago

Just wanted to point out that there's a fork of Select2 that fixes some of the a11y problems, in case anyone is interested in contributing to it, and then working to get those changes merged upstream.

#75 @smbrown
5 years ago

  • Focuses accessibility added

I requested @prettyboymp post this ticket 7 years ago. we have > 275,000 users. still a problem. now we don't even bother and do direct SQL on the production DB cluster. not a great practice but this is functionally unusable for us.

#76 @SergeyBiryukov
4 years ago

#50430 was marked as a duplicate.

#77 @patopaiar
4 years ago

Confirming that this is in fact a critical problem breaking down the Delete user functionality in this site with ~400,000 users when the function attempts to load them all into the selector.

How to solve this?

  • Paginating results somehow when a large user base is detected?
  • Displaying a search box that fetches specific results? The search might result an expensive operation too in a large user base, re-introducing the problem trying to be solved.

I have practically no experience contributing to core, but would be interested in helping with a patch if there's interest in solving this quite nasty problem in core.

Version 0, edited 4 years ago by patopaiar (next)

#78 @SergeyBiryukov
3 years ago

#51876 was marked as a duplicate.

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


3 years ago

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


17 months ago

#81 @joedolson
15 months ago

  • Owner set to joedolson
  • Status changed from new to accepted

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


14 months ago

#83 @joedolson
14 months ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from Future Release to 6.3

We're going to take another stab at this for 6.3.

#84 @spacedmonkey
13 months ago

Now that #40613 is in core, I wonder if this ticket is valid. User Query queries now cache, so would help this a lot.

#85 follow-up: @joedolson
13 months ago

It’s still valid. There are still major in-browser performance and usability problems when loading user select inputs with 10,000 values - especially in contexts like the importer, that could be loading each select dozens of times on the page.

#86 in reply to: ↑ 85 @azaozz
13 months ago

Replying to joedolson:

It’s still valid. There are still major in-browser performance and usability problems

Yea, in my opinion the browser/UI/UX problems have always been much larger than the PHP or DB problems, and much harder to fix. Seems the only possibilities would either be autocomplete or some sort of "advanced search" popup/modal. Any type of "drop-downs", even multi-row, seem impossibly hard to use when the entries are somewhere over 50-ish, no matter how you look at it.

#87 follow-up: @iandunn
13 months ago

What do you think about using ComboboxControl as an autocomplete? It'd probably be easier to iterate on compared to Select[2|Woo], since it's first-party code.

IIRC, in the past we've used a single instance of an object when many are needed on the same page, and then swapped the data on the fly (maybe with TinyMCE)? Would that be helpful here?

#88 in reply to: ↑ 87 @azaozz
13 months ago

Replying to iandunn:

What do you think about using ComboboxControl as an autocomplete?

Yep, this looks really good imho, and meets the accessibility requirements I think.

...then swapped the data on the fly

Think one of the features of the ComboboxControl is that the (previously used) data is stored locally and can be reused by all instances. So perhaps the drop-down part of all instances can be updated with the new(er) entries after every search (still need to experiment with how that would "feel" but think it would be nicer than showing some default data everywhere).

#89 @oglekler
11 months ago

  • Milestone changed from 6.3 to 6.4

This is an enhancement, and we are in 12 days until Beta 1 after which we will not add new enhancement to the release, so, because there is no patch so far, I am moving this ticket to 6.4.

By the look of the amount of watching ticket (they are not getting updates yet if they didn't participate in the discussion) this is a fascinating enhancement. And it seems that general solution is founded, so, if you want to work on this interesting ticket, plan it for the patch to be in the trunk until Beta 1 of the 6.4 release 🙌

#90 @bedas
10 months ago

#58906 was marked as a duplicate.

#91 @davidbaumwald
10 months ago

#58906 was marked as a duplicate.

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


10 months ago

#93 @joedolson
10 months ago

I'd support using ComboBoxControl for this. I agree that using a first-party component gives us a lot more flexibility on iteration.

#94 @oglekler
9 months ago

  • Keywords needs-screenshots added

@joedolson if the decision is to use ComboBoxControl, what needs to be done next, do we need help from the Editor developers?

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


9 months ago

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


9 months ago

#97 @joedolson
9 months ago

Somebody comfortable with React probably needs to step in to move this forward. I don't really know what would need to happen for that component to be used elsewhere in core. Somebody from the Editor team would definitely be helpful.

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


9 months ago

#99 @mukesh27
8 months ago

  • Milestone changed from 6.4 to 6.5

This is considered an enhancement, and we are currently in the week leading up to Beta 1. After this point, no new enhancements will be added to the release. Therefore, since there is no patch available at the moment, I am moving this ticket to milestone 6.5.

If anyone believes it's ready for 6.4, please feel free to move it back to that milestone.

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


8 months ago

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


7 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


7 months ago

#103 @joemcgill
7 months ago

  • Milestone changed from 6.5 to Future Release
  • Owner joedolson deleted
  • Status changed from accepted to assigned

During routine ticket refinement for the 6.5 release, we noticed that this ticket continues to get moved from release to release. Based on @joedolson's last comment, I'm going to move this to Future Release and un-assign, so someone else can pick this up. If an approach is decided, this can always come back into the milestone.

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


7 months ago

This ticket was mentioned in Slack in #core-php by khadreal. View the logs.


4 months ago

#106 @swissspidy
3 months ago

#60642 was marked as a duplicate.

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


2 months ago

#108 @pbearne
2 months ago

@mtias With A new WP-admin being written this should move to that project

#109 @bedas
2 months ago

#49416 was marked as a duplicate.

Note: See TracTickets for help on using tickets.