Make WordPress Core

Opened 12 years ago

Closed 10 years ago

Last modified 10 years ago

#20124 closed defect (bug) (fixed)

Smilies Fail Combinations

Reported by: soulseekah's profile soulseekah Owned by: wonderboymusic's profile wonderboymusic
Milestone: 3.8 Priority: normal
Severity: normal Version: 3.3.1
Component: Formatting Keywords: has-patch
Focuses: Cc:

Description

I have found that certain combinations of smilies break the monster of a regular expression that parses them.

8-O :-(]
8-) 8-O
8-) 8O[
8-) :-(
8-) :twisted:
8O :twisted: :( 8-( :(

...are some examples where the regular expression appears to be failing to match. Any smilie after a smilie with an 8 in it appears to be skipped. Will try to pinpoint the flaw in the regexp soon.

Attachments (8)

wordpress_smilie_bug.png (18.9 KB) - added by soulseekah 12 years ago.
Missed smilies
functions.php.diff (347 bytes) - added by soulseekah 12 years ago.
not expecting trailing spaces at all
functions.php.2.diff (355 bytes) - added by soulseekah 12 years ago.
last group expecting trailing space, but not consuming it
functions.php.3.diff (850 bytes) - added by soulseekah 12 years ago.
all groups expect trailing space, but don't consume them
smilies-unittests.diff (18.7 KB) - added by mdbitz 10 years ago.
Enhanced Unit Tests to cover Tickets #16448, 20124, 25303
Smilies-unit-tests.out (6.9 KB) - added by mdbitz 10 years ago.
Output of Unit Tests enhancements against trunk (revision 26048)
smilies-patch.diff (21.8 KB) - added by mdbitz 10 years ago.
diff of changes to formatting, functions and Unit Tests to include resolution of 20124, 16448 and 25303
Smilies-diff.out (356 bytes) - added by mdbitz 10 years ago.
Output of Unit Tests enhancements against patch

Download all attachments as: .zip

Change History (19)

@soulseekah
12 years ago

Missed smilies

#1 @soulseekah
12 years ago

8-O :-(
8-) 8-O
8-) 8O
8-) :-(
8-) :twisted:
8O :twisted: :( 8) :(

This is the correct test case above. I missed removing a couple of formatting braces. Attached is the result.

Last edited 12 years ago by soulseekah (previous) (diff)

#2 @soulseekah
12 years ago

This is the regular expression that is matched against the content:

/(?:\s|^);(?:\-\)|\))|(?:\s|^)\:(?:\||x|wink\:|twisted\:|smile\:|shock\:|sad\:|roll\:|razz\:|oops\:|o|neutral\:|mrgreen\:|mad\:|lol\:|idea\:|grin\:|evil\:|eek\:|cry\:|cool\:|arrow\:|P|D|\?\?\?\:|\?\:|\?|\-\||\-x|\-o|\-P|\-D|\-\?|\-\)|\-\(|\)|\(|\!\:)|(?:\s|^)8(?:O|\-O|\-\)|\))(?:\s|$)/m

This can be simplified visually to

(?:\s|^);(...variants...)
or
(?:\s|^):(...variants...)
or
(?:\s|^)8(...variants...)(?:\s|$)

The problem has been pinpointed to the last (?:\s|$) clause. This is attached to "8"-type smilies and consumes the single trailing whitespace following it, meaning that anything that comes afterwards matching any of the groups, does not match ^ and the whitespace would have already been consumed by the previous group.

One solution would be to remove the trailing-space-muncher.

(?:\s|^);(...variants...)
or
(?:\s|^):(...variants...)
or
(?:\s|^)8(...variants...)

The (?:\s|$) clause never helped any of the ":" and ";" type smilies to be parsed only when surrounded by whitespace ( :)text will result in a parsed smilie, not no space after bracket, should this be parsed or not?) Noticed that the 8) smilie is gone in the trunk due to it breaking regular text.

Another solution would be to use lookaheads to test for trailing spaces, these do not consume the spaces themselves.

((?:\s|^);(...variants...)(?=\s|$))
or
((?:\s|^):(...variants...)(?=\s|$))
or
((?:\s|^)8(...variants...)(?=\s|$))

