WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 17 months ago

#6444 closed defect (bug) (fixed)

Changeset 7561 (post rc3) leaves an open <p> at the end of the inserted gallery html

Reported by: johnconners Owned by: andy
Milestone: 2.5.1 Priority: high
Severity: critical Version: 2.5
Component: Template Keywords: has-patch needs-testing
Focuses: Cc:

Description

Prior to changeset:7561 the gallery html looked pretty much like this when you put [gallery] in a post containing 2 photos:

<p>
	<style type='text/css'>
		<!-- the stylesheet -->
	</style>

	<!-- see gallery_shortcode() in wp-includes/media.php -->
	<div class='gallery'><dl class='gallery-item'>
		<dt class='gallery-icon'>
			<!-- the thumbnail -->
		</dt></dl><dl class='gallery-item'>
		<dt class='gallery-icon'>
			<!-- the thumbnail -->
		</dt></dl>
		<br style='clear: both;' >

	</div>
</p>

Note that the whole thing is enclosed in <p></p> and everything is nested correctly.

However now the html looks like this:

<style type='text/css'>
		<!-- the stylesheet -->
	</style>
<p>	<!-- see gallery_shortcode() in wp-includes/media.php --></p>

<div class='gallery'>
<dl class='gallery-item'>
<dt class='gallery-icon'>
	<!-- the thumbnail -->
			</dt>
</dl>
<dl class='gallery-item'>
<dt class='gallery-icon'>
	<!-- the thumbnail -->
			</dt>
</dl>
<p>			<br style='clear: both;' >
		</div>

Now the bug (after all that!) is that the opening <p> on the 2nd from bottom line is never closed.

Attachments (8)

6444.001.diff (1.2 KB) - added by markjaquith 7 years ago.
6444.autop.001.diff (2.0 KB) - added by markjaquith 7 years ago.
6444.002.diff (4.1 KB) - added by AaronCampbell 7 years ago.
6444.003.autop.diff (4.1 KB) - added by markjaquith 7 years ago.
6444.004.diff (5.8 KB) - added by AaronCampbell 7 years ago.
6444-no-p-wrap-shortcodes-r7811.patch (1.8 KB) - added by tellyworth 7 years ago.
compromise: don't p-wrap stand-alone shortcodes
6444-no-p-wrap-shortcodes-r7811-2.patch (2.2 KB) - added by tellyworth 7 years ago.
as per the last but with br's stripped too
6444.006.diff (1.4 KB) - added by markjaquith 7 years ago.

Download all attachments as: .zip

Change History (44)

comment:1 @westi7 years ago

  • Component changed from General to Template
  • Milestone set to 2.5
  • Owner changed from anonymous to andy
  • Priority changed from normal to high
  • Severity changed from normal to blocker

comment:2 @lloydbudd7 years ago

  • Milestone changed from 2.6 to 2.5.1
  • Severity changed from blocker to critical

@markjaquith7 years ago

comment:3 @markjaquith7 years ago

  • Keywords has-patch needs-testing added

Expanding HTML before wpautop() fires is a bad idea. My patch puts it to 11 (so we can be darn sure it'll be after wpautop()) and deals with quotes in shorttags the same way we deal with quotes in HTML elements.

comment:4 @markjaquith7 years ago

Matt gave this a +1, but raised a separate issue which is tracked here: #6496

comment:5 @markjaquith7 years ago

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

(In [7581]) Parse shortcodes AFTER wpautop() to avoid mangling. Have wptexturize() ignore shortcodes so quotes stay straight. fixes #6444 for trunk

comment:6 @markjaquith7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Let's bang on that for a little bit before sending it into 2.5.1

comment:7 @AaronCampbell7 years ago

Looks like this is already in trunk, but I actually support a slightly different solution. I think that there are cases where post-wpautop() is great (most of the time for me), but I can see that there are times where you would want pre-wpautop() (especially for the less-advanced users). We should be able to do both. I'm attaching a patch that will allow people to use shortcodes as they worked before this ticket (pre-wpautop), or they can use them post-wpautop by passing a 3rd argument to add_shortcode like:

add_shortcode('googleMap', array($wpGoogleMaps, 'testTags'), true);

For me personally, the default should be the latter (priority 11), but I though this would have as little impact on people currently developing for 2.5. However, if you think the default should change, I can upload another diff.

comment:8 @markjaquith7 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [7594]) Parse shortcodes AFTER wpautop() to avoid mangling. Have wptexturize() ignore shortcodes so quotes stay straight. fixes #6444 for 2.5.1

