Self closing shortcodes

[rb-cohen]

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.

[rb-cohen]

Patch for wp-includes/shortcodes.php

[Viper007Bond]

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.

[DD32]

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)

[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.

[rb-cohen]

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.

[Denis-de-Bernardy]

fixing this might fix #8553 as a bonus.

[Denis-de-Bernardy]

would be sweet if this got committed in 2.8

[hakre]

+1 for that.

[ryan]

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

[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.

[rb-cohen]

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."

[Denis-de-Bernardy]

Not sure if that helps?

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

[Denis-de-Bernardy]

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

[Denis-de-Bernardy]

still applies clean

[Denis-de-Bernardy]

see some test data here:

​http://core.trac.wordpress.org/ticket/8553#comment:38

[Denis-de-Bernardy]

our existing shortcode regex also fails on:

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

[Viper007Bond]

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).

[Denis-de-Bernardy]

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.

[Viper007Bond]

Oh, right right.

[Denis-de-Bernardy]

see ​http://core.trac.wordpress.org/attachment/ticket/10082/10082.diff

[sebastien.barre]

+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

[shidouhikari]

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.

[rmccue]

Priority and severity should not be high.

[nacin]

I don't think we

[nacin]

Whoops, started typing there. Just meant to punt.

[rb-cohen]

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.

[mdawaffe]

[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.

[Viper007Bond]

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.