Extra P tags added to custom dynamic blocks

[mattheu]

As requested - opening a ticket on Trac for ​https://github.com/WordPress/gutenberg/issues/12646

I have a a custom block that is dynamic and rendered using the render callback.
The custom block renders the excerpt.

The render callback returns something like this:

<div>
    <a href="http://example.com"><?php echo get_the_excerpt(); ?></a>
</div>

Prior to Gutenberg 4.6 this worked as expected. After updating, I now get paragraph tags added around the link.

Digging into this a bit - it seems the regression was caused by this change: ​https://github.com/WordPress/gutenberg/commit/2a66db0fc99a9fb1ba99f61e59e6a0b2a4a5f9ef#diff-6ff32417da0658502e7caa1a1abbeae6

The key difference is the new code restores the wpautop filter at a later priority, which is still getting run.

Digging into this some more - it is actually because get_the_excerpt internally calls apply_filters( 'the_content' - which is a bit of a headache.

There is a test case plugin on the github ticket to reproduce the bug.

[aldavigdis]

This has been causing me massive headaches and I think I just found the cure.

This issue seems to have been introduced in blocks.php https://core.trac.wordpress.org/changeset/44261 just before Christmas 2018 and the change introduced to do_blocks in that case is the addition of render_block.

This seems to have prevented remove_filter from being executed at the correct moment.

---

Moving the remove_filter and add_filter statements to just before the return statement seems to have cured this issue on my side and blocks are not forced to include extra p or br elements.

[aldavigdis]

A patch that cures the issue

[aldavigdis]

Note that I have tested this both with Gutenberg and Classic editors with blocks that I'm working and it seems to have the intended results of disabling the wpautop filter for those blocks.

[pento]

Thank you for the patch, @aldavigdis!

Would you mind adding some unit tests for it, as well?

[aldavigdis]

@pento The strange thing here is that this should have been covered in test_blocks_arent_autopeed, which was done during https://core.trac.wordpress.org/ticket/45290.

It's almost as if the wpautop is applied/disabled as expected in the test suite, but somehow it doesn't when rendering a block on a live site.

I have the latest nightly in my dev environment and I can show that wpautop is in fact being run on the_content if it includes blocks unless my patch is applied. However, I don't seem to be able to proof this with a unit test.

[pento]

For folks following the ticket, @aldavigdis and I have been brainstorming a bit on this.

So, the problem appears to be when a dynamic block callback run the the_content filter. @mattheu's example does so when it calls get_the_excerpt(), and the feature that @aldavigdis is working on (Jetpack's related posts feature) does so, too.

It doesn't matter if the_content is being run on the same post that originally triggered the do_blocks() call. The important part is that it's being run a second time before the first finishes.

Here's the order of proceedings:

• the_content filter is started.
  • do_blocks() is called.
    • wpautop() is removed from the_content.
    • _restore_wpautop_hook() is added to the_content.
    • The dynamic block callback is called.
      • The dynamic block callback triggers an inner run of the_content filter.
        • do_blocks() is called.
          • wpautop isn't registered, so it isn't removed in this inner call.
          • do_blocks() returns.
        • _restore_wpautop_hook(), having been added to the_content by the outer do_blocks() call, is run.
          • wpautop() is re-added to the_content.
          • _restore_wpautop_hook() is removed from the_content.
        • The inner the_content filter finishes.
      • The dynamic block callback returns.
    • The outer do_blocks() call returns.
  • Having been added by the inner the_content run, wpautop() is called.
  • Having been removed by the inner the_content run, _restore_wpautop_hook() is not called.
  • The outer the_content run finishes.

Provided the dynamic block callback doesn't start a different filter, this could also be triggered by the dynamic block callback calling do_blocks() directly.

I suspect that @aldavigdis' patch is the correct way to tackle this problem. 45495.diff​ includes a unit test showing this particular behaviour.

Things I'm not yet certain of:

• Are there side effects to this fix which aren't accounted for?
• Are there plugins with workarounds in place for this bug? If so, how will they be affected? If they just remove wpautop themselves, that should be fine: it'll be re-added at the end by outer the_content filter.
• Shortcodes expect to have wpautop() run on their inner content. Does this change things?
• What happens with classic blocks?

[pento]

In 45139:

Blocks: Allow for nested the_content calls within do_blocks().

When do_blocks() is run, it sets up some special handling of the wpautop filter, as we don't want wpautop to run on block content, but we do want it to be available for subsequent runs of the_content, which may be happening on non-block content.

As we set this up before rendering dynamic blocks, however, a dynamic block choosing to run the_content will cause unintentially structural deficiences in this particular recursive block tower.

Moving this handling to after dynamic blocks are rendered makes our tower lean a little less.

Props aldavigdis, pento.
Fixes #45495.