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

[johnconners]

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.

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.

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

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

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

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.

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

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

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.

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);

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.

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.

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.

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.

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

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

(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

(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

Most excellent.

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!

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?

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.

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.

2. 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

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.

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.

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

2. 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.

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

That should be [7700]

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.

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

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

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

as per the last but with br's stripped too

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

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

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.