truncate_utf8() only works for latin languages (and drupal_substr has a bug)

Issues related to this:

#11883: Add test coverage for auto-comment title functionality and word-breaking
#34171: Title truncation from description always removes last word
#86461: Truncate long paths and path aliases on admin/build/path
#134090: Change 'Top referrers' report to 'Recent referrers'
#147045: Forum: Link to last post in 'Last post' column
#200185: truncate_utf8() is used as a substring function
#269911: Search result trimming should not fall inside HTML entities/tags
#362609: node teaser generation breaks when using entities
#404356: Aggregator should strip tags before using beginning of description tag as title
#504376: Watchdog entries go missing because of non-plain link in the watchdog view
#718662: DBLog listings truncate messages in the middle of HTML tags

As there are so many other issues that rely on this functionality, I'm bumping this one up to critical.

There is also
#180452: Need new truncate function with more features
which is a D8 feature request at this point.

Here's a first attempt at a patch using methodology discussed above. It may change something in existing code, because the word boundary character class includes punctuation and symbols. But the change should be for the better.

Note that the patch moves the regexp class from search.module to unicode.inc (with a rename), and translates the '...' added at the end.

Let's see what the test bot says and go from there...

What? There were only 25,480 exceptions. I don't see any problem. :)

Must have been a Monday patch... I'll see if I can do better (and test it this time before submitting it).

Here's a new patch.

I'm not sure about the heuristic of only truncating to a word boundary if the string would be 1/2 of the desired length or more. That's not how the old function worked, and I'm not sure we want to change it in that way.

So, this patch has the benefit of actually working (at least, the dblog looks reasonable). It may cause a few test failures, though, since the definition of a word boundary has changed to include punctuation. Let's see what the test bot says this time...

I should also note that I think putting a space before the ... is silly. I left that in there for backwards compatibility, but I think the normal thing (in English at least) would be to have text like:

This is a really long sentence that has been...

rather than

This is a really long sentence that has been ...

Thoughts?

The patch in #8 requires at least one character in the string to use the word boundary, which is the same as the original heuristic. I think if we apply any other heuristics, it should be an additional input to the function, like $min_length. Thoughts?

So I guess we are agreed that there shouldn't be a space before the ...

I can remove that (and likely will need to fix a few tests where it's assuming the space is there). It looks like the patch in #8 works this time, though, which is good.

The helip entity may be the correct punctuation, but it is probably not the right punctuation for all cases, because sometimes you just need ascii characters and not HTML entities.

We could conceivably add another input argument to this function to give you the option of ... or the elipses entity. Or callers of this function could not use the ... argument and add helip themselves.

But, as discussed before, ... or even the entity is probably not the right thing for non-latin languages anyway. Which is why t('...') is probably the right thing to do... I think?

Not sure... is it a valid UTF-8 character? I also don't understand why ... in plain ASCII is so objectionable?

OK... [that was three dots :) ]

So what about the patch in #8 then? Questions/comments that have been brought up:
a) Should we have a heuristic like "must be at least half the string" rather than the current "must be at least 1 character" to use word-safe? My inclination is not, unless we make it an input (let the caller specify the minimum length for wordsafe truncation). I'd be inclinded to leave the default at 1 if we make it an input, for backwards compatibility.
b) Should we remove the space before the ... that is appended?
c) Should we switch to appending the horizontal elipses character/entity rather than three . characters?

The patch in #8 does none of the above, but is not backwards-compatible because it is different on what it considers a word boundary (previously it was a space, now it's a space, punctuation, or CJK word-boundary indicators).

I am inclined to say yes on (b) and no on (a) and (c), but am open to community consensus because they are all reasonable suggestions in my opinion.

In any case, we we need to document this change in the module upgrade guide when we decide/commit, and likely update tests if we make any of the changes above, and update the function header along with the code if we implement any of the three.

It would also be great if someone could come up with a CJK word-safe trunctation test.

