Search result trimming should not fall inside HTML entities/tags

Looks like this still needs to be fixed, and can we have a test for it too?

The patch looks like it should work in the case that the exerpt didn't find the keywords... A couple of questions:

a) Is it possible for search_exerpt to truncate incorrectly if it did find a keyword? ... I looked at the code: No. So that's OK.

b) Is putting " ..." after the excerpt the right thing to do in all languages? I doubt it. However, that's probably a separate issue.

c) Will this patch work for languages (Chinese, Japanese) that don't have spaces between words? No, I don't think so. It looks to me as though in this case, the proposed patch will probably return a 0-character excerpt, which is not OK. Or is there something I am missing? I think the correct solution is to use $boundary to find a word boundary, rather than the truncate_utf8() function?

Interesting question about word boundary. Suppose $boundary is language specific so should be defined in iso.inc

c) Will this patch work for languages (Chinese, Japanese) that don't have spaces between words? No, I don't think so. It looks to me as though in this case, the proposed patch will probably return a 0-character excerpt, which is not OK. Or is there something I am missing? I think the correct solution is to use $boundary to find a word boundary, rather than the truncate_utf8() function?

If this is indeed an issue, and it might very well be, it should be filed as a separate issue against the truncate_utf8() function.

Anyway, both this patch and #718662 patch are using the PHP function truncate_utf8(), which obviously wouldn't care about Drupal's iso.inc or any other definitions of word boundaries. I don't know whether or not truncate_utf9() does something sensible for Chinese/Japanese [...]

truncate_utf8() is a Drupal function, not a PHP function. (There's also no such thing as "UTF-9".)

Sorry, my mistake about PHP vs Drupal function on trucate_utf8.

Obviously utf9 was a typo. There is really no need to be so snarky in the issue queue when someone makes a typo. Thanks!

Can someone find an answer about whether spaces are valid word boundaries in all languages? I am extremely busy this week and don't have time to research it on IRC.

Thanks brianV for confirming what I suspected: a space is not a word boundary in all languages.

So the patch in #3 is not going to work for us in general. And since the search module unfortunately doesn't know anything about what language it's dealing with, I don't know what will. Not an easy problem.

Actually, we could fix the actual reported issue about HTML entities by converting HTML entities in this function to characters before truncating (not on a word boundary). It would still stop in a word, but it at least wouldn't make half-entities.

So I also took a quick look at the the 12 functions reported by api.drupal.org in D7 that call truncate_utf8() [note that there could be other calls in classes, since api.drupa.org is not displaying them yet!]. Most do not use the word boundary functionality. Those that do:
http://api.drupal.org/api/function/_menu_parents_recurse/7
http://api.drupal.org/api/function/_book_toc_recurse/7
http://api.drupal.org/api/function/theme_dblog_message/7
http://api.drupal.org/api/function/comment_submit/7

Those are also suspect.

Also I think the "..." after truncating that is in truncate_utf8 as well (almost all the calls either use that or put their own "..." afterwards) is possibly also not correct? I don't know about that.

We have some CJK word boundary code in the search module already, by the way. I don't know how well it works.

Not too sure what to do about all of this.

Well, until truncate_utf8() is fixed, the solution suggested here is not acceptable, because it will likely result in zero-length excerpts in some languages, which would be worse than what we have currently (which is excerpts that break in the middle of a word). So I'm marking this back to "needs work".

Plus, I still don't know if putting "..." at the end of an excerpt is the right thing for all languages. Any ideas on that?

So, let's get back to basics here. The problem reported here is that the current way of doing things is broken in that it can break in the middle of an HTML entity. I think that one solution to that would be to use http://php.net/manual/en/function.html-entity-decode.php to decode the entities, then truncate without the word boundary restriction). That way we wouldn't be breaking up entities, and that would solve the reported issue.

And by the way, there are about 25 issues that come up when you search for truncate_utf8 (including this one). I will go through them later today and file a new one if this is a new issue about it not working for CJK.

But this one is almost certainly relevant:
#200185: truncate_utf8() is used as a substring function
As noted by Gabor:
http://drupal.org/node/200185#comment-662567
truncate_utf8() is NOT meant to be used as a substring function. So we should not be using it here.

#3 is ok.

@jhodgdon: in the current form of truncate_utf8(), if there is no space anywhere it will simply truncate at the indicated character length. This will never result in a zero-length string.

This likely should be backported to D5 and D6.

Sorry about the cross-posting there, and about being prickly, Freso. :)

And Damien is correct about the truncate function working OK if a space isn't found (#16), although in this case it will still be broken as far as possibly breaking in the middle of an entity or HTML tag. (Hopefully we'll also fix the truncate function to work better for non-Latin languages, on that other issue).

But what about #14 suggestion -- shouldn't we be doing an entity decode and maybe a tag removal before we truncate?

See #20. Shouldn't we be decoding entitites and removing html tags before truncating?

Here's a patch that does an entity decode/encode.

It appears to work. I was able to test both the "I found keywords to highlight" and "I didn't" paths in the code by using the Porter Stemmer module, and both are displaying OK on the search results page.

Looks like those test failures are real... will need to investigate.

Yeah, probably the exact string comparison changed a bit and the test needs a rewrite if we go with this patch.

Addresses the issue in the OP. Can we have a test for this?

OK.

I fixed the above issues and I wrote a test. It (of course) uncovered some other problems in this function, so I had to fix them too in order to get the test to pass.

Regarding space before/after:

Otherwise, I think I've addressed your comments - thanks!

Looks great! I'm not ecstatic about the comments, but let's address those separately .. maybe we can work together on all search comments and have a single cleanup patch.

I added the following as an introduction to the test case:

'Passes keywords and a sample marked up string, "The quick brown fox jumps over the lazy dog", and compares it to the correctly marked up string. The correctly marked up string contains either highlighted keywords or the original marked up string if no keywords matched the string.'

#42: 269911-cleanup-docs.patch queued for re-testing.

I re-rolled the patch, which didn't apply anymore. It seems my editor caught some spaces that should have been removed.

Let's try once more.

#50: 269911_cleanup_docs_cleaned.patch queued for re-testing.

#50: 269911_cleanup_docs_cleaned.patch queued for re-testing.

#50: 269911_cleanup_docs_cleaned.patch queued for re-testing.

Please use our own decode_entities() instead of html_entity_decode(), which is buggy. And check_plain() instead of htmlspecialchars().

Also, this is not really great for translatability purposes:

I suggest we do it this way:

That will give more context to translators.

Here we go (I hope)... let's see if it still passes the tests at least.

Hm... Failed "Entities are converted in excerpt". Perhaps the substitution of the drupal functions wasn't successful. Will need to investigate.

jhodgdon, could you use cvs diff -up to see which functions are changing

While you on it, is it possible to make highlighting swappable?
As maintainer of http://drupal.org/project/rustemmer I need to patch search.module because I need highlight a stemmed words which mostly different from words contained in fragment

I think it's possible we can rip decode_entities() in favor of the php built-in html_entity_decode() since Drupal 7 requires PHP 5.2 and utf8 support was added in 5.0. http://www.php.net/manual/en/function.html-entity-decode.php

However, that should be a separate issue. This patch looks good.

@pwolanin: I can confirm that html_entity_decode($input, ENT_QUOTES, 'UTF-8') works correctly and passes our tests. This charset parameter must have hidden in a cave or something... we refactored the whole decode_entities() in D7 to workaround supposed limitations in the entity table set of PHP :)

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