has_shortcode not recognizing nested shortcodes

[sunyatasattva]

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.

[sunyatasattva]

Proposed solution

[ocean90]

#26355 was marked as a duplicate.

[nofearinc]

patch refresh

[nofearinc]

Updating the patch due to a missing trailing comma, adding isset for the shortcode argument and new parentheses syntax for short if conditionals.

[sunyatasattva]

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.

[nofearinc]

Given the ​voodoo magic within the shortcode regex I wouldn't blindly rely on that assumption, but it's just my suggestion :)

[engelen]

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

[katzwebdesign]

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?

[katzwebdesign]

Truly recursive has_shortcode(): the previous patches assumed 1 level deep.

[wonderboymusic]

In 29197:

Make has_shortcode() recursive/work for nested shortcodes.

Adds unit test.

Props katzwebdesign.
Fixes #26343.

[kovshenin]

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

[wonderboymusic]

In 29207:

After [29197], use ! empty() instead of isset(). Don't call has_shortcode() internally twice if it's true.

Props kovshenin.
Fixes #26343.

[engelen]

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.

[engelen]

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.

[kovshenin]

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.

[aaroncampbell]

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?

[kovshenin]

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.

[aaroncampbell]

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?

[engelen]

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.

[wonderboymusic]

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.

[engelen]

Interesting, I didn't know that... For reference: #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?