Ensure the entire template is passed to the output buffer callback for Optimization Detective to process

[westonruter]

This is based on @nextend's recommendation in WordPress/gutenberg#61212 (comment) feedback which is drafted in WordPress/gutenberg#62770. If a template calls ob_clean() at present, the output buffer being thrown away will be sent into the output buffer callback for needless processing even though it won't be served. This PR fixes addresses this issue by making sure the output buffer phase is PHP_OUTPUT_HANDLER_FINAL. Another issue is that calls to ob_flush() will result in template fragments being sent into the output buffer callback. To address these issues:

1. The output buffer is started with the PHP_OUTPUT_HANDLER_FLUSHABLE removed. This results in calls to ob_flush() not having any effect.
2. The output buffer callback checks if the callback was invoked due to a call to ob_clean(), in which case the callback short-circuits and does not pass the buffer into the filter for processing.

For more details, see #1317 (comment).

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <[email protected]>
Co-authored-by: nextend <[email protected]>
Co-authored-by: felixarntz <[email protected]>
Co-authored-by: adamsilverstein <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[felixarntz]

@westonruter This is probably good, I'd just like to understand it better. :)

If this is merged, should we also update https://github.com/WordPress/performance/blob/trunk/plugins/performance-lab/includes/server-timing/class-perflab-server-timing.php#L272?

[felixarntz]

A few questions so that I understand this:

• This is only relevant for when someone calls ob_clean()? Or for other things as well?
• What's the purpose of $phase and what's the purpose of PHP_OUTPUT_HANDLER_FINAL in this? Why are both checks needed?

[westonruter]

• This is only relevant for when someone calls ob_clean()? Or for other things as well?

That's a good point. There's also the consideration of ob_flush(). Currently if this gets called then the resulting buffer passed into the filter is not complete. I've improved this in f218dc8 by making it so that the buffer is cleanable but not flushable.

• What's the purpose of $phase and what's the purpose of PHP_OUTPUT_HANDLER_FINAL in this? Why are both checks needed?

The $phase provides information about why the handler was invoked. Note that bitwise operations are being used here (& not &&), so it is checking if the $phase contains the final flag.

[felixarntz]

@westonruter Thanks, I had indeed missed the bitwise &, so this is only one check, makes sense.

Reading your code comments around the custom flag handling, I'm trying to understand: Does this mean that calling ob_flush() anywhere while this output buffer is active will trigger an error? If so, I think we need to review whether/how that function is commonly used by plugins, mostly caching plugins of course. While it seems necessary for our functionality to be reliable, it also seems risky in terms of cross-plugin compatibility, like you're already indicating in your TODO.

[westonruter]

It won't cause an error, no. It just results in ob_flush() returning false, which is included in the unit tests.

I'm addressing the TODO in another PR as it's not really related to output buffering.

[felixarntz]

Ok, but what about breakage by flushing not being possible in a plugin where that might be intended? That's where I think we need to take a look at the ecosystem to see if that's a real or just a theoretical problem.

[westonruter]

That would be a situation where a plugin is attempting to flush the output buffer for a buffer it didn't open, which would be poor form. I did look over the instances of ob_flush() in WPDirectory and I didn't see any obvious examples where ob_flush() was being used in frontend contexts without also having called ob_start(): so they'd only be flushing their own buffer. So I think we're fine here. We'll need to do more testing with actual sites to discover if there is a compatibility problem.

[westonruter]

If this is merged, should we also update https://github.com/WordPress/performance/blob/trunk/plugins/performance-lab/includes/server-timing/class-perflab-server-timing.php#L272?

Actually, I don't think so because for server timing we're not manipulating the buffer in any way. We're simply using output buffering to delay sending the headers.

[westonruter]

This actually does need to be removable because WordPress core calls wp_ob_end_flush_all() at shutdown, and the lack of PHP_OUTPUT_HANDLER_REMOVABLE results in:

Notice: ob_end_flush(): Failed to send buffer of Closure::__invoke (1) in /var/www/html/wp-includes/functions.php on line 5427

[westonruter]

Addressed in 0b4f2cd

[westonruter]

@nextend I did some more testing and it seems there is something else this is not accounting for: whether there are any calls to ob_flush(). If so, then the result is that fragments of the entire template are sent into the output buffer callback. I think what is required is to not check for PHP_OUTPUT_HANDLER_FINAL but rather:

1. Call ob_start() without the PHP_OUTPUT_HANDLER_FLUSHABLE flag.
2. Return early if $phase & PHP_OUTPUT_HANDLER_CLEAN.