comment:9 @markjaquith7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

We also have the issue of wpautop() wrapping a paragraph around block-level shortcodes. wpautop() should refrain from wrapping a paragraph around shortcodes.

@markjaquith7 years ago

comment:10 @markjaquith7 years ago

6444.autop.001.diff moves the shortcode regex snippet generation into its own function so that we can call it in wpautop().

Desired result is that known shortcodes (like [gallery] do NOT get <p></p>-wrapped, while unknown shortcodes like [foobar] DO get wrapped.

HTML generation is up to the shortcode plugin author. They can use wpautop() themselves if they want paragraph'd output, or they can just spit out block level HTML (like divs and know that we won't mess them up or wrap them in a paragraph.

@AaronCampbell7 years ago

comment:11 @AaronCampbell7 years ago

I'm still not sure if anyone is interested in having it both ways, but just in case, I fixed the autop issue in my patch too.

As a side note, while working on this my attention was drawn to #3670 because I inserted some JavaScript with CDATA tags in place of a shortcode. If you know what the purpose of this line in the_content() in wp-includes/post-template.php is for, I would like to know so I could patch that ticket:
$content = str_replace(']]>', ']]&gt;', $content);

comment:12 @tellyworth7 years ago

The shortcode unit tests pass with either 6444.002.diff or 6444.002.diff applied. But I have no wpautop tests - any samples of test data much appreciated.

comment:13 @markjaquith7 years ago

Hm, I wasn't too interested in letting people have it both ways, but your method gets around the problem of an inline shortcode being used on its own line and not getting paragraph-wrapped.

For example, if I had a [myname] shortcode that outputted "Mark Jaquith" and I did:

foo paragraph

[myname]

bar paragraph

I'd end up with:

<p>foo paragraph</p>
Mark Jaquith
<p>bar paragraph</p>

But with your patch I could just set that shortcode to run at 9 and it'd take care of it.

comment:14 @AaronCampbell7 years ago

I wasn't real interested in it at first either. I doubt I'll ever really need it, because I don't mind marking up the content I send it. However, I was aiming for two things:

1) Backwards compatibility. I wasn't sure how many plugin authors had already been developing using the existing method, assuming that their content would be formatted by WP.

2) There are a lot of less-than-thorough (I put that as nicely as I could) plugin developers, who need their content cleaned up just as badly as regular users do.

I want to make it clear that in the end, I'm completely happy with either way. I had already started with my fix before I found this ticket, so I figured I'd go ahead and offer it as a suggestion. It's definitely more complex/convoluted, but it's also slightly more flexible.

@markjaquith7 years ago

comment:15 @markjaquith7 years ago

6444.003.autop.diff builds on AaronCampbell's patch.

  • uses $after_formatting instead of $post_formatting because "post" has two contradictory meanings here
  • moves the gallery shortcode to after formatting

Testing now, but so far this looks like the best solution.

comment:16 @AaronCampbell7 years ago

Where in there is the part that moves the gallery shortcode to after formatting?

@AaronCampbell7 years ago

comment:17 @AaronCampbell7 years ago

6444.004.diff should be the same as you 003, but include the change to wp-includes.php to fix the gallery shortcode.

comment:18 @markjaquith7 years ago

(In [7699]) Allow shortcodes to run before or after wpautop()/texturize() formatting. Default to before for WP 2.5 compat. Props AaronCampbell. fixes #6444 for trunk

comment:19 @markjaquith7 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [7700]) Allow shortcodes to run before or after wpautop()/texturize() formatting. Default to before for WP 2.5 compat. Props AaronCampbell. fixes #6444 for 2.5.1

comment:20 @Viper007Bond7 years ago

Most excellent.

comment:21 @AaronCampbell7 years ago

I'm glad to see this in the trunk, a big thank you to markjaquith for making it happen. Now if we could just get #3670 fixed, my Google Maps plugin that uses shortcodes will work flawlessly!

comment:22 @tellyworth7 years ago

For whatever it's worth, being somewhat late to the party: I think the changes in [7699] and [7700] introduce unnecessary coupling that might cause problems later on. There's a problem to be solved but I'm not convinced that's the right way to do it.

Given that it establishes an API change that we'll have to support in future, is there time to reconsider it?

comment:23 @AaronCampbell7 years ago

