Unintended blank lines in plugins break XML for feeds

[bchecketts]

A plugin or theme file may unintentionally output blank lines. When displaying an RSS feed, this breaks XML validation of the page.

To recreate, simply create and activate a plugin that has an extra blank line after the closing ?> characters in the file.

Google Chrome displays this error. Other browsers display something similar:

error on line 4 at column 6: XML declaration allowed only at the start of the document

Although technically a problem with the plugin, Wordpress can easily be modified to discard the undesired output by calling ob_clean() prior to outputting the XML. It seems that ob_start() is already called earlier, so ob_clean() discards any output since that call.

For example, in the default configuration, you can add this line in wp-includes/feed-rss2.php just above the header() function call (in about line 7)

ob_clean();

The top of wp-includes/feed-rss2.php now looks like this:

<?php
/**
 * RSS2 Feed Template for displaying RSS2 Posts feed.
 *
 * @package WordPress
 */

ob_clean();
header('Content-Type: ' . feed_content_type('rss-http') . '; charset=' . get_option('blog_charset'), true);
$more = 1;

[filosofo]

This complaint appears every so often (see #9610 for example), and the consensus seems to be that we shouldn't add core bloat to paste over plugins' problems.

It seems that ob_start() is already called earlier,

Where is that?

[miqrogroove]

It would be so easy to fix this. function activate_plugin() calls ob_end_clean() without checking the buffer. With an if statement a few lines above that, WordPress could assert wp_error is returned if the buffer isn't empty. In effect, anyone who tries to activate a plugin that dirties the output stream will see a syntax error and the plugin will not activate.

[nacin]

According to the phpdoc, it looks like that is what was intended:

 333   * If any errors are found or text is outputted, then it will be captured to
 334   * ensure that the success redirection will update the error redirection.

[filosofo]

Replying to miqrogroove:

It would be so easy to fix this. function activate_plugin() calls ob_end_clean() without checking the buffer. With an if statement a few lines above that, WordPress could assert wp_error is returned if the buffer isn't empty. In effect, anyone who tries to activate a plugin that dirties the output stream will see a syntax error and the plugin will not activate.

The problem is that often these errors creep in attached to action hooks that won't fire during activation.

[miqrogroove]

I was under the impression this ticket was about syntax errors such as whitespace after the closing PHP tag. If a plugin is randomly generating "blank lines" as a result of actions, then it's an awful, horrible plugin. :P

Still, wouldn't it be nice to catch those closing tag errors using just a few extra lines of code?

[filosofo]

Replying to miqrogroove:

Still, wouldn't it be nice to catch those closing tag errors using just a few extra lines of code?

In my opinion it would be better to give plugins the necessary feedback they need to correct the problem (e.g., "I activated your plugin and my feed broke.").

If we're going to go the route of flushing all output printed before X event, we need to do it in a thorough, controlled way, perhaps as part of a templating system, not just tacked on to a few arbitrary parts of WP (such as feeds).

Besides, how would you propose to implement this? If you start output buffering say before plugins are loaded, when are you going to flush it in the non-feed situations?

[miqrogroove]

Maybe we're not on the same page yet. I'm thinking of heres-my-bad-plugin.php:

<?php
/** Plugin-Author: Me. **/
add_action('blah', 'phpinfo');
?>
oops there's stuff here

In activate_plugin, there's a buffer that's used to block non-fatal errors. If the buffer survives, it gets deleted and the plugin is activated. I'm proposing a fix so that the buffer is examined, and if it's not empty for any reason, it should get reported the same way as the fatal errors. The buffer could say "oops there's stuff here", or it could just as easily say, "Warning: blah not defined, assuming 'blah'". Prefix that with something informative like, "This plugin generated the following unexpected output:"

[filosofo]

Replying to miqrogroove:

Maybe we're not on the same page yet. I'm thinking of heres-my-bad-plugin.php:

Ok, I thought you were talking about fixes like the OP's, not just during activation.

I'm proposing a fix so that the buffer is examined, and if it's not empty for any reason, it should get reported the same way as the fatal errors.

That sounds like a good idea.

[miqrogroove]

For anyone who's curious, this plugin caused a deactivation failure.

[miqrogroove]

I just did a baseline test and found that the admin screen failed to load when I deactivated the plugin. I'll see if I can pin that down while I'm hacking on the activation parts.

[miqrogroove]

Hack for wp_redirect errors

[miqrogroove]

This version gets us to a wp_die() screen when activating testplug.php. It's simple and it works. I'm looking now at how crazy it would get if we try to keep it in the "sandbox" rather than using wp_die().

[miqrogroove]

Full solution with new "sandbox" error for unexpected output.

[miqrogroove]

And now it's ready :)

[miqrogroove]

Don't override WP_DEBUG, props sivel. Also discard the error control operator.

[nacin]

Mark wrote the original sandbox. Assigning to him.

[miqrogroove]

Changeset [13167] by markjaquith

Detect plugin output on activation, as well as fatal errors. props miqrogroove. fixes #12089

[hakre]

That would not be for feeds only but for proerply parsed "XHTML as in XML" pages as well, right?

---

What I strongly dislike with this patch is that it is using ob_ output functions. The usage of those has not been clearly defined until today. That can really mess things up and this patch might not work as intended.

the ob-length need to be measured before and after the include. the delta between the two values should be 0, not the overall length. that should make it more robust.

I would love to see a comment on this one prior to open (another) new ticket.

[nacin]

ob_start() is right above the include. On activation a plugin should not be echoing anything, whether it is a warning or blank lines. So your delta is equal to ob_get_length().

[miqrogroove]

Viper007Bond reported an error activating Akismet in debug mode, where the error was reproducible in the sandbox, but would never display in the error window.

Reopen. Expect patch to put the error window in the sandbox so as to generate identical output during the error_scrape.

[miqrogroove]

Fix error window scope

[markjaquith]

(In [13452]) Plugin activation attempt goes through a function to show the same errors as the test. props miqrogroove. fixes #12089

[hakre]

Just a slight variant of play

[hakre]

Plugins that are reporting output on activation should be considered to be deactived completely. I think about adding a button if that happens on activation so a user can directly undo the activation.

Could be a more user-friendly step in between preventing the activation completely.

[hakre]

Related: #16485