FIX getElementsForSearch() to replace tags with spaces
#897
Conversation
| @@ -43,7 +43,8 @@ public function getElementsForSearch() | |||
| /** @var ElementalArea $area */ | |||
| $area = $this->owner->$key(); | |||
| if ($area) { | |||
| $output[] = strip_tags($area->forTemplate()); | |||
| // Replace HTML tags with spaces | |||
| $output[] = strip_tags(str_replace('<', ' <', $area->forTemplate())); | |||
There was a problem hiding this comment.
I think this presents a trade off that's worth it in this instance, as now if you have the string There are <5 potatoes it will become There are < 5 potatoes which means content previously indexable by ElementSiteTreeFilterSearch::applyDefaultFilters will no longer be picked up. But I think this is enough of an edge case that this is a good update ❤️
There was a problem hiding this comment.
It's easy to target HTML tags via regex, not sure what is the benefit of not doing that here, while possibly changing the meaning of the indexed content (marginally, depending on the settings, but can't say it's definitely not).
There was a problem hiding this comment.
The idea is that we fall back to strip_tags() to do the heavy lifting in terms of HTML detection.
There's a lot of edge-cases either way really, if we hand-roll our own regex we are just as likely to add another issue, like "we wanted to make sure there were < 5 errors, but > 10 comments" being caught, or innerhtml such as:
<button onclick="document.getElementById('alert').innerHTML='<strong>MESSAGE</strong>';">Not being rendered properly. I admit that's unlikely but it's possible.
@adrhumphreys it will become There are <5 potatoes and shouldn't break anything, the space is added before the < - I'll take a look at those tests, not sure exactly why they are failing, it seems unrelated to my changes
There was a problem hiding this comment.
Suspicion that the Behat failures are related to chromedriver 91? Having trouble replicating locally
|
@andrewandante I've rerun the tests. Looks like there's a failure related to phpunit9 - https://app.travis-ci.com/github/silverstripe/silverstripe-elemental/jobs/555927169 |
andrewandante
force-pushed
the
hotfix/get_elements_for_search_spaces
branch
from
bf16e9b to
d0d539e
Compare
|
@emteknetnz PHPUnit tests updated, still seeing Behat failures though. I see that they are also failing on |
|
Yes unrelated |
Having an issue where this method combines
<p>tags into a single string, muddying my search results, i.e.becomes
bananahamburgerwhen this method is called.Starting with a failing test - going to propose a fix which should replace these with spaces