Functions like hook_apachesolr_query_prepare() or hook_apachesolr_query_alter() need to be aware of the current search page!
Currently it's not easy to implement custom sorts or queries for dedicated search pages.
I created a patch that add a method getSearchPageId() to the query class.
Everything has been achieved in way that does not break the existing API.
BTW this patch is essential to make Apache Solr Multilingual functional for different search pages.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mkalkbrenner’s picture

Status: Active » Needs review
FileSize
5.96 KB
pwolanin’s picture

Status: Needs review » Postponed (maintainer needs more info)

Markus, can you give more specific examples of your use case?

Right now I 'd think about creating a search environment for each set of custom alterations, and then using that env for one or more pages (or blocks, etc).

mkalkbrenner’s picture

Status: Postponed (maintainer needs more info) » Needs review

In Apache Solr Multilingual you can for example define if the search results should be limited to the current language by default. Without that patch the query could only be altered for all search pages. With that patch this could be done individually per search page:

/**
 * Implements hook_apachesolr_modify_query().
 */
function apachesolr_multilingual_apachesolr_query_alter($query) {
  global $language;
  $search_page = apachesolr_search_page_load($query->getSearchPageId());

  $filter_language = '';

  // REVIEW We assume that a user is not able to select two different languages as filter
  $language_filters = $query->getFilters('ss_language');
  if (!empty($language_filters)) {
    $filter_language = $language_filters[0]['#value'];
  }

  if (!$filter_language && $search_page['settings']['custom_settings']['apachesolr_multilingual_auto_language_filter'] &&
    (!$search_page['settings']['custom_settings']['apachesolr_multilingual_auto_language_filter_detachable'] ||
    ($search_page['settings']['custom_settings']['apachesolr_multilingual_auto_language_filter_detachable'] && empty($_GET['detach-auto-language-filter'])))) {
 
    // ...
  }
}

Another use case is that you use hook_apachesolr_query_prepare($query) to modify the sorts.
There's also noe easy way to implement different sorts for different search pages which looks like a standard feature.

mkalkbrenner’s picture

To be more specific, this is the example code in apachsesolr.api.php:

function hook_apachesolr_query_prepare(DrupalSolrQueryInterface $query) {
  // Add a sort on the node ID.
  $query->setAvailableSort('entity_id', array(
    'title' => t('Node ID'),
    'default' => 'asc',
  ));
}

This will modify the sort for ALL search pages.
With my patch you can to something like this without breaking the existing API and existing implementations of different contrib modules:

function hook_apachesolr_query_prepare(DrupalSolrQueryInterface $query) {
  $page_id = $query->getSearchPageId();

  if ($page_id = 'solr') {
    // Add a sort on the node ID.
    $query->setAvailableSort('entity_id', array(
      'title' => t('Node ID'),
      'default' => 'asc',
    ));
  }
  elseif ($page_id = 'A_DIFFERENT_SEARCH_PAGE') {
    // do some sophisticated stuff which should not be applied to core / default search
  }
}

I think the motivation for this patch is obvious, because hook_apachesolr_query_prepare is older than the concept of search pages.

pwolanin’s picture

So, I would suggest, as above, using environments like:

function hook_apachesolr_query_prepare(DrupalSolrQueryInterface $query) {
  $env_id = $query->solr('getId');

  if ($env_id == 'solr') {
    // Add a sort on the node ID.
    $query->setAvailableSort('entity_id', array(
      'title' => t('Node ID'),
      'default' => 'asc',
    ));
  }
  elseif ($env_id == 'A_DIFFERENT_SEARCH_ENV') {
    // do some sophisticated stuff which should not be applied to the original environment
  }
}
klonos’s picture

mkalkbrenner’s picture

Peter, you're right that you can use environments as workaround for that example.

But I don't see what's wrong in having the possibility to distinguish between different search pages in hooks like query_alter or query_prepare. The patch doesn't break anything.

