Problem/Motivation

Coming out of https://www.drupal.org/project/search_api/issues/3253738 there's another str_replace being thrown:

Deprecated function: str_replace(): Passing null to parameter #3 ($subject) of type array|string is deprecated in Drupal\search_api\Plugin\search_api\processor\Highlight->createExcerpt() (line 476 of modules/contrib/search_api/src/Plugin/search_api/processor/Highlight.php).
Drupal\search_api\Plugin\search_api\processor\Highlight->createExcerpt(NULL, Array) (Line: 314)
Drupal\search_api\Plugin\search_api\processor\Highlight->addExcerpts(Array, Array, Array) (Line: 268)
Drupal\search_api\Plugin\search_api\processor\Highlight->postprocessSearchResults(Object) (Line: 675)

This happens because preg_replace, which runs right before this call to str_replace can return NULL if there's an error.

Steps to reproduce

In my case, I had a field with some malformed html -- a <style> tag that never closed, like this:
<style>.background { color: red; }<p>Hello</p>

Proposed resolution

Maybe something like

$text = strip_tags(str_replace(['<', '>'], [' <', '> '], $text ?? ''));

Remaining tasks

Provide patch.

Issue fork search_api-3461936

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mariacha1 created an issue. See original summary.

vishwenderbaloria made their first commit to this issue’s fork.

vishwenderbaloria’s picture

Adding patch to solve the issue. Please review.

mariacha1’s picture

Status: Active » Reviewed & tested by the community

Yep, looks great!

drunken monkey made their first commit to this issue’s fork.

drunken monkey’s picture

Version: 8.x-1.35 » 8.x-1.x-dev
Component: General code » Plugins
Priority: Minor » Normal
Status: Reviewed & tested by the community » Needs review

Thanks for reporting this issue, and thanks for posting a patch for fixing it.
However, since Drupal CI isn’t working anymore, please use MRs instead of patches for proposing changes. I now created an MR with the changes from your patch.

However, instead of just adding ?? '' I think we’d better explicitly check for an error condition and return NULL in that case.
As an alternative, we could also just use the original $text in this case, skipping the stripping of <style>/<script> element contents – since the tags themselves will still be stripped, there should be no security problem, it might just lead to weird excerpts that contain highlighted Javascript or CSS – though only in the rare cases where the preg_replace() fails. (E.g., when a visitor searches for “function” they might get an excerpt with something like “… eslint-disable-next-line func-names, prefer-arrow-callback, no-shadow function (preloadFontPaths) { const basePath = drupalSettings.path.baseUrl; …”.)
What would you say to that suggestion? Otherwise, those items would end up with an empty excerpt, and I think avoiding those might be worth risking unhelpful/confusing excerpts in rare cases.