Problem/Motivation

When executing a search that also includes a filter that is not a facet the generated URLs are errorneously added as 'n-facet' as opposed to the original fields.

Steps to reproduce

Configure a facet to appear in the results.
Execute a search using a filter that isn't configured as a facet.
Generated URLs should have the filter present in them as well as the facet to be applied.

Proposed resolution

When getUrlAliasByFacetId or getFacetIdByUrlAlias returns FALSE use the raw field value.

CommentFileSizeAuthor
#11 3176923-11.patch5.85 KBmglaman
#11 3176923-test-only.patch3.53 KBmglaman
#9 3176923-9.patch3.17 KBmglaman
#9 interdiff-3176923-5-9.txt2.08 KBmglaman
#5 3176923-5.patch3.22 KBmglaman
#2 jsonapi_search_api-3176923-2.patch2.99 KBJordanDukart
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JordanDukart created an issue. See original summary.

JordanDukart’s picture

JordanDukart CreditAttribution: JordanDukart as a volunteer commented
Status: Active » Needs review
FileSize
jsonapi_search_api-3176923-2.patch2.99 KB

Attaching patch that fixes this issue + random coding standards clean up.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

I just hit this as well, giving it a test run

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

We need to get some test coverage added.

I had the given existing filter:

filter[group-0][group][conjunction]: AND
filter[group-0-product-catalog-terms-0][condition][path]: product_catalog_terms
filter[group-0-product-catalog-terms-0][condition][operator]: =
filter[group-0-product-catalog-terms-0][condition][value]: 98913d3c-bcb6-421b-ac1e-ea61a75b63b0
filter[group-0-product-catalog-terms-0][condition][memberOf]: group-0

The term URL provided for a color facet on BLACK returned:

filter[product_catalog_terms-facet][condition][path]: product_catalog_terms
filter[product_catalog_terms-facet][condition][operator]: IN
filter[color_name]: BLACK

This broke the existing filter conditions

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
FileSize
3176923-5.patch3.22 KB

Here is a new patch which preserves all non-facet filter params and passes them to the built URL for each facet result.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Issue tags: +Needs tests

Definitely need to get a test in there.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

This isn't quite right yet.

Original

filter[group-0][group][conjunction]: AND
filter[group-0-product-catalog-terms-0][condition][path]: product_catalog_terms
filter[group-0-product-catalog-terms-0][condition][operator]: =
filter[group-0-product-catalog-terms-0][condition][value]: 98913d3c-bcb6-421b-ac1e-ea61a75b63b0
filter[group-0-product-catalog-terms-0][condition][memberOf]: group-0

Generated

filter[group-0][conjunction]: AND
filter[group-0-product-catalog-terms-0][path]: product_catalog_terms
filter[group-0-product-catalog-terms-0][operator]: =
filter[group-0-product-catalog-terms-0][value]: 98913d3c-bcb6-421b-ac1e-ea61a75b63b0
filter[group-0-product-catalog-terms-0][memberOf]: group-0

We're missing the [group] and [condition] parameters.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented

Ah, here it is:


      // Rewrite into JSON:API style filters now, start by removing all
      // existing filters as they will be re-created.
      $result_get_params->remove('filter');
      foreach ($filter_params as $filter_path => $values) {
        $existing_params = $result_get_params->all();
        // Simple facets can be done.
        if (count($values) === 1) {
          $value = reset($values);
          $params = array_merge_recursive($existing_params, [
            'filter' => [
              $filter_path => $value,
            ],
          ]);

The logic which rewrites into filters needs to be visited.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
FileSize
interdiff-3176923-5-9.txt2.08 KB
3176923-9.patch3.17 KB

This did the trick. Using the original filters when rebuilding that query key.

JordanDukart’s picture

JordanDukart CreditAttribution: JordanDukart as a volunteer commented

This is awesome @mglaman! Pulled in the patch locally, preserves the original non-facet based filters as it should. Only thing I can see is some nit coding standards things:

ILE: ...pi_facets/src/Plugin/facets/url_processor/JsonApiQueryString.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 60 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
    |       |     "NULL" but found "null"
    |       |     (Generic.PHP.UpperCaseConstant.Found)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

FILE: ...dules/jsonapi_search_api_facets/jsonapi_search_api_facets.module
----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
  1 | ERROR | [x] Missing file doc comment
    |       |     (Drupal.Commenting.FileComment.Missing)
 30 | ERROR | [x] Inline comments must end in full-stops, exclamation
    |       |     marks, question marks, colons, or closing
    |       |     parentheses
    |       |     (Drupal.Commenting.InlineComment.InvalidEndChar)
 50 | ERROR | [x] Additional blank lines found at end of doc
    |       |     comment
    |       |     (Drupal.Commenting.DocComment.SpacingAfter)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Think the module ones were introduced in the form_alter improvements but nbd either way.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Issue tags: -Needs tests
FileSize
3176923-test-only.patch3.53 KB
3176923-11.patch5.85 KB

Thanks @JordanDukart! Here's a patch with a test-only run. And the fix+test. And phpcs fixes.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Status: Needs review » Reviewed & tested by the community

Going to merge after lunch 🥳

  • mglaman committed 81547bb on 8.x-1.x 
    Issue #3176923 by mglaman, JordanDukart: URL generation for facets doesn...
mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Status: Reviewed & tested by the community » Fixed

🥳 Committed

JordanDukart’s picture

JordanDukart CreditAttribution: JordanDukart commented

🥳

Status: Fixed » Closed (fixed)

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