WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 9 months ago

#12061 new defect (bug)

Treatment of shortcodes by wpautop is incomplete

Reported by: gcorne Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Shortcodes Keywords: wpautop needs-testing
Focuses: Cc:

Description

There are a number of issues that arise when shortcodes are embedded in a block of content that is filtered by wpautop. After studying the issue, I have come to the conclusion that the underlying issue is that a shortcode can result in a block-level HTML element or an inline snippet of HTML. Neither wpautop nor shortcode_unautop have the information necessary to know what the intended to be block-level or text-level, which results in <br /> tags to appear in places where they were not intended and causes invalid markup when paragraphs are not closed properly.

1) shorcodes when registered need to state the display type of the html that they return (inline or block). In addition, the javascript version of filters need to be provided with the same data.

2) An alternate mechanism for handling shortcodes prior to wpautop needs to be implemented. To maintain a separation between wpautop and shortcodes, temporarily replacing block-level shortcodes with unique tokens, running wpautop, running wpautop on the content of the shortcode (since this is a separate filter, plugins can disable it on a per shortcode basis), and then replacing the token with shortcodes post-wpautop.

As WP and plugin authors use shortcodes more and more, I think this is an issue that should not be left to the wayside. Shortcodes especially tied to tinymce buttons make working with WordPress easier for the average content producer.

Attachments (4)

12061.diff (623 bytes) - added by mdawaffe 11 months ago.
Add no_unautop_shortcodes filter.
12061.2.diff (734 bytes) - added by DrewAPicture 11 months ago.
'unautop_shortcodes' + hook docs
12061.3.diff (766 bytes) - added by aaroncampbell 9 months ago.
12061.4.diff (1.8 KB) - added by aaroncampbell 9 months ago.

Download all attachments as: .zip

Change History (14)

comment:1 @scribu6 years ago

  • Keywords needs-patch added
  • Milestone changed from Unassigned to 3.0

I agree with both suggestions.

The problem now is coming up with a working patch.

comment:2 @gcorne6 years ago

  • Cc gcorne@… added

comment:3 @dd325 years ago

  • Keywords wpautop added
  • Milestone changed from 3.0 to Future Release

Moving to Future release pending a patch or traction.

comment:5 @lkraav21 months ago

  • Cc leho@… added

@mdawaffe11 months ago

Add no_unautop_shortcodes filter.

comment:6 @mdawaffe11 months ago

Some shortcodes may output markup that could be either inline or block. Imagine a shortcode that outputs an image tag. The post author would expect that shortcode to be wpautop()ed the same way manually entering the image tag would be:

  • no additional markup if entered inline
  • <br /> or <p> tags as appropriate if entered on its own line.

Perhaps, just like our no_texturize_shortcodes filter, we should also have a no_unautop_shortcodes filter.

That way, shortcode authors could opt out of being unautoped (that is, opt in to being wpautop()ed).

Example patch attachment:12061.diff.

comment:7 @DrewAPicture11 months ago

  • Keywords has-patch added; needs-patch removed

I like where this is going, though I'm not a big fan of 'no_unautop_shortcodes' -- as it's effectively a double negative.

So rather than leveraging the filter to specify the shortcodes that should be skipped, I think it would be more the "WordPress way" to simply make the $shortcode_tags array filterable here. If a developer wants to skip a tag, they can unset it via the filter.

This is demonstrated in 12061.2.diff. I've also added hook docs.

@DrewAPicture11 months ago

'unautop_shortcodes' + hook docs

@aaroncampbell9 months ago

comment:8 @aaroncampbell9 months ago

  • Keywords needs-unit-tests added

I prefer the less convoluted sounding "unautop_shortcodes" but 12061.2.diff had some PHP notices and didn't work. 12061.3.diff is an update of that which works as expected.

I'd like to see unit tests for this too, but they need to be pretty simple since unautop doesn't work quite right and I don't want a fix for #14050 to invalidate our tests here. I'll try to add some in a little bit if no one beats me to it.

@aaroncampbell9 months ago

comment:9 @aaroncampbell9 months ago

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

I talked to @mdawaffe and we agreed that we should use @DrewAPicture's naming convention. 12061.4.diff is the current patch along with a unit test that covers this without stepping on the toes of #14050

I think we're good to go here but would love if we could get some people to test.

comment:10 @mdawaffe9 months ago

The only thing about my original no_unautop_shortcodes is that if you have a shortcode you want to change both the default autop and texturize behaviors of, you can do it by hooking the same function to no_texturize_shortcodes and no_unautop_shortcodes.

With unautop_shortcodes, you have to have two separate functions: one that adds to no_texturize_shortcodes, and one that removes from unautop_shortcodes. I'm fine with that situation and still like @DrewAPicture's naming. I just wanted to point it out.

(Sorry I forgot to mention that when we chatted, @aaroncampbell.)

Note: See TracTickets for help on using tickets.