WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

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

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

Download all attachments as: .zip

Change History (29)

@sunyatasattva
5 years ago

Proposed solution

#1 @sunyatasattva
5 years ago

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

#2 @SergeyBiryukov
5 years ago

  • Version changed from trunk to 3.6

#3 @ocean90
5 years ago

#26355 was marked as a duplicate.

@nofearinc
5 years ago

patch refresh

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

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

#8 @engelen
5 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
5 years ago

  • Keywords 2nd-opinion added

#10 @katzwebdesign
5 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
5 years ago

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

#11 @wonderboymusic
5 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
5 years ago

#12 @kovshenin
5 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
5 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
5 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
5 years ago

  • Keywords 2nd-opinion added

#16 @engelen
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@engelen
5 years ago

#17 @engelen
5 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 5 years ago by engelen (previous) (diff)

#18 in reply to: ↑ 14 @kovshenin
5 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 5 years ago by kovshenin (previous) (diff)

#19 follow-up: @aaroncampbell
5 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
5 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
5 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
5 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
5 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
5 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 5 years ago by SergeyBiryukov (previous) (diff)
Note: See TracTickets for help on using tickets.