That's two for (b)... anyone else?

Damien (see comment #10 above) also agreed about (b), and it sounds like we're all "OK but not mandatory" about (a). I'll see what I can do.

Here's a new patch.

Changes from orginal function:
1. If using wordsafe, finds non-latin-safe word boundary (including punctuation), instead of specifically looking for ' ' as word boundary.
2. If appending dots, appends t('...') instead of ' ...' -- so '...' is translated, and there is no space before the ...
3. If using wordsafe mode, allows you to specify a minimum length (defaults to 1 character for backwards compatibility).

2 & 3 are changes from last patch.

Let's see what the test bot says. There may be tests that are looking for 'truncated stuff ...' somewhere and now they'll have 'truncated stuff...' without the extra space.

It would be great if someone could provide a CJK test of this function, too, and maybe we should write some unit tests too?

OK, how about this version? I decided it might make things clearer to change the name of the 2nd arg for clarity, and I rewrote the rest of the doc.

As for why to have $min_length, this is discussed above in #10, #18, #21. The reason is that if the last space is only after a couple of characters, followed by maybe a really long URL with no spaces in it, you probably don't want to return that really short string because no one will understand what it is supposed to be.

I changed max_length to max_return_length (which refers to string with dots) to distinguish it from min_length (which refers to the string without dots). It seemed wrong for min_length and max_length to not be parallel, but they are used for different reasons presumably (max to truncate for db storage, min for understandability).

Thoughts?

I can make another go of clarifying the doc...

RE #28 -- OK, I will add that example to the patch.

OK, here's a new patch...

One more go after some verbal comments from drewish...

Bump. Can we get this reviewed and hopefully committed? I have some search patches to do that will need to use this preg class whose name is changing...

#32: 768040-v6.patch queued for re-testing.

- The reason max_return_length and min_length are not named in a parallel way is that they do not behave in a parallel way. Read the doc/code for details -- one governs return length and the other is length before dots are added. I didn't want them to look like they behaved the same, so I didn't name them the same.

- Agreed on other thoughts. I'll update the patch.

OK. Here's a new patch with many updated variable names and some updated documentation. I realized the URL example was wrong, since . and : are punctuation, so I modified it slightly. I also realized that the search.module chunk of this patch has been missing since that patch in #23, so this patch puts it back in.

As noted in #23, do we need some tests for this function before the patch is committed? Also, the changes to the function will need to go into the module update guide.

Seriously, memory allocation for one non-array varialble worries you?

The reason I used a second variable $max_length inside the function is that it no longer has the same meaning -- it's the length to truncate the string to, not the total returned length.

It's a local variable and should be popped off the stack as soon as the function finishes, so I fail to see how it would increase the footprint of Drupal much to have a few extra bytes in memory for a miniscule amount of time. It's not like we have thousands of functions in the call stack at a time.

Roll a new patch or mark it "needs work" if you feel strongly enough about it. I'm happy with what it is now and it makes logical sense to me that "max_return_length" refers to the length of the returned string in the argument, and "max_length" refers to the length of the string before adding dots to it within the function, which is not the same thing. I don't feel strongly enough about it to object if you want to not use a different variable within the function.

RE #46 - Yeah, I missed that when I moved it over from search.module. Here's a patch with the text changed to be generic rather than talking about what gets into the search index.

It also adopts codycraven's renaming of the max_length value form the "alternative" patch, which was the name I had given the parameter back in earlier versions of the patches anyway, before codycraven objected to that name back in #35. Whatever. :)

#47: 768040-fixsearchcomment.patch queued for re-testing.

The max length param is supposed to be a hard limit. If the person requests ellipses and the length of t('...') is > max_length, and we need to truncate, I think we should return an empty string.

Good catch on the bad logic though!

Also, I think your tests are great, but maybe we should also test with some other inputs rather than hard-wiring everything except the length?

What do you see when you run the tests through simpletest on your testing system, as opposed to the bot?

Looking at the logic in your current truncate_utf8() function, it looks like drupal_substr() is returning 24 characters when asked for 23, or else drupal_strlen() is saying that the string length is <= 23.

  if (drupal_strlen($string) <= $max_length) {
    // No truncation needed, so don't add ellipsis, just return.
     return $string;
   }
... (if wordsafe {} )

   else {
    $string = drupal_substr($string, 0, $max_length);
   }

So there could possibly be some difference in the behavior of drupal_substr() or drupal_strlen() based on some global variables that could be different between the test bot and your system:
http://api.drupal.org/api/function/drupal_substr/7
http://api.drupal.org/api/function/drupal_strlen/7

If so, that's a bug. Would be great to debug it! You can try putting some debug() statements into your tests, see how they work on your testing system, and then submit a patch and see what the test bot says.

OK. Thanks for all the work you've done! I will see if I can find some bandwidth to debug it.

I guess it's already assigned to me.

The patch in #56 is also missing the chunk for search.module.
EDIT: scratch that. I can't read.

I am debugging this now. I can see the test failures. And that patch in #56 is missing the search.module chunk.

Hah. I added a few more tests to unicode.test, to the part where it is testing drupal_substr() and the bug is happening there -- it is not truncating correctly.

So I will need to delve into that...

Here's the current patch, so you can see drupal_substr() failing. I also modified the last set of tests a bit - modularized it slightly, and put it into the set of tests that is running in both non-multibyte and multibyte modes, for better test coverage.

So the drupal_substr() tests are failing when the global $multibyte is set to emulation mode, for several cases:
truncating '...' to two characters
truncating 'frànçAIS is über-åwesome' to 23 characters

It seems to be a problem when the requested length is 1 less than the actual string length.

I will continue to investigate, but I thought it might be useful to attach this patch now so you can see the tests failing.

OK, I found where the bug is. In drupal_substr():

      // Backtrace one byte if the end of the string was not reached.
      if ($iend < $strlen - 1) {
        $iend--;
      }

The problem is that if $length is equal to one less than the string length, at this point $iend == $strlen - 1, and so it doesn't step back one byte even though it should. So this logic is wrong.

I'm going to add some additional tests for negative lengths and negative starts (there were some but not all cases were covered) and see if that has problems too.

Here's a fix.

This is the only way I could figure out to fix the logic problem in drupal_substr. It comes back all green on my test box for the unicode tests. Let's see what the testbot and reviewers say...

I added some additional test cases to the drupal_substr() tests, beyond the ones in #65, which I think should cover all the bases now. Hopefully.

Hmmm.

This is failing on the CJK line I tried to put in the patch. The test works fine on my machine, but I think my patch-creation software is choking on those characters.

And now my patch-applying program is not able to apply the patch above either.

Help?

The problem in the CJK is these lines:

+      array('以呂波耳・�?��?��?��?�。リヌルヲ。', 1, 3,
+              '呂波耳'),

which are coming through garbled... I took this string of characters from search.test around line 347... Can someone perhaps re-roll the patch?

Sigh.

Never mind. Tried my other machine and I think it has worked better this time.

Good points. I will reroll later today or tomorrow.

Here's a new patch, with a redo of the comments and fix for the extra comma mentioned above. Hopefully the unicode characters didn't get screwed up again...

Just as a note, if this is committed, we should probably port to Drupal 6, which has the same exact functions currently.

Oops, cross post.

I forgot about that. We can't really remove that DEFINE from search.module, as contrib modules are definitely depending on it.

So probably the only thing we could do is move it (with the same name) to unicode.inc.

Although... that could break other contrib modules. I know of one that copied that define to their own module and left the name the same. Which was a bad idea, but... anyway.

No, it has not been fixed in Drupal 6. Someone needs to port the patch. Hence, the issue status...

Unassigning. If someone wants to port this...