The feature added at #1252648: Allow for enabling facet blocks on non-search pages doesn't wrap the apachesolr_search_run_empty() function in a try / catch block, so pages configured to display facets throw uncaught exceptions and thus fatal errors when the Solr server is unavailable.

CommentFileSizeAuthor
#5 apachesolr-1391850-5.patch866 bytescpliakas
#1 apachesolr-1391850-1.patch921 bytescpliakas

Comments

cpliakas’s picture

cpliakas commented
Status: Active » Needs review
StatusFileSize
new apachesolr-1391850-1.patch921 bytes

The attached patch catches Exceptions and passes the message to the appropriate error handling functions.

pwolanin’s picture

pwolanin commented
Status: Needs review » Needs work

I don't think we want to call apachesolr_failure()

cpliakas’s picture

cpliakas commented
Status: Needs review » Needs work

I see why not. We wouldn't want to redirect on failure if that setting is configured. What about displaying a message? Would we want a drupal_set_message() if the setting is configured or just have it fail silently to watchdog?

pwolanin’s picture

pwolanin commented

I think it should fail and maybe put a message in watchdog - like we do for MLT block failure in apachesolr_search_mlt_suggestions()

cpliakas’s picture

cpliakas commented
Status: Needs work » Needs review
StatusFileSize
new apachesolr-1391850-5.patch866 bytes

Attached patch makes the suggested changes.

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review

I'm curious how we are populating the value for $adapter->getSearchPath() since there is not a specific path associated with an environment. Maybe that's a setting in the earlier patch?

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented

from apachesolr/plugins/facetapi/adapter.inc

      if ($path = module_invoke($this->info['module'] . '_search', 'search_info')) {
        $this->searchPath = 'search/' . $path['path'];
        if (!isset($_GET['keys']) && ($keys = $this->getSearchKeys())) {
          $this->searchPath .= '/' . $keys;
        }
      }

It goes to the hook_search_info path by default

cpliakas’s picture

cpliakas commented

I guess this goes back to the facets per search page discussion. To me it doesn't seem right to associate a path with an environment, since multiple search pages and therefore paths can be associated with a single environment. Another approach could be to add a setting to this feature looping over all paths associated with this environment and selecting which one the facet links should redirect to? I haven't looked too deeply at the code to see if this is possible, just throwing it out there.

But I don't mean to derail the topic of this thread. Regardless of the path issue, the patch does catch any exceptions and prevents fatal errors when the server is unavailable.

pwolanin’s picture

pwolanin commented

Yes, I think we need to revisit this, especially if we remove the dependency on core search module.

I think conceptually it would make more sense to link to a specific search page, then we'd always know where the facet links go. The search page references an environment, but that can be changed.

pwolanin’s picture

pwolanin commented
Status: Needs review » Fixed

committed bugfix patch, created #1394136: Link non-search page facets to a search page rather than environment as a follow-up

cpliakas’s picture

cpliakas commented

Excellent. Thanks!

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Status: Fixed » Closed (fixed)
cpliakas’s picture

cpliakas commented

Posted a follow-up issue at #1397526: Add an option to select which search page facets link to when displayed on non-search pages.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Version: 7.x-1.x-dev » 6.x-3.x-dev

Committed exactly the same patch to the 6.x-3.x branch