The search page shows a warning with deprecated function

Deprecated function: preg_split(): Passing null to parameter #2 ($subject) of type string is deprecated in Drupal\search_api\Plugin\search_api\processor\Highlight->highlightField() (line 659 of modules/contrib/search_api/src/Plugin/search_api/processor/Highlight.php).

Steps to reproduce

Search API + Solr + Metatag
I have configured the search view to look into multiple fields (content + keywords & description metatag fields). both metatag fields are set as a fulltext search index.
Searching for a word in the keywords field of an item that has an empty description metatag throws the above warning.

Proposed resolution

The highlightFields method in \Plugin\search_api\processor\Highlight.php should do a check on the field value before calling the highlightField method on the field value.

Remaining tasks

CommentFileSizeAuthor
#13 3323594-13--fix_bc_break_v2.patch2.04 KBdrunken monkey
#13 3323594-13--fix_bc_break_v2--interdiff_from_before_issue.txt670 bytesdrunken monkey
#10 3323594_avoid_bc_break.patch1.61 KBmkalkbrenner
#2 deprecated_function_preg_split-3323594-2.patch861 bytesbogdan.dinu

Comments

bogdan.dinu created an issue. See original summary.

bogdan.dinu’s picture

bogdan.dinu commented
StatusFileSize
new deprecated_function_preg_split-3323594-2.patch861 bytes

I've created a patch with the check proposed in the issue description.

bogdan.dinu’s picture

bogdan.dinu commented
rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Active » Needs review
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Fixed
Issue tags: -preg_split, -Deprecated function

Makes sense, thanks for posting!
Committed.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
Status: Fixed » Needs work

Since the commit of this patch, the search result highlighting integration tests of search_api_solr are failing.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented

Undefined array key "manu"
in
/home/runner/work/search_api_solr/search_api_solr/drupal/modules/contrib/search_api_solr/tests/src/Kernel/SearchApiSolrTechproductsTest.php:54

    // Test processor based highlighting.
    $query = $this->buildSearch('Technology', [], ['manu']);
    $results = $query->execute();
    $this->assertEquals(1, $results->getResultCount(), 'Search for »Technology« returned correct number of results.');
    /** @var \Drupal\search_api\Item\ItemInterface $result */
    foreach ($results as $result) {
      $this->assertStringContainsString('<strong>Technology</strong>', (string) $result->getExtraData('highlighted_fields', ['manu' => ['']])['manu'][0]);
      $this->assertEmpty($result->getExtraData('highlighted_keys', []));
      $this->assertEquals('… A-DATA <strong>Technology</strong> Inc. …', $result->getExcerpt());
    }
mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented

I just verified that reverting the patch lets the test pass again and "Technology" is highlighted again.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
commented
Version: 8.x-1.28 » 8.x-1.x-dev
Priority: Normal » Major
Status: Needs work » Needs review
StatusFileSize
new 3323594_avoid_bc_break.patch1.61 KB

In case of "foreign" indexes like Search API Solr Document datasources or Drupal multisite searches, the value could be NULL. In this case the backend is responsible to modify the highlighted_fields extra data later if it allows to use the highlight processor.
Therefore, we must not remove the information about the field that "should" be highlighted at this point. Converting the value into an empty string is the backward compatible behaviour of this processor.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Fixed

Thanks for noticing and posting a patch, and sorry for breaking your stuff again. ;)
Patch looks good – committed.
Thanks again!

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Fixed » Needs review
StatusFileSize
new 3323594-13--fix_bc_break_v2--interdiff_from_before_issue.txt670 bytes
new 3323594-13--fix_bc_break_v2.patch2.04 KB

Whelp, spoke too soon. Problem is actually that $value can be an object of type \Drupal\search_api\Plugin\search_api\data_type\value\TextValue at this point. While preg_split() doesn’t seem to have any problems accepting that as it’s second parameter (for some reason), it of course fails the is_string() check, letting its value be ignored.
Instead of all the above, let’s just explicitly cast $text to a string in highlightField(), which should also handle NULL values correctly.

Patch attached (plus interdiff compared to version before any of the patches here was committed), please test/review!

  • drunken monkey committed d1999726 on 8.x-1.x 
    Follow-up to #3323594 by mkalkbrenner, drunken monkey: Fixed regression...
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Fixed

Seems to work, so: committed.

ghalusa’s picture

ghalusa commented

- Installing drupal/search_api (1.28.0): Extracting archive
- Applying patches for drupal/search_api
https://www.drupal.org/files/issues/2023-02-11/3323594-13--fix_bc_break_... (3323594: PHP 8.1 deprecation warning preg_split)
Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2023-02-11/3323594-13--fix_bc_break_...

EDIT: I apologize. I just noticed that this patch is intended for the 8.x-1.x-dev branch.

Status: Fixed » Closed (fixed)

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