WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 months ago

#14050 new defect (bug)

shortcode_unautop() should also remove the <br /> added after shortcodes

Reported by: aaroncampbell Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Formatting Keywords: dev-feedback needs-refresh wpautop
Focuses: Cc:

Description

Currently wpautop() wraps a shortcode in <p> tags as well as adding a <br /> tag after the shortcode. We then use shortcode_unautop() to remove the <p> tags, but the <br /> stays.

To replicate, just drop a few caption shortcodes into a post and set them all to align left or right. You'll see that even though they all float (assuming that's how your theme handles them) they stair step down because of the extra <br />

I'm not a regex expert so someone should probably double check my patch, but it seems to work for me.

Attachments (7)

14050.001.diff (586 bytes) - added by aaroncampbell 4 years ago.
14050.002.diff (586 bytes) - added by aaroncampbell 3 years ago.
shortcode_unautop-unit-tests.diff (908 bytes) - added by aaroncampbell 3 years ago.
Unit Tests
t14050-shortcode-unautop-upgrade-tests.diff (2.1 KB) - added by lkraav 5 months ago.
t14050-shortcode-unautop-upgrade.diff (4.6 KB) - added by lkraav 5 months ago.
is-your-regular-expressions-workspace-better.png (90.7 KB) - added by lkraav 5 months ago.
I'd really like to know what your toolset is, whoever you are deciding on whether to merge the work. Maybe this RegexBuddy setup demo I did my patches on is helpful to someone.
plugin.php (3.3 KB) - added by lkraav 5 months ago.
Drop-in plugin v1 to demonstrate and test current patchwork

Download all attachments as: .zip

Change History (32)

aaroncampbell4 years ago

comment:1 hakre4 years ago

  • Keywords has-patch dev-feedback needs-testing needs-unit-tests added

Please take note of existing wpautop() tickets that are releated to the <p> and <br> tag. I just compiled a list of related tickets in here: #13340 maybe it's of use, and I think there is one ticket related to unautop() already but don't blame me if not.

comment:2 nacin3 years ago

  • Milestone changed from Awaiting Review to Future Release

comment:3 aaroncampbell3 years ago

  • Cc aaroncampbell added
  • Keywords 3.2-early added

comment:4 bobbingwide3 years ago

Hi, I'm new to this Trac system but I have a couple of comments.

  1. This problem is documented as having been fixed in WP 2.5.1.

extract from http://codex.wordpress.org/Shortcode_API#Output

wpautop recognizes shortcode syntax and will attempt not to wrap p or br tags around shortcodes that stand alone on a line by themselves. Shortcodes intended for use in this manner should ensure that the output is wrapped in an appropriate block tag such as <p> or <div>.

Note: in WordPress 2.5.0, shortcodes were parsed before post formatting was applied, so the shortcode output HTML was sometimes wrapped in p or br tags. This was incorrect behaviour that has been fixed in 2.5.1.

  1. Shouldn't someone update the above text to say it's still a problem? Or is there some etiquette that says WordPress should not air its dirty washing.
  2. I've applied the fix and it's fine for me.
  3. I'm not sure if I agree with the comments in #13340 as they suggest that this fix will need a lot of regression testing. I agree that there may be some sites that will appear to break by the removal of the unwanted <br />'s BUT in my opinion their continued presence is particularly annoying given the above documentation.
  4. I would like to see this fix as a candidate for 3.0.6 (3.2 is far too late)
  5. If not then I would like to see the workaround documented. Actually I've had a go myself. See http://www.bobbingwidewebdesign.com/2011/02/21/shortcode-breaks-and-wordpress-bug-14050/