The only remaining question I have is whether ob_start() can be called with the PHP_OUTPUT_HANDLER_REMOVABLE flag as well. This is required for unit testing, but for normal page responses would there ever be a need to end the buffer? For example, I understand there are two ways that an entire template may be buffered for processing:

ob_start( 'process_output_buffer' );

And:

add_action( 'template_redirect', static function () {
	ob_start();
	add_action(
		'shutdown',
		function () {
			echo process_output_buffer( ob_get_clean() );
		}
	);
} );

(This seems brittle given that other output buffers may have been opened. So additional logic would be needed to check if the ob_get_level() is the same as when ob_start() was opened.)

And clearly the former is simpler! Also, if the output buffer were to be started without PHP_OUTPUT_HANDLER_REMOVABLE, then only the former would be an option. However, presently this it not compatible in WP because wp_ob_end_flush_all() is called at shutdown, resulting in a warning:

Notice: ob_end_flush(): Failed to send buffer of Closure::__invoke (1) in /var/www/html/wp-includes/functions.php on line 5427

To work around that, the wp_ob_end_flush_all() call would need to be removed:

remove_action( 'shutdown', 'wp_ob_end_flush_all', 1 );

I believe this may be obsolete since it was introduced to fix compatibility with PHP 5.2, but I'm wary of the implications for removing this. In particular, the shutdown_action_hook() function is triggering the shutdown action to call wp_ob_end_flush_all() and then afterward it does wp_cache_close(). If the open buffers aren't all flushed before calling wp_cache_close() then it could be that a page caching plugin that is storing the output in an external object cache could fail to store anything since the cache would be closed. So to safely start a non-removable buffer, core would need to stop doing wp_ob_end_flush_all() and also stop doing wp_cache_close() so that the cache is available when the output buffers implicitly close at the end of execution. This being said, take note of the function description for wp_cache_close():

 * This function has ceased to do anything since WordPress 2.5.
 * The functionality was removed along with the rest of the persistent cache.
 * This does not mean that plugins can't implement this function when they need
 * to make sure that the cache is cleaned up after WordPress no longer needs it.

In core, this function just does return true. I checked WPdirectory and most plugins also define the function in the same way. However, some plugins do this:

function wp_cache_close() {
	global $wp_object_cache;

	return $wp_object_cache->close();
}

These plugins in particular to something like the above:

• Speed Optimizer (1,000,0000 installs)
• A2 Optimized WP (80,000 installs)
• Docket Cache (10,000 installs)

So indeed it seems like we cannot use a non-removable output buffer without risking the object cache being closed when a plugin may want to leverage the od_template_output_buffer filter to store the output buffer in the object cache (that is, when/if the filter is merged into core as wp_template_output_buffer to implement Core-43258).

[felixarntz]

Not sure about using assert() as I don't think there's a precedent in WP or this plugin for that. Can we use one of the more common ways to aid debugging in WordPress?

[westonruter]

These are sanity checks that should not ever return false given the flags.

There are instances of assert() being used in core, although in bundled libraries: https://github.com/search?q=repo%3AWordPress%2Fwordpress-develop+%2Fassert%5C%28%2F+path%3A%2Fsrc%2F&type=code

[felixarntz]

Right, but not in WordPress itself, and I don't know how relevant the presence of these flags is in said code, e.g. whether the code paths are even reached.

If they should never return false, then I'd say we should either go with what's more established for debugging in WordPress, or simply omit them.

[westonruter]

OK. Removed in 364fa43.

[felixarntz]

Is this TODO intended for this PR or another issue?

[westonruter]

Yes, I'm opening a sub-PR for this. It's unrelated to output buffering.

[westonruter]

See #1442

[felixarntz]

@westonruter

If this is merged, should we also update https://github.com/WordPress/performance/blob/trunk/plugins/performance-lab/includes/server-timing/class-perflab-server-timing.php#L272?

Actually, I don't think so because for server timing we're not manipulating the buffer in any way. We're simply using output buffering to delay sending the headers.

Could the way that Server-Timing code is currently written lead to duplicate output of the header?

[westonruter]

Could the way that Server-Timing code is currently written lead to duplicate output of the header?

Oh, yes, you're right.

[westonruter]

For reviewing this PR now, disregard the changes introduced in these follow-up PRs (sub-PRs) #1442 and #1443 which were merged into this branch.

[felixarntz]

Thanks @westonruter!

[adamsilverstein]

swell