Make WordPress Core

Opened 15 years ago

Closed 11 years ago

Last modified 10 years ago

#10082 closed defect (bug) (fixed)

Shortcodes need a character separating them to work

Reported by: denis-de-bernardy's profile Denis-de-Bernardy Owned by:
Milestone: 3.3 Priority: high
Severity: major Version: 2.8
Component: Shortcodes Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

the following works:

[media id="448"]test[/media]
[media id="448"]test[/media]

the following doesn't:

[media id="448"]test[/media][media id="448"]test[/media]

Attachments (7)

patch-#10082-1.diff (542 bytes) - added by petervanderdoes 15 years ago.
shortcode-tests.php (2.8 KB) - added by Denis-de-Bernardy 15 years ago.
shortcode-tests.2.php (6.7 KB) - added by Denis-de-Bernardy 15 years ago.
10082.diff (2.0 KB) - added by Denis-de-Bernardy 15 years ago.
10082.2.diff (2.1 KB) - added by dd32 15 years ago.
10082.3.diff (2.2 KB) - added by dd32 15 years ago.
ut_10082.diff (1.1 KB) - added by mdbitz 10 years ago.
PHPUnit Test Cases for validition

Download all attachments as: .zip

Change History (60)

#1 @GamerZ
15 years ago

Hi Denis,

I think in WP 2.8, the parsing of the Shortcode is acting weird for me as well. For example in WP2.7, I have the following shortcode in a page.

[box1][page_useronline][box_footer]

All the 3 shortcode will get parsed, but for WP2.8, I have to change it to:

[box1]
[page_useronline]
[box_footer]

Each shortcode must be on its own line before it gets parsed.

#2 @navjotjsingh
15 years ago

  • Cc navjotjsingh@… added
  • Milestone changed from 2.9 to 2.8.1

#3 @Denis-de-Bernardy
15 years ago

  • Severity changed from normal to critical

Gamerz - I'm pretty certain it's related to the newly introduced escaping methods. Marking this as blocking for 2.8.1, as it's a pretty major regression.

#4 @petervanderdoes
15 years ago

  • Owner set to petervanderdoes
  • Status changed from new to accepted

Uploaded a patch that fixes it.

#5 follow-up: @Denis-de-Bernardy
15 years ago

might this work too?

return '(\[?)\[('.$tagregexp.')\b(.*?)(?:(\/))?\](?:(.+?)\[\/\2\])?(\]?)';

our only interest are things like this, for escaping purposes:

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

#6 in reply to: ↑ 5 @petervanderdoes
15 years ago

Replying to Denis-de-Bernardy:

That will fail something like:
[foo][foo]barfoo or
[foo]This is not part of shortcode[foo]barbar

