Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#45290 closed defect (bug) (fixed)

The equivalent of `gutenberg_wpautop` from Gutenberg plugin is missing in the 5.0 beta

Reported by: nerrad's profile nerrad Owned by: pento's profile pento
Milestone: 5.0 Priority: high
Severity: major Version: 5.0
Component: Editor Keywords: has-patch has-unit-tests fixed-5.0
Focuses: Cc:

Description

In gutenberg/lib/compat.php is the following code:

/**
 * As a substitute for the default content `wpautop` filter, applies autop
 * behavior only for posts where content does not contain blocks.
 *
 * @param  string $content Post content.
 * @return string          Paragraph-converted text if non-block content.
 */
function gutenberg_wpautop( $content ) {
	if ( has_blocks( $content ) ) {
		return $content;
	}

	return wpautop( $content );
}
remove_filter( 'the_content', 'wpautop' );
add_filter( 'the_content', 'gutenberg_wpautop', 6 );

The equivalent for this is not found in WordPress core. Where I stumbled across this is in a plugin I'm integrating with Gutenberg, I have an html structure created from server side render (dynamic block) that is similar to this:

<li><img><span>Some text</span></li>

With just WordPress 5.0.beta.3 active the output in the post content on the frontend is:

<li><img><p><span>Some text</span></p></li>

With WordPress 5.0.beta.3 and the Gutenberg plugin active (latest master) I get this:

<li><img><span>Some text</span></li>

I'm willing to work on a patch for this, but I'm not sure if this is an intentional omission in WP 5.0 or not.

Attachments (2)

45290.diff (799 bytes) - added by nerrad 6 years ago.
45290.2.diff (3.4 KB) - added by pento 6 years ago.

Download all attachments as: .zip

Change History (29)

This ticket was mentioned in Slack in #core-editor by nerrad. View the logs.


6 years ago

#2 @SergeyBiryukov
6 years ago

  • Milestone changed from Awaiting Review to 5.0

#3 @littler.chicken
6 years ago

I can confirm that I'm seeing this same behavior (using Gravity Forms' block, the beta version). Complex fields get paragraph tags and line breaks added unexpectedly.

#4 @pento
6 years ago

This is an intentional change, as wpautop() now exits early if the content contains blocks.

@nerrad: Are you able to debug if wpautop() is exiting early correctly? Alternatively, a sample that reproduces this behaviour would be super helpful.

@nerrad
6 years ago

#5 @nerrad
6 years ago

So ya I see:

// We don't need to autop posts with blocks in them.
if ( has_blocks( $pee ) ) {
    return $pee;
}

Howwever wpautop is hooked into the_content on priority 10, and do_blocks is hooked in on priority 9. So that means the blocks have been parsed and the block serialized comments removed before hitting wpautop. Thus the has_blocks check here is never going to be true.

The implementation in core differs from what was in the gutenberg plugin because wpautop was re-registered running at priority 6 before the blocks were parsed from content.

So the quick fix here is to just have wpautop run before block parsing. In the attached patch I've set the priority of wpautop to 8 so it runs just before do_blocks which is at priority 9. I've verified this resolves the issue reported in this ticket but I'm not sure if there are other side-effects to having wpautop run at a lower priority.

Version 0, edited 6 years ago by nerrad (next)

#6 @nerrad
6 years ago

  • Keywords has-patch added; needs-patch removed
  • Severity changed from normal to major

I'm going to bump the priority on this a bit. I think its a fairly significant behaviour change between pre-merge and post-merge and has the potential to lead to more problems.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


6 years ago

#8 @desrosj
6 years ago

  • Priority changed from normal to high

#9 @nerrad
6 years ago

  • Keywords needs-testing added

#10 @mkaz
6 years ago

One side effect from bumping up the priority for the wpautop, which was moved from 8 to 6 in 4.1.1, the Syntaxhighlighter plugin which manages the [code] shortcode needs to hook in before autop and had priority 7.

The result is <br> show in the code block for older posts, regardless of the content being edited or not in Gutenberg. The filter would get applied and process prior to autop to fix.

It's an easy fix for the plugin, by moving its priority up, but possible it will effect others.

#11 @littler.chicken
6 years ago

Would it be possible to change the priority on the do_blocks call instead of wpautop? Since that's the new element?

#12 @nerrad
6 years ago

This looks like a related pull: https://github.com/WordPress/gutenberg/pull/11050 (that's also fairly recent).

#13 @nerrad
6 years ago

So I think part of the issue here is that when the GB plugin is active things don't look broken because of its de-registering of the default wpautop content filter and adding its own. That's how this has slipped by. I think along with fixing the issue in core, there should be the removal of the compat.php code from GB plugin (or if its intended to work on pre 5.0 WP, do WP version detection).

Last edited 6 years ago by nerrad (previous) (diff)

#14 @nerrad
6 years ago

Would it be possible to change the priority on the do_blocks call instead of wpautop? Since that's the new element?

Don't take this as a definitive answer, but I think the reason do_blocks is at an earlier priority is because the intent is to ensure block parsing is handled before any other wp_content filter callbacks. This allows the blocks to be parsed out of the content and have their serialized data removed from wp_content reducing the potential mangling of the serialization by other callbacks hooking into post_content.

I think a more solid long term solution might be to parse blocks from the_content field as soon as its retrieved from the db rather than on the fly via the_content (maybe set a has_blocks property on WP_Post?). That way it guarantees we have unadulterated block data.

@pento
6 years ago

#15 @pento
6 years ago

  • Keywords has-unit-tests added; dev-feedback removed

Thank you for the patch and further research, @nerrad! You're absolutely right that this change in behaviour was an oversight during merging.

I agree that do_blocks() needs to be run before other things in the_content run, but I don't think that changing the priority of wpautop() is a viable option, as many plugins assume it runs at priority 10. Changing it will cause subtle issues like the one @mkaz mentioned, or problems like remove_action( 'the_content', 'wpautop' ) no longer working as it has in the past.

45290.2.diff is an attempt to work around this: when do_blocks() is run, it decides if it needs to remove wpautop(), does so, then schedules a filter to re-add it immediately after it would've run.

#16 @nerrad
6 years ago

@pento I've verified your patch works in testing.

If I'm reading the code correctly, the intention is to remove wpautop from the callstack so it doesn't execute on that current_iteration of the_content but re-added for any future non block iterations. Clever.

In the long run the necessity of having this workaround for content with blocks seems fragile, but for now it works. I'm assuming when classic editor is no longer a part of core, wpautop will no longer be needed?

This ticket was mentioned in Slack in #core-editor by nerrad. View the logs.


6 years ago

#18 @pento
6 years ago

  • Owner set to pento
  • Resolution set to fixed
  • Status changed from new to closed

In 43879:

Formatting: Ensure wpautop() isn't run on content generated from blocks.

As do_blocks() is run before wpautop() in the_content filter, we can remove in a Just In Time fashion, before that filter is run.

After wpautop()s original priority has passed, we can re-add it in a Just Too Late fashion, to ensure it's available if the_content filter is run multiple times on a page load.

Props pento, nerrad.
Fixes #45290.

#19 @pento
6 years ago

  • Keywords fixed-5.0 added; needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for trunk.

#20 @pento
6 years ago

In 43881:

Tests: Fix a failing test after [43879].

See #45290.

#21 @pento
6 years ago

In 43883:

Tests: Fix tests broken in PHP 5.x after [43879].

See #45290.

#22 @tyxla
6 years ago

I've just noticed that we're now inadvertently omitting any <p> tags from Custom HTML blocks. To reproduce:

  • Start a post.
  • Insert a Custom HTML block.
  • Insert <p>test</p> in it.
  • Save the post.
  • Refresh.
  • The <p> is gone.

This doesn't seem expected behavior to me. It might also affect custom blocks that store content potentially containing <p>.

#23 @pento
6 years ago

@tyxla: The several folks that have reported similar issues have noted that turning off Jetpack Markdown fixes it. Could you try the same?

#24 @tyxla
6 years ago

Thanks, this appears to have solved the issue.

#25 @jeremyfelt
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 44226:

Formatting: Ensure wpautop() isn't run on content generated from blocks.

As do_blocks() is run before wpautop() in the_content filter, we can remove in a Just In Time fashion, before that filter is run.

After wpautop()s original priority has passed, we can re-add it in a Just Too Late fashion, to ensure it's available if the_content filter is run multiple times on a page load.

Merges [43879] and [43881] from the 5.0 branch to trunk.

Props pento, nerrad.
Fixes #45290.

#26 @SergeyBiryukov
6 years ago

In 44242:

Tests: Fix tests broken in PHP 5.x after [43879].

Props pento.
Merges [43883] to trunk.
See #45290.

#27 @ocean90
6 years ago

In 44431:

Formatting: Remove unused global import for $wp_filter in _restore_wpautop_hook().

See #45290.

Note: See TracTickets for help on using tickets.