WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 8 months ago

Last modified 3 months ago

#11330 closed defect (bug) (fixed)

Empty search takes you to homepage instead of empty search page

Reported by: jacobfogg Owned by: wonderboymusic
Milestone: 4.0 Priority: low
Severity: minor Version: 2.8.5
Component: Query Keywords:
Focuses: Cc:

Description

  1. In the admin tool, if you go to "Settings->Reading" then choose "A static page (select below)" and choose a page for your "Front page" and a different page for your "Posts page".
  2. Visit your blog (http://localhost/ in my case) and you will see your static page.
  3. Navigate to your blog (http://localhost/blog/ in my case).
  4. Enter a search string (e.g. "test") and click "Search". This will take you to the search results page (http://localhost/?s=test in my case). Notice that it is located at the base of your site.
  5. Return to your blog, leave the search textbox blank and click "Search". This _should_ take you to the search results page but takes you to the _Homepage_ instead (http://localhost/?s= in may case).

IMHO, a blank search string _should_ take you to the search page and state "No posts found. Try a different search?" as it does when you enter an invalid search. At the very least, it should take you to the blog page.

The reason it takes you to the homepage is due to how the get_search_form() function in the /wp-includes/general-template.php file creates the Action url for the search form:

$form = '<form role="search" method="get" id="searchform" action="' . get_option('home') . '" >'

I propose a change in the code that will do the following:

if(get_option('show_on_front') == 'page'){//if the blog is not the front page
	$searchAction = get_page_link(get_option('page_for_posts'));//get the link to the blog
}else{//if it is the front page
	$searchAction = get_option('home')."/";//get the link to the home
}

$form = '<form role="search" method="get" id="searchform" action="' . $searchAction . '" >

That way, at the very least, you know the search is landing where it belongs.

This, however, does not _fix_ the issue where a blank search does not land you on the search page. My guess is, wherever it is determined what page is displayed, there is a check for something like:

if($_REQUEST['s']!=''){
  //if the request var 's' is not blank, this is a search
}

If that is indeed the case, a simple change of the code to something like this should do the trick:

if(isset($_REQUEST['s'])){
  //if the request var 's' isset, this is a search
}

Attachments (8)

empty-search-string.diff (1.2 KB) - added by wonderboymusic 3 years ago.
empty-search-string.2.diff (1.3 KB) - added by wonderboymusic 3 years ago.
11330.diff (1.4 KB) - added by wonderboymusic 21 months ago.
11330.2.diff (1.3 KB) - added by wonderboymusic 12 months ago.
11330.3.diff (1.2 KB) - added by SergeyBiryukov 11 months ago.
11330.4.diff (388 bytes) - added by SergeyBiryukov 11 months ago.
11330.5.diff (740 bytes) - added by SergeyBiryukov 11 months ago.
11330.6.diff (453 bytes) - added by SergeyBiryukov 11 months ago.

Download all attachments as: .zip

Change History (66)

comment:1 @Denis-de-Bernardy5 years ago

  • Component changed from General to Template
  • Keywords close added
  • Milestone changed from Unassigned to Future Release
  • Priority changed from normal to low
  • Severity changed from normal to trivial
  • Type changed from defect (bug) to enhancement

itching to close this as wontfix.

you're describing the expected behavior, which works fine, so this technically is an enhancement. and then, it seems extremely specialized.

comment:2 @strider725 years ago

  • Keywords needs-patch added; close removed
  • Severity changed from trivial to minor

Well as enhancements go, it makes a lot of sense. Hitting the Search button should take you to a Search result page, even if there is no result.

comment:3 @scribu5 years ago

This is related to #10710.

comment:4 @jacobfogg5 years ago

This may seem trivial if WP is using as _just_ a blog, however you are using the site as a web page with a blog, a search performed on a blog page should NOT send you back to the home page.

comment:5 @jacobfogg5 years ago

  • Cc jacobfogg added

comment:6 @thomask5 years ago

  • Type changed from enhancement to defect (bug)

please when this trivial needs patch will be solved?

Imho it is bug, search should definitely put you on search page even when you search nothing. So we would be easily able to create separate search page (in many countries it is a must-have accesibility rule when creating webs e.g. for public administration)

P.S.: if i would consider this as an enhancement, it would be to have an option to give it some sexy url, but this could be solved by something like

add_action('template_redirect', 'nice_search');
function nice_search() {
    if (is_search()) {
        $search_uri = get_bloginfo('url') .'/search/'. urlencode(get_query_var('s')) . ((get_query_var('paged')) ? '/page/'. get_query_var('paged') .'/' : '/');
        if (!empty($_GET['s']) || !empty($_GET['paged']))
            wp_redirect($search_uri);
    }
}

comment:7 follow-up: @strop3 years ago

  • Component changed from Template to Query
  • Summary changed from Empty search takes you to hompage, even when homepage is not your blog to Empty search takes you to hompage instead of empty search page
  • Version changed from 2.8.5 to 3.3

Other solutions are cropping up that indicate that this "should" really be updated.
What's the logic in not counting an empty search as search?

function fix_empty_search ($query){
    global $wp_query;
    if (isset($_GET['s']) && empty($_GET['s'])){
        $wp_query->is_search=true;
    }
    return $query;
}
add_action('pre_get_posts','fix_empty_search');
Last edited 3 years ago by strop (previous) (diff)

comment:8 @strop3 years ago

  • Cc oskar@… added

comment:9 in reply to: ↑ 7 @scribu3 years ago

  • Version changed from 3.3 to 2.8.5

Please don't "bump" the version.

Replying to strop:

What's the logic in not counting an empty search as search?

The logic was that WP_Query can't distinguish between when the search parameter is empty vs. when it's not set at all.

comment:10 follow-up: @scribu3 years ago

Like every patch involving WP_Query, extreme care will have to be applied in order to not cause other unexpected behavior.

comment:11 in reply to: ↑ 10 @strop3 years ago

Replying to scribu:

Like every patch involving WP_Query, extreme care will have to be applied in order to not cause other unexpected behavior.

Of course, but it shouldn't stop an obvious bug from being fixed.

Sorry about the version bump.

comment:12 @foxinni3 years ago

This issue has come up in my development cycle too.

I'm opting for strop's solution but I've modified it to work for me, especially the need to put is_home to false.

function search_query_filter($query) {
	
   // If 's' request variable is set but empty
   if (isset($_GET['s']) && empty($_GET['s']) && $query->is_main_query()){
      $query->is_search = true;
      $query->is_home = false;
   }  
   return $query;

}
add_filter('pre_get_posts', 'search_query_filter');

comment:13 @SergeyBiryukov3 years ago

  • Summary changed from Empty search takes you to hompage instead of empty search page to Empty search takes you to homepage instead of empty search page

Closed #10156 as a duplicate.

comment:14 @wonderboymusic3 years ago

  • Keywords has-patch added; needs-patch removed

Patch does the following:

1) WP_Query::is_search is true if 's' query var exists (isset)

2) The search query becomes AND 0 if the term is empty so the SQL query will tank right away

3) Ditches the comment join generation if the search term is empty

Result: empty search terms go to the search page

comment:15 @SergeyBiryukov3 years ago

Replying to wonderboymusic:

1) WP_Query::is_search is true if 's' query var exists (isset)

