Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#20835 closed defect (bug) (fixed)

Multisite subdirectory autocompletes are broken

Reported by: nacin's profile nacin Owned by: nacin's profile nacin
Milestone: 3.4 Priority: high
Severity: blocker Version:
Component: Multisite Keywords: has-patch commit
Focuses: Cc:

Description (last modified by nacin)

I have the following sites set up:

  • localhost/beta/ ("WordPress")
  • localhost/beta/test/ ("Test")
  • localhost/beta/testsite/ ("Test Site")
  • localhost/beta/author-blog/ ("Author's Blog")
  • localhost/beta/author-second-blog/ ("Author's Second Blog")

1. Search term is not broken up based on domain and path

You can search for the domain "localhost" or the path "author", but not "localhost/author". We need to be smart enough inside wp_ajax_autocomplete_site() to split based on domain and path and search both fields, at which point we need to do an AND rather than an OR.

It should be noted that I am not sure if the wp-ms-sites list table performs this same check. It looks like it does not, and simply allows for searching domains for subdomain installs, and paths for subdirectory installs. So we may just need to strip off the domain for subdirectory searches (and vice versa with the path for subdomain searches). This, of course, is not very nice for installs with custom URLs that handle mapping via sunrise (with or without the domain mapping plugin).

But it is important to at least stick to a subset of the logic in wp-ms-sites to ensure that results in one place are results in where you end up, rather than logic that can return sites in the autocomplete that would not show up in the search.

2. The domain is autocompleted, rather than the path

The return value in wp_ajax_autocomplete_site() is always the domain. This is not helpful for subdirectories. Typing in "author" and selecting /localhost/author-blog/ autocompletes to "localhost".

3. Display of domains should use the same logic as sites.php

On sites.php, we only show /beta/, /test/, /author-blog/, etc. Depending on whether we are on a subdirectory or a subdomain install, we trim off domains and paths where possible. (This is nicely compatible with mapped domain setups, as the code then doesn't trim off anything, and you see the full URL.) But in the autocomplete, we show /localhost/beta. If this autocompletes to /localhost/beta (see point 2), sites.php won't return anything. If it autocompletes to just /beta, then it is confusing.

We should try to adhere to the searching and displaying logic of sites.php as much as possible. We should probably just wrap up the searching code into a method in the list table, then just use that method in wp_ajax_autocomplete_site().

4. The name of the blog should not be shown

In a user autocomplete, we show user_login and user_email. The only other field we search is user_nicename. But in site autocomplete, we only search domain and/or path, but we also show the site's blogname. This *could* be a nice feature, but it actually gets in the way, as A) they can't search for names, B) it shows names first, even though paths are the important, unique, identifiable part, C) sites.php doesn't even show names, D) it's unnecessary extra queries, and E) apostrophes are not properly converted ("Author's Blog").

Attachments (4)

autocomplete-site-patch.diff (2.0 KB) - added by wonderboymusic 11 years ago.
20835.diff (7.8 KB) - added by nacin 11 years ago.
20835.2.diff (11.2 KB) - added by nacin 11 years ago.
20835.3.diff (12.9 KB) - added by nacin 11 years ago.

Download all attachments as: .zip

Change History (16)

#1 @DrewAPicture
11 years ago

  • Cc xoodrew@… added

#2 @wonderboymusic
11 years ago

  • Keywords has-patch added

#3 @wonderboymusic
11 years ago

I think this attacks all of your use cases... ? take a look

#4 @nacin
11 years ago

  • Description modified (diff)

I messed up my example URLs above. My main install is not /localhost/, it is /localhost/beta/. The /beta/ is therefore $current_site->path, something that needs to be accounted for.

11 years ago

#5 @nacin
11 years ago

20835.diff should handle all four points. I liked where wonderboymusic was going, but after some discussion, we decided to leverage the list table.

#6 @nacin
11 years ago

  • Severity changed from major to blocker

#7 @Japh
11 years ago

  • Cc japh@… added

#8 @nacin
11 years ago

New thoughts, after an IRC conversation earlier in the day:

  • Autocomplete for a search field is useless, unless it allows you to click a suggestion and go directly to that result.
  • Autocomplete should therefore be removed for any "search" inputs, which would account for the user and site searches on the network dashboard, on network/sites.php, network/users.php, and wp-admin/users.php, etc.
  • This removes all site auto-completes, so we would kill off site-search.js and wp_ajax_autocomplete_site(). This likely removes a decent amount of code from user-search.js. This may remove some code from wp_ajax_autocomplete_user().
  • Autocomplete is most effective for adding existing users to a site, in multisite, whether that is on network/site-users.php or wp-admin/user-new.php. This is really the only place the feature should have been implemented with the current UX. (See point 1 for what that might look like.)
  • Should the autocomplete handlers in user-search.js be dependent on IDs? Seems like they would work better as classes.
  • "user-search" (and "site-search") should really be "user-autocomplete", or "user-suggest". I think I intend to rename the script handles and files to reflect their actual purpose.

I hope to resolve this in its entirety, and quickly, on June 5.

11 years ago

#9 @nacin
11 years ago

20835.2.diff renames user-search to user-suggest, eliminates it from search inputs, adds a .wp-user-suggest class to for plugin use, and eliminates search inputs entirely.

Mark Jaquith expressed interest in allowing suggestions to be clicked, to take them directly to that result. While relatively easy, it would still require that to only occur in non-autocomplete mode (adding a user versus looking for one in a search field), and we're so late in the cycle that the change in UX should wait.

11 years ago

#10 @nacin
11 years ago

  • Keywords commit added

Marking 20835.3.diff for commit. Signed off by helenyhou and MarkJaquith in IRC.

#11 @ryan
11 years ago

Looks good.

#12 @nacin
11 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [21003]:

Remove user/site suggestions (autocompletion) from search inputs, as the UX isn't proper.

  • Removes all instances of site-search, so away it goes. Sidesteps a number of bugs with site-search.
  • Renames user-search to user-suggest, which means it better describes the current behavior (autocompletion) while allowing for future behavior (instant search).
  • Ties user suggestions to a single .wp-suggest-user class.

with help from markjaquith, helenyhou, wonderboymusic.
fixes #20835.

Note: See TracTickets for help on using tickets.