Make WordPress Core

Opened 13 years ago

Last modified 43 hours ago

#22239 new defect (bug)

wp_ob_end_flush_all() tries to destroy non-destroyable output buffers

Reported by: dd32's profile dd32 Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.2
Component: Bootstrap/Load Keywords: has-patch has-unit-tests dev-feedback has-test-info
Focuses: Cc:

Description

wp_ob_end_flush_all() currently tries to flush & destroy every PHP Output buffer that is enabled on the current installation.
However, not every type of PHP Output buffer can be destroyed, which will cause a PHP Notice when attempts to do so are made.
An example of this is PHP extensions such as New Relic, or PHP output buffers created with the $erase flag set to false in ob_start()

An example is when New Relic is installed on a host, also see New Relics FAQ entry on the issue.

( ! ) Notice: ob_end_flush(): failed to send buffer of New Relic auto-RUM (1) in ../trunk/wp-includes/functions.php on line 2641
Call Stack
#	Time	Memory	Function	Location
1	0.8510	4328360	shutdown_action_hook( )	../load.php:0
2	0.8510	4328440	do_action( )	../load.php:556
3	0.8510	4329856	call_user_func_array ( )	../plugin.php:406
4	0.8510	4329888	wp_ob_end_flush_all( )	../plugin.php:406
5	0.8511	4330064	ob_end_flush ( )	../functions.php:2641

Somewhat similar to #18239
I'm not sure of what the ideal solution for this would be for WordPress, but wanted to record the issue being known.

Attachments (1)

22239.diff (645 bytes) - added by dd32 13 years ago.

Download all attachments as: .zip

Change History (21)

#1 @dd32
13 years ago

  • Summary changed from wp_ob_end_flush_all() tries to flush non-flushable buffers to wp_ob_end_flush_all() tries to destroy non-destroyable output buffers

@dd32
13 years ago

#2 @dd32
13 years ago

One option is to just silence the PHP Notice to get the message out of developers faces.. (Ignore the extra ob in the diff..)

#3 @nacin
11 years ago

  • Component changed from Warnings/Notices to Bootstrap/Load

#4 @chriscct7
10 years ago

  • Keywords has-patch needs-refresh added

Extra item in the docblock

This ticket was mentioned in PR #5771 on WordPress/wordpress-develop by @kkmuffme.


18 months ago
#5

  • Keywords needs-refresh removed

#6 @kkmuffme
18 months ago

  • Version set to trunk

I created a proper fix for this now, please merge.

The fix checks if the ob can be ended or not - and if it can't it aborts (since any parents cannot be ended either, since we can't skip a level).

What do I need to do so this gets included in the next WP version (6.5)?

#7 @kkmuffme
18 months ago

I guess my PR also fixes these issues:
#22430
#55833
#18525

As well as tons of issues people had but didn't report in trac (see stackoverflow and the first couple pages of Google)

Last edited 14 months ago by SergeyBiryukov (previous) (diff)

This ticket was mentioned in Slack in #core by kkmuffme. View the logs.


16 months ago

#9 @swissspidy
16 months ago

  • Milestone set to Future Release

#10 @swissspidy
16 months ago

  • Keywords needs-testing added

The existing patches in those other tickets have lots of discussion around them and vastly different approaches, including things like padding the output etc.

How can testers verify that this patch here addresses those cases as well?

#11 @kkmuffme
16 months ago

How can testers verify that this patch here addresses those cases as well?

By testing it for those cases? :) Can you please clarify what you mean?

The padding stuff was for some old IE browsers (that PR is 12 years old) and not relevant anymore today (you can check WP core where these comments were removed)

#12 @swissspidy
16 months ago

Sure, I meant that testing instructions would be helpful for contributors to verify that this patch works as intended.

#13 @kkmuffme
16 months ago

Multiple possible scenarios. I guess the easiest is to just turn on zlib.output_compression in the ini.

Without server modification, I guess this should work too (would give a notice in current case, but notice is fixed with the PR):

advanced-cache.php

<?php
function my_ob_cb( $output ) {
    return $output;
}

ob_start( 'my_ob_cb', 0, 0 );

#14 @jorbin
14 months ago

  • Version changed from 6.5 to 2.2

Updating version based on when wp_ob_end_flush_all was introduced since this is not new to 6.5

This ticket was mentioned in Slack in #core by kkmuffme. View the logs.


14 months ago

This ticket was mentioned in Slack in #core-test by ankit-k-gupta. View the logs.


12 months ago

#17 @jorbin
12 months ago

  • Keywords needs-unit-tests added

This might require building additional docker images for testing, but I think this needs some automated tests before it lands.

#18 @kkmuffme
12 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

@jorbin I added unit tests that show how/that it works as previously with regular obs (= the unit tests that should have existed previously already)
Additionally I added a unit test to show how it works/fixes the issue with non-removable handlers, however this is skipped due to a bug in phpunit itself (see https://github.com/sebastianbergmann/phpunit/pull/5852)
However, this shouldn't be a blocker as the unit tests I added show that it still works as it did previously.

This ticket was mentioned in Slack in #core by mikachan. View the logs.


12 months ago

#20 @SirLouen
43 hours ago

  • Keywords dev-feedback has-test-info added; needs-testing removed

Test Report

Description

✅ This report validates that the indicated patch works as expected.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/5771.diff

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.28
  • Server: nginx/1.27.5
  • Database: mysqli (Server: 8.4.5 / Client: mysqlnd 8.2.28)
  • Browser: Chrome 136.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Nineteen 3.1
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Reproduction Steps

  1. First, zlib.output_compression = 1 should be added to php.ini in the server. Otherwise, it will not happen. In wordpress-develop add it to tools/local-env/php-config.ini. The OP was referring to New Relic, that according to the Archive, it seems that also deals with zlib.
  2. Now simply add the code attached in Artifacts, wherever it can be executed in WP (a plugin, functions.php, you name it)
  3. 🐞 The first part of the code triggers the notice: ( ! ) Notice: ob_end_flush(): Failed to send buffer of my_ob_cb (1) in /var/www/src/wp-includes/functions.php on line 5470

Expected Results

  • Notice should be handled

Actual Results

  1. ✅ Issue resolved with patch.

Additional Notes

  • ⚠️ The only problem is what @kkmuffme mentions: He cannot really unit test the error here with ob_start(callable,0,0), because of a limitation in PHPUnit. I wonder how this is handled. I suppose that the committer should subscribe to the GitHub issue to see if there are any changes in the future and update unit tests to remove the skipped part.
  • The original solution by @dd32 was not great because it did not handle the problem, it only ignored it.
  • @jorbin, apart from this issue, this patch is ready to be shipped.

Supplemental Artifacts

Code for testing

function my_ob_cb( $output ) {
    return $output;
}
ob_start( 'my_ob_cb', 0, 0 );

function my_ob_cb2( $output ) {
    return $output;
}
ob_start( 'my_ob_cb2' );
echo 'Hello';
ob_end_flush();
Note: See TracTickets for help on using tickets.