WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 6 years ago

Last modified 6 years ago

#9264 closed defect (bug) (fixed)

Self closing shortcodes

Reported by: rb-cohen Owned by: westi
Milestone: 3.3 Priority: normal
Severity: normal Version: 2.7.1
Component: Shortcodes Keywords: needs-unit-tests
Focuses: Cc:

Description

First bug report, be gentle.

I've noticed that the shortcode regex doesn't take note of the self closing "/" properly, and continues to search for a closing tag within the content passed to it.

I think it should stop looking for a closing tag if the tag is self closing. See the following example:

[test id="1"/] first self closed, now [test id="2"]with content[/test]

This gets sent to the shortcode callback function as:

Attributes: id="1" Content: first self closed, now [test id="2"]with content

I've posted some further tests at http://blograndom.com/tests/shortcode/ , to prove the bug exists and my proposed fix (see attached diff file). The fix also offers slight speed enhancements for self closing divs because it stops looking.

It looks like the trunk version of shortcodes.php is slightly different to 2.7.1, but I've applied the patch from the trunk version which still seems to have the same problem.

Attachments (1)

shortcodes.diff (619 bytes) - added by rb-cohen 9 years ago.
Patch for wp-includes/shortcodes.php

Download all attachments as: .zip

Change History (37)

@rb-cohen
9 years ago

Patch for wp-includes/shortcodes.php

#1 @Viper007Bond
9 years ago

I think ideally the slash shouldn't be needed as WordPress should be smart enough to work backwards and balance the tags. However that may not be possible due to technical reasons, I'm not sure.

If it isn't, than using a slash to manually mark a shortcode as self-closing is a fair enough solution. I've run into this problem a few times and it'd be great to have a fix.

#2 @DD32
9 years ago

I think ideally the slash shouldn't be needed as WordPress should be smart enough to work backwards and balance the tags. However that may not be possible due to technical reasons, I'm not sure.

The problem is, Whilst WordPress can work backwards and work without the closing bracket, WordPress still looks for the closing bracket, If you start your post with [TestShortcode] then it'll search to the end of the post for a TetShortCode, not a problem with a small post, but problematic for long posts

(Thats purely based off the reporters description, thats the way i've interpreted it at least)

#3 follow-up: @westi
9 years ago

  • Cc westi added
  • Keywords needs-unit-tests added; shortcode removed

It would be good to see a patch to add these test cases to the shortcode tests:

http://svn.automattic.com/wordpress-tests/wp-testcase/test_shortcode.php

Then we can make sure the code changes satisfy the existing tests as well as these changes.

#4 @rb-cohen
9 years ago

Is this something that I need to do, and if so, is there any information on how I can add a patch to the test cases? I did a quick search and couldn't find much information.

#5 @Denis-de-Bernardy
9 years ago

  • Milestone changed from Unassigned to 2.8

#6 @ryan
9 years ago

  • Component changed from General to Shortcodes
  • Owner anonymous deleted

#7 @Denis-de-Bernardy
9 years ago

fixing this might fix #8553 as a bonus.

#8 @Denis-de-Bernardy
9 years ago

  • Keywords has-patch added

#9 @westi
9 years ago

  • Owner set to westi
  • Status changed from new to assigned

#10 @Denis-de-Bernardy
9 years ago

would be sweet if this got committed in 2.8

#11 @hakre
9 years ago

+1 for that.

#12 @ryan
9 years ago

Asked tellyworth for some feedback since he did a lot of the shortcode stuff.

#13 in reply to: ↑ 3 ; follow-up: @Denis-de-Bernardy
9 years ago

Replying to westi:

It would be good to see a patch to add these test cases to the shortcode tests:

http://svn.automattic.com/wordpress-tests/wp-testcase/test_shortcode.php

Then we can make sure the code changes satisfy the existing tests as well as these changes.

It does, provided that the ?(:digit:) works on all platforms. never ran into this one, but if anyone is familiar with it and can confirm it's pcre standard than it should go right in.

my understanding is it stands for "if we've 4 backrefs so far", and that would only be true if there is a /] around.

#14 in reply to: ↑ 13 @rb-cohen
9 years ago

Replying to Denis-de-Bernardy:

Replying to westi:

It would be good to see a patch to add these test cases to the shortcode tests:

http://svn.automattic.com/wordpress-tests/wp-testcase/test_shortcode.php

Then we can make sure the code changes satisfy the existing tests as well as these changes.

It does, provided that the ?(:digit:) works on all platforms. never ran into this one, but if anyone is familiar with it and can confirm it's pcre standard than it should go right in.

my understanding is it stands for "if we've 4 backrefs so far", and that would only be true if there is a /] around.

