WordPress.org

Make WordPress Core

Opened 12 years ago

Closed 11 years ago

Last modified 10 years ago

#10667 closed enhancement (fixed)

Integration of Search API into core

Reported by: jshreve Owned by: westi
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.9
Component: General Keywords: close
Focuses: Cc:

Description

The Search API plugin (http://wordpress.org/extend/plugins/search/) should be converted to a patch for the next release. The MySQL fulltext plugin should also be included in the distro to replace the default search.

Attachments (8)

searchapi.diff (3.4 KB) - added by jshreve 12 years ago.
Makes the neccessary changes to convert the WPSearchAPI plugin to a working part of the core
search.php (34.3 KB) - added by jshreve 12 years ago.
SearchAPI Core file. (to be placed in wp-includes/)
mysql.php (9.6 KB) - added by jshreve 12 years ago.
MySQL FullText Search Plugin (wp-plugins/). Changes to be compatible with the SearchAPI Core.
schema.txt (553 bytes) - added by jshreve 12 years ago.
The schema is included in the installer code but I'm providing it for reference
search.minimal.diff (359 bytes) - added by andy 12 years ago.
Minimalistic approach to making search pluggable.
plug-objects.diff (667 bytes) - added by andy 12 years ago.
Allow core classes WP_Query and WP to be replaced before instantiating.
search-pagination-1.diff (2.6 KB) - added by andy 12 years ago.
search-pagination-2.diff (3.0 KB) - added by andy 12 years ago.

Download all attachments as: .zip

Change History (36)

#1 @scribu
12 years ago

  • Keywords needs-patch added; search search-api removed
  • Version set to 2.9

@jshreve
12 years ago

Makes the neccessary changes to convert the WPSearchAPI plugin to a working part of the core

@jshreve
12 years ago

SearchAPI Core file. (to be placed in wp-includes/)

@jshreve
12 years ago

MySQL FullText Search Plugin (wp-plugins/). Changes to be compatible with the SearchAPI Core.

@jshreve
12 years ago

The schema is included in the installer code but I'm providing it for reference

#2 @andy
12 years ago

Excellent first draft!

What happens in WP_Query::get_posts when is_search? It looks like it will waste a database query and ignore the incorrect results. There are also some problems with execution order. Populating $wp_query->posts during template_redirect will expose any number of plugins to the incorrect results from above.

I think the search query should actually happen during WP_Query::get_posts, populate the appropriate object vars, run the appropriate filters on the result set, and skip everything else in that method. If possible, run the usual filters (for caching, etc.) on the query parts and then let the query happen as usual.

I expect some search plugins will return nothing more than an array of post_id's. The posts query would then look like SELECT * FROM $wpdb->posts WHERE ID IN(78,8,26).

#3 @ryan
12 years ago

  • Milestone changed from 2.9 to 3.0

#4 @mattrude
12 years ago

  • Cc m@… added

#5 @hakre
12 years ago

Can you compile a single patchfile so that it can be tested and reviewed better?

@andy
12 years ago

Minimalistic approach to making search pluggable.

#7 @beaulebens
12 years ago

  • Cc beau@… added

#8 @jshreve
12 years ago

  • Cc jshreve added

#9 follow-up: @ryan
12 years ago

(In [13037]) posts_search filter. Props skeltoac. see #10667

@andy
12 years ago

Allow core classes WP_Query and WP to be replaced before instantiating.

#10 @filosofo
12 years ago

plug-objects.diff really needs its own ticket.

But while it's here, wouldn't it be better handled by a filter?

$WP_Query = apply_filters('wp_query_class', 'WP_Query');
$wp_the_query =& new $WP_Query()

That way you have a decent chance through priority of deciding among competing plugins.

And don't forget about the half-dozen or so other instantiations of WP_Query in core.

#11 follow-up: @ryan
12 years ago

If the intent is to replace the class only for The Query, the filter could be called the_wp_query_class.

#12 in reply to: ↑ 11 ; follow-up: @filosofo
12 years ago

Replying to ryan:

If the intent is to replace the class only for The Query, the filter could be called the_wp_query_class.

Or maybe pass the context as the second argument to the filter?

I don't know why you'd want to replace the class in only one place, particularly since many themes call query_posts() directly.

#13 in reply to: ↑ 12 @andy
12 years ago

I think I like it better as a filter. I wrote it that way first but I'm not sure why I didn't keep it that way.

Replying to filosofo:

I don't know why you'd want to replace the class in only one place, particularly since many themes call query_posts() directly.

For example, a search plugin could use this. That would probably only want to replace/extend the core instance and only when isset($_GETs?).

Sometimes you're going to want to ignore the core posts query results so you might as well bypass all of WP_Query::get_posts. Heck, why not just stub it out in WP::main. That's why I showed up wanting to be able to replace or extend these classes.

While I'm at it, I'm thinking of how to fix wpdb so it can be extended. HyperDB is a PITA to keep synced with wpdb. It would be better if it extended wpdb.

So this should be two new tickets then.

#14 @andy
12 years ago

Twenty Ten uses "Older posts" and "Newer posts" labels for pagination links in search.php. The labels are only appropriate for chronological lists of posts and they are not filtered. A search plugin that supplies results in non-chronological order has easy way to correct the labels.

There should be $label filters in get_(next|previous)_posts_link(). Also, "Older posts" and "Newer posts" are unfortunate pagination labels for search.php. "Next page" or, better, "Next results" makes more sense here.

The Next and Previous links should be reversed on search result pages. It's going to become more common to see search results sorted other than reverse-chronological, and to include things other than posts.

Current (okay for browsing chronological archives):

<-- Older ..... Newer -->

Better for search results in general:

<-- Previous ... Next -->

Eventually I'd like to move all pagination into an abstract and filterable template tag. For now, let's set a better example for search results and apply filters to the the labels.

Attaching a patch, search-pagination-1.diff:

  • Reverses direction of search pagination links
  • Changes labels of same
  • Adds filters for pagination link labels

#15 @nacin
12 years ago

Related to your thoughts on abstracting that: #10219 - "Older Entries" and "Newer Entries" links are wrong when entries displayed in ascending order.

#16 in reply to: ↑ 9 ; follow-up: @westi
12 years ago

Replying to ryan:

(In [13037]) posts_search filter. Props skeltoac. see #10667

Is this PHP4 compatible?

Should we not be passing by reference so we don't end up with a copy - is this another reason we need an apply_filters_ref_array()? = #9886

#17 @nacin
11 years ago

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

#18 in reply to: ↑ 16 ; follow-up: @westi
11 years ago

Replying to westi:

Replying to ryan:

(In [13037]) posts_search filter. Props skeltoac. see #10667

Is this PHP4 compatible?

Should we not be passing by reference so we don't end up with a copy - is this another reason we need an apply_filters_ref_array()? = #9886

This was done when we introduced apply_filters_ref_array()

#19 @nacin
11 years ago

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

#20 @brentes
11 years ago

  • Cc thenbrent@… added
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Type changed from task (blessed) to defect (bug)

I've reopened this as the 'posts_search' filter is being applied on every query, rather than only those queries where a search pattern is specified.

I'm guessing that's a bug and not by design?

If it is a bug, it's easily fixed, the filter just needs to moved up one line above the curly brace. :)

#21 @scribu
11 years ago

  • Keywords needs-patch removed

I'm not so sure about that. A plugin could want to do some searching even when there isn't a search pattern defined.

Anyway, I don't really see the problem. Just bail if the search pattern is empty.

#22 @scribu
11 years ago

  • Keywords close added
  • Type changed from defect (bug) to enhancement

#23 @brentes
11 years ago

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

Yeah that's fair enough and as you say, it's not a problem to test within the hooked function if a search pattern is defined.

I guess the hook's documentation will just need to show it's not actually called only on searches, as the name implies, to avoid that trap for young players (like myself).

#24 @westi
11 years ago

(In [14603]) Add commentary about the posts_search filter. See #10667.

#25 @sussane
10 years ago

[spam comment removed by dd32]

Last edited 10 years ago by dd32 (previous) (diff)

#26 @dmitrihughes
10 years ago

[spam comment removed by dd32]

Last edited 10 years ago by dd32 (previous) (diff)

#27 @sussane
10 years ago

(spam removed, WP.org account banned)

Last edited 10 years ago by nacin (previous) (diff)

#28 in reply to: ↑ 18 @Testuser1
10 years ago

Nothing.

Last edited 10 years ago by SergeyBiryukov (previous) (diff)
Note: See TracTickets for help on using tickets.