New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid changing default wpautop priority #11722

Merged
merged 1 commit into from Nov 29, 2018

Conversation

Projects
None yet
7 participants
@brandonpayton
Member

brandonpayton commented Nov 10, 2018

Description

This is a change to avoid problems due to changing the default wpautop priority.

Fixes #11691.

How has this been tested?

  • Created a post in the classic editor and verified auto-embeds render correctly for the following raw content (mentioned in #11691):
Some text above
https://www.youtube.com/watch?v=3V9QHBgrPNY

https://www.youtube.com/watch?v=f5KyMNDJE6o
  • Tested that block content opened in the classic editor retains its paragraphs.
  • Tested that non-block content is autop'd.
  • Unit tests.

Types of changes

Take a similar approach to core by removing the wpautop filter when block content is detected and subsequently restore it for later applications of the the_content filter.

The core changeset used for reference:
https://core.trac.wordpress.org/changeset/43879

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
*/
function gutenberg_wpautop( $content ) {
if ( has_blocks( $content ) ) {
return $content;
// If there are blocks in this content, we shouldn't run wpautop() on it later.

This comment has been minimized.

@pento

pento Nov 11, 2018

Member

In Core, this block of code is run at the top of do_blocks(), there is no equivalent of gutenberg_wpautop(). Any particular reason why you chose to do it this way?

This comment has been minimized.

@brandonpayton

brandonpayton Nov 12, 2018

Member

There's no good reason.

I may have made a mental mistake and avoided do_blocks because it is only defined conditionally by this plugin, but actually, we only need to worry about avoiding wpautop when we define do_blocks. Thanks for asking.

*
* @access private
*
* @since 4.4

This comment has been minimized.

@pento

pento Nov 11, 2018

Member
Suggested change Beta
* @since 4.4
* @since 4.4.0

@brandonpayton brandonpayton force-pushed the fix/broken-wpautop-priority branch from e116708 to bd83c88 Nov 12, 2018

$priority = has_filter( 'the_content', 'wpautop' );
if ( false !== $priority && doing_filter( 'the_content' ) && has_blocks( $content ) ) {
remove_filter( 'the_content', 'wpautop', $priority );
add_filter( 'the_content', 'wpautop', $priority + 1 );

This comment has been minimized.

@brandonpayton

brandonpayton Nov 12, 2018

Member

Immediately re-adding wpautop here. 🤦‍♂️ Good example of why one shouldn't work during time set aside for recharging.

@brandonpayton brandonpayton requested a review from WordPress/gutenberg-core Nov 12, 2018

@youknowriad youknowriad modified the milestones: 4.4, 4.5 Nov 15, 2018

@@ -187,6 +194,29 @@ function do_blocks( $content ) {
add_filter( 'the_content', 'do_blocks', 7 ); // BEFORE do_shortcode() and oembed.
}
if ( ! function_exists( '_restore_wpautop_hook' ) ) {
/**
* If do_blocks() needs to remove wp_autop() from the `the_content` filter,

This comment has been minimized.

@lkraav

lkraav Nov 15, 2018

wp_autop ~> wpautop

This comment has been minimized.

@brandonpayton

brandonpayton Nov 16, 2018

Member

Thanks for catching that. It's fixed now.

@brandonpayton brandonpayton force-pushed the fix/broken-wpautop-priority branch from ae40f6d to 46b6887 Nov 16, 2018

@mtias mtias modified the milestones: 4.5, 4.6 Nov 19, 2018

@catehstn

This comment has been minimized.

catehstn commented Nov 21, 2018

This looks like it's ready for another pass, @brandonpayton?

@brandonpayton brandonpayton force-pushed the fix/broken-wpautop-priority branch from 46b6887 to e5ecd4a Nov 21, 2018

@brandonpayton

This comment has been minimized.

Member

brandonpayton commented Nov 21, 2018

This looks like it's ready for another pass, @brandonpayton?

Yep. It just needs a review.

@mtias mtias added [Type] Task and removed [Type] Bug labels Nov 22, 2018

@brandonpayton

This comment has been minimized.

Member

brandonpayton commented Nov 26, 2018

Hi @mtias, why are we changing this from a Bug to a Task? Loading Gutenberg in 4.9 breaks auto embeds as mentioned in #11691. That is buggy behavior.

@noisysocks

Confirmed that this fixes #11691 👍

Could we include the unit test that's in https://core.trac.wordpress.org/changeset/43879?

Will be good to remove all of these workarounds once the Gutenberg plugin relies on having WordPress 5.0 installed.

@noisysocks

This comment has been minimized.

Member

noisysocks commented Nov 26, 2018

This should go into the next plugin release. My understanding is that that will be 4.7, not 4.6.

@mtias

This comment has been minimized.

Contributor

mtias commented Nov 28, 2018

@brandonpayton changed it to task because this is retrofitting the plugin with behaviour present in core, and since we are not doing another plugin release yet.

@brandonpayton

This comment has been minimized.

Member

brandonpayton commented Nov 28, 2018

ah, maybe I shouldn't be tagging PRs with "bug" at all. thanks for explaining @mtias

@mtias

This comment has been minimized.

Contributor

mtias commented Nov 28, 2018

It's fine :) I just wanted to have a clear overview in the milestone to figure out what needs to be merged before release and what can be added after release but before 5.0.1, if that makes sense.

@youknowriad youknowriad merged commit 2a66db0 into master Nov 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the fix/broken-wpautop-priority branch Nov 29, 2018

daniloercoli added a commit that referenced this pull request Nov 29, 2018

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…rnmobile/danilo-try-to-fix-undo-redo

* 'master' of https://github.com/WordPress/gutenberg:
  Avoid changing default wpautop priority (#11722)
  Try fixing the tab navigation issue (#12390)
  Polish editor style documentation (#12381)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment