Make WordPress Core

Opened 3 years ago

Last modified 11 months ago

#52517 new defect (bug)

Shortcodes inside AMP tags do not work

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

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
3 years ago

  • Component changed from Shortcodes to Formatting
  • Severity changed from critical to normal
  • Version changed from 5.6.1 to 4.7

Seems to be something that was missed in #34105.

#2 @Krstarica
2 years ago

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

#3 @westonruter
2 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
2 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.


17 months ago
#5

  • Keywords has-patch added

Trac ticket: 52517

#6 @costdev
17 months 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
12 months ago

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

#8 @pawelkmpt
11 months 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.