search query_vars bug when using search base

[shidouhikari]

Use this debug code anywhere in a theme file:

global $wp_query;
$search_string = $wp_query->query_vars['s'];
echo "<p>$search_string</p>";

Then use a search form to do a normal search using more than a keyword, for exemple "search string", so that your testing site has posts that match it. It will result in something like ​http://domain.com/?s=search+string&submit=Go, debug will print "search string" and index.php will list all found posts as expected.

Now use search base version: ​http://domain.com/search/search+string ... debug prints "search+string" and since there's no post with "search-string", it is says there's no post matching.

I was unable to find where '+' should be but isn't replaced to ' ', and it's even more strange that only when search base is used that this bug happens.

[nacin]

Use get_search_query() and the_search_query().

[dd32]

The issue here is that ?s=test%20search results in get_search_query() returning 'test search', whilst, /search/test+search/ results in 'test+search' (as it should, as + is not the space character in this case), /search/test%20search/ results in 'test%20search' which IS wrong.

[dd32]

This also causes problems for non-ASCII language search terms: #16093

As originally ​reported by Vladimir Kolesnikov, search results for Cyrillic keywords are incorrect when using pretty permalinks.

When using ?s=%D1%81%D0%BB%D0%BE%D0%B2%D0%BE, the search string is decoded by PHP. When using search/%D1%81%D0%BB%D0%BE%D0%B2%D0%BE/, we should perform urldecode() ourselves.

[dd32]

The ​patch posted on 16093 does not work in some cases.

Patch is bad, It double-decodes ?s=... search requests, For example, ?s=test%2520search decodes to test search instead of test%20search.

It does however work for /search/test%2520search/ it searches for test%20search as correct.

[dd32]

Do we need to stripslashes it when it's coming from a Permastruct?

We probably should also use $wp_rewrite->using_permastruct or something instead of empty($_GET)

[SergeyBiryukov]

Replying to dd32:

Do we need to stripslashes it when it's coming from a Permastruct?

Looks like not. However it doesn't seem to have a difference in this case.

Replying to dd32:

We probably should also use $wp_rewrite->using_permastruct or something instead of empty($_GET)

Did you mean $wp_rewrite->using_permalinks()? Since a search request can come through $_GET in this case too (and it does so in Twenty Ten search widget, regardless of the permalink structure), I guess empty($_GET) is a more reliable check.

[dd32]

I guess empty($_GET) is a more reliable check.

You're right in that case, patch needs testing for stripslashes/etc but seems good for 3.2

[bennettmcelwee]

A note for testers. This patch should also fix a related issue to do with spaces. The URL

http://domain.com/search/search%20string

should carry out a search for "search string", but it currently searches for "search%20string" instead and thus fails to find any results.

For some live examples, try these:

• ​http://wpdevel.wordpress.com/?s=open%20source
• ​http://wpdevel.wordpress.com/?s=open+source
• ​http://wpdevel.wordpress.com/search/open%20source
• ​http://wpdevel.wordpress.com/search/open+source

The first two work and the third fails, but oddly enough the fourth one works... except that the displayed search string still has the plus sign in it.

[bennettmcelwee]

I have created a little plugin that effectively applies this patch.

​http://wordpress.org/extend/plugins/search-fixer/

[SergeyBiryukov]

Replying to dd32:

You're right in that case, patch needs testing for stripslashes/etc but seems good for 3.2

Tested the patch with keywords like //test%20search and other examples from here.

Refreshed for 3.3.

[nacin]

$_GET is a clever check but it seems like something could go wrong. It should probably only apply for is_main_query() in that case.

[nacin]

Also, I would think that + should be manually translated to a space for searches. That sounds like typical/proper behavior.

[SergeyBiryukov]

Replying to nacin:

It should probably only apply for is_main_query() in that case.

Done in 13961.3.patch​.

[azaozz]

Not a regression or new bug.

[SergeyBiryukov]

Closed #19694 as a duplicate.

[nacin]

Unit tests would be cool, if possible.

[SergeyBiryukov]

Closed #21118 as a duplicate.

[markjaquith]

In [21187]:

urldecode() search strings that come in from /search/foo. props SergeyBiryukov. fixes #13961

[markjaquith]

Unit test: ​[UT755]

[nacin]

Should this be $this->is_main_query()? Or should we just always decode?

[markjaquith]

Replying to nacin:

Should this be $this->is_main_query()? Or should we just always decode?

If someone is manually doing a new WP_Query, I don't think we want to decode.

[nacin]

Replying to markjaquith:

Replying to nacin:

Should this be $this->is_main_query()? Or should we just always decode?

If someone is manually doing a new WP_Query, I don't think we want to decode.

Sure. But right now this will always fire on new WP_Query, unless query_posts() was used, as is_main_query() will just return true. This needs to be $this->is_main_query().

Kinda want to deprecate the is_main_query() function and leave just the method. It's confusing.

[SergeyBiryukov]

Just reviewed the process again. We only want to decode values that come from /search/, not from a custom query. $this->is_main_query() indeed makes more sense.

[nacin]

In [21248]:

Use is_main_query() method, rather than the function. Only decode the search query variable if we are executing the main query and it came from the /search/ base, rather than the query string. fixes #13961.

[SergeyBiryukov]

Replying to nacin:

Kinda want to deprecate the is_main_query() function and leave just the method. It's confusing.

Follow-up: #23329