Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#45495 closed defect (bug) (fixed)

Extra P tags added to custom dynamic blocks

Reported by: mattheu's profile mattheu Owned by: pento's profile pento
Milestone: 5.2 Priority: normal
Severity: normal Version:
Component: Editor Keywords: has-patch has-unit-tests needs-testing
Focuses: Cc:

Description

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.

Attachments (2)

do_blocks.diff (1.1 KB) - added by aldavigdis 6 years ago.
A patch that cures the issue
45495.diff (2.1 KB) - added by pento 6 years ago.

Download all attachments as: .zip

Change History (10)

#1 @SergeyBiryukov
6 years ago

  • Component changed from General to Editor

#2 @aldavigdis
6 years ago

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
6 years ago

A patch that cures the issue

#3 @aldavigdis
6 years ago

  • Keywords has-patch added

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.

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

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


6 years ago

#5 @pento
6 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.2

Thank you for the patch, @aldavigdis!

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

#6 @aldavigdis
6 years ago

@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
6 years ago

#7 @pento
6 years ago

  • Keywords has-unit-tests needs-testing added; needs-unit-tests removed

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?
Last edited 6 years ago by pento (previous) (diff)

#8 @pento
6 years ago

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

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.

Note: See TracTickets for help on using tickets.