Undefined offset in SearchApiHtmlFilter

subscribing.

I observe this issue, I am using solr 5.0.1
with search api version: 7.x-1.11

Looks like I have a really old version of search api, I am going to try upgrading!

Upgrading did not help however ...

Here's a patch that helps, feel free to improve upon it.

setting to needs review. I have only done very simple testing on it so I cannot guarantee it's effectiveness.

ok, test this patch instead

Looks like the test bot is broken, I bet a non-change would provoke an error.

Ok, one last try.

Ok, testbot looks broken, but I have another patch here.

I tested my patch extensively in a dev environment, the unpatched issue occurs when indexing "archived web content" that may contain invalid html. We're not going to fix our archived content, however the patch does correctly handle the unset array and allows indexing the content correctly. I did a search on the content after applying patch 12 and it's indexed correctly although the contents of the index appears to have some left over > < characters with or without the patch.

The patch correctly prevents the watchdog from filling up with notices and warnings.

For now the patch does the job doesn't seem to have any bad side-effects. I'm not entirely happy with the patch because I'd like to improve it even more. The logic in SearchApiHtmlFilter->parseText() could be improved and made in a more structured way. With that said, I'll let a fresh set of eyes take over. From my debugging it looks like the parseText() function removes html tags, but it only does so correctly if the html is well structured.

To improve this: test parseText() with invalid html. This is where you'll see the need for perhaps a more robust/structured logic.

setting this to needs review in the hopes that the head is fixed.

This should pass testing, if it doesn't then HEAD is broken (before the patch even is tested)

not sure why this isn't passing testing, it fails on something that doesn't seem to be related to the patch. I retested another issue and it passed so it appears that HEAD is not broken.
I'll have a look at this later maybe.

running a head test on a no-logic-change patch (add a comment, that's it)

fire up the test bot

filename correction.

Once the patch for "HEAD is broken, simple test failure on adding a comment" is committed, then we can get back to testing patch 12.

Thanks for reporting!
I can't reproduce this and it's weird that the problem would appear only now, after the code hasn't been changed for more than a year.

In any case, I'd guess the proper solution here would be to just check whether preg_match() actually matches, and continue the loop otherwise, under the assumption that the HTML is just broken. The attached patch should implement this.

It would just be good to know which input leads to this problem for you. Could you please find that out, and maybe at which position in the string the method encounters the not-matching (assumed) tag name?

@drunken monkey , yes in my case I've got content classified as "archived" (aka, we don't maintain it or keep archived content up to date with the ever evolving w3c standards) , it's our archived content that has "broken" html and it's going to stay broken.

According to my debuging the undefined offset occurs only when processing broken html.

I'll have a look at your newer patch.

as for the simpletests failing according to the research I did calling

 checkPermission(array(), TRUE); 

before assigning permissions resets the permissions cache and can reduce problems when drupalCreateUser is invoked after setUp loads a simpletest environment.

Thanks for the patch, I'll test it asap (when I get back to the office).

Hi @drunken_monkey, your fix seems to work.

I re-indexed 4000 nodes , including archived content, looks great, the archived content is indexed correctly, I was able to search for the content.

By the way, my patch on patch 12 also works and passed testing as well.

Feel free to give me a bit of credit on the commit :)

Looks good, thanks.

oh yes, and no more undefined offset notices in watchdog (the reason for the patch in the first place), so this is good.

OK, great to hear, thanks for testing!
Committed. Thanks again!

And sure, I always try to credit contributors fairly. In this case, you clearly did the bulk of the work, I just refactored your patch, so you get the credit.