Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#26343 closed defect (bug) (fixed)

has_shortcode not recognizing nested shortcodes

Reported by: sunyatasattva's profile sunyatasattva Owned by: wonderboymusic's profile 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)

has_shortcode.patch (457 bytes) - added by sunyatasattva 10 years ago.
Proposed solution
26343.diff (496 bytes) - added by nofearinc 10 years ago.
patch refresh
recursive-has_shortcodes.diff (528 bytes) - added by katzwebdesign 10 years ago.
Truly recursive has_shortcode(): the previous patches assumed 1 level deep.
26343.2.diff (571 bytes) - added by kovshenin 10 years ago.
26343.3.diff (2.4 KB) - added by engelen 10 years ago.

Download all attachments as: .zip

Change History (29)

@sunyatasattva
10 years ago

Proposed solution

#1 @sunyatasattva
10 years ago

  • Component changed from General to Shortcodes
  • Keywords has-patch added

#2 @SergeyBiryukov
10 years ago

  • Version changed from trunk to 3.6

#3 @ocean90
10 years ago

#26355 was marked as a duplicate.

@nofearinc
10 years ago

patch refresh

#4 @nofearinc
10 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 @sunyatasattva
10 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 @nofearinc
10 years ago

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

#7 @wonderboymusic
10 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.0

#8 @engelen
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).

#9 @engelen
10 years ago

  • Keywords 2nd-opinion added

#10 @katzwebdesign
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?

@katzwebdesign
10 years ago

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

#11 @wonderboymusic
10 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 29197:

Make has_shortcode() recursive/work for nested shortcodes.

Adds unit test.

Props katzwebdesign.
Fixes #26343.

@kovshenin
10 years ago

#12 @kovshenin
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.

#13 @wonderboymusic
10 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 29207:

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

Props kovshenin.
Fixes #26343.

#14 follow-up: @engelen
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.

#15 @engelen
10 years ago

  • Keywords 2nd-opinion added

#16 @engelen
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@engelen
10 years ago

#17 @engelen
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.

Last edited 10 years ago by engelen (previous) (diff)

#18 in reply to: ↑ 14 @kovshenin
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.

Last edited 10 years ago by kovshenin (previous) (diff)

#19 follow-up: @aaroncampbell
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 @kovshenin
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 @aaroncampbell
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 @engelen
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 @wonderboymusic
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 @engelen
10 years ago

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?

Last edited 10 years ago by SergeyBiryukov (previous) (diff)
Note: See TracTickets for help on using tickets.