I should have made a note about what it did. I *think* it's actually a conditional, checking whether group 4 was used and then looking for the end tag only if its not self closing.

According to http://www.regular-expressions.info/conditional.html, conditionals are supported in pcre. Not sure if that helps?

"Conditionals are supported by the JGsoft engine, Perl, PCRE and the .NET framework."

#15 @Denis-de-Bernardy
9 years ago

  • Keywords commit added; needs-unit-tests removed

#16 @Denis-de-Bernardy
9 years ago

Not sure if that helps?

It does. It's probably the information that Westi was missing.

#17 @Denis-de-Bernardy
9 years ago

I'll be happy to look into regressions if this one gets committed in 2.8. I don't expect any, but I can picture a single problematic use-case:

[foo att="bar"]something[/foo]

Depending on how we check for "something" and/or "foo" over in the shortcodes-related functions, we might have null fields where we're expecting something that contains an empty string.

That being said, it's miles more efficient that the current code and it should fix the severe problems outlined over in #8553

#18 @Denis-de-Bernardy
9 years ago

  • Keywords early added
  • Milestone changed from 2.8 to 2.9

#19 @Denis-de-Bernardy
9 years ago

still applies clean

#21 @Denis-de-Bernardy
9 years ago

  • Keywords needs-patch added; has-patch commit removed

#22 follow-up: @Denis-de-Bernardy
9 years ago

our existing shortcode regex also fails on:

[foo attr/]bar[foo attr/]bar[/foo]

#23 in reply to: ↑ 22 @Viper007Bond
9 years ago

Replying to Denis-de-Bernardy:

our existing shortcode regex also fails on:

[foo attr/]bar[foo attr/]bar[/foo]

Because that's very poorly formatted text -- there's an orphaned [/foo] on the end (as you have two self-closing tags before it).

#24 @Denis-de-Bernardy
9 years ago

Indeed, and for good reason since it's test data. it's a test case that's supposed to be processed properly, i.e. two shortcodes rather than a single one. see the php test file I attached to #10082.

#25 @Viper007Bond
9 years ago

Oh, right right.

#27 @sebastien.barre
9 years ago

+1 / I second that. Even with the diff above, it seems to persist. I'm surprised it's not affecting more people; within the same page, you can't have the same shortcode with and without content.

Example/Test:

function shortcode_test($atts, $content=null) {
  return '<p>Shortcode Test Content: {' . $content . '}</p>';
}
add_shortcode('test', 'shortcode_test');

Page:

[test/]
[test]foobar[/test]

Output:

Shortcode Test Content: {
[test]foobar}

i.e. the first short code completely swallowed the first half of the second shortcode. Is the regexp set to be "greedy"?

Thanks

#28 @ryan
9 years ago

  • Milestone changed from 2.9 to 3.0

#29 @shidouhikari
8 years ago

  • Cc shidouhikari added
  • Priority changed from normal to high
  • Severity changed from normal to major

Shortcode parsing also fails for

[test id="1"][/test]

It doesn't detect the closing test and keeps looking for it.

That's even worse when a shortcode uses $content but also accepts it empty so that default value is used.

The solution I found in my plugin was use

[test id="1"]{{empty}}[/test]

Ugly but at least worked.

#30 @rmccue
8 years ago

  • Priority changed from high to normal
  • Severity changed from major to normal

Priority and severity should not be high.

#31 @nacin
8 years ago

  • Keywords needs-unit-tests added; early removed
  • Milestone changed from 3.0 to Future Release

I don't think we

#32 @nacin
8 years ago

Whoops, started typing there. Just meant to punt.

#33 @rb-cohen
7 years ago

Is there anywhere we can track the future plans for shortcode parsing in wordpress? I see a number of shortcode bugs in trac all set to "Future release" but some seem trivial to fix.

Is it worth posting patches for the more trivial ones if the patch will pass these tests http://svn.automattic.com/wordpress-tests/wp-testcase/test_shortcode.php ?

With both [test/][test]content[\/test] and [test][\/test][test]content[\/test] broken it seems impossible to enter a shortcode with null content before another instance of the same shortcode tag.

#34 @mdawaffe
7 years ago

  • Keywords needs-patch removed
[test id="1"/] first self closed, now [test id="2"]with content[/test]
[foo attr/]bar[foo attr/]bar[/foo]
[test/]
[test]foobar[/test]
[test id="1"][/test]

All these cases now work. Apart from unit tests, this ticket can be closed.

#35 @Viper007Bond
6 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

Looks like many of these have unit tests. We can worry about that over on the unit test Trac. No progress here in a year.

This ticket was probably fixed in [18952] by the way.

#36 @SergeyBiryukov
6 years ago

  • Milestone changed from Future Release to 3.3
Note: See TracTickets for help on using tickets.