Hi,

I've added a patch that provides a search blocks for each search page.

Greetz

Tim

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Title: Patct to provide search block for each page » Patch to provide search block for each page
drunken monkey’s picture

Status: Needs review » Needs work
FileSize
5.32 KB

Wow, great work, thank you very much!
The patch generally seems to work fine — however, there are a few issues still before I can commit it:

  • Why use hook_forms()? I don't see any reason for it, or anything it really does. Also, its implementation is wrong, $args shouldn't be included in the "callback arguments". Because of this, currently all search forms are "compact".
  • When executing a search through a block, the keys shouldn't be displayed in the block form. (Oh, that's one thing hook_forms() does, right?)
  • Since they can now be used on all page loads, the form functions should be moved into the .module file.
  • Instead of a helper function _search_api_page_load_pages you should just use entity_load or a new search_api_page_load_multiple function. Static caching should also use drupal_static, entity_load will already do the aching for you and the module_exists is unnecessary.

Attached is a patch changing a few smaller things. Please also file patches agains the project's root directory, not the contrib/search_api_page directory — that makes them easier to apply.

But thanks again for doing this, it's great to see other people chip in with coding!

Anonymous’s picture

Hi,

The reason I used hook_forms() is to overcome the problem of having multiple forms on the same page. otherwise on submit the forms are not unique and for example form_set_error will highlight all form fields. or the page redirect will for all forms take the same path and search keys.

The forms should indeed be moved to the .module now

The reason i used a helper function with the module_exists :-) but i forgot to add it to the path :-S is, i replaced the for in the hook_menu with the helper function so that you would get an array of the pages.

Thanks for the drupal_static hint, didn't know that one yet.

I also had another question, when creating an search page, you can choose the query type, this seams to give the same results whatever you choose?

Tim.

Anonymous’s picture

By the way, I'll try to rework the patch and post here later today.

drunken monkey’s picture

The reason I used hook_forms() is to overcome the problem of having multiple forms on the same page. otherwise on submit the forms are not unique and for example form_set_error will highlight all form fields. or the page redirect will for all forms take the same path and search keys.

Oh, OK, that does make sense. I know that the Form API is pretty poor in that respect, but didn't think of that.

The reason i used a helper function with the module_exists :-) but i forgot to add it to the path :-S is, i replaced the for in the hook_menu with the helper function so that you would get an array of the pages.

But there still wouldn't be any way the module wouldn't exist when a function in that module's .module file is run, right? Also, I can't really follow you there, I'll look at that in code form …

I also had another question, when creating an search page, you can choose the query type, this seams to give the same results whatever you choose?

Because of restrictions of the database backend (that can't deal with phrase queries) this is true for that backend. However, it should have very noticable effects when using the Solr (or any other "professional") backend and doing a search with more than one keyword. (Or, at least, "Single term" should behave noticably different.)

Looking forward to your improved patch! (No rush, though, there's no feature freeze in sight … ;))

Anonymous’s picture

The reason i used a helper function with the module_exists :-) but i forgot to add it to the path :-S is, i replaced the for in the hook_menu with the helper function so that you would get an array of the pages.

But there still wouldn't be any way the module wouldn't exist when a function in that module's .module file is run, right? Also, I can't really follow you there, I'll look at that in code form …

I used it in that way because in the current hook_menu this is the following code:

// During uninstallation, this would lead to a fatal error otherwise.
if (module_exists('search_api_page')) {
......
}

So i didn't want to break that... :-)

Because of restrictions of the database backend (that can't deal with phrase queries) this is true for that backend. However, it should have very noticable effects when using the Solr (or any other "professional") backend and doing a search with more than one keyword. (Or, at least, "Single term" should behave noticably different.)

I tried it with solr, but it only had like 4 dummy record, and i didn't see any difference there. I will try again with some more realistic and more data, and see if i get different results then.

Greetz

Tim

Anonymous’s picture

I've reworked the patch, a little less code, also added a search_api_page_load_multiple function to get all or multiple search pages by an array of id's, placed the forms in .module

I've tried to remove the module_exists from the hook_menu, but it indeed fails when trying to uninstall.

Do you know if there is someone already working on a apache solr attachments integration?

Greetz

Tim.

drunken monkey’s picture

I tried it with solr, but it only had like 4 dummy record, and i didn't see any difference there. I will try again with some more realistic and more data, and see if i get different results then.

As said, the difference is the parsing of multiple keywords. While with "Multiple terms" (and, for most backends, "Direct query" too) a search for »foo bar« will e.g. match in the text "baz bar lol foo", with "Single term" the keywords will be treated as one single phrase query and therefore only match something like "baz foo bar blah".

I've reworked the patch, a little less code, also added a search_api_page_load_multiple function to get all or multiple search pages by an array of id's

*_load_multiple() functions should always follow the same pattern, i.e., receive ($ids, $conditions, $reset) as arguments. Hardcoding it to only returned enabled pages is confusing for a function with that name. For this functionality, you should have named it something like search_api_page_load_enabled() — but for now, I corrected its definition and use and we should just leave it at that. Three uses to get all enabled pages doesn't really warrant an own function, anyways.

I've tried to remove the module_exists from the hook_menu, but it indeed fails when trying to uninstall.

Yes, of course — sorry, my bad again, completely forgot that, and the use of module_exists() in hook_menu()

Do you know if there is someone already working on a apache solr attachments integration?

No, I don't think so.
Do you know, how attachments are currently implemented? Are they just file fields added via the Field API?
In that case, a data alteration that offers the text content of all file-typed fields would probably the best way to go. But in any case, I would be glad to help, if you want to work on that and need some assistance. Since it will probably need an additional library, we just can't really add it to the core module, even though it would be a great addition. But I'd definitely mention it on the project page.

Back to the issue at hand: Attached is a revised patch that seems to work alright. If you want to change anything, please work from that patch, otherwise I'll just commit it in the next couple of days (or after your response).

Anonymous’s picture

Tested latest patch and everything seems to work, I'm a happy camper :-)
To index the content of files we would have to use apache tika. I'm going to experiment a bit with it.

Greetz

Tim.

drunken monkey’s picture

Title: Patch to provide search block for each page » Provide a search block for each page
Status: Needs work » Fixed

Committed, thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.