Followup to #2159011. Search input with slashes is still not escaped properly, result in the notice:

Warning: preg_match(): Unknown modifier '(' in SearchApiHighlight->createExcerpt() (line 312 in /[...]/search_api/includes/processor_highlight.inc).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

idebr’s picture

Status: Active » Needs review
FileSize
1.29 KB

Attached patch preg_quotes input with the '/'-delimiter

drunken monkey’s picture

Status: Needs review » Fixed

Thanks a lot for spotting and fixing this! This time, I also made sure that this really is all of it, so we'll hopefully have no more of these issues.
So, committed. Thanks again!

FeyP’s picture

Status: Fixed » Needs work

Unfortunately, patch #1 does not fix the issue completely. The problem is in this line:

$keys = implode('|', array_map('preg_quote', $keys, array('/')));

What it probably should do: Apply preg_quote with second parameter '/' to every value of array $keys.

What it actually does: Apply preg_quote with second parameter '/' to the first value of array $keys and apply preg_quote without a second parameter to the rest of the values.

So the patch will work, if the slash to be escaped is in the first key, but not if it is e.g. in the second key or later.

drunken monkey’s picture

Status: Needs work » Fixed

Thanks for spotting that, I'm rather embarassed I didn't catch it myself!
Fix committed, I hope now it's finally permanently resolved.

Status: Fixed » Closed (fixed)

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

drunken monkey’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Closed (fixed) » Patch (to be ported)

Needs to be ported to D8. (createExcerpt() still uses a key verbatim in preg_match().)

drunken monkey’s picture

Issue tags: +Novice
rosinegrean’s picture

Assigned: Unassigned » rosinegrean
rosinegrean’s picture

Status: Patch (to be ported) » Needs review
FileSize
933 bytes

Status: Needs review » Needs work

The last submitted patch, 9: search_api-escape-input-2168713-9.patch, failed testing.

The last submitted patch, 9: search_api-escape-input-2168713-9.patch, failed testing.

The last submitted patch, 9: search_api-escape-input-2168713-9.patch, failed testing.

drunken monkey’s picture

I can reproduce the test fails locally, though I cannot see what could cause them, either. But the patch definitely breaks the highlight tests (though they complete normally for me, that part is strange of the test bot).

mashermike’s picture

Status: Needs work » Needs review
FileSize
947 bytes

There was a typo in the variable name and the spaces were removed from around the $text.

rosinegrean’s picture

Status: Needs review » Reviewed & tested by the community

@mashermike Thanks, i did not notice them.
Looks good now

The last submitted patch, 9: search_api-escape-input-2168713-9.patch, failed testing.

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Ah, yes, of course. Thanks a lot for spotting that, mashermike!
Looks great now. Committed.
Thanks again, everyone!

Status: Fixed » Closed (fixed)

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