This doesn't work as intended. Query vars (including 's') are always set in fill_query_vars():
http://core.trac.wordpress.org/browser/tags/3.4.2/wp-includes/query.php#L1356

With the patch applied, home page is displayed in search.php template and shows "Nothing Found".

Related: #18485

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

comment:16 @wonderboymusic3 years ago

ah yes I see - I attached a new patch, is_search = not empty OR main_query and 's' query_var exists

comment:18 @scribu3 years ago

This seems like a bolted-on solution. Why should we treat just the 's' parameter differently?

comment:19 @wonderboymusic3 years ago

That's my problem with 800 line methods and 200 line loops (elsewhere), makes it hard to do surgery. I am open to alternative solutions.

comment:20 @scribu3 years ago

The length of the method has nothing to do with it, though it will probably grow, since refactoring for it's own sake is frowned upon, since we don't have unit tests to ensure that everything still works afterwards.

Anyway, the issue is that we ignore all empty query vars, except for 's', but I guess that's ok, since it's the only one that does a search, rather than an exact match. Well, we also have 'sentence', actually.

comment:21 @dcowgill2 years ago

  • Cc dcowgill@… added

comment:22 @MikeHansenMe2 years ago

  • Cc mdhansen@… added

comment:23 @dotnetCarpenter23 months ago

  • Cc dotnetCarpenter added

@wonderboymusic21 months ago

comment:24 @wonderboymusic21 months ago

  • Milestone changed from Future Release to 3.7

Refreshed, works

comment:25 @nacin19 months ago

Not so sure. Isn't 's' always going to be a filled query var? Seems like $_GET might have been desired here? Or, better probably, looking in $wp->query_vars.

comment:26 @nacin19 months ago

  • Milestone changed from 3.7 to 3.8

comment:27 follow-up: @nacin17 months ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 3.8 to Future Release

"s" is always going to be a filled query variable. This patch doesn't work.

comment:28 in reply to: ↑ 27 @GhostToast15 months ago

Replying to nacin:

"s" is always going to be a filled query variable. This patch doesn't work.

When I reproduce this anomaly, get_queried_object() appears to return absolutely nothing. Could this be turned into a possible solution? When else is that the case, 404?

@wonderboymusic12 months ago

comment:29 @wonderboymusic12 months ago

  • Keywords has-patch commit added; needs-patch removed
  • Milestone changed from Future Release to 4.0

11330.2.diff checks $this->query for existence of unadulterated's'

comment:30 @wonderboymusic11 months ago

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

In 28612:

When parsing the main query, if s is set to empty: ?s= and $this->is_main_query() && array_key_exists( 's', $this->query ) - kill the query instead of loading the homepage. This will load the search page with no results.

Fixes #11330.

@SergeyBiryukov11 months ago

comment:31 @SergeyBiryukov11 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened
  1. Why limiting this to the main query? I'd expect WP_Query( 's=' ) to work the same way, which would be consistent with #28099.
  2. array_key_exists() can be replaced with isset(), and removing $this->is_main_query() makes the $qv['s'] check redundant (a non-empty $qv['s'] means that $this->query['s'] is set).
  3. $search = 'AND 0' looks confusing where it is, it could be moved to a more appropriate place.

11330.3.diff addresses these points.

comment:32 @wonderboymusic11 months ago

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

In 28623:

Simplify the logic for determining isset( $this->query['s'] ) after [28612], and don't limit this logic to just the main query.

Props SergeyBiryukov.
Fixes #11330.

@SergeyBiryukov11 months ago

