Make WordPress Core

Opened 7 years ago

Last modified 15 months ago

#6297 reopened defect (bug)

Unbalanced tags across more and nextpage tags

Reported by: tellyworth Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.5
Component: Formatting Keywords: has-patch
Focuses: Cc:


It's easy to produce broken front page markup by including a --nextpage-- tag that breaks an enclosing bold or italic tag. There's some code in get_the_content that fixes this for --more-- tags, but it doesn't handle --nextpage--, and it'd be more efficient to do it at post save time.

The enclosed patch fixes this by splitting the content into slices at those boundaries and separately balancing each slice. Balancing happens in the content_save_pre action. No filtering is needed on the output side for posts saved after this filter.

It was a bit of a struggle figuring out where to fit this but I think the solution is fairly clean. It includes a new split_nextpage() function that can be used instead of ad-hoc regexps for splitting a post into pages.

Attachments (5)

balance-tags-with-more-r7392.patch (3.2 KB) - added by tellyworth 7 years ago.
balanceTags_quicktags-trunk.patch (825 bytes) - added by devesine 3 years ago.
6297.diff (964 bytes) - added by mdawaffe 19 months ago.
props go to devesine
6297-renamed.diff (928 bytes) - added by devesine 19 months ago.
6297-untitests.diff (2.8 KB) - added by MikeHansenMe 4 months ago.
see #30284

Download all attachments as: .zip

Change History (33)

comment:1 @tellyworth7 years ago

By "No filtering is needed on the output side" I mean no _balance_ filtering of course.

comment:3 @lloydbudd7 years ago

  • Milestone changed from 2.7 to 2.6
  • Version set to 2.5

comment:4 @ryan7 years ago

  • Milestone changed from 2.9 to 2.7

comment:5 @ryan6 years ago

  • Milestone changed from 2.7 to 2.8

comment:6 @Denis-de-Bernardy6 years ago

ah, yes, the number of sidebar gets knocked down issues that this might fix...

comment:7 @Denis-de-Bernardy6 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 2.8 to Future Release

broken patch

comment:8 @Denis-de-Bernardy6 years ago

  • Component changed from General to Formatting
  • Owner anonymous deleted

comment:10 @Denis-de-Bernardy6 years ago

  • Milestone Future Release deleted
  • Resolution set to duplicate
  • Status changed from new to closed

fixed in r11398

comment:11 @nacin3 years ago

  • Milestone set to Awaiting Review
  • Resolution duplicate deleted
  • Status changed from closed to reopened

Was not fixed. Unit tests are still broken. Closed #20278 as a duplicate.

comment:12 @devesine3 years ago

Bringing over patch from #20278:

The attached patch splits up the text (using a regex patterned after the regexes used to split out quicktags in other areas such as get_the_content()) runs force_balance_tags against each part, and glues them back together.