This solves :)text , if it is a problem.

(?:\s|^);(...variants...)
or
(?:\s|^):(...variants...)
or
(?:\s|^)8(...variants...)(?=\s|$)

Appears to be valid as well, only "8"-type smilies will expect tailing whitespace, but will not affect expected front-whitespace, fixing the combinations bug.

I'm probably missing something but attached are very simple diffs then.

@soulseekah
12 years ago

not expecting trailing spaces at all

@soulseekah
12 years ago

last group expecting trailing space, but not consuming it

@soulseekah
12 years ago

all groups expect trailing space, but don't consume them

#3 @soulseekah
12 years ago

  • Keywords has-patch added

#4 @soulseekah
12 years ago

  • Keywords changed from smilies, regexp, formatting, has-patch to smilies regexp formatting has-patch

#5 @kawauso
12 years ago

  • Keywords smilies regexp formatting removed

Manual keywords generally aren't necessary. Explanation.

Last edited 12 years ago by kawauso (previous) (diff)

@mdbitz
10 years ago

Enhanced Unit Tests to cover Tickets #16448, 20124, 25303

@mdbitz
10 years ago

Output of Unit Tests enhancements against trunk (revision 26048)

@mdbitz
10 years ago

diff of changes to formatting, functions and Unit Tests to include resolution of 20124, 16448 and 25303

@mdbitz
10 years ago

Output of Unit Tests enhancements against patch

#7 @mdbitz
10 years ago

Cleanup of Pending Smilies tickets with fixes

Hi, I was reviewing the pending tickets concerning the Smilies formatting functionality and have attached enhanced unit tests to cover the various scenarios as well as a patch that includes all changes outlined by the different tickets.

  • smilies-unittests.diff -> Rewrite of Unit Tests to split up test conditions, include data providers and test cases for tickets #16448, #20124 and #25303
  • smilies-unit-tests.out -> output of running unit tests against trunk (revision 26048)
  • smilies-patch.diff -> diff of all changes to formatting, functions and unit tests as outlined by the tickets
  • smilies-diff.out -> output of running unit tests against patch

Overview of Included Fixes

Using the function.php.3.diff in the code resolved both #20124 and #25303. However the new logic adds an additional single whitespace after smilies. I don't think this will be a concern as when rendered to browser it gets shown as a single space.

I've included the changes from #16448 with modification so that pre, code, script, style and textarea blocks are ignored for smilie translation. This aligns with logic being used in wpautop.

#8 @wonderboymusic
10 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 26191:

Don't place smilies inside of pre or code tags. Don't skip smilie after a smilie with an 8 in it. Fix regular expression used for smiley translations to work when there is only one registered emoticon.

Props solarissmoke, soulseekah, mdbitz, yonasy. ht to mdbitz for the Unit Tests and a comprehensive patch.
Fixes #16448, #20124, #25303.

#9 @wonderboymusic
10 years ago

  • Milestone changed from Awaiting Review to 3.8

#10 @soulseekah
10 years ago

Getting fail on Tests_Formatting_Smilies::test_convert_smilies test. Anyone else?

Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'<strong>Welcome to the jungle!</strong> We got fun n games! <img src='http://example.org/wp-includes/images/smilies/icon_smile.gif' alt=':)' class='wp-smiley' />  We got everything you want <img src='http://example.org/wp-includes/images/smilies/icon_cool.gif' alt='8-)' class='wp-smiley' /> <em>Honey we know the names <img src='http://example.org/wp-includes/images/smilies/icon_smile.gif' alt=':)' class='wp-smiley' /> </em>'
+'<strong>Welcome to the jungle!</strong> We got fun n games! <img src='http://example.org/wp-includes/images/smilies/icon_smile.gif' alt=':)' class='wp-smiley' />  We got everything you want <img src='http://example.org/wp-includes/images/smilies/icon_cool.gif' alt='8-)' class='wp-smiley' />  <em>Honey we know the names <img src='http://example.org/wp-includes/images/smilies/icon_smile.gif' alt=':)' class='wp-smiley' /> </em>'
Last edited 10 years ago by soulseekah (previous) (diff)

#11 @soulseekah
10 years ago

nvm, stale tests.

Note: See TracTickets for help on using tickets.