#26343 closed defect (bug) (fixed)
has_shortcode not recognizing nested shortcodes
Reported by: | sunyatasattva | Owned by: | wonderboymusic |
---|---|---|---|
Milestone: | 4.0 | Priority: | normal |
Severity: | normal | Version: | 3.6 |
Component: | Shortcodes | Keywords: | has-patch |
Focuses: | Cc: |
Description
The function has_shortcode
doesn't detect nested shortcode, such as:
[block_shortcode][shortcode][/block_shortcode]
As we know, nested shortcodes don't benefit from stellar support in the core, as you have to call do_shortcode()
recursively on the block shortcode's content.
There would be different ways to approach the problem (like changing the RegEx
on the function); I think aiming for a consistent approach with a recursive lookup might be the best idea.
Attachments (5)
Change History (29)
#4
@
11 years ago
Updating the patch due to a missing trailing comma, adding isset
for the shortcode argument and new parentheses syntax for short if
conditionals.
#5
@
11 years ago
Thank you!
I am not sure, but off the top of my head I would ask: isn't shortcode[5]
always going to be set? Perhaps !empty
would be better? Perhaps I am wrong, I just remember that it was set when I was debugging the patch.
#6
@
11 years ago
Given the voodoo magic within the shortcode regex I wouldn't blindly rely on that assumption, but it's just my suggestion :)
#8
@
10 years ago
Nested shortcodes aren't supported by core by default. As stated in the ticket description, one has to call do_shortcode
in the shortcode contents themselves. As such, shortcode content should actually be considered as "raw" content: it's all parsed by the shortcode, and not handled outside of that.
Therefore, one could argue that has_shortcode
shouldn't return true for nested shortcodes. That's my belief, at least. I think the function works as it should currently, and that no support for nested shortcodes should be offered as long as core doesn't start parsing shortcodes bottom-up (i.e. first parse the most deeply nested shortcode, moving a level up each iteration).
#10
@
10 years ago
I just ran into this issue, and I'd love to see it fixed. I do see the point that @engelen is making, so here's an idea: add a third parameter to the has_shortcode()
function: $recursive
.
That would allow for the default to stay "pure". The patch would then read:
} elseif ( $recursive && isset( $shortcode[5] ) && has_shortcode( $shortcode[5], $tag ) ) {
Thoughts?
#11
@
10 years ago
- Owner set to wonderboymusic
- Resolution set to fixed
- Status changed from new to closed
In 29197:
#12
@
10 years ago
- Keywords needs-unit-tests 2nd-opinion removed
- Resolution fixed deleted
- Status changed from closed to reopened
Two things regarding r29197.
First, $shortcode[5]
is always set and sometimes empty, so the isset
check is pretty useless here. Second, since we already call has_shortcode()
in the elseif statement, we can safely return true without running it again. See 26343.2.diff.
#14
follow-up:
↓ 18
@
10 years ago
We can't just implement this like this, it will break existing installs. We will need a $recursive
parameter that's set to false
by default. Backwards compatibility should be maintained, I suggest we do not move ahead with the proposed changeset.
#17
@
10 years ago
26343.3.diff implements a new parameter (boolean $recursive
) for has_shortcode
, and more advanced unit tests that don't rely on the existence of the currently default gallery
shortcode.
#18
in reply to:
↑ 14
@
10 years ago
Replying to engelen: I disagree. The fact that has_shortcode() didn't return true for nested shortcodes is a bug, and the attempt here is to fix a bug, not an enhancement. I don't see a reason why we should implement an argument that would make the function work "half-way" by default.
#19
follow-up:
↓ 20
@
10 years ago
The problem is that has_shortcode()
can now return true for a shortcode that will never be run. Try this:
<?php function test_ticket_26343( $atts, $content = '' ) { return 'Content is: ' . $content; } add_shortcode( 'ticket26343', 'test_ticket_26343' ); add_shortcode( 'ticket26343-2', 'test_ticket_26343' ); $content = "Testing [ticket26343]In one Before [ticket26343-2]In Both[/ticket26343-2] In one After[/ticket26343]\n"; var_dump( has_shortcode( $content, 'ticket26343' ) ); // True before patch and after, always runs var_dump( has_shortcode( $content, 'ticket26343-2' ) ); // False before patch, true after, never runs var_dump( do_shortcode( $content ) ); $content = "Testing again [ticket26343-2]In one Before [ticket26343]In Both[/ticket26343] In one After[/ticket26343-2]\n"; var_dump( has_shortcode( $content, 'ticket26343' ) ); // True before patch and after, always runs var_dump( has_shortcode( $content, 'ticket26343-2' ) ); // False before patch, true after, never runs var_dump( do_shortcode( $content ) );
It's the second one in both of those tests that's the problem. The shortcode never runs, but *is* in the content. Should has_shortcode()
return true or false in that case?
#20
in reply to:
↑ 19
@
10 years ago
Replying to aaroncampbell: I understand. I think whether the shortcode will actually be executed or not is a slightly different matter. The has_shortcode() function checks wether a specific shortcode exists within some content.
So does [foo]
exist within [bar][foo][/bar]
? Yes it does. Will it be executed? Who cares :) The fact that it doesn't (by default) is also a problem, and calling do_shortcode()
within a shortcode callback is just a suggested workaround. Probably related: #10702.
#21
@
10 years ago
You think so? I think the use case is usually something like "if the shortcode exists, load resources to handle it" in which case you care whether it will run not whether the string exists. Having said that, I also agree that it would be better if shortcodes were recursive by default, but since they aren't should this be?
#22
@
10 years ago
Thanks @aaroncampbell, you've voiced my opinion perfectly. has_shortcode
is not intended as a function for a simple string match on a content string. It's intended for checking whether a piece of content contains a shortcode that gets executed. Nested shortcodes aren't executed by WordPress core, because the raw content between shortcode opening and closing tags is handled by the shortcode's callback function. Thus, has_shortcode
should not look for nested shortcodes by default.
However, this is purely theoretical: this isn't a bug, it's intended behaviour (otherwise, the function wouldn't fully parse the content for shortcodes in the same way do_shortcode
does), and we thus have to maintain backwards compatibility. Therefore, $recursive
must be false
by default.
#23
@
10 years ago
- Keywords 2nd-opinion removed
- Resolution set to fixed
- Status changed from reopened to closed
has_shortcode()
was written by me during 3.6, specifically to identify an arbitrary shortcode in a blob of text, nested or not. The fact that it didn't return nested shortcodes was a mistake, a bug.
#24
@
10 years ago
Interesting, I didn't know that... For reference: https://core.trac.wordpress.org/ticket/23282. It's funny to see that this issue never came up in that discussion, even though has_shortcode
was an important part of it.
However, even though your intention was to make it simply check for a shortcode in a piece of content, I'm not sure that your intention was the correct behaviour for has_shortcode
. It seems to me that it currently works as would be expected from has_shortcode
, checking only for unnested shortcodes. An interesting discussion...
Besides all that, focusing on moving ahead with this issue: can we agree that, regardless whether this is a bug or not, $recursive
should be false
by default to maintain backwards compatibility?
Proposed solution