One of the needs identified at the search sprint was to refactor core search to pull the custom indexing into a separate module. This would allow other pluggable search backends, e.g., apache_solr.

Attached patch is a first cut at implementing this. The custom indexing is pulled into a new module, In writing this patch, I've tried hard to avoid introducing any new code or API changes beyond those minimally needed to accomplish this refactoring. There are many other good ideas about how core search could and should be refactored. I've left them all out of this patch.

A few notes:

* hook_search() becomes a way for search backends ('handlers') to register themselves.
* On search settings form, admin selects the handler to use.
* Moves node search and user search code to .inc files, includes in hook_init(). This approach is inconsistent with what we do elsewhere, not sure if it's a good idea.
* Added a couple of lines to system_settings_form() to add a weight to the buttons. Needed because the patch form_alters() a settings form to add settings.
* Patch doesn't include update support.

CommentFileSizeAuthor
#16 search_handlers.png84.2 KBmradcliffe
#14 search_backend_broken.patch71.24 KBmradcliffe
#7 search_backend_2.patch70.15 KBaufumy
sql_search.tar_.gz10.62 KBnedjo
search-backends.patch70.67 KBnedjo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

douggreen’s picture

douggreen CreditAttribution: douggreen commented

subscribing, I'll try to review this later this week, sorry it's taken so long already :(

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

While probably a useful start, I realize this patch has a major shortcoming: it allows only a single search handler to be active at a time. It should be rewritten to allow multiple simultaneous handlers.

lilou’s picture

lilou CreditAttribution: lilou commented
Status: Active » Needs work
nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
Status: Needs work » Needs review

My comment above was not meant as "don't bother reviewing". Yes, it has the issue I noted. But the patch is reviewable as is and I'd appreciate feedback on the general approach before wading into reworking the patch.

Status: Needs review » Needs work

Patch failed to apply. More information can be found at http://testing.drupal.org/node/14230. If you need help with creating patches please look at http://drupal.org/patch/create

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

Okay, robots must be right ;)

aufumy’s picture

aufumy CreditAttribution: aufumy commented
Status: Needs work » Needs review
FileSize
search_backend_2.patch70.15 KB

Reworked the patch with latest drupal7 cvs download.

I think this patch is going in the right direction. I don't mind the .inc files, at least is removes optional code from the main modules.

markus_petrux’s picture

markus_petrux CreditAttribution: markus_petrux commented

subscribing

robertDouglass’s picture

robertDouglass CreditAttribution: robertDouglass commented

Funny I hadn't seen this. Subscribed.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Needs work

The last submitted patch failed testing.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

Should be revisited in light of #367282: A simple handler solution.

aufumy’s picture

aufumy CreditAttribution: aufumy commented

Had a quick chat with chx at the code sprint, as far as he knows, the handler core patch is dead because of lack of traction with Dries, unless someone changes his mind.

So will not take into consideration the handler stuff.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
CreditAttribution: mradcliffe commented
FileSize
search_backend_broken.patch71.24 KB

Here's an updated patch that aufumy and I worked on this morning's code sprint.

TODO:

  • search_node_links was for some reason put in sql_search. We don't know why. This is broken.
  • DB TNG: fix all updates inserts deletes and selects
  • testing
  • other admin interface ideas we tossed around briefly for handlers

broken patch incoming

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

Thanks for taking on updating the patch :)

Currently, the patch allows only a single search handler, though multiple search types per handler. E.g., core would include the sql_search handler, with 'node' and 'user' types.

As noted in #2 (and referenced in a code comment), though, we really need to rework this to allow multiple search handlers to be enabled at a time rather than only one. Prior to the patch, it's possible to use e.g. apache_solr for searching content and some other handler for searching users. As is, the patch would break this by allowing only a single search handler.

I think one of the main reasons I didn't write this for multiple handlers was to avoid breaking paths. Prior to the patch, the path includes the module that handles search, e.g., search/node or search/user. Currently in the patch, this is changed to the search type, with no place for designating the handler.

Options for fixing this might include:

1. Put the handler as an argument in the path, e.g., search/sql-search/node/.

2. Concatenate the handler and type in the patch, e.g., search/sql-search-node/.

In either case, sql_search looks like it should be changed to something without "search" in it, to avoid redundancy.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
CreditAttribution: mradcliffe commented
FileSize
search_handlers.png84.2 KB

Some of what we just talked about in that short period was exposing datasets for search handlers to work with. And then when doing a default search you go through all the handlers. Have to watch out for duplicate results if multiple handlers can run searches on the same datasets though...

Datasets have methods to return what data to be indexed. Handlers have methods to index and to search datasets made available to it via search.module's interface.

see attached flow chart.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Version: 7.x-dev » 8.x-dev
Category: task » feature

Drupal 8 material at this point.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

A start on this has been made in a GSOC project:
http://drupal.org/project/search_api

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Closed (duplicate)

Search indexing is being put into the Dependency Injection system in
#2003482: Convert hook_search_info to plugin system
so it could in theory be replaced by a contrib module.