Make WordPress Core

Opened 6 years ago

Last modified 3 years ago

#52517 new defect (bug)

Shortcodes inside AMP tags do not work

Reported by: Krstarica Owned by:
Priority: normal Milestone: Awaiting Review
Component: Formatting Version: 4.7
Severity: normal Keywords: has-patch
Cc: Focuses:

Description

Example code to demonstrate shortcode not working inside AMP tag:

add_shortcode ( 'test', '__return_false' );
echo do_shortcode('<amp-ad width=320 height=100 [test]>');

displays

<amp-ad width=320 height=100 [test]>

Example code to demonstrate shortcode working inside regular tag:

add_shortcode ( 'test', '__return_false' );
echo do_shortcode('<img width=320 height=100 [test]>');exit;

displays

<img width=320 height=100 >

The reason is wp_kses_attr_parse function not supporting tags with hyphens. All AMP tags contain hyphen, e.g. "amp-ad":
​https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml/?format=websites#html-tags

var_dump(wp_kses_attr_parse( '<amp-ad width=320 height=100>' ))

displays

bool(false)

instead of

array(4) {
  [0]=>
  string(7) "<amp-ad "
  [1]=>
  string(10) "width=320 "
  [2]=>
  string(10) "height=100"
  [3]=>
  string(1) ">"
}

The fix is to change function wp_kses_attr_parse( $element ) in wp-includes/kses.php from:

$valid = preg_match( '%^(<\s*)(/\s*)?([a-zA-Z0-9]+\s*)([^>]*)(>?)$%', $element, $matches );

to

$valid = preg_match( '%^(<\s*)(/\s*)?([a-zA-Z0-9\-]+\s*)([^>]*)(>?)$%', $element, $matches );

Change History (8)

#1 @westonruter
6 years ago

  • Component Shortcodes → Formatting
  • Severity critical → normal
  • Version 5.6.1 → 4.7

Seems to be something that was missed in #34105.

#2 @Krstarica
5 years ago

@westonruter any update on this? This a tiny change.

#3 @westonruter
5 years ago

Given the perils of shortcodes and sanitization, I'm not the best to advising on whether this change should be made.

In any case, using a shortcode as an HTML attribute doesn't seem like the best approach. I'd recommend some alternative approach to adding dynamic attributes to custom elements. Even a plain old the_content filter.

#4 @Krstarica
5 years ago

I'm not happy with using shortcode as an attribute, but other approaches are just way too slow, like parsing HTML.

It seems that the real issue here is that function wp_kses_attr_parse() which finds all attributes of an HTML element does not work properly. It assumes hyphen cannot be a part of HTML element.

All AMP tags (e.g. amp-img, amp-video, amp-audio, amp-iframe, amp-form) contain hyphen and all other custom HTML elements:
​https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml/
​https://html.spec.whatwg.org/multipage/custom-elements.html

The consequence is that shortcodes inside such tags do not work.

This ticket was mentioned in ​PR #3642 on ​WordPress/wordpress-develop by ​ivptr.


4 years ago
#5

  • Keywords has-patch added

Trac ticket: 52517

#6 @costdev
4 years ago

Similar to Weston - if there's an alternative approach that you think is feasible, I'd recommend using it.

However, I can verify that this example from the working group link above fails tests:

array(
        'element'  => '<flag-icon country="nl">',
        'expected' => array( '<flag-icon ', 'country="nl"', '>' ),
),

I think this should be supported by wp_kses_attr_parse(), and the above example passes with ​PR 3642.

#7 @Krstarica
3 years ago

According to ​HTML specification a custom element name can include hyphen.

#8 @pawelkmpt
3 years ago

This is not only an AMP problem but general problem of web components as they are required to have - in the tag name.

Note: See TracTickets for help on using tickets.