comment:33 @SergeyBiryukov11 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This broke filters in the admin:

  1. Go to All Posts screen.
  2. Select any category in "View all categories" dropdown and click "Filter".
  3. You'll get "No posts found", because the URL contains an empty s parameter.
  4. If you choose a month in "All dates" dropdown instead, you'll also get an SQL error:
    WordPress database error: [You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '0 AND develop_posts.post_type = 'post' AND (develop_posts.post_status = 'publish' at line 1]
    SELECT SQL_CALC_FOUND_ROWS develop_posts.ID FROM develop_posts WHERE 1=1 AND YEAR(develop_posts.post_date)=2014 AND MONTH(develop_posts.post_date)=06AND 0 AND develop_posts.post_type = 'post' AND (develop_posts.post_status = 'publish' OR develop_posts.post_status = 'future' OR develop_posts.post_status = 'draft' OR develop_posts.post_status = 'pending' OR develop_posts.post_status = 'private') ORDER BY develop_posts.post_date DESC LIMIT 0, 20

11330.4.diff fixes the SQL error.

I'm not quite sure what to do with the general filter breakage. Looks like this fix can lead to unexpected results elsewhere, so perhaps it should be reverted.

comment:34 @SergeyBiryukov11 months ago

I mean, we could add a ! is_admin() check, but it still can affect custom filter forms on front-end.

Last edited 11 months ago by SergeyBiryukov (previous) (diff)

comment:35 follow-up: @wonderboymusic11 months ago

In 28666:

Prevent admin breakage for s in WP_Query after [28623].

See #11330.

comment:36 @nacin11 months ago

Worth pointing out we have a $query->is_admin for a check like this.

comment:37 @wonderboymusic11 months ago

yeah that sounds a lot better - will look post-meeting

comment:38 follow-up: @johnbillion11 months ago

It sounds like this has the potential to break in the same way for faceted search forms on the front end of a site.

@SergeyBiryukov11 months ago

comment:39 in reply to: ↑ 35 @SergeyBiryukov11 months ago

Replying to wonderboymusic:

Prevent admin breakage for s in WP_Query after [28623].

To clarify, $this->is_main_query() is true for list tables, so this was technically broken in [28612], not in [28623].

Replying to johnbillion:

It sounds like this has the potential to break in the same way for faceted search forms on the front end of a site.

Yep, that's what I meant.

11330.5.diff uses $this->is_admin and moves the check to a more appropriate place if we want to keep it.

comment:40 @SergeyBiryukov11 months ago

In 28668:

Move is_admin check to a more appropriate place.

see #11330.

@SergeyBiryukov11 months ago

comment:41 in reply to: ↑ 38 @SergeyBiryukov11 months ago

Replying to johnbillion:

It sounds like this has the potential to break in the same way for faceted search forms on the front end of a site.

We should probably drop the ' AND 0' part (11330.6.diff).

The main point of this ticket (loading search template for an empty search instead of front page) is resolved.

Last edited 11 months ago by SergeyBiryukov (previous) (diff)

comment:42 @wonderboymusic11 months ago

@SergeyBiryukov: commit whatever you think is appropriate

comment:43 follow-up: @kovshenin10 months ago

r28623 broke infinite scroll in Jetpack for the same reason #28099 did. See this comment for some insight.

comment:44 in reply to: ↑ 43 @SergeyBiryukov10 months ago

Replying to kovshenin:

r28623 broke infinite scroll in Jetpack for the same reason #28099 did. See this comment for some insight.

Would 11330.6.diff fix it, or should we revert [28612]/[28623]/[28666]/[28668] completely?

comment:45 @wonderboymusic10 months ago

Jetpack should be passing the values from ->query, not ->query_vars

comment:46 @kovshenin10 months ago

Tested with .6.diff, seems to have resolved the issue.

Jetpack should be passing the values from ->query, not ->query_vars

As I mentioned in my comment on the other ticket, I believe it's fair to assume that the resulting query has been built with query_vars (which is publicly accessible), and by passing the same array to a new WP_Query, I would expect to get the same results. Obviously Jetpack and other plugins can use the query array instead, which is also publicly accessible, but they didn't.

comment:47 @SergeyBiryukov10 months ago

In 28804:

Don't kill an empty search query.

see #11330.

comment:48 @SergeyBiryukov10 months ago

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

To summarize the changesets here, we've changed this:

if ( ! empty( $qv['s'] ) ) {
	$this->is_search = true;
}

to this:

if ( isset( $this->query['s'] ) ) {
	$this->is_search = true;
}

The main point of this ticket (loading search template for an empty search instead of front page) is resolved.

comment:49 @nacin10 months ago

I've reviewed this through [28804]. I concur with the decisions here. Looks good.

comment:50 @kovshenin9 months ago

  • Keywords has-patch commit removed

Reporting some infinite scroll breakage (again), sorry! Since IS builds off of query_vars and not query, the scroll request sets an empty search, which is expected given this change. The results are still correct, however the context is wrong.

IS uses a theme's content.php file to produce output, and many such files will display the_content() or the_excerpt() based on is_search(), like Twenty Twelve, so when scrolling Twenty Twelve is loading excerpts, rather than the post content because with an empty s the context is always search.

Changing IS to build the query from query and not query_vars seems to fix the issue, but reporting it anyway, just in case you guys wanna revert this ;)

comment:51 @nacin8 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening due to kovshenin's two-month-old comment.

comment:52 @ircbot8 months ago

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.

comment:53 @wonderboymusic8 months ago

I actually checked out IS today on one of my sites. It was broken for a while, now it works. IS should be built by passing query not query_vars.

They patched for 4.0:
https://github.com/Automattic/jetpack/commit/baa5b7a8ffab784dbbb9f7e136dcbf387a6ffe38

Since they are passing to WP_Query directly, those args are actually getting double parsed:
https://github.com/Automattic/jetpack/blob/master/modules/infinite-scroll/infinity.php#L900

Nobody makes a WP_Query with every arg passed. If you only pass the args that were originally passed (represented by WP_Query->query, you'll end up with all of those args anyways.

comment:54 @ircbot8 months ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

comment:55 @SergeyBiryukov8 months ago

We can probably leave it as is for now, haven't seen any other reports in alpha/beta forums.

comment:56 @helen8 months ago

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

comment:57 @Stagger Lee8 months ago

What is now snippet for functions.php ?
All i try give me blank page and errors.

comment:58 @slackbot3 months ago

This ticket was mentioned in Slack in #core by sergeybiryukov. View the logs.

Note: See TracTickets for help on using tickets.