Make WordPress Core

Opened 10 years ago

Last modified 5 years ago

#26674 new defect (bug)

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

Reported by: kopepasah's profile kopepasah Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.9
Component: Formatting Keywords: has-patch needs-testing needs-refresh
Focuses: Cc:

Description

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.

Attachments (3)

get-tag-regex.diff (456 bytes) - added by kopepasah 10 years ago.
unit-test.patch (1.0 KB) - added by kopepasah 10 years ago.
26674.patch (1.4 KB) - added by Kopepasah 10 years ago.

Download all attachments as: .zip

Change History (15)

#1 @kopepasah
10 years ago

  • Cc justin@… added
  • Keywords has-patch needs-testing added

#2 @ocean90
10 years ago

  • Keywords needs-unit-tests added; needs-testing removed

#3 follow-up: @kopepasah
10 years ago

  • Keywords needs-testing added; needs-unit-tests removed

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

#4 in reply to: ↑ 3 @kopepasah
10 years ago

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

#5 @kopepasah
10 years ago

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

#6 @Kopepasah
10 years ago

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

This ticket was mentioned in IRC in #wordpress-dev by kopepasah. View the logs.


10 years ago

#8 @nacin
10 years ago

  • Milestone changed from Awaiting Review to Future Release

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

#9 @Kopepasah
10 years ago

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.

This ticket was mentioned in IRC in #wordpress-dev by kopepasah. View the logs.


10 years ago

@Kopepasah
10 years ago

#11 @afercia
10 years ago

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.

#12 @chriscct7
8 years ago

  • Keywords needs-refresh added
Note: See TracTickets for help on using tickets.