Make WordPress Core

Opened 16 months ago

Last modified 3 months ago

#39691 assigned enhancement

wpLink autocomplete search improvement

Reported by: enrico.sorcinelli Owned by: adamsilverstein
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.8
Component: Editor Keywords:
Focuses: Cc:


This patch offers a small improvement to wpLink (and tinymce plugin) autocomplete search in order to reduce requests to the server.

It normalizes the search terms by make it lowercase (based on DB collate setting) and trims / removes redundant blank spaces before to send a new one search.

I know, this way (for example by putting two words separate by several blank spaces), it can skip the first CASE/WHEN in the ORDER BY clause, since the exact query is used into a LIKE statement to determine the order value.

The patch also exposes dbcollate JavaScript variabile in the admin header that is used to check case insensitive DB setting. I think that it could be useful also in other situations.

Feedback and suggestions are appreciated


PS: I haven't seen any tests (phpunit/qunit) related to wpLink so currently I haven't provided a new one.

Attachments (4)

39691.patch (2.3 KB) - added by enrico.sorcinelli 16 months ago.
39691.2.patch (2.3 KB) - added by enrico.sorcinelli 11 months ago.
I just updated the patch to the current trunk.
39691.3.patch (2.3 KB) - added by enrico.sorcinelli 11 months ago.
Fixed code (added semicolon)
39691.4.patch (4.5 KB) - added by enrico.sorcinelli 11 months ago.
Updated with unit test

Download all attachments as: .zip

Change History (14)

11 months ago

I just updated the patch to the current trunk.

#1 @enrico.sorcinelli
11 months ago

I just updated the patch to the current trunk.

This ticket was mentioned in Slack in #core-js by enrico.sorcinelli. View the logs.

11 months ago

11 months ago

Fixed code (added semicolon)

#3 @adamsilverstein
11 months ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

@enrico.sorcinelli Thanks for your patch! This seems like a useful enhancement.

So this would remove an extra request to the server after typing a space?

I'm not sure I understand how the search string normalization affects the underlying db query, can you give some examples of the search string and query before/after normalization? Getting some of these cases in tests and adding a test for normalizeQuery would be very helpful. We have some tests in tests/wp-includes/js/tinymce/plugins/ as a starting point.

I wonder if normalizeQuery would be more generally useful across the admin and should be added to the wp namespace?

A few tips to match the WordPress JavaScript Coding Standards. The code needs a little more whitespace, and the conditionals should be Yoda conditionals. Also, the comment lines should be capitalized. Finally, dbcollate should probably be camel cased: dbCollate

#4 @afercia
11 months ago

Related, as a proposal to use the REST API: #38920 (note: see current limitations of the REST API for this use case described in the ticket).

11 months ago

Updated with unit test

#5 @enrico.sorcinelli
11 months ago

@adamsilverstein I just updated the patch according to your suggestions.

Yes, this patch will avoid to send request to server by typing additional spaces to the current query. Also, based on DB collate setting, it make the queries lowercase (this can reduce also requests to server and/or any server caching using query as key).

The normalizeQuery function doesn't strictly related to TinyMCE plugins, so I added a new one unit test file tests/qunit/wp-includes/js/wplink.js where you could see some normalization examples.

And finally, yes, the function can be added to the wp namespace (like wp.api.utils for example) if you think it appropriate.

#6 @adamsilverstein
11 months ago

@enrico.sorcinelli Thanks - your patch and tests look good. I'm still not sure I understand the dbCollate part: this line:

/_ci$/.test( 'undefined' !== typeof dbCollate ? dbCollate : '' )

So when dbcollate is devined and ends in _ci that means the database is case insensitive, so might was well pass lower case only to the query? how does that help? it seems like a caching mechanism could do this on the server side (lowercase the key based on dbcollate), and you are going to quite a bit of effort to make that happen here.

I would leave that part out unless there is some clear advantage, in which case I would explain that wuth an inline doc and ideally we would add tests demonstrating the advantage or at least how the dbColate calue affects normalizeQuery.

#7 @enrico.sorcinelli
11 months ago

@adamsilverstein The line above is the default value for options.lower that lowercases the string in the normalizeQuery function.

So if the database is case insensitive, typing Foo or foo from client, the search will return same results: the patch simply prevents to send to the server both requests if the user types subsequently two or more queries differing only by spaces and/or case sensitive.

I was thinking that exposing dbCollate as a global JavaScript variable could be useful in other client situations, even if on the server side we can lowercase the keys based on $wpdb->collate. Anyway, currently, the core does not by default, so we can improve also that code.

#8 @adamsilverstein
10 months ago

I like the trimming here, that makes sense. I'm a bit unsure about the db collate setting and the benefits of lowercasing searches there.

maybe @pento could review and weigh in on the db part and say if this is a good approach/idea? Any risk? enough benefit?

#9 @pento
10 months ago

Thanks for the patch, @enrico.sorcinelli! Checking the collation is a cool idea, but I don't think this is the right place for it, for a few reasons.

There's no real risk to checking the collation, though I don't think that there's a benefit.

Caching strategies should be handled by the caching code - there are many places in Core that don't consider the collation's case sensitivity before running the query, micro-optimising this one place adds code complexity, without addressing the bigger picture.

For a global query cache, like the one you're describing, that would really need to be handled by a specialised caching tool, as it's heavily dependent on site behaviour. For example, MySQL is removing it's own Query Cache, because it was never really viable as a general caching option.

#10 @adamsilverstein
3 months ago

@enrico.sorcinelli given the feedback above from pento, the only part of the patch that seems possibly useful is trimming spaces off the search term. Even that change may not be without side effects - it is possible not sending a space could change results if the term isn't already being trimmed on the back end. Also, I'm a bit concerned about only changing this search instance when we have so many search fields in core that don't operate this way.

Note: See TracTickets for help on using tickets.