Reproducible Output Failure

[miqrogroove]

Steps to reproduce:

1. Open any browser and start a new post.

2. Paste the attached file into HTML view.

3. Click the Preview button.

Expected Result: Preview :)

Actual Result: Blank Post D:

Tested only on the one site so far.

[miqrogroove]

This post body makes WordPress epic fail.

[ninjaWR]

Using r12273 and 2.7.1 (first time in a while using that site), I don't have any problems with the attached code.

In any case, moving to Future Release since 2.9 is in beta.

[scribu]

What is the URL of the page you are sent to?

If it contains post-new.php, it just means that the post hasn't been saved yet. In that case, this ticket would be invalid.

[miqrogroove]

Hi

ninjaWR, I'm not sure if the 2.7.1 test would be relevant. Hopefully I will have some time to set up a proper test environment to get some more detail for you. Moving back to 2.9 because that's what betas are for. If we can't reproduce this bug before release then it has no business being assigned to another version.

scribu, the URL can't be relevant, because I was able to reproduce this on a published post as well as in preview.

This bug is blocking me from reporting some other bugs, so I will be increasing the Severity if I can pinpoint the cause/trigger.

[azaozz]

Seems to work as expected in trunk. Technically you cannot preview something that's not in the db yet so when clicking Preview for an unsaved post, it's saved as draft first and then the browser is redirected to a new tab/window to display it. So the URL your browser is redirected to may be relevant when that fails.

[miqrogroove]

Problem did not show up on a clean install, nor with the usual suspect plugins.

Debugging on the problem site so far shows that the function wpautop() is not returning a value when filters such as 'the_content' are applied.

[miqrogroove]

I traced it down to the last line of function wpautop()

$pee = preg_replace('/<p>\s*?(' . get_shortcode_regex() . ')\s*<\/p>/s', '$1', $pee);

I don't have time tonight to debug get_shortcode_regex(), but I'll keep you all posted when I do.

[dd32]

Sounds like the all too common preg backtrace limit to me: #8553

[miqrogroove]

hi dd32, thanks for the heads up, but if this little post of mine triggered 100,000 recursion depth then something has gone horribly wrong. I'm going to check it out now.

[miqrogroove]

I've confirmed get_shortcode_regex is not the cause of the problem.

It's starting to look like there is a conflict between the P elements and shortcode matching.

Note the regexp is incorrect for situations like

<p>[shortcode]<p>[shortcode]<p>Hello World</p>

... which is exactly what wpautop() is emitting before the last statement. Could the syntax error be causing the recursion problem? I guess I'll have to test that theory as well.

[dd32]

hi dd32, thanks for the heads up, but if this little post of mine triggered 100,000 recursion depth then something has gone horribly wrong. I'm going to check it out now.

I assume you actually checked the depth on your setup? I've seen it set incredibly small :)

If that wasnt your problem (then GREAT!.. because that tickets been open for awhile now)

The syntax could definately be causing it, causing the regex not to match and then infinitely loop.. or, remove the entire post if it matches the right(wrong) rule possibly.

[miqrogroove]

Wait a sec. Doesn't "\2" backtrace the WRONG value?

And on another note, removing some of the extraneous parentheses from get_shortcode_regex() actually solved my test case, but I'm still looking if it's dependent on post length before patching.

[miqrogroove]

I'm too sexy for my PHP [) Patch coming right up.

[miqrogroove]

Resolves unreasonable recursion depth during wpautop. Explanation to follow.

[miqrogroove]

The primary change made by this patch is to remove the following code, which until now has served only to heat up server CPUs across the planet:

(?:(\/))?

It is replaced by the equivalent non-recursive form:

\\/?

This patch also fixes a related bug wherein wpautop() backreferences the wrong preg match when looking for shortcode close tags like /caption.

[miqrogroove]

First version missed strip_shortcodes

[miqrogroove]

I'm working on better steps to reproduce. There's something going on before that one line of code that's causing the bug to show up/not on my different sites.

[miqrogroove]

OK FYI, when you use my file wordpress-io-crash.html, WordPress will rewrite all of the HREF values based on the attachment ID values. This will prevent you from reproducing the bug using my code.

[scribu]

If you can prove that your patch performs better and doesn't cause any regressions, that would be good enough.

[miqrogroove]

Test Case demonstrates failure of the last assignment to $pee in wpautop()

[miqrogroove]

Hi scribu, using the new Test Case, I am able to fail the test using the old regexp, and pass it using the new regexp, on two different servers. The patch is also installed and running on my 2.8.4 site with no problems.

[azaozz]

Not sure if we need to use the same regex there, no need to capture all the stuff that's needed for parsing the shortcodes.

We are only looking for <p> before and </p> after. Perhaps it would be better to build another pattern using the global $shortcode_tags, even split it in two. Wouldn't be affected by future changes to get_shortcode_regex() and will be faster.

[miqrogroove]

Alternative patch design with changes suggested by azaozz.

[azaozz]

(In [12302]) Separate the removal of <p> wrapping from shortcodes into another function and apply it with different filter, props miqrogroove, props mdawaffe, see #11257, see #11249

[ryan]

Downgrading severity since it seems like the critical part is taken care of.

[miqrogroove]

ryan, are you sure shortcodes.php isn't going to NULL-out the content right after wpautop() finishes? I haven't tested the patch piecemeal like this.

[miqrogroove]

OK it does pass the test case without the second half of the patch. The shortcodes.php portion will just be a performance boost until someone fixes #8553

[miqrogroove]

Punting the performance boost part of this patch seems like a waste of time. Can we call it fixed and move on to something that's still broken?