WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 8 years ago

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

Download all attachments as: .zip

Change History (44)

#1 @westi
8 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

#2 @lloydbudd
8 years ago

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

@markjaquith
8 years ago

#3 @markjaquith
8 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.

#4 @markjaquith
8 years ago

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

#5 @markjaquith
8 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

#6 @markjaquith
8 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

#7 @AaronCampbell
8 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.

#8 @markjaquith
8 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

#9 @markjaquith
8 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.

#10 @markjaquith
8 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.

#11 @AaronCampbell
8 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);

#12 @tellyworth
8 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.

#13 @markjaquith
8 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.

#14 @AaronCampbell
8 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.

#15 @markjaquith
8 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.

#16 @AaronCampbell
8 years ago

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

#17 @AaronCampbell
8 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.

#18 @markjaquith
8 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

#19 @markjaquith
8 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

#20 @Viper007Bond
8 years ago

Most excellent.

#21 @AaronCampbell
8 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!

#22 @tellyworth
8 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?

#23 @AaronCampbell
8 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.

#24 @tellyworth
8 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

#25 @AaronCampbell
8 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.

#26 @tellyworth
8 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.

#27 @tellyworth
8 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.

#28 @ryan
8 years ago

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

#29 @ryan
8 years ago

That should be [7700]

#30 @AaronCampbell
8 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.

#31 @markjaquith
8 years ago

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

@tellyworth
8 years ago

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

#32 @tellyworth
8 years ago

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

@tellyworth
8 years ago

as per the last but with br's stripped too

@markjaquith
8 years ago

#33 @markjaquith
8 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

#34 @markjaquith
8 years ago

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

#35 @AaronCampbell
8 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.

#36 @lkraav
2 years ago

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