Problem with Recognition of Word Boundaries eg. two consecutive smilies

No time to actually test now, but at least let me to confirm that this is a problem. On my site, stand-alone smilies were broken (not translated from textual form) occasionally, in particular every second one in a row of smilies, and I was never really able to put my finger on the source of the problem.

OK, today I had a bit of time, so I tested this.

First, let me to clarify steps to reproduce the bug (as this took me quite a while to figure out): You need a row of smileys, one after another, set to "stand alone", and also they must be all the same smileys (multiple occurences of the same). Only identical smileys are processed by the same regex, which hesitate to process the space between smileys twice, resulting in this bug. If you have different smileys, they are processed separately by different instances of the regex, which re-inserts the spaces (so that they don't disappear), and the problem doesn't occur then.

I confirm that this patch fixes that.

I reviewed the regex carefully, based on the PHP manual website, and I believe it's correct (but might be improved, as explained below). Also the transition from deprecated eregi_replace() to preg_replace() is highly desirable IMO.

But there's more about these few lines of code:

- It didn't fix all my problems, I still had untranslated smileys (particularly at end of lines, if using the Texy filter in conjunction with Smileys). After a lot of debugging I finally found out that it's the issue I already attempted to fix ages ago, at #159835: More "blank" characters to recognize around smileys. Unfortunately, that issue got closed a year ago without commit, due to being considered as not "problematic for most of the websites". I forgot about it (but left the patch running on my site), and later when I upgraded my site to 6.x I got bitten by this again - and having not enough patience to troubleshoot, I just disabled "stand-alone" on every single smiley on the site.

While the addition of   is all nice and correct, I believe we should also add the corresponding UTF-8 character (hex. A0, UTF-encoded as sequence C2 A0). This is also white-space, and what's worse, it's almost impossible to troubleshoot once some filter adds it, or it gets copy-pasted into a post. It doesn't show anywhere, I even tried to save the page from browser to disk, and dump the file hexadecimally, but it was already stripped away. Only after I added a hex-dumping script to the smileys module code, right in front of the regex, only then I was able to detect this sequence which caused my smileys to break.

I didn't dare to play with utf-8 support at regex level just for this one character, but I believe that adding it at php-level as hexadecimally-escaped sequence is safe, as both the bytes are safe themselves, and it doesn't really matter whether regex treats them as string of two codes, or a single UTF character - the match is still the same. It works fine in my use cases.

- Additionally, I also included assertions for start/end of the whole string, otherwise smileys glued to the string boundaries (with whitespace trimmed, that is) will also fail (given no other filter added some wrapping markup yet).

- While the stand-alone smileys have a css class, the non-stand-alone part was missing that class. This is obviously an oversight, we should generate the same markup independent of the stand-alone setting. (I had problems with this, it's difficult to theme smileys having no class on the img tags...) So I added that missing class too, while we touch that line.

- As I already proposed on the other issue, the usage of a regex for non-stand-alone part is unnecessary. In that case, all we actually do is plain old str_replace() - so I used that instead. Should be faster, and is therefore recommended according to PHP documentation.

- As a code-style fix, I cleaned up some unneeded escaped double quotes, where just putting the whole into single quotes is fine. Again, the two lines were out-of-sync in this regard: The non-stand-alone part was already cleaned up, so I just copied that into the stand-alone counterpart, which missed this cleanup so far.

- Also, I believe that we don't use if/else constructs without curly braces in Drupal (and in fact I got bitten by that while inserting temporary debug code), so I fixed code style in this part, too.

Quite a bit for 2 lines of code... ;-)
(I'm unsure about the usage of a GLOBAL for the base path, but I left that untouched for now.)

The attached patch contains my additional fixes. I tested it both in stand-alone and non-stand-alone mode, against the posts I had trouble with previously, and it works 100% fine for me.

Leaving at "needs review" due to changes I did in the patch. Also this is (and was, as I see) rolled against dev version (applies to alpha5 with offset).

Do not use $GLOBALS['base_url'], use $base_url instead.

Always use API functions if available, please. See http://api.drupal.org/api/function/theme_image/6

If you would like to add Unicode support, I think the provided regex is not quite enough. According to http://www.unicode.org/charts/PDF/U0000.pdf, http://www.unicode.org/charts/PDF/U0080.pdf and http://www.unicode.org/charts/PDF/U2000.pdf at least 10 more spaces need to be added (e.g. EN SPACE, EM SPACE, THREE-PER-EM SPACE, THIN SPACE, ZERO WIDTH SPACE, NARROW NO-BREAK SPACE...). All of these spaces might occur if text is added from Text Processing Software, so there is IMHO no reason to just add the \x02\xA0 and leave the other white space characters unhandled.

Another possibility to resolve the problem would be to use the PCRE-Expression \p{Zs} (see: http://www.php.net/manual/en/regexp.reference.unicode.php), but playing around with it, it occured, that there would be additional testing of the used encoding context and the PHP version used (there seems to be a difference between PHP-4.3.2 and PHP-4.3.3). So I'm not quite sure about the way, Unicode characters should be handled in drupal.

I think, that the Unicode-Problem is out of scope of this issue, even including the HTML-Entity   is out of scope since the original issue was about not using lookahead expressions.

Besides that, I'm happy with the last supplied patch, it fixes the problem and probably does not create a new one...

Thanks for pointing out the issue with Texy module and the minimum PHP version.

I found the problem I had minutes after writing my last comment. All I had to do was using the "/u"-modifier (http://php.net/manual/en/reference.pcre.pattern.modifiers.php). But at the same time I learned, that \p{Zs} only works from >=PHP-4.4.0 - so that would be no option for the current setup.

So just for the record: using $chunk = preg_replace("/(?<=&nbsp;|\p{Zs}|[,\.\?!:\(\)\r\n\<\>])". preg_quote($a) ."(?=&nbsp;|\p{Zs}|[,\.\?!:\(\)\r\n\<\>])/ui", '<img src="'. check_url($base_url .'/'. $smiley->image) .'" title="'. check_plain($alt) .'" alt="'. check_plain($alt) .'" class="smiley-content" />', $chunk); would probably be correct, but would need PHP-4.4.0 at least.

I think for the current setup the patch3 provided in #7 is the best we can do to resolve this issue. I strongly support your patch.

I tried to test that #12 pattern somehow, it was a bit hard time for me, and my result is: We cannot use the Unicode properties.

I had hoped that it would get just somehow ignored if php version was too low, but instead it breaks things badly, sometimes even if php version is ok (!). This is a feature of additional PCRE library, that may be disabled at compile time of that library. My testing setup have PHP 5.1.6 (i.e. OK, >4.4.0, >5.1.0), with PCRE library 6.7 (i.e. the one listed on php.net as bundled with 5.2.0), running in default configuration (as for PCRE) as distributed in RPM package (Mandriva Linux desktop). If I try this pattern, however, I get:
Warning: preg_replace() [function.preg-replace]: Compilation failed: support for \P, \p, and \X has not been compiled at offset [position] in [file] on line [line]
And what's worse, preg_replace() returns NULL, so the node in question show as completely empty (not just smileys unconverted - it removes all the text).

On production server it works (PHP 5.2.9), but in a situation where the new pattern truly breaks the whole display of nodes, not only on known (still supported by Drupal) older php versions, but on just any version where this or that setting was different at compile time, there's no chioce but to go with #7.

Thanks for support, frenkx.

Anyone going to either voice further concerns, or hit RTBC? ;-)

The #15 suggestion is likely to work (not tested yet), except that we would need to use something like @preg_match('/\p{Lu}/u', 'A'), to avoid the watchdog being flooded with warnings per #14, and possibly also cache this check in a static variable - we're running that regex over and over again, for every smiley configured on the site, for every filtered post (this may easily mean 1000 runs on frontpage after cache flush, assuming 10 promoted nodes and 6 custom blocks, 30 smileys with 2 acronyms each - i.e. :-) and :smile:). This might be noticeable in performance - per http://www.php.net/manual/en/regexp.reference.unicode.php :

Matching characters by Unicode property is not fast, because PCRE has to search a structure that contains data for over fifteen thousand characters.

But otherwise this looks to me like over-complicated. I don't think we're using such fallbacks in Drupal much (i.e. having different behavior on different PHP versions), and it's only just covering edge cases of advanced text-processing codes being pasted into a post, at a supposedly significant performance cost. So my opinion now is - let's stick with the simple list of codes for now. We may extend that list, if a use case is encountered with more characters involved, and we might mention the \p{Zs} option in a code comment as a future alternative, so that this may be revisited after we can rely on the feature (that's either 7.x, or even 8.x stuff - current 7.x HEAD requires PHP 5.2.0, which is OK version-wise, but means exactly the version of PCRE library I had trouble with).

If you feel strong about the patch being OK (and you tested, preferrably), you can change status to "rewieved & tested by the community" (also known as RTBC), as this is how we put things into the maintainer's queue at Drupal. It's considered unfair if the author of the patch (i.e. me) did that himself. This workflow is perhaps not strictly followed at less-busy contributed projects (like Smileys is), but still it may help to draw appropriate attention here.

Otherwise you're right. Thanks for support.

The patch is at #7 (unless we need to add a code comment per #16). Otherwise the extended discussion just explored additional options, and finally went back to the option already rolled and tested in #7.

(As for "good lending hand", whatever that exactly means, I'm afraid there's no more additional space on my personal time/schedule. These patches [for various projects, recently] are my maximum currently.)

To sum it up, #21 only just added the suggested code comment. My only concern about that comment is that the PHP version should actually be 5.1.0: On 4.x.x branch of PHP it works from 4.4.0 up, and on 5.x.x branch from 5.1.0 - so we have a gap >=5.0.0. <5.1.0 where it won't work. So let's simply say 5.1.0 - after all, D7 already wants 5.2.0.

It would be nice if someone re-rolled the patch with this, but since I can't do so right here in this minute, I'm not blocking the issue (so leaving at "needs review"). After all, there's a link to this issue in the comment, and the whole thing would need more research anyway if revisited (i.e. performance, availability of properly compiled PCRE library etc.)

Great, so I hit the RTBC button. If I need to take further action, I ask you to guide me through.

I guess you should reorder the smileys, because the regex is applied repeatedly, one possible smiley at a time. So there's no way to fix this in the regex itself, as it each time knows about just one acronym.

@Gurapartap: I don't know... Every time I try to re-arrange them, it gives me an error. :( I'll try again later. Still not an ideal solution, though, because that's going to change the order they are displayed in the box. I don't know a better solution, though, so I guess it will have to do... If it ever lets me do it...

Thanks,

Michelle

Argh... I went through every single one and put in the weights... Only to discover that there's no way to save the page with javascript off.

Yeah, this is totally OT, sorry. I only meant to ask if the regex in the patch could be made to handle it, not totally derail the issue. I'll try fiddling with the order in the database.

Thanks,

Michelle

@JirkaRybka: Changing the weights in the database had no effect, either. I'm going to put this on the back burner for now and may file an issue at some point if I figure something out. Thanks for the help. :)

Michelle

Another RTBC +1.