This could be done in force_balance_tags instead, but my first thought is that force_balance_tags is a lower-level function; since it ignores options (use_balanceTags) and has no option to not balance (truism), it should ignore other WordPress-specific conceits like quicktags. Among other things, there's no expectation that it's always being used for content that should respect those quicktags (it's also used on the comment_text filter and in get_the_content() after the more and nextpage quicktags have been handled).

comment:13 @devesine3 years ago

  • Keywords has-patch added; needs-patch removed

comment:14 @SergeyBiryukov23 months ago

Closed #24191 as a duplicate.

@mdawaffe19 months ago

props go to devesine

comment:15 follow-up: @mdawaffe19 months ago

  • Keywords 2nd-opinion added

attachment:balanceTags_quicktags-trunk.patch​ looks solid to me. Refreshed for trunk in attachment:6297.diff.

Passes all relevant unit tests including the ones that are skipped for this ticket:

phpunit --group 6297 tests/post/filtering.php

I don't like the "quicktags" naming. Maybe "balance_tags_delimiters"? Maybe roll the <!-- and --> into the values returned by the filter?

comment:16 in reply to: ↑ 15 ; follow-up: @devesine19 months ago

Replying to mdawaffe:

I don't like the "quicktags" naming. Maybe "balance_tags_delimiters"?

That seems better - when I originally wrote the patch, I mistakenly gathered that 'quicktags' was the general name of comment tags with WP functionality (<!--more--> and <!--nextpage-->) rather than the name of the button bar in the editor.

Maybe roll the <!-- and --> into the values returned by the filter?

Ah, to provide more flexibility in delimiters?

Something like this, if I understand your suggestions correctly:

$balance_tags_delimiters = apply_filters( 'balance_tags_delimiters', array( '<!--more.*?-->', '<!--nextpage-->' ) );  
// Capture lets PREG_SPLIT_DELIM_CAPTURE return the delimiters 
$delimiters_regex = '/(' . implode( '|', $balance_tags_delimiters ) . ')/'; 
$parts = preg_split( $delimiters_regex, $text, -1, PREG_SPLIT_DELIM_CAPTURE ); 

comment:17 in reply to: ↑ 16 ; follow-up: @mdawaffe19 months ago

Replying to devesine:

Maybe roll the <!-- and --> into the values returned by the filter?

Ah, to provide more flexibility in delimiters?

Yes. Maybe it makes sense to split on some custom shortcode, for example. I'm not sure.

Something like this, if I understand your suggestions correctly:

Yes. That's what I had in mind. Again, I'm not positive it's better, but I think it probably is :)

@devesine19 months ago

comment:18 in reply to: ↑ 17 @devesine19 months ago

Replying to mdawaffe:

Again, I'm not positive it's better, but I think it probably is :)

Seems entirely reasonable.

attachment:6297-renamed.diff has those changes now.

comment:19 @mdawaffe19 months ago

  • Milestone changed from Awaiting Review to 3.7

comment:20 @nacin17 months ago

  • Keywords commit added; 2nd-opinion removed
  • Milestone changed from 3.7 to 3.8

Looks good. The filter will need to be commented. Let's get this in early 3.8.

comment:21 @wonderboymusic16 months ago

In 26050:

Split the content to balance open tags when <!--nextpage--> and <!--more.*?--> are used. Needs filter inline docs. The 4 unit tests that were previously failing for ticket 6297 now pass.

See #6297.
Props devesine.

comment:22 @wonderboymusic16 months ago

  • Keywords needs-docs added; commit removed

comment:23 @Hanni15 months ago

Looks like @simonweatley has taken care of the docs in #25518.

comment:24 @Hanni15 months ago

  • Keywords needs-docs removed

comment:25 @mdawaffe15 months ago

On further inspection, the behavior here is a little odd.

For nextpage, it makes sense to fix the markup on save to ensure the markup of each page is sane.

For more, it doesn't. For example, for the following:


On an index page, I would expect to see either:

<a href="#more-123">...</a>

or maybe:

<a href="#more-123">...</a>

In 3.7.1, I see the former. In [26050], I see the latter. I'm not sure which is "better", but the difference is not backward compatible.

More importantly, on a permalink page, I would expect to see:

<a id="more-123></a>

That's what I see in 3.7.1. In [26050], though, I see:

<a id="more-123></a>

Which is unexpected.

For nextpage, each piece of text is only ever visible on one page, so fixing each page's markup individually makes sense.

For more, some pieces of text are shown in two different contexts, so fixing the markup "before the jump" independently from the markup "after the jump" doesn't quite work.

Idea: Balance tags on save only by page. Balance tags of index page's pre-more-markup on display.

  • More work on display.
  • Using a filter on display that plugins may expect to only be used on save?
  • Toggling the setting changes the display of already written posts.

comment:26 @nacin15 months ago

In 26557:

Revert [26050], stop trying to balance open HTML tags across nextpage and more tags for now.

see #6297.

comment:27 @nacin15 months ago

  • Milestone changed from 3.8 to Future Release

comment:28 @kraftbj15 months ago

  • Cc bk@… added
Note: See TracTickets for help on using tickets.