search_by_page_strip_tags() currently does the following to remove selected HTML tags:
$text = preg_replace('|<' . $tag . '>.*</' . $tag . '>|isUu', '', $text);

That would remove something like <script>...</script>, but would fail on <script type="text/javascript">...<script>

Here is a simple improvement:
$text = preg_replace('|<' . $tag . ' .*>.*</' . $tag . '>|isUu', '', $text);

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Category: feature » bug

Sounds like a bug! Thanks for pointing it out and giving a solution.

ralf.strobel’s picture

ralf.strobel commented

Your're probably right. Also, upon re-reading my own code, I saw that I had made a mistake myself. The space I inserted after the first $tag was meant to ensure that the expression will not match tags that simply begin with our tag. Problem is it also misses tags without attributes now.

I think for HTML, you could just replace my " .*" with ".*" because to my knowledge, there are no tags that look like the beginning of another tag. For perfect XML compatibility, however, it should probably become "(?:\s.*)?".

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Version: 7.x-1.1 » 7.x-1.x-dev
Issue tags: +Needs backport to D6

Really I think it needs to be [^>]*, to make sure it doesn't go past the > that is the end of the tag. Anyway, thanks for reporting the bug, and I plan to make a test as well so that I make sure not to break either type of tag (with or without attributes).

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Active » Fixed

It turns out I already had a test, but it didn't include a tag with an attribute. So I added an attribute to the test, verified that the test failed, and then fixed the bug.

I've committed this fix to both the 6.x-1.x and 7.x-1.x development branches of Search by Page. Well, actually, it looks like it was only wrong in the 7.x-1.x branch (the 6.x branch had the correct regular expression, but for some reason it wasn't in the 7.x branch). But I updated the test for the 6.x branch.

You can get the updated module now by using the Git source code repository. If you wait up to 12 hours, you can get the updates in the "Development" release section of the project page (Drupal.org automatically makes new zip archives every 12 hours or so). Or, you can wait until the next full stable release, which will include this fix.

ralf.strobel’s picture

ralf.strobel commented

Cool. But to be honest, I had only briefly tested your module. In the end I decided Search API offered a better solution. Sorry. :) But still glad I was able to contribute.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Of course, you need the module that does the right thing for your site! :)

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

strip_tags question already answered... the function doesn't allow you to specify tags to remove