wpLink autocomplete search improvement

[enrico.sorcinelli]

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

Regards

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

[enrico.sorcinelli]

I just updated the patch to the current trunk.

[enrico.sorcinelli]

I just updated the patch to the current trunk.

[enrico.sorcinelli]

Fixed code (added semicolon)

[adamsilverstein]

@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

[afercia]

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).

[enrico.sorcinelli]

Updated with unit test

[enrico.sorcinelli]

@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.

[adamsilverstein]

@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.

[enrico.sorcinelli]

@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.

[adamsilverstein]

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?

[pento]

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.

[adamsilverstein]

@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.

[adamsilverstein]

I am closing this as "maybelater" because it doesn't have a complete solution - the overall goal of improving optimizing searches is valid so maybe we can work on that elsewhere for example in Gutenberg autocompleter fields.

Thanks again for the contribution @enrico.sorcinelli even though we wound up not using it.