WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 4 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:

Description

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

balance-tags-with-more-r7392.patch (3.2 KB) - added by tellyworth 6 years ago.
balanceTags_quicktags-trunk.patch (825 bytes) - added by devesine 2 years ago.
6297.diff (964 bytes) - added by mdawaffe 9 months ago.
props go to devesine
6297-renamed.diff (928 bytes) - added by devesine 9 months ago.

Download all attachments as: .zip

Change History (32)

comment:1 tellyworth6 years ago

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

comment:3 lloydbudd6 years ago

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

comment:4 ryan6 years ago

  • Milestone changed from 2.9 to 2.7

comment:5 ryan5 years ago

  • Milestone changed from 2.7 to 2.8

comment:6 Denis-de-Bernardy5 years ago

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

comment:7 Denis-de-Bernardy5 years ago

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

broken patch

comment:8 Denis-de-Bernardy5 years ago

  • Component changed from General to Formatting
  • Owner anonymous deleted

comment:10 Denis-de-Bernardy5 years ago

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

fixed in r11398

comment:11 nacin2 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 devesine2 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 devesine2 years ago

  • Keywords has-patch added; needs-patch removed

comment:14 SergeyBiryukov12 months ago

Closed #24191 as a duplicate.

mdawaffe9 months ago

props go to devesine

comment:15 follow-up: mdawaffe9 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: devesine9 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: mdawaffe9 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 :)

devesine9 months ago

comment:18 in reply to: ↑ 17 devesine9 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 mdawaffe9 months ago

  • Milestone changed from Awaiting Review to 3.7

comment:20 nacin7 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 wonderboymusic5 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 wonderboymusic5 months ago

  • Keywords needs-docs added; commit removed

comment:23 Hanni5 months ago

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

comment:24 Hanni5 months ago

  • Keywords needs-docs removed

comment:25 mdawaffe5 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:

<blockquote>
Above
<!--more-->
Below
</blockquote>

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

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

or maybe:

<blockquote>
Above
</blockquote>
<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:

<blockquote>
Above
<a id="more-123></a>
Below
</blockquote>

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

<blockquote>
Above
</blockquote>
<a id="more-123></a>
Below

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.
Cons:

  • 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 nacin4 months ago

In 26557:

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

see #6297.

comment:27 nacin4 months ago

  • Milestone changed from 3.8 to Future Release

comment:28 kraftbj4 months ago

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