tellyworth: So you are advocating the simpler solution where processing is simply moved to priority 11 rather than 9? If so, I'll say that I'm fine with that, but here are the advantages and disadvantages as I see them:

Advantages:

  • Less code/simpler code, which is actually a HUGE plus

Disadvantages:

  • Might break some current plugins (some plugin developers may have seen that the data was wpautop'd and developed accordingly)
  • While it's simpler to use in general, I think that some of the less experienced plugin developers could benefit from inserting pre-wpautop content. Having said that, they could just be taught to use wpautop on their content if needed.

Like I said, I'm up for either, I was mostly focused on the backwards compatibility for the early-adopters of shortcodes.

comment:24 @tellyworth7 years ago

I'm not necessarily advocating the priority 11 solution. I'm concerned with the API change and coupling this introduces, in particular:

  1. The $after_formatting arg and 11/9 priorities might be fine for the_content, but do_shortcodes() is intended for use elsewhere in future.
  1. The $after_formatting arg isn't passed down the stack if do_shortcodes() is called recursively. I'm not quite sure what the consequences of that are but they're likely to be confusing. Recursive calls are explicitly documented in the API codex - http://codex.wordpress.org/Shortcode_API

comment:25 @AaronCampbell7 years ago

I'm thinking about it, but so far I don't see a real quality way of handling this. As it sits now, the user is supposed to call do_shortcodes on their content if they want to allow recursive handling inside their tags, so they would need to call do_shortcodes($content) if they used pre-autop, and do_shortcodes($content, true); or do_shortcode_after_formatting($content); if they used the post-autop.

If this is ultimately meant for other applications (such as comments), I'm thinking it might be useful to allow for different sets of shortcodes anyway (more advanced than we currently have). I don't want one of my users adding [gallery id='123'] in order to add a gallery related to another post to this one.

I'm just trying to throw this out there for discussion, so we can solve as many problems as soon as possible, rather than later. It's better (in my opinion) to break backwards compatibility now (when there isn't a lot of history to worry about), than worry about it later when 100s of plugins already rely on it.

comment:26 @tellyworth7 years ago

I've been thinking about possible solutions, and there are some, but they're not trivial and require careful testing. (I've planned all along to expand the API later with a method of passing a stack down through do_shortcode() calls, but there are aspects of that I still haven't figured out yet).

On balance, I think that we should (trivially) break backwards compatibility now, back out the $after_formatting stuff, and go with the priority 11 solution (i.e. all shortcodes are parsed after formatting). There's nothing in the API docs that suggests the shortcode output will be formatted, and for developers who need it there's a simple solution -- call wpautop() yourself, as markjaquith suggested above.

comment:27 @tellyworth7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I just re-ran the unit tests with [7699] reverted and I think the differences are acceptable:

  1. There's a <p> tag around the gallery output
  1. Shortcode output isn't wpautop formatted

Those are the only changes I can see. I'm reopening the ticket to suggest we revert [7699] for 2.5.1.

comment:28 @ryan7 years ago

(In [7811]) Revert [770] for 2.5 pending further discussion. see #6444

comment:29 @ryan7 years ago

That should be [7700]

comment:30 @AaronCampbell7 years ago

I'm not sure I like the idea of it being wrapped in a <p> tag, but it's better than what we currently have.

comment:31 @markjaquith7 years ago

paragraph-wrapping block-level shortcodes is deal-breaker for me. We're discussing possible solutions in IRC.

@tellyworth7 years ago

compromise: don't p-wrap stand-alone shortcodes

comment:32 @tellyworth7 years ago

The new patch restores Mark's wpautop fix to unwrap p-tags from shortcodes that are placed alone in a block.

@tellyworth7 years ago

as per the last but with br's stripped too

@markjaquith7 years ago

comment:33 @markjaquith7 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [7815]) Don't wpautop()-wrap shortcodes that stand alone. fixes #6444 for trunk

comment:34 @markjaquith7 years ago

(In [7816]) Don't wpautop()-wrap shortcodes that stand alone. fixes #6444 for 2.5.1

comment:35 @AaronCampbell7 years ago

Knowing a bit more about the future plan, and reflecting on how short of a time shortCodes have been out (as far as the impact of breaking backwards compatibility), I'm happy with the solution. I'll test it out tomorrow, and update my google maps plugin to work with it.

Sorry I missed the IRC discussion, but it sounds like you guys hashed it out pretty well.

comment:36 @lkraav17 months ago

  • Cc leho@… added
Note: See TracTickets for help on using tickets.