comment:5 aaroncampbell3 years ago

  1. This is similar to the problem that was fixed in 2.5.1, but it's not quite the same thing. At that point all shortcode content was being run through autop and it caused ALL THE CONTENT of the shortcode to be treated accordingly. Thus if your shortcode returned 3 paragraphs of text all three paragraphs would be wrapped in p tags. Right now if you return the same three paragraphs, they are left untouched except that a br tag is added at the end.
  2. You are more than welcome to update the codex as long as you are clear in describing what exactly is happening, AND clear that this behavior *may* change in the future (as this ticket is not yet "accepted" you shouldn't say *will*).
  3. I'm glad this fix works for you!
  4. In #13340 hakre is talking about changes to wpautop() and in this ticket we're not suggesting a change to that. Instead I want to make an adjustment to shortcode_unautop(). The former is used in many places and a small change can GREATLY affect things on the front end of thousands (or even hundreds of thousands) of sites. It's used on such a vast array of content that it's almost mind blowing to consider. However, shortcode_unautop() is used far less, and since it's specifically meant to undo what wpautop() does to shorcodes, I think it will be safe to effect this change.
  5. This fix will definitely not make it into 3.0.6 because there won't be a 3.0.6 (and if for some reason there is, it will only be for security issues). Likewise, this was punted from 3.1, which I agree with. There just isn't time to get something like this in there. Likewise it probably won't go in 3.1.1 because while I consider it a bug, it's not a regression. This is how it's worked ever since shortcode_unautop() was introduced. In my opinion 3.2 is the perfect time to try to get this fixed.
  6. I think the article you linked to is fine. When you update the codex to point out that you get a trailing br on shortcodes, make sure to mention that while this *may* be fixed in the future, right now you can put the shortcodes on the same line as a workaround.
Last edited 3 years ago by aaroncampbell (previous) (diff)

comment:6 aaroncampbell3 years ago

  • Milestone changed from Future Release to 3.2

comment:7 aaroncampbell3 years ago

  • Keywords 3.2-early removed

aaroncampbell3 years ago

comment:8 aaroncampbell3 years ago

Same patch, just refreshed for current trunk.

aaroncampbell3 years ago

Unit Tests

comment:9 aaroncampbell3 years ago

  • Keywords needs-unit-tests removed

Before patch:
Tests: 27, Assertions: 51, Failures: 3, Skipped: 4.

After patch:
Tests: 27, Assertions: 51, Failures: 2, Skipped: 4.

The two that are failing were failing when I first set everything up. I'm not sure if we need to look into them or if they were already like that (test_utf8_whitespace_1 and test_utf8_whitespace_2)

Last edited 3 years ago by aaroncampbell (previous) (diff)

comment:10 aaroncampbell3 years ago

  • Keywords 3.3-early added; needs-testing removed
  • Milestone changed from 3.2 to Future Release

Moving to 3.3. See also #15600

comment:11 pauldewouters2 years ago

  • Cc pauldewouters added

comment:12 aaroncampbell2 years ago

  • Keywords needs-refresh added; has-patch removed

comment:13 MikeHansenMe15 months ago

  • Cc mdhansen@… added

Is this still a problem? I have been trying to reproduce but do not get a <br/> anywhere in the returned shortcode or after the shortcode. Just wondering if the patch needs refreshed again or if the ticket is obsolete.

comment:14 follow-up: SergeyBiryukov15 months ago

shortcode_unautop() was rewritten in [18952], however the associated test still fails:

1) Tests_Shortcode::test_multiple_shortcode_unautop
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'[footag]
-[footag]
+'<p>[footag]<br />
+[footag]</p>
 '

comment:16 aaroncampbell9 months ago

  • Keywords 3.3-early removed
  • Milestone changed from Future Release to 3.7

comment:17 follow-up: nacin7 months ago

  • Milestone changed from 3.7 to Future Release

Still needs a patch after [18952].

comment:18 lkraav5 months ago

  • Cc leho@… added

comment:19 in reply to: ↑ 14 lkraav5 months ago

Replying to SergeyBiryukov:

shortcode_unautop() was rewritten in [18952], however the associated test still fails:

1) Tests_Shortcode::test_multiple_shortcode_unautop
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'[footag]
-[footag]
+'<p>[footag]<br />
+[footag]</p>
 '

This is really bugging a few of my projects, so I'm trying to attack the following. Not sure yet if the underlying problem is the same as for the quoted.

2) Tests_Shortcode::test_shortcode_unautop_html
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
 '<h1>Fun, safe, small group tours &amp; no roughing it</h1>
