When using this module alongside Drupal Search API Solr version 1 it appeared to be masking an issue (if my understanding is correct) for non-range date fields.

Previously Search API Solr was not supporting the search_api_granular feature, this meant it would never enter into this section of the QueryTypeRangeBase class.

    // Range grouping is supported.
    if (isset($supportedFeatures['search_api_granular'])) {
      $facet_results = [];
      foreach ($this->results as $result) {
        if ($result['count'] || $query_operator == 'or') {
          $result_filter = trim($result['filter'], '"');
          $facet_results[] = new Result($this->facet, $result_filter, $result_filter, $result['count']);
        }
      }
      $this->facet->setResults($facet_results);

      return $this->facet;
    }

However now that this is supported in Search API Solr version 2, it prevents the additional functionality of the SearchApiDate facet query type from being used - as this is called through the calculateResultFilter method (not called inside the if statement above).

CommentFileSizeAuthor
#12 3018546_true_oo.patch2.96 KBmkalkbrenner
#10 3018546_query_type.patch672 bytesmkalkbrenner
#5 3018546-check-granularity-enabled.patch817 bytesandy_w
#2 3018546-non-range-date-items.patch865 bytesandy_w
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andy_w created an issue. See original summary.

andy_w’s picture

andy_w CreditAttribution: andy_w at Numiko commented
FileSize
3018546-non-range-date-items.patch865 bytes

Adding the calculateResultFilter method inside the "search_api_granular supported" if statement seems to then cause it to function as expected.

Steps to reproduce:
1. Install Search API, Search API Solr 2.3, Facets
2. Add a facet for the created date to a Solr Search index
3. Tick to include the date item processor and configure to use: [*] Actual date with granularity & [*] Month & ["M Y"] Date Format

Expected outcome:
When viewing the page with the facet the facets should show in the format "M Y".

Actual Outcome:
The facets display with each individual result as a timestamp.

andy_w’s picture

andy_w CreditAttribution: andy_w at Numiko commented
Status: Active » Needs review
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Calibrate commented

Thanks for reporting this issue and providing a fix. It's kind of annoying that we can't add a test to verify this behavior on the d.o infrastructure. But if we can get someone else to verify this, we can get this in.

andy_w’s picture

andy_w CreditAttribution: andy_w at Numiko commented
FileSize
3018546-check-granularity-enabled.patch817 bytes

It looks like I was tackling the wrong part of the problem here. Actually we need to be checking that granularity is being used on the facet alongside whether it is supported.

morenstrat’s picture

morenstrat
he/him
Primary language German
Location Bochum, Germany
CreditAttribution: morenstrat at Reinblau commented
Status: Needs review » Reviewed & tested by the community

I also ran into this issue and I can confirm that the patch fixed the issue for me.

Mupsi’s picture

Mupsi
she/her
CreditAttribution: Mupsi commented

Hello, the patch in #5 fixed the issue for me too. Thanks!

a.dmitriiev’s picture

a.dmitriiev CreditAttribution: a.dmitriiev as a volunteer and at 1xINTERNET commented

#5 works like a charm. Good idea to not rely completely on backend settings, facets also should decide on their own wether they need Granularity or not. Well done.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH commented
Status: Reviewed & tested by the community » Needs work

It's not about relaying on the backend's capabilities!
it is wrong to check for the capability but not sending a Search API query that contains "granular".

Checking for the processor seems to be a good workaround. But the correct implementation would be to
1. check for the backend's capability
2. check if the final query sent to the server contained a facet that uses it

$facets = $query->getOption('search_api_facets', []);
foreach ($facets as $info) {
  if (isset($info['query_type']) && $info['query_type'] == 'search_api_granular') {

    // granular!

  }
}
mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH commented
Status: Needs work » Needs review
FileSize
3018546_query_type.patch672 bytes

Could it be that easy?

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH commented

Independent from the patch which should be committed anyway, I still have a question:
If the backend supports search_api_granular and the processor is enabled to trigger it, who is responsible to format dates?

From my point of view it is possible to configure a granular facet for a timestamp/date field. The code for formatting the date lives in facets but will not be triggered in this case!

We have SearchApiDate facet and SearchApiGranular facet. But for me it seems that we need something like SearchApiGranularDate on top.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH commented
FileSize
3018546_true_oo.patch2.96 KB

I reviewed the code again. I think that the code in QueryTypeRangeBase::build() violates an OO pattern and this is the main reason for the issue.

So this would be the better patch that would also reduce the code base for SearchApiDate facets. Opinions?

The question in #11 still remains.

KarimB’s picture

KarimB CreditAttribution: KarimB as a volunteer commented
Status: Needs review » Reviewed & tested by the community

#12 works like a charm. Nice job @mkalkbrenner.

Tested with:
search_api_solr 8.x-2.5
facets 8.x-1.2
and Core 8.6.9

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Calibrate commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed, thanks!

Status: Fixed » Closed (fixed)

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