The get_tag_regex() function is a too greedy when searching for a closing tag.

[kopepasah]

At its current state, the get_tag_regex() function is a too greedy when searching for a closing tag. This causes content with more than one of the same media tag (e.g iframe followed by an iframe) to be grouped together as one value, with the content between in that value.

This patch, while lazier, makes the regex function as expected.

[kopepasah]

The attached unit test is a patch for the trunk/media.php unit test. The original test only looked for one occurrence of a type in the content. This tests for multiple occurrences. The test_get_media_embedded_in_content() test runs get_tag_regex().

[kopepasah]

Never mind that unit test patch. It needs to be rewritten for enhancements added by ​#26675. However, the ​current unit test for get_media_embedded_in_content() does test the prowess of get_tag_regex().

[kopepasah]

UPDATE: Unit test has been added to ​get_media_embed_in_content() patch.

[Kopepasah]

I may have the terms 'greedy' and 'lazy' backwards regarding my title and description above. Please correct me if I am wrong.

[nacin]

Hi kopepasah, are there any other changes required here, given #26675? Or is the initial patch still the suggested one?

[Kopepasah]

Hey Nacin, sorry for the delayed reply. Just moved to Korea and finally getting settled in.

The initial patch for this ticket is still my suggestion. I am going to make some changes on #26675 based on your suggestions, but that does not affect how get_tag_regex() should work.

[afercia]

hi,
playing with the ​Singl theme today I noticed the mess when there are multiple embeds of the same type, and came to a patch very similar to @Kopepasah one. I've seen also related #26675.
Just one thing about the regex: self-closing embed tags may or may not have a trailing space and slash so I would just change the last part with this

|\s*?\/?>

where first ? makes the space quantifier lazy and second ? makes the slash optional.
The unit test should also be updated to test for a self-closing tag without space nor slash.

[r1k0]

The patch fails to apply. Testing is not necessary at this time, removed needs-testing and a refresh is more appropriate.

Error:

Running "patch:https://core.trac.wordpress.org/attachment/ticket/26674/26674.patch" (patch) task
patching file wp-includes/functions.php
Hunk #1 FAILED at 4116.
1 out of 1 hunk FAILED -- saving rejects to file wp-includes/functions.php.rej
can't find file to patch at input line 21
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
| 
|
|
|Index: tests/phpunit/tests/media.php
|===================================================================
|--- tests/phpunit/tests/media.php	(revision 26862)
|+++ tests/phpunit/tests/media.php	(working copy)
--------------------------
File to patch: 

## What

get_tag_regex() builds a pattern with a greedy body match:

<iframe[^<]*(?:>[\s\S]*<\/iframe>|\s*\/>)

Because [\s\S]* is greedy, content with more than one tag of the same type matches from the first opening tag all the way to the last closing tag, so both tags — and everything between them — collapse into a single match:

$content = '<iframe src="a"></iframe> text <iframe src="b"></iframe>';
preg_match_all( '#' . get_tag_regex( 'iframe' ) . '#', $content, $m );
// $m[0] currently has ONE entry containing both iframes and the text between.

## Fix

Make the attribute and body matches lazy:

<iframe[^<]*?(?:>[\s\S]*?<\/iframe>|\s*\/>)

This is the same pattern core already uses inline in get_media_embedded_in_content() ([^<]*? … [\s\S]*?), so this really just brings the standalone get_tag_regex() back in line with it. With the change, the two iframes above match as two separate entries, and single-tag, multiline-body, and self-closing cases keep matching as before.

get_tag_regex() has no callers in core today (get_media_embedded_in_content() carries its own copy of the pattern), but it's a public function since 3.6.0, so the greedy behavior affects anything outside core that relies on it.

## Tests

get_tag_regex() had no test coverage, so this adds tests/phpunit/tests/functions/getTagRegex.php covering a single tag with a body, two adjacent tags matched separately (the regression), a self-closing tag, a multiline body, and the no-match and empty-tag cases. The two-adjacent-tags case fails against the current greedy pattern and passes with the fix.

## Notes

This refreshes the long-stale patch on the ticket (it no longer applied against trunk). I kept the self-closing branch as \s*\/> to match the pattern in get_media_embedded_in_content(); @afercia's earlier suggestion to also make the slash optional (\s*?\/?>) would broaden what counts as self-closing, so that feels like a separate decision from the greedy fix and I left it out here.

Props @kopepasah for the report and original patch, and @afercia for the regex review.

[gunjanjaswal]

Refreshed against trunk as a PR: ​https://github.com/WordPress/wordpress-develop/pull/13037

The greedy body match ([\s\S]*) meant content with more than one tag of the same type (e.g. two iframes) collapsed from the first opening tag to the last closing tag into a single match. The PR makes the attribute and body matches lazy, which is the same pattern get_media_embedded_in_content() already uses inline, so it just brings get_tag_regex() back in line with it.

I also added unit tests for get_tag_regex(), which had none before, including the two-adjacent-tags case that fails on the current pattern. Ready for testing.

[afercia]

Thank you @gunjanjaswal for your PR.
My original comment (from 12 years ago!) about self-closing tags was incorrect because an <iframe> tag can't be self-closing.

However, the regex should be tested for self-closing tags, for example an image or an input, with and without the space before the last slash:

<input type="text" />
<input type="text"/>

The functiton documentation explicitly mentions self-closing tags,

[gunjanjaswal]

Thanks @afercia! Good catch on the iframe — I've swapped the self-closing case to an <input> and now cover both <input type="text" /> and <input type="text"/>, with and without the trailing space before the slash. Pushed to the PR.

[afercia]

Noting that the linked PR would close also ticket #59791 so that props should be attributed also to the contributors on that ticket / PR.

[afercia]

get_tag_regex() has no callers in core today (get_media_embedded_in_content() carries its own copy of the pattern) ...

For history, after some software archeology, get_tag_regex() was introduced in [23729] and I couldn't find any usage of the function in that commit. It was later used internally in get_media_embedded_in_content() and removed in [31574]. Today, get_media_embedded_in_content() uses its own regex, which is very similar to this one. and it's lazy. It would be nice to create a new ticket to add tests for get_media_embedded_in_content().

[afercia]

Fixed in [63331], where the commit message missed the # before the ticket number.

Fixed in https://core.trac.wordpress.org/changeset/63331, where the commit message missed the # before the ticket number.

Thanks so much, @afercia — great to see this land in core (r63331), and I appreciate the careful review along the way, especially the push to test against realistic embedded HTML. Glad the 12-year-old ticket is finally closed.