WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#36306 closed defect (bug) (fixed)

Smiley in shortcode attribute breaks the parsing

Reported by: Unyson Owned by: pento
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: Emoji Keywords:
Focuses: template Cc:
PR Number:

Description

The Problem

When using smiley in shortcode attrbibutes it breaks the parsing, because smiley are transformed to html too early, before the attribute parsing begins, so do_shortcode receives

[footag foo="Hello <img src="..."> World"]

Steps to reproduce:

  1. Create the a test shortcode
<?php
function footag_func( $atts ) {
        return "foo = {$atts['foo']}";
}
add_shortcode( 'footag', 'footag_func' );
  1. Add in post content:
[footag foo="Hello World"]

Save the post and check the frontend - Works

  1. Edit post content:
[footag foo="Hello :-) World"]

Save the post and check the frontend - Notice: Undefined index: foo

The Fix

Replace this line with

<?php
add_filter( 'the_content', 'convert_smilies',               12 ); // after do_shortcode()

Change History (5)

#1 @pento
4 years ago

  • Milestone changed from Awaiting Review to 4.6
  • Version 4.4.2 deleted

Thanks for the bug report, @Unyson!

The good news is, this is rapidly becoming less of a concern, as smilies are now being translated into emoji (except for :mrgreen:, which has no emoji equivalent).

However, I agree that it could still be a problem, so we should fix this. :-)

#2 @pento
4 years ago

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

In 37298:

Smilies: Move convert_smilies to happen later in the the_content filter.

In particular, we want it to occur after shortcode handling. The smiley conversion routine doesn't have any concept of shortcode structure, so may inadvertantly replace a smiley with HTML inside a shortcode attribute, which will cause the shortcode to not be parsed correctly.

Props Unyson for the initial suggested fix.

Fixes #36306.

#3 @Unyson
4 years ago

Thank you

#4 @Unyson
4 years ago

@pento Please add a comment next to that priority or a link to this ticket, so others will know why there is set priority 20 and will not change it in future.

Last edited 4 years ago by Unyson (previous) (diff)

#5 @pento
4 years ago

Thanks for the feedback on the patch, @Unyson!

There's no need to add a comment, as all custom priorities in default-filters.php are assumed to be there for a good reason, and shouldn't be changed without careful consideration. If it ever came up in the future, part of that consideration would be to look through the annotated source for why it was changed. A comment pointing at this ticket would be superfluous.

Note: See TracTickets for help on using tickets.