Problem/Motivation

This warning occurs when searching for a specific term, during highlighting.

Steps to reproduce

See http://sandbox.onlinephpfunctions.com/code/193afb31c93cc7a9012f5cc4a0a2e...

Proposed resolution

Remaining tasks

CommentFileSizeAuthor
#8 3264239-8--pcre_error_highlighting.patch3.58 KBdrunken monkey
#8 3264239-8--pcre_error_highlighting--tests_only.patch2.57 KBdrunken monkey
#8 3264239-8--pcre_error_highlighting--interdiff.txt3.25 KBdrunken monkey
#7 3264239-6.patch3.82 KBdieterholvoet

Issue fork search_api-3264239

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:

About issue forks

Comments

DieterHolvoet created an issue. See original summary.

dieterholvoet’s picture

dieterholvoet commented
Issue summary: View changes
bonrita’s picture

bonrita commented

Also see: https://www.drupal.org/project/search_api/issues/3115343#comment-14269782

dieterholvoet’s picture

dieterholvoet commented

Thanks for sharing, but I was already aware. That issue is linked below Related issues.

dieterholvoet’s picture

dieterholvoet commented

I was able to narrow down the issue, it can be reproduced by passing the following string:

<img src="" width="" alt="danielle" ">

through the regex in Highlight::highlightField:

#((?:</?[[:alpha:]](?:[^>"\']*|"[^"]*"|\'[^\']\')*>)+)#i

The problem is invalid HTML, so I think a possible solution would be to fix any broken markup before passing it to that regex. We could also update the regex, but I'm afraid that regex is too complex for me.

DieterHolvoet opened merge request !38

dieterholvoet’s picture

dieterholvoet commented
Status: Active » Needs review
StatusFileSize
new 3264239-6.patch3.82 KB

I added a fix and a test. I also attached a patch for inclusion in Composer projects.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Version: 8.x-1.23 » 8.x-1.x-dev
StatusFileSize
new 3264239-8--pcre_error_highlighting--interdiff.txt3.25 KB
new 3264239-8--pcre_error_highlighting--tests_only.patch2.57 KB
new 3264239-8--pcre_error_highlighting.patch3.58 KB

Thanks for reporting this issue and already providing a patch – very nice work!
Thanks, especially, that you even provided a test already! The test, however, is slightly off, as it’s actually not the excerpt that’s making problems (as we already strip all tags from the tags before creating the excerpt) but fields highlighting.
I therefore adapted the test slightly to better tell what’s actually being tested.

The problem itself seems to come from excessive backtracking, and therefore seems only slightly related to invalid HTML. Therefore, I think we’re better off trying to fix the regex to not need as much backtracking anymore. Please see/test/review my attached revision and tell me what you think.
Anyways, thanks again for your nice work on this!

(Unfortunately, the test bot doesn’t work for MRs in this project, so please use the old patch workflow instead.)

The last submitted patch, 8: 3264239-8--pcre_error_highlighting--tests_only.patch, failed testing. View results

dieterholvoet’s picture

dieterholvoet commented

These changes look good, thanks for the feedback!

  • drunken monkey committed ee56453a on 8.x-1.x 
    Issue #3264239 by drunken monkey, DieterHolvoet: Fixed the regular...
drunken monkey’s picture

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

Good to hear, thanks for reporting back!
Committed. Thanks again!

Status: Fixed » Closed (fixed)

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

bonsaipuppy’s picture

bonsaipuppy
he/him
commented

I might be misinterpreting the use of the preg_split, but as far as i understand from the source code it's supposed to throw away any html tags, opening and closing ones, to just leave the text inside those to highlight in.

then the regex in the patch won't work properly for nodes with attributes having quoted values.
check it out on regex101: https://regex101.com/r/w0OSao/1