-
-[footag]
-
-Enjoy guided day walks in extraordinary natural areas. Return to comfortable accommodation and delicious meals each evening. No heavy backpack, no camping.
-
-[/footag]'
+[footag]</p>
+<p>Enjoy guided day walks in extraordinary natural areas. Return to comfortable accommodation and delicious meals each evening. No heavy backpack, no camping.</p>
+<p>[/footag]
+'

EDIT phpunit --group 14050 | colordiff helps.

Last edited 5 months ago by lkraav (previous) (diff)

comment:20 lkraav5 months ago

Here's a very nice online regex tool, that lets you permalink the whole setup. My test from comment:19 is at http://www.phpliveregex.com/p/29E

EDIT I found it challenging trying to get that shortcode_unautop regex string back into one piece. Is there some best practice for that, or do we just temporarily say echo $pattern for copy-pasting?

EDIT2 Whoever is working on this (type of stuff), get RegexBuddy. It really helps getting a grip on this monster.

Last edited 5 months ago by lkraav (previous) (diff)

comment:21 in reply to: ↑ 17 lkraav5 months ago

Replying to nacin:

Still needs a patch after [18952].

New regex from [18952] doesn't match shortcodes with attributes at all, while the previous short one does.

<p>[hypertag class="foo"]Bar[/hypertag]</p>

Is this a regression then?

comment:22 lkraav5 months ago

Above is a squash of the following 4 commits.

* e18aa9f - (HEAD, master) Formatting: shortcode_unautop() should support shortcode attributes, see #14050 (17 minutes ago) <Leho Kraav>
* f05a03e - Formatting: shortcode_unautop() clarifying comment, see #14050 (17 minutes ago) <Leho Kraav>
* 2bddf79 - Formatting: shortcode_unautop() should support shortcodes on separate lines, see #14050 (17 minutes ago) <Leho Kraav>
* 2ac5bcd - Formatting: shortcode_unautop should support [footag /], see #14050 (18 minutes ago) <Leho Kraav>
* 6927ba5 - (origin/master, origin/HEAD) Fix JSHint errors in widgets.js. (16 hours ago) <Sergey Biryukov>
...

Unit tests are separate from SVN because I haven't found a a git repo for them yet, and didn't feel like cloning it myself.

Putting a near-full working day into this made me understand better what exactly is going on here. Regex from [18952] is missing many things that are standard operation in WordPress for a long time and even seems to regress on a couple of features. Seems kind of important to me, because HTML output will be broken if the user makes the slightest "mistake" in his shortcode entry process. I'm not even sure what the "safe" way to enter shortcodes is right now.

To me, double-linebreak

[foo]

content

[/foo]

[bar]

[proton]

seems the cleanest and wpautop() is also agreeing with this statement so far (#24085).

Funny thing is, I really don't know what to do with that original test case from comment:14. If someone could state the rules / algorithm for it in plain English, I could probably patch the regex up for that, too.

lkraav5 months ago

I'd really like to know what your toolset is, whoever you are deciding on whether to merge the work. Maybe this RegexBuddy setup demo I did my patches on is helpful to someone.

lkraav5 months ago

Drop-in plugin v1 to demonstrate and test current patchwork

comment:23 follow-up: lkraav3 months ago

Just wondering, for this to ever move forward, who is the core guy/girl capable of leading it?

I'm activating my hotfix plugin on all my new sites in development and it is performing without mistakes. Obviously I'd like to get off that maintenance train eventually..

Talking about my own patchwork, this could also be moved forward one patch at a time to minimize breakage surface. I built the improvements independent of each other to simply support additional different ways of entering shortcodes.

Wondering if in the meanwhile I should upload the drop-in to Plugins repo and try to get a bigger testing crowd going.

comment:24 in reply to: ↑ 23 samuelsidler3 months ago

Replying to lkraav:

Just wondering, for this to ever move forward, who is the core guy/girl capable of leading it?

Probably @aaroncampell or @nacin.

comment:25 nacin3 months ago

  • Keywords wpautop added
Note: See TracTickets for help on using tickets.