The function getSearchPath() in the class FacetapiAdapter says

if ($path = module_invoke($this->info['module'] . '_search', 'search_info')) {

I guess it should be

if ($path = module_invoke($this->info['module'], 'search_info')) {

You don't want to invoke another module, do you?

I came to this because the search_facetapi module doesn't output right paths.

CommentFileSizeAuthor
#5 makara-facetapi.jpg50.83 KBcpliakas
#3 1305748_fix_module_invoke.patch1.06 KBmakara

Comments

cpliakas’s picture

cpliakas commented
Title: Bug with getSearchPath() ? » Bug with getSearchPath()?
Priority: Normal » Critical
Status: Active » Needs work

I see the issue here. Good catch! The problem is that this method was added in Facet API beta6. Therefore it had to be backwards compatible with Apache Solr so it didn't break 800 installations. The "apachesolr" module defines the searcher, however it effects the "apachesolr_search' module. Therefore the "_search" was appended to accomodate this. The issue posted at #1294846: Refactoring of the search pages actually overrides this method so it shouldn't be a problem if we remove the "_search" going forward, however any change that is made should be backwards compatible in case someone can't upgrade apachesolr for some reason.

Marking the change as "needs work" because we need to take backwards compatibility with apachesolr <= beta8 into account. It sucks, but the solution here might be to add a conditional to add the "_search" if the $this->info['module'] == 'apachesolr'. Would love to see a patch from you for this.

Thanks for posting,
Chris

cpliakas’s picture

cpliakas commented
Issue tags: +low hanging fruit

Adding the "low hanging fruit" tag.

makara’s picture

makara commented
Status: Needs work » Needs review
StatusFileSize
new 1305748_fix_module_invoke.patch1.06 KB

Patch attached.

A small follow up. Do you think you should pass arguments with the module_invoke()? E.g. $this.

cpliakas’s picture

cpliakas commented
Title: Bug with getSearchPath()? » The getSearchPath() method has an assumption that breaks non-apachesolr search paths
Status: Needs review » Reviewed & tested by the community

Thanks for posting the patch, and it looks good to me.

Regarding passing $this to the search_info() hook, I do like the thought however it assumes that modules are implementing that hook in a non-standard way. I think the approach in the patch has enough flexibility to take care of the common use cases, and modules that implement the adapter should be overriding this method anyways.

Thanks,
Chris

cpliakas’s picture

cpliakas commented
Status: Reviewed & tested by the community » Fixed
StatusFileSize
new makara-facetapi.jpg50.83 KB

Committed at 362a8a2.

Congrats on becoming the 7th committer to Facet API!

~Chris

Facet API committers

makara’s picture

makara commented

Thanks.

Agreed with #4 about passing $this.

makara’s picture

makara commented

BTW. I'm curious. I only submitted a diff. How did you use it as a patch that has me as the author?

Thanks.

cpliakas’s picture

cpliakas commented

I just did a git apply then a git commit --author="makara <makara@132402.no-reply.drupal.org>". If you visit a user's profile page you can see the appropriate --author string that can be used for this purpose.

~Chris

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