patch-#10082-1.diff will handle the
foo?
foo]bar[/bar?

and
foo?foo]bar[/foo?

#7 @petervanderdoes
15 years ago

Replying to Denis-de-Bernardy:
Forgot the code stuff (grr)
That will fail something like:

[foo][foo]bar[/foo]

or

[foo]This is not part of shortcode[foo]bar[/bar]

patch-#10082-1.diff will handle the

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

and

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

#8 @Denis-de-Bernardy
15 years ago

petervanderdoes - I'm quite aware. I was mostly curious to know if using \and \? worked too, since it would be more efficient than a dot or a class.

#9 @plakadiva
15 years ago

Hallo, maybe this could help: I patched wp-includes/shortcodes.php and my shortcodes worked fine after that.

change line 178 from

return '(.?)\[('.$tagregexp.')\b(.*?)(?:(\/))?\](?:(.+?)\[\/\2\])?(.?)';

to:

return '(.*?)\[('.$tagregexp.')\b(.*?)(?:(\/))?\](?:(.+?)\[\/\2\])?(.*?)';

So the only change was the amount of strings before and after shortcodes. I´m not sure if that´s a complete solution but it worked well with the problems discribed by GamerZ

#10 @Denis-de-Bernardy
15 years ago

plkavida: that "fix" isn't one. you've merely disabled shortcode escaping (which is fine if you're not using it).

#11 @Denis-de-Bernardy
15 years ago

  • Keywords needs-patch added

#12 @Denis-de-Bernardy
15 years ago

Did some tests, and the only one that clearly completely fixes this is to change the initial and final (.?) and (.?) into (\and (\?) respectively.

But working on a patch for #10082, #9264 and #8553 all in one go.

#13 @Denis-de-Bernardy
15 years ago

the regexp highlighted in the attached file passes all 1152 tests.

#14 @Denis-de-Bernardy
15 years ago

Added the full code I used for tests, including the bad regexp'. For reference, the current shortcode regexp in WP completes only 472 tests, out of 1152.

#15 @Denis-de-Bernardy
15 years ago

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

#16 @hakre
15 years ago

With all those shortcode problems I'm just wondering if there does not exist some code already in the PHP-world that has solved this just simply and working? It feels pretty much like re-inventing the wheel. see #8553 .

#17 @westi
15 years ago

  • Keywords tested removed

Current patch does not pass all the current shortcode tests in wordpress-tests

peter-westwoods-computer:wordpress-tests peterwestwood$ php wp-test.php -t TestShortcode
Test Suite
 TestShortcode
 ...............F...SSSSSS

Time: 3 seconds

There was 1 failure:

1) test_nested_tags(TestShortcode)
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ -1,3 +1,4 @@
 content = abc = foo
 def = 123
 0 = http://wordpress.com
+[/baztag]
\ No newline at end of file

/Users/peterwestwood/Documents/workspace/wordpress-tests/wp-testcase/test_shortcode.php:167
/Users/peterwestwood/Documents/workspace/wordpress-tests/wp-testlib/base.php:526
/Users/peterwestwood/Documents/workspace/wordpress-tests/wp-test.php:107

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

#18 @Denis-de-Bernardy
15 years ago

Err... are you 100% that the test is checking against the expected behavior though?

recall that we made a slight change to the behavior -- it's now non-greedy.

#19 @westi
15 years ago

Those tests are the specification tests for shortcode behaviour so any regression against them needs heavy justification.

This particular test is for: [baztag][dumptag abc="foo" def=123 http://wordpress.com/][/baztag]

You need to run tests against do_shortcode as the wordpress-tests tests do not just on the regex.

#20 @Denis-de-Bernardy
15 years ago

Err, that one is precisely the one we had discussed in IRC, and the conclusion was that we'd want baztag processed as [baztag/]... :-|

#21 @westi
15 years ago

The discussion I recall was more to do unmatched shortcodes of the same type.

This is example is completely valid nested shortcodes and something we should still support.

#22 @Denis-de-Bernardy
15 years ago

no no, it was rather explicitly about how to deal with nested shortcodes. :-)

I'll change the patch accordingly on monday.

#23 in reply to: ↑ description @godfreykfc
15 years ago

Opened ticket #10473, not exactly the same problem, but would probably be of interest for those who have commented on and contributed to this ticket.

#24 @navjotjsingh
15 years ago

I can even give example of a site where none of the shortcodes are working - not even one!

@dd32
15 years ago

#25 @dd32
15 years ago

attachment 10082.2.diff added

  • Main changes from denis's patch:
  • Switches from using \b to a custom assertion so as to allow shortcodes with - in them
  • removes $begin / $end from the return, as its not needed since they only match []'s, which is short-circuited earlier in the function anyway
  • + => * there somewhere, to allow for [tag]tag (previously, it would just not match tag with an empty content, leaving a closing shortcode in the post)

Just looking into the still-valid nested shortcodes now though.. which this patch might not help either

@dd32
15 years ago

#26 follow-ups: @dd32
15 years ago

attachment 10082.3.diff added

  • I couldnt see any other way of doing nested shortcodes with the suggested regex of denis's, But I cant see a better way of doing that regex either, Its exactly what i was thinking of changing it to (well close enough i'd have got there)
  • Strips out the 2nd use of $tagregex in the regex instead only stopping on the closing tag of the CURRENTLY MATCHED shortcode
  • ie. [1][2]....21 1 will only match a closing 1 not the closing 2 (Which results in a 1 left in the post content otherwise)

This still needs testing, I've not attempted running the test set yet, other than the test-data posts i've just setup.

#28 @dd32
15 years ago

@navjotjsingh: Without knowing the full raw post content, bit hard to help..

#29 @navjotjsingh
15 years ago

The content of the post seen is the exact content which I posted. Posted for reference:

[freshfont id=acmesa name="A.C.M.E. Secret Agent Font"]
<h4>Included Fonts:</h4>
<h4>A.C.M.E. Secret Agent Italic Font</h4>
[freshfont id=acmesai name="A.C.M.E. Secret Agent Italic Font"]
<h4>A.C.M.E. Secret Agent Bold Font</h4>
[freshfont name="A.C.M.E. Secret Agent Bold Font" id=acmesab]
<h4>Download</h4>
[download id="11,12"]
[freshfont id=AARVC___ name="Aarvark Cafe Font"][/freshfont]

 [download id="1,2"]

#30 in reply to: ↑ 26 @Denis-de-Bernardy
15 years ago

Replying to dd32:

I've not attempted running the test set yet, other than the test-data posts i've just setup.

don't worry about it. the test set is built assuming that nested shortcodes shouldn't work at all. so they'd be invalid. :-)

#31 follow-up: @godfreykfc
15 years ago

If we are supporting nested shortcode, then we'll have to deal with situations like these:

[shortcode]
[shortcode]...[/shortcode]

If nesting is not supported, then this should clearly be interpreted as an (implied) self-closing [shortcode /] followed by a [shortcode]...[/shortcode]. However, if nesting is permitted, then it can also be interpreted as [shortcode] [/shortcode] pair wrapping around the content [shortcode /].... IMO, either interpretation is fine, but it should be clearly defined (and test cases should be written) and we should make sure that interpretation won't change from implementation to implementation. We should also deprecate the implied self-closing syntax and encourage people to start using the explict [shortcode /] syntax to avoid ambiguity.

By the way, as pointed out in #10473, this is not working in the current release at all, so please make sure it is fixed by the submitted patch.

#32 in reply to: ↑ 26 @godfreykfc
15 years ago

Replying to dd32:

dd32 can you post some short instructions for testing your patch? I've got a handful of test cases from #10473 for you.

#33 in reply to: ↑ 31 @godfreykfc
15 years ago

Replying to godfreykfc:

Apparently that has already been discussed above, but what's the conclusion?

#34 follow-up: @dd32
15 years ago

dd32 can you post some short instructions for testing your patch? I've got a handful of test cases from #10473 for you.

Run all the test cases you can think of. I dont have any specifically handy for it..

Heres some I can think of though which work in mine, but (may) not in denis's:

[s1][/s1]
[s1]Content[/s1]
[s1][s2][/s1]
[s1][s2][/s2]

#35 in reply to: ↑ 34 @godfreykfc
15 years ago

Replying to dd32:

I know this is a noob question, but how do I apply the patch? :)

#36 @dd32
15 years ago

I know this is a noob question, but how do I apply the patch? :)

Oh.. :)

Have a look at http://codex.wordpress.org/Using_Subversion http://wordpress.org/download/svn/ and TortoiseSVN (If using Windows)

#37 @godfreykfc
15 years ago

Ah. Got it. the patch command is the missing piece. Thanks :)

#38 @hakre
15 years ago

Wouldn't it be possible to make shortcode's simpler by definition. to just document how they work, then we can write the testcases that must work and must fail and then we can properly patch for now and in the future. if some users did shortcode based on a guess, then they should be flexible enough to lookup a then existing documentation of the correct usage and will have fixed their stuff in no time.

#39 @alanjohndix
14 years ago

  • Cc alanjohndix added

I missed this thread and submitted a separate bug report with a patch http://core.trac.wordpress.org/ticket/10490

The patch seems to cover all the cases discussed.

A core problem is the back reference "\2" in the regular expression which which changes its meaning when used in a different context by wpautop.

I also describe it in a short blog as I couldn't work out how to get some things to format in the comment box.

http://www.alandix.com/blog/2009/07/26/fix-for-wordpress-shortcode-bug/

#40 @gcorne
14 years ago

  • Cc gcorne added

#42 @ryan
14 years ago

  • Severity changed from critical to major

#43 @westi
14 years ago

  • Milestone changed from 2.9 to 3.0

Moving to 3.0

We can't be changing the shortcode regex this close to release.

#44 @TobiasBg
14 years ago

  • Priority changed from normal to high

Wouldn't it be worth to try and commit the new regex now, to get some testing in?

Justing asking, because #11675 and #11674 show that apparantly there is a need.
Thus raising the priority a little bit, so that it doesn't have to be punted again. Sorry, if that is wrong!

#45 @plakadiva
14 years ago

I installed the WP3.0Beta1 and the bug discribed above is still there. I don´t want to complain, just would like to know if there are "real" plans to fix this bug and the p-tag bugs (http://www.alandix.com/blog/2009/07/26/fix-for-wordpress-shortcode-bug/) in the shotcode-API. I had to edit a lot of pages/posts because of those bugs and now we are planning a relaunch, so we would like to know if the shortcodes have "no Future" or something like that?

#46 @dd32
14 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from 3.0 to 3.1

Unfortunately its too late in the dev cycle for this to be commited.

Some serious unit tests are needed for both shortcodes and wpautop

#47 @dd32
14 years ago

  • Owner petervanderdoes deleted
  • Status changed from accepted to assigned

#48 @mattsains
14 years ago

Maybe this is a bit late to suggest, but what about requiring a tag that doesn't have a matching closing tag to be self closing (as in XML), like [foo /][foo]Hellofoo

It would solve the problem of not knowing whether the tag is enclosing something.

#49 @nacin
13 years ago

  • Milestone changed from Awaiting Triage to Future Release
  • Summary changed from shortcode bug to Shortcodes need a character separating them to work

Closing #14557 as a duplicate, giving this one a descriptive title, and moving to future.

#50 @Viper007Bond
11 years ago

  • Milestone Future Release deleted
  • Resolution set to worksforme
  • Status changed from assigned to closed

This works fine for me. It must have been resolved when the shortcode regex was rewritten in [18952].

#51 @Viper007Bond
11 years ago

  • Resolution changed from worksforme to fixed

#52 @SergeyBiryukov
11 years ago

  • Milestone set to 3.3

@mdbitz
10 years ago

PHPUnit Test Cases for validition

#53 @mdbitz
10 years ago

Hi, first time contributing so let me know if I should do this in a different manner. I've attached ut_10082.diff which contains some test cases for validating that shortcode tags without spacing are correctly parsed.

I'll be looking through other tickets in the "need test cases" bucket so please let me know if I should include the test cases differently and feedback is appreciated.

Note: See TracTickets for help on using tickets.