In a default installation you have two search pages for one environment (core search, taxonomy) and one mlt block.
In the hooks query_alter and query_prepare there's no common way to target one of them using the existing properties of the query object:
core search and taxonomy use the same name 'apachesolr', the base_path could be modified, the mlt block has a dedicated name 'apachesolr_mlt' but might occur on different pages.

pwolanin’s picture

@mkalkbrenner - so... what if I want to do some alterations specific to the MLT block?

I think there are a couple options:
1) Use only environments as I suggested above.
2) Add some kind of "context" or "source" property or method to the query object.

#2 would basically depend on people correctly setting the value when constructing the query

mkalkbrenner’s picture

1) Use only environments as I suggested above.

In apachesolr the site administrator can create environments and search pages using these environments.
The current dev version of apachesolr_multilingual adds index related options to the environment forms and query related options to the search page form. To access theses settings at runtime, the second part requires the patch we discuss here.
Without that patch, we will implement the same approach in apachesolr_multiligual, but we need much more code and it will be harder to maintain.
To access the environment id, we already use exactly the same code you posted in #5.

2) Add some kind of "context" or "source" property or method to the query object.
#2 would basically depend on people correctly setting the value when constructing the query

That's what we want. In fact the environment id is already part of the query object. The environment Id in combination with the page id will describe full context.
Using my patch, the site administrator doesn't have to care about setting the right context manually. It will be set automatically.
If you think about developers as "people", the context will be set if the use the apachesolr functions to create a query object.
If they create a query manually, you're right that they have to set the context manually. But if they don't, that doesn't matter! The query runs in a "global" or - let's say - "environment only" context as before without my patch. It' won't break anything.

I'm still convinced that a "complete" context for the query object will be a benefit for apachesolr.
If you're not satisfied with the page id property, I'm also fine with a context property containing environment id and page id.

pwolanin’s picture

Status: Needs review » Needs work
FileSize
8.64 KB

not all "contexts" are pages - e.g. the MLT block

This patch needs more docs and probably more discussion/work, but I think this would be more useful and generic than linking it to page ID.

mkalkbrenner’s picture

Category: bug » feature
Status: Needs work » Needs review
FileSize
8.65 KB

Thank you Peter.

This approach is fine for me and you're right that the generic approach is more useful.

I tested this patch and it works well with apachesolr multilingual, beside that I had to fix a small bug in the interface definition that caused a fatal error:

Fatal error: Declaration of SolrBaseQuery::addContext() must be compatible with that of DrupalSolrQueryInterface::addContext() in /Volumes/Local Storage/workspace/bio.logis_pgs_portal/htdocs/sites/all/modules/contrib/apachesolr/Solr_Base_Query.php on line 251

Status: Needs review » Needs work

The last submitted patch, 1918012.patch, failed testing.

mkalkbrenner’s picture

BTW these hooks benefit from the context as well:
hook_apachesolr_search_result_alter($document, &$extra, DrupalSolrQueryInterface $query)
hook_apachesolr_process_results(&$results, DrupalSolrQueryInterface $query)

mkalkbrenner’s picture

Status: Needs work » Needs review

#11: 1918012.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1918012.patch, failed testing.

pwolanin’s picture

Title: Solr query needs to be aware of Search Page ID » Solr query needs to be aware of more context such as Search Page ID

updating title

the test error is:

Fatal error: Call to a member function getId() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/sites/default/modules/apachesolr/Solr_Base_Query.php on line 354
Nick_vh’s picture

+++ b/apachesolr.interface.incundefined
@@ -4,6 +4,15 @@
+  function getName();
+
+  function getSearcher();
+
+  function getContext();
+
+  function addContext(array $context);

add these functions to the Dummy_Solr.php file in the test directory

mkalkbrenner’s picture

DummySolr doesn't implement the modified interface DrupalSolrQueryInterface.

mkalkbrenner’s picture

Status: Needs work » Needs review
FileSize
10.09 KB

The patch in #11 discovered two already existing bugs in the test cases where a string has been passed instead of an object:

  private function _apachesolr_drupal_query($name, $params = array(), $solrsort = '', $base_path = '', $solr = 'DrupalApacheSolrService') {
    return new SolrBaseQuery($name, $solr, $params, $solrsort, $base_path);
  }

needs to be

  private function _apachesolr_drupal_query($name, $params = array(), $solrsort = '', $base_path = '', $solr = NULL) {
    if (is_null($solr)) {
      $solr = new DummySolr(NULL);
    }
    return new SolrBaseQuery($name, $solr, $params, $solrsort, $base_path);
  }

Status: Needs review » Needs work

The last submitted patch, 1918012.patch, failed testing.

Nick_vh’s picture

Status: Needs work » Needs review
FileSize
10.88 KB

Status: Needs review » Needs work

The last submitted patch, 918012-21.patch, failed testing.

Nick_vh’s picture

Status: Needs work » Needs review

#21: 918012-21.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 918012-21.patch, failed testing.

Nick_vh’s picture

Status: Needs work » Needs review
FileSize
11.72 KB
Nick_vh’s picture

FileSize
11.72 KB

Make the function definition more consistent and use NULL. The issue with the test was that the dummy solr file was not included

Nick_vh’s picture

Version: 7.x-1.x-dev » 6.x-3.x-dev
Status: Needs review » Patch (to be ported)

Committed and pushed to 7.x-1.x - thanks!

pwolanin’s picture

Version: 6.x-3.x-dev » 7.x-1.x-dev
Status: Patch (to be ported) » Needs work

I'm worried that the interface change is an API break that we should not be introducing.

mkalkbrenner’s picture

Status: Needs work » Needs review

Peter, can you explain where you see an API change that breaks anything?
From my point of view we only add additional methods.
All exiting code in contributed apachesolr modules should still work without any changes!

pwolanin’s picture

Status: Needs review » Needs work

ok, so I guess the constructor is not part of the interface?

pwolanin’s picture

Status: Needs work » Needs review

Sorry, didn't mean to flip the status again.

As you saw with the test class, anyone implementing a version of this that doesn't inherit from the normal class will have their code break because it won't match the interface. However, I doubt anyone is doing that in reality.

mkalkbrenner’s picture

Right, the constructor is not part of the interface.

You introduced getSearcher and getName with your patch, which aren't related to this issue.
But that doesn't really matter because getContext and addContext require the API change anyway.

So the additional methods in the interaface are an API change which could break a contrib module if someone implemented his own DrupalSolrQuery class.

But I second your opinion:

However, I doubt anyone is doing that in reality.

And if it's the case it will be easy to fix as you can see at the fixed tests.

So I vote for that kind of "API enhancement" for the next release.

BTW my initial patch didn't change the interface at all.

pwolanin’s picture

Version: 7.x-1.x-dev » 6.x-3.x-dev
Status: Needs review » Patch (to be ported)

ok, well we can put it in the release notes... back to needing to be ported then

kevin.dutra’s picture

Status: Patch (to be ported) » Needs review
FileSize
11.66 KB
mkalkbrenner’s picture

Assigned: Unassigned » mkalkbrenner
Issue summary: View changes
FileSize
12.62 KB

Patch #34 does not apply to latest 6.x-3.x-dev anymore. Here's an updated version.

mkalkbrenner’s picture

Issue tags: +i18n

  • Commit ed8c66c on 6.x-3.x by mkalkbrenner:
    Issue #1918012 by mkalkbrenner: Solr query needs to be aware of more...
mkalkbrenner’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

rahul_sankrit’s picture

I am using Apache Solr autocomplete module for solr auto search but suggestion is not coming in search box, I did debug and got fatal error like this :

Debug url : example.com/apachesolr_autocomplete?query=test (this is the search key)

Fatal error: Call to undefined method SolrBaseQuery::get_fq() in /var/www/html/drupal/sites/all/modules/contributed/apachesolr/apachesolr.module on line 2805

I am using : Apache Solr framework - 6.x-3.0-rc1 and Apache Solr autocomplete - 6.x-1.1

mkalkbrenner’s picture

Sorry, we don't support 6.x anymore. I even don't have an environment to reproduce the issue.