Skip to content

Facet URL Processor (not on us) is buggy when dealing with facet strings that contain double quotes #339

@DiegoPino

Description

@DiegoPino

What?

Super specific, long term, many years open issue:

See https://www.drupal.org/project/facets/issues/3020427

The issue in particular is in the Facets URL processor (present in 1.x, 2.x and 3.x) and occurs when a faceted against string contains double quotes that are not surrounding the whole string

e.g "the dog is the best" will be converted to the dog is the best which makes Solr Search API somehow happy? (poorly documented, also code decisions are not inline documented nor in the pulls so I am guessing)
but
e.g Diego "a good pumpkin" gets converted into Diego "a good pumpkin which does not match on the Search API query what we need and fails

This here

https://git.drupalcode.org/project/facets/-/blob/3.0.x/src/UrlProcessor/UrlProcessorPluginBase.php?ref_type=heads#L167-174

Really should be (and why remove the double quotes is needed is a mystery to me really) something like this

/**
   * {@inheritdoc}
   */
  public function setActiveItems(FacetInterface $facet) {
    // Get the filter key of the facet.
    if (isset($this->activeFilters[$facet->id()])) {
      foreach ($this->activeFilters[$facet->id()] as $value) {
        // Check if sourrounded by doublequotes 
        $original_length = strlen($value);
        $trimmed_length = strlen(trim($value, '"'));
        if (($original_length - $trimmed_length) == 2) {
          $facet->setActiveItem(trim($value, '"'));
        }
        else {
         $facet->setActiveItem($value);
        }
      }
    }
  }

I like my approach here. Instead of using a complex string position/regular expression or conditions at all, I strip. If the stripped length is 2 less than the original length means it was surrounded by double quotes and I remove them. If not, then it means there are partial, in between quotes and I need to preserve all

@patdunlavey @aksm @alliomeria what do you think? Making a pull request against drupal/facet might require Tests being written/and waiting maybe another 5 years.

I can also provide a patch in our deployment composer.json for this (or both things).

Tested in production and it works well, does not break anything else

Metadata

Metadata

Assignees

Labels

Drupal 10Upgrade economyDrupal 9Everyone's favorite version until Drupal 10 comesFacetsSafely navigating the too many choicesSearch APISearch and DiscoveryMess around and find outexternal bugIt is not us, it is themhelp wantedExtra attention is neededquestionFurther information is requested

Type

No type

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions