Support compatibility with Search_Api_Location/Facets_Map_Widget

Interesting :-)

Some quick notes on the patch:

1. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -463,7 +463,9 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +      'search_api_geohash',
   +      'search_api_rpt',
   +      'search_api_location',
   

   Is there an issue for Search API to declare these features?
   Aren't these data types?

2. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -2480,12 +2511,17 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +    // @todo FIX THIS METHOD SO IT WORKS.
   +
   

   What do you mean exactly?

Beside these there're a lot of todos in the code ;-)

I think the most important missing piece is a test. Have a look at the existing tests for location search. Using travis it is easy to apply patches to any module required in this test and to provide an integration test even in this early stage. Simply fork https://github.com/mkalkbrenner/search_api_solr and start PRs to start the tests. Do you need help with that?

The datatypes are supported by the search_api_location module as you can see in the latest patch there. https://www.drupal.org/node/2892620#comment-12217677

The solr patch has been tested there so that it works as expected. There is currently no search_api_db support for those datatypes yet and I'm doubting if it should be part of the requirement before this can be committed. The test obviously is a requirement :)

As for search_api, I don't think it needs to declare these features anywhere as they are only supported in search_api_solr backend. Each backend can define new features to support so no patch for search_api itself is needed (I think).

1. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -1569,6 +1576,24 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +          // This special key is defined in setSpatial().
   

   Is that true?

2. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -2570,6 +2598,41 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +    $solarium_query->createFilterQuery($solr_field)->setQuery($solr_field . ':' .$rpt['geom']);
   

   a missing space

3. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -2570,6 +2598,41 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +    $solarium_query->addParam('facet.heatmap', $solr_field);
   +    $solarium_query->addParam('facet.heatmap.geom', $rpt['geom']);
   +    $solarium_query->addParam('facet.heatmap.format', $rpt['format']);
   +    $solarium_query->addParam('facet.heatmap.maxCells', $rpt['maxCells']);
   +    $solarium_query->addParam('facet.heatmap.gridLevel', $rpt['gridLevel']);
   

   Is there a minimum Solr version required for that?

4. +++ b/tests/src/Kernel/SearchApiSolrRptTest.php
   @@ -0,0 +1,185 @@
   +    $index->addField($fieldsHelper->createField($index, 'rpt', $info));
   

   The new test class is not required. The purpose of the existing class SearchApiLocationTest is to test the integration with the Search API Location module. Therefore it's sufficient to add this line to the existing class to have a location and a rpt field.

5. +++ b/tests/src/Kernel/SearchApiSolrRptTest.php
   @@ -0,0 +1,185 @@
   +  public function testBackend() {
   +  }
   

   Have a look at SearchApiSolrLocationTest::testBackend(). I guess you could simply add some more queries to it to test the rpt field.

#3 Minimum Solr 5.1. No schema modifications needed.

+++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
@@ -1569,6 +1576,24 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
+    // Extract heatmaps.
+    if (isset($result_data['facet_counts']['facet_heatmaps'])) {
+      if ($spatials = $query->getOption('search_api_rpt')) {
+        foreach ($result_data['facet_counts']['facet_heatmaps'] as $key => $value) {
+          // This special key is defined in setSpatial().
+          if (!preg_match('/^rpts_(.*)$/', $key, $matches)) {
+            continue;
+          }
+          if (empty($extract_facets[$matches[1]])) {
+            continue;
+          }
+          $facets[$matches[1]][] = array(
+            'filter' => $value,
+            'count' => 1,
+          );
+        }
+      }
+    }

We decided to not support heatmaps in the patch in location, so no tests for this either, we can remove this part I think.

As Markus said, "rpt" is a data type, no need to also define it as a feature.

+++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
@@ -463,7 +463,6 @@ public function getSupportedFeatures() {
       'search_api_data_type_location',

@drunken monkey: Isn't this a "data-type-feature" as well? ;-)

@dbjpanda: My comments from #7 are still valid. based on Nick's comment in #9 we should wrap some parts of the patch with a Solr version check.

Thanks for pointing out the coding standards violations. We should definitely fix them. But this should be done in separate issue because the patch isn't easy to read and understand anymore. a feature patch should focus only on the feature itself.

Yes, the tests require a "d8" core on localhost. But the module contains a docker-compose file for easy test setup.

Great progress an I I think we're getting closer :-)

1. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -464,6 +464,7 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +      'search_api_rpt',
   

   drunken monkey suggested to remove this feature definition entirely in #12.

2. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -979,6 +981,17 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +    // Handle spatial filters.
   +    if (isset($options['search_api_rpt'])) {
   +      if (version_compare($connector->getSolrVersion(), 5.1, '>=')) {
   +        $this->setRpt($solarium_query, $options['search_api_rpt'], $field_names);
   +      }
   +      else {
   +        throw new SearchApiSolrException('This feature is only supported by Solr version 5.1 or higher.');
   +      }
   +
   +    }
   

   Throwing an exception isn't the appropriate way here. I prefer to write a log message and to let the code continue. In this case it should visually lead to zero results.

3. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -1569,6 +1582,24 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +          $facets[$matches[1]][] = array(
   

   new code should follow the coding standards ;-)

1. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -1569,6 +1582,24 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +          // This special key is defined in setSpatial().
   +          if (!preg_match('/^rpts_(.*)$/', $key, $matches)) {
   

   The comment is wrong. It's a static field name prefix and not a dynamically defined one.

2. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -2570,6 +2604,41 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +   * Adds spatial features to the search query.
   

   rpt spatial

3. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -2570,6 +2604,41 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +   *   The rpt spation options to add.
   

   typo: spatial

4. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -2570,6 +2604,41 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +   *   The field names, to add the spatial options for.
   

   rpt spatial options

5. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -2570,6 +2604,41 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +   *   Thrown when too many spatial searches are added.
   

   Thrown if more than one is added.

6. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -2570,6 +2604,41 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +  protected function setRpt(Query $solarium_query, array &$rpt_options, $field_names = array()) {
   

   I think there's no need to pass the $rpt_options as reference.

7. +++ b/tests/src/Kernel/SearchApiSolrLocationTest.php
   @@ -101,6 +101,14 @@ class SearchApiSolrLocationTest extends BackendTestBase {
        $index->save();
   ...
   +    $index->save();
   

   save() should only be called once.

8. +++ b/tests/src/Kernel/SearchApiSolrLocationTest.php
   @@ -286,6 +294,54 @@ class SearchApiSolrLocationTest extends BackendTestBase {
   +      'geom' => '["-180 -90" TO "180 90"]',
   ...
   +        'count' => 1,
   

   It would be good to add additional test queries that don't use the default and that return more than one result.

1. +++ b/src/Utility/Utility.php
   @@ -87,6 +87,10 @@ class Utility {
   +        'rpt' => array(
   

   coding standard ;-)

2. +++ b/tests/src/Kernel/SearchApiSolrLocationTest.php
   @@ -286,6 +295,100 @@ class SearchApiSolrLocationTest extends BackendTestBase {
   +    $this->assertEquals($expected, $facets, 'The correct location facets are returned');
   ...
   +    $this->assertEquals($expected, $facets, 'The correct location facets are returned');
   

   These two assertions will fail on Solr 4 which is still part of our CI on travis.
   Therefore these two assertions needs to be wrapped by a version check.
   You can use §this->getServer()->getBackend()->getSolrConnector()->getSolrVersion().

3. +++ b/tests/src/Kernel/SearchApiSolrLocationTest.php
   @@ -286,6 +295,100 @@ class SearchApiSolrLocationTest extends BackendTestBase {
   +        'count' => 1,
   

   What does this count mean? It looks a bit confusing to have two results and a count of 1.

It would be nice to add interdiffs every time you upload an adjusted patch. That would ease the review a lot.

1. +++ b/tests/src/Kernel/SearchApiSolrLocationTest.php
   @@ -286,6 +295,104 @@ class SearchApiSolrLocationTest extends BackendTestBase {
   +    if ($this->getServer()->getBackend()->getSolrConnector()->getSolrVersion() >= 5.1) {
   ...
   +    if ($this->getServer()->getBackend()->getSolrConnector()->getSolrVersion() >= 5.1) {
   

   I suggest to detect the Solr version only once, store it in a variable and then comparetwo times against the variable.
   The implemented check itself is wrong, greater than float 5.1 doesn't work in all cases. For example 5.10 is greater than 5.1 as a version but not mathematical.
   You have to use version_compare() like in the backend class.

2. +++ b/tests/src/Kernel/SearchApiSolrLocationTest.php
   @@ -286,6 +295,104 @@ class SearchApiSolrLocationTest extends BackendTestBase {
   +    if ($this->getServer()->getBackend()->getSolrConnector()->getSolrVersion() >= 5.1) {
   +      $this->assertEquals($expected, $facets, 'The correct location facets are returned');
   +    }
   ...
   +    if ($this->getServer()->getBackend()->getSolrConnector()->getSolrVersion() >= 5.1) {
   +      $this->assertEquals($expected, $facets, 'The correct location facets are returned');
   +    }
   

   We should add the else part and an assertion that verifies the log message about the wrong Solr version.

If we fix the findings in the test above, we're ready to commit your patch :-)
Which patches to other modules are required exactly to get the tests to pass?

I added an assertion for the log messages if Solr is older than 5.1.

The tests now pass on Solr 5 and 6:
https://travis-ci.org/mkalkbrenner/search_api_solr/builds/267696702
(They are not green because of the currently failing autocomplete tests.)

But for Solr 4 the tests fail:
Undefined index: rpt

I think this line is causing the error:

$facets = $result->getExtraData('search_api_facets', [])['rpt'];

This code has to be moved to under the version detection because there can't be a key 'rpt' on a empty Solr 4 result. Anyway, accessing the result this way is a kind of unsafe.

The location tests look good now:
https://travis-ci.org/mkalkbrenner/search_api_solr/builds/267870596

I attached the final patch and will merge the branch.

It's a little bit complicated caused by the fact that we can't run Solr test on drupal.org infrastructure.
All the communication has to happen here on drupal.org in order to be visible and transparent for everyone.

Every patch that is reviewed here on drupal.org get committed to github first to let travis execute the tests. If they pass I merge the commit from github to drupal.org.

If you want to leverage travis it the easiest way to provide a Pull Request on github to get a patch tested. But one it's ready, you should post the patch here in the issue queue to satisfy to process described above.