#10082 closed defect (bug) (fixed)
Shortcodes need a character separating them to work
Reported by: |
|
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)
Change History (60)
#3
@
16 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
@
16 years ago
- Owner set to petervanderdoes
- Status changed from new to accepted
Uploaded a patch that fixes it.
#5
follow-up:
↓ 6
@
16 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
@
16 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?
#7
@
16 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
@
16 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
@
16 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
@
16 years ago
plkavida: that "fix" isn't one. you've merely disabled shortcode escaping (which is fine if you're not using it).
#14
@
16 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.
#16
@
16 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
@
16 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
@
16 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
@
16 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
@
16 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
@
16 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
@
16 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
@
16 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
@
16 years ago
I can even give example of a site where none of the shortcodes are working - not even one!
#25
@
16 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
#26
follow-ups:
↓ 30
↓ 32
@
16 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.
#27
@
16 years ago
Here are some of my posts where it does not work on 2.8.2: http://www.freshfonts.info/acme-secret-agent-font/ and http://www.freshfonts.info/aarvark-cafe-font/
#29
@
16 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
@
16 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:
↓ 33
@
16 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.
#33
in reply to:
↑ 31
@
16 years ago
Replying to godfreykfc:
Apparently that has already been discussed above, but what's the conclusion?
#34
follow-up:
↓ 35
@
16 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
@
16 years ago
Replying to dd32:
I know this is a noob question, but how do I apply the patch? :)
#36
@
16 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)
#38
@
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
@
15 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/
#43
@
15 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.
#45
@
15 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
@
15 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
#48
@
15 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
@
14 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
@
12 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].
#53
@
11 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.
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.