Fix embedded blocks when converted to reusable blocks #21043
Conversation
|
Size Change: +37 B (0%) Total Size: 856 kB
ℹ️ View Unchanged
|
earnjam
changed the title
earnjam
requested review from
ajitbohra,
nerrad,
ntwb,
talldan and
TimothyBJacobs
as code owners
| remove_filter( 'the_content', array( $wp_embed, 'autoembed' ), 8 ); | ||
| remove_filter( 'the_content', 'do_blocks', 9 ); | ||
| add_filter( 'the_content', 'do_blocks', 8 ); | ||
| add_filter( 'the_content', array( $wp_embed, 'autoembed' ), 9 ); |
There was a problem hiding this comment.
Apparently these has the potential of breaking autop things, so I'd like some good testing for this PR. Also cc @aduth since you're better aware of the potential issues.
There was a problem hiding this comment.
Also, this has the potential to affect dynamic blocks, so we need to check these too.
There was a problem hiding this comment.
Agreed. I was thinking that the tests from #11050 had those things covered now, but it's always possible we're missing things.
There was a problem hiding this comment.
At least within the block filtering itself, the handling of wpautop is pretty self-contained, and generally tolerant to adjustments in its own priority.
This specific change also wouldn't have an impact so far as: Blocks filtering occurred before wpautop before these changes, and still occurs before wpautop after these changes. Specifically, wpautop happens at the default priority (10).
Relevant to my previous comment:
All of this processing should be expected to occur before the default priority (
10) since a filter at the default priority would expect to receive markup in roughly its "ready" state, at least so far as what is provided by core.
At first I wondered if this contradicted my statement. But before default-filters.php runs fairly early, even though it uses the same priority, since the filter is added early, it's likely still run long before any plugins would. Aside from the bugs and resulting changes here, it may have even been appropriate that do_blocks a few lines above be registered with the default priority as well, since the order of the filter adds would have largely emulated the same effect.
There was a problem hiding this comment.
I'm approving this PR as it solves the issue but I'd love more eyes from people more familiar with the hooks setup. Maybe @pento
|
I don't recall all of the specifics for why certain priorities were chosen. In trying to sift through some history, I observe:
Thinking about the priorities in general:
In fact, the implementation in Gutenberg prior to WordPress 5.0 seemed to go this direction, noted by the inline code comment of the filtering mentioned earlier. A few things that are worth following-up on:
|
|
This is an interesting problem, and a fairly neat demonstration of the inherent conflict between the (ideally) self-contained nature of blocks, and the historical handling of I do not recommend changing either the priority or order of these filters. It will almost certainly break things out in the WordPress ecosystem, and it'll break them in wildly unexpected ways. Instead, I highly recommend taking a "lightest touch" approach when fixing issues related to filter ordering. In this case, I think @aduth's suggestion of having the embed block handle its own embedding would address the original issues, and avoid having to re-order these filters. The Another method to explore, which is a little more complex than @aduth's suggestion, but will potentially give more reliable results, is to modify |
noisysocks
added
[Feature] Synced Patterns
|
I'm going to close this PR in favor of pursuing the alternative options proposed above. |
This PR fixes the issue of embed blocks not displaying on the front-end after being converted to reusable blocks.
#14663 added a test for this, but that PR had become stale. This PR updates that new test to support the UI changes that have occurred since it was written. It also reorders
the_contentfilters, which fixes the issue and gets that new test to pass. This would fix #18098, and close #14608.It feels very risky to mess with the filter order because it is so brittle. I've run all the tests for this on core and in Gutenberg, as well as done some manual testing and it appears to work and resolve the issue.
We'd probably want to commit the core change early in release cycle to try to catch any issues.
Related Trac ticket: https://core.trac.wordpress.org/ticket/46457
Pull request that shows core tests passing: WordPress/wordpress-develop#192