Opened 6 years ago
Closed 6 years ago
#45495 closed defect (bug) (fixed)
Extra P tags added to custom dynamic blocks
Reported by: | mattheu | Owned by: | 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)
Change History (10)
#3
@
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.
This ticket was mentioned in Slack in #core by aldavigdis. View the logs.
6 years ago
#5
@
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
@
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.
#7
@
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 fromthe_content
._restore_wpautop_hook()
is added tothe_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 tothe_content
by the outerdo_blocks()
call, is run.wpautop()
is re-added tothe_content
._restore_wpautop_hook()
is removed fromthe_content
.
- The inner
the_content
filter finishes.
- The dynamic block callback returns.
- The dynamic block callback triggers an inner run of
- 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 outerthe_content
filter. - Shortcodes expect to have
wpautop()
run on their inner content. Does this change things? - What happens with classic blocks?
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 todo_blocks
in that case is the addition ofrender_block
.This seems to have prevented
remove_filter
from being executed at the correct moment.Moving the
remove_filter
andadd_filter
statements to just before thereturn
statement seems to have cured this issue on my side and blocks are not forced to include extrap
orbr
elements.