WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 22 months ago

Last modified 15 months ago

#13961 closed defect (bug) (fixed)

search query_vars bug when using search base

Reported by: shidouhikari Owned by: markjaquith
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.0
Component: Query Keywords: has-patch 3.5-early
Focuses: Cc:

Description

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.

Attachments (4)

13961.patch (508 bytes) - added by SergeyBiryukov 3 years ago.
13961.2.patch (508 bytes) - added by SergeyBiryukov 3 years ago.
13961.3.patch (527 bytes) - added by SergeyBiryukov 2 years ago.
13961.4.patch (571 bytes) - added by SergeyBiryukov 22 months ago.

Download all attachments as: .zip

Change History (36)

comment:1 nacin4 years ago

  • Milestone changed from 3.0.1 to Awaiting Review

comment:2 nacin3 years ago

  • Keywords reporter-feedback added

Use get_search_query() and the_search_query().

comment:3 dd323 years ago

  • Component changed from General to Query
  • Keywords 3.2-early needs-patch added; reporter-feedback removed
  • Milestone changed from Awaiting Review to Future Release

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.

comment:4 dd323 years ago

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.

comment:5 dd323 years ago

The patch posted on 16093 does not work:

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

Version 0, edited 3 years ago by dd32 (next)

SergeyBiryukov3 years ago

comment:6 SergeyBiryukov3 years ago

  • Keywords has-patch added; needs-patch removed

comment:7 follow-up: dd323 years ago

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)

comment:8 in reply to: ↑ 7 SergeyBiryukov3 years ago

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.

comment:9 follow-up: dd323 years ago

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

comment:10 SergeyBiryukov3 years ago

  • Keywords needs-testing added

comment:11 bennettmcelwee3 years ago

  • Cc bennettmcelwee added

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:

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.

comment:12 bennettmcelwee3 years ago

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

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

SergeyBiryukov3 years ago

comment:13 in reply to: ↑ 9 SergeyBiryukov3 years ago

  • Keywords 3.2-early removed
  • Milestone changed from Future Release to 3.3

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.

comment:14 follow-up: nacin2 years ago

$_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.

comment:15 nacin2 years ago

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

SergeyBiryukov2 years ago

comment:16 in reply to: ↑ 14 SergeyBiryukov2 years ago

Replying to nacin:

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

Done in 13961.3.patch.

comment:17 azaozz2 years ago

  • Milestone changed from 3.3 to Future Release

Not a regression or new bug.

comment:18 SergeyBiryukov2 years ago

  • Milestone changed from Future Release to 3.4

Closed #19694 as a duplicate.

comment:19 sirzooro2 years ago

  • Cc sirzooro added

comment:20 nacin2 years ago

  • Milestone changed from 3.4 to Future Release

comment:21 nacin2 years ago

  • Keywords 3.5-early added

comment:22 nacin2 years ago

Unit tests would be cool, if possible.

comment:23 SergeyBiryukov22 months ago

  • Keywords needs-unit-tests added; needs-testing removed

comment:24 SergeyBiryukov22 months ago

  • Milestone changed from Future Release to 3.5

Closed #21118 as a duplicate.

comment:25 markjaquith22 months ago

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

In [21187]:

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

comment:26 markjaquith22 months ago

  • Keywords needs-unit-tests removed

Unit test: [UT755]

comment:27 follow-up: nacin22 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

comment:28 in reply to: ↑ 27 ; follow-up: markjaquith22 months ago

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.

comment:29 in reply to: ↑ 28 ; follow-up: nacin22 months ago

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.

SergeyBiryukov22 months ago

comment:30 SergeyBiryukov22 months ago

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.

comment:31 nacin22 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

comment:32 in reply to: ↑ 29 SergeyBiryukov15 months ago

Replying to nacin:

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

Follow-up: #23329

Note: See TracTickets for help on using tickets.