diff --git a/inc/compatibilities/groovy_menu.php b/inc/compatibilities/groovy_menu.php index 2e61cf01..34fe0c83 100644 --- a/inc/compatibilities/groovy_menu.php +++ b/inc/compatibilities/groovy_menu.php @@ -4,15 +4,14 @@ * * @reason Groovy Menu's auto-integration opens its own output buffer on `init` * and, on `shutdown` at priority 0, calls ob_get_clean() on whichever buffer is - * on top to insert the menu markup after . Since 4.2.12 we capture and - * process our buffer at `shutdown` (PHP_INT_MIN) and re-arm an empty one, so - * Groovy Menu receives an empty string, finds no and drops the menu. + * on top to insert the menu markup after . * - * We apply Groovy Menu's final-output filter to the page we capture, before - * image replacement, and unhook its own shutdown step at that moment. The menu - * is inserted regardless of buffer order and its images are optimized too. When - * our capture does not run (legacy `optml_capture_at_shutdown` mode, or a - * third-party flush of our buffer) Groovy Menu keeps its own shutdown step. + * This only matters with the `optml_capture_at_shutdown` opt-in, where we capture + * and process our buffer at `shutdown` (PHP_INT_MIN) and re-arm an empty one, so + * Groovy Menu would receive an empty string and drop the menu. In that mode we + * apply Groovy Menu's final-output filter to the page we capture, before image + * replacement, and unhook its own shutdown step at that moment. In the default + * in-handler mode our capture never runs and Groovy Menu keeps its own step. */ class Optml_groovy_menu extends Optml_compatibility { /** diff --git a/inc/manager.php b/inc/manager.php index 6de2afa6..587fe756 100644 --- a/inc/manager.php +++ b/inc/manager.php @@ -913,19 +913,22 @@ public function process_template_redirect_content() { } /** - * Start an output buffer that captures the page HTML. + * Start the output buffer that holds the page HTML. * - * On normal requests the buffer is captured and processed by close_buffer() - * at shutdown, outside of PHP's display-handler context, so callbacks hooked - * into our filters are free to use output buffering themselves and fatal - * errors raised during processing keep their real message instead of being - * masked by "Cannot use output buffering in output buffering display handlers". + * By default the page is processed by the attached handler when the buffer + * is flushed, the way it worked up to 4.2.11. We never flush other buffers + * and open no buffer at shutdown, so code that opens a buffer early and reads + * it back with ob_get_clean() at shutdown (FacetWP, Groovy Menu) keeps working. * - * The attached handler is only a fallback for buffers flushed outside of - * close_buffer() — third-party force-flush loops, ob_flush() streaming, or - * core's wp_ob_end_flush_all() reaching the re-armed buffer. A named method - * is used instead of a closure so the buffer can be identified as ours via - * ob_get_status()['name']. + * When the optml_capture_at_shutdown filter returns true, close_buffer() + * captures and processes the buffer at shutdown instead, outside of PHP's + * display-handler context. Callbacks hooked into our filters can then use + * output buffering themselves, and fatal errors raised during processing + * keep their real message instead of being masked by "Cannot use output + * buffering in output buffering display handlers". + * + * A named method is used instead of a closure so the buffer can be + * identified as ours via ob_get_status()['name']. * * @return void */ @@ -941,15 +944,35 @@ private function start_capture_buffer() { const OB_HANDLER_NAME = 'Optml_Manager::handle_buffer_fallback'; /** - * Output-buffer handler attached to our capture buffer. + * Whether the page is captured and processed at shutdown, outside of PHP's display-handler context. + * + * @return bool + */ + private function captures_at_shutdown() { + /** + * Filters whether the page is captured and processed at shutdown, outside + * of PHP's display-handler context, instead of inside the output-buffer + * handler. + * + * Off by default: the shutdown capture flushes the buffers + * stacked above ours and opens a new buffer afterwards, which breaks code + * that reads its own buffer back at shutdown. Return true to opt in. + * + * @param bool $capture_at_shutdown Whether to process the buffer at shutdown. + */ + return apply_filters( 'optml_capture_at_shutdown', false ) === true; + } + + /** + * Output-buffer handler attached to our buffer. * - * Runs only when the buffer is flushed outside of close_buffer(). Content is - * passed through UNPROCESSED here: running the replacement filter graph - * inside a PHP display handler would turn any third-party ob_*() call into - * an uncatchable fatal ("Cannot use output buffering in output buffering - * display handlers") — the very crash this rework removes. The only - * exception is the legacy mode selected via the optml_capture_at_shutdown - * filter, which explicitly restores the previous in-handler processing. + * By default this is where the page is processed. An exception thrown by the + * replacement never breaks the page: the content is returned untouched. + * + * With the optml_capture_at_shutdown opt-in the handler runs only when the + * buffer is flushed outside of close_buffer(), and the content is passed + * through unprocessed, because running the replacement filter graph inside a + * display handler is what that mode exists to avoid. * * @param string $content The buffered content. * @param int $phase PHP's output-handler phase bitmask (unused; keeps replace_content()'s $partial parameter shielded from it). @@ -960,7 +983,7 @@ public function handle_buffer_fallback( $content, $phase = 0 ) { if ( self::$ob_processed || $content === '' ) { return $content; } - if ( apply_filters( 'optml_capture_at_shutdown', true ) === false ) { + if ( ! $this->captures_at_shutdown() ) { try { return $this->replace_content( $content, self::is_ajax_request() ); } catch ( Throwable $t ) { @@ -982,14 +1005,7 @@ public function close_buffer() { return; } - /** - * Filters whether the captured page is processed at shutdown, outside of - * PHP's display-handler context. Return false to restore the legacy - * behavior of processing inside the output-buffer handler. - * - * @param bool $capture_at_shutdown Whether to process the buffer at shutdown. - */ - if ( apply_filters( 'optml_capture_at_shutdown', true ) === false ) { + if ( ! $this->captures_at_shutdown() ) { if ( ob_get_length() ) { ob_end_flush(); } @@ -1027,7 +1043,7 @@ public function close_buffer() { * @return void */ public function close_final_buffer() { - if ( ! self::$ob_started ) { + if ( ! self::$ob_started || ! $this->captures_at_shutdown() ) { return; } $this->capture_and_process_buffer( false ); diff --git a/tests/test-zz-buffer.php b/tests/test-zz-buffer.php index 15425d5c..a7f8e2fa 100644 --- a/tests/test-zz-buffer.php +++ b/tests/test-zz-buffer.php @@ -39,6 +39,9 @@ public function setUp(): void { Optml_Tag_Replacer::instance()->init(); Optml_Manager::instance()->init(); + // The shutdown capture is opt-in; most tests below exercise it. + add_filter( 'optml_capture_at_shutdown', '__return_true' ); + $this->reset_buffer_state(); $this->base_level = ob_get_level(); } @@ -53,6 +56,8 @@ public function tearDown(): void { } } $this->reset_buffer_state(); + remove_filter( 'optml_capture_at_shutdown', '__return_true' ); + remove_filter( 'optml_capture_at_shutdown', '__return_false' ); parent::tearDown(); } @@ -290,4 +295,69 @@ public function test_legacy_in_handler_mode() { $this->assertSame( 1, substr_count( $out, 'i.optimole.com' ) ); $this->assertSame( $this->base_level, ob_get_level() ); } + + /** + * Without the opt-in the page is processed inside the output handler, as up to 4.2.11. + */ + public function test_default_processes_in_handler() { + remove_filter( 'optml_capture_at_shutdown', '__return_true' ); + $manager = Optml_Manager::instance(); + ob_start(); + $manager->process_template_redirect_content(); + echo self::IMG_TAGS; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + $manager->close_buffer(); + + $this->assertSame( $this->base_level + 1, ob_get_level(), 'No buffer is opened at shutdown.' ); + $manager->close_final_buffer(); + $out = ob_get_clean(); + + $this->assertSame( 1, substr_count( $out, 'i.optimole.com' ) ); + $this->assertSame( $this->base_level, ob_get_level() ); + } + + /** + * By default, code that opened a buffer before ours and reads it back on + * shutdown priority 0 (FacetWP refresh, Groovy Menu) finds its own buffer on + * top, holding the processed page. + */ + public function test_default_foreign_buffer_below_can_be_read_back() { + remove_filter( 'optml_capture_at_shutdown', '__return_true' ); + $manager = Optml_Manager::instance(); + ob_start(); + ob_start(); // Third-party buffer opened on init, before ours. + $foreign_level = ob_get_level(); + $manager->process_template_redirect_content(); + echo self::IMG_TAGS; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + $manager->close_buffer(); + + $this->assertSame( $foreign_level, ob_get_level(), 'The foreign buffer is on top again.' ); + + // The third party reads its buffer back on shutdown priority 0. + $page = ob_get_clean(); + $manager->close_final_buffer(); + + $this->assertSame( 1, substr_count( $page, 'i.optimole.com' ) ); + $this->assertSame( '', ob_get_clean() ); + $this->assertSame( $this->base_level, ob_get_level() ); + } + + /** + * By default an exception thrown during the replacement never breaks the page. + */ + public function test_default_exception_passes_content_through() { + remove_filter( 'optml_capture_at_shutdown', '__return_true' ); + $thrower = function () { + throw new RuntimeException( 'broken third-party callback' ); + }; + add_filter( 'optml_url_pre_process', $thrower ); + $manager = Optml_Manager::instance(); + ob_start(); + $manager->process_template_redirect_content(); + echo self::IMG_TAGS; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + $manager->close_buffer(); + $out = ob_get_clean(); + remove_filter( 'optml_url_pre_process', $thrower ); + + $this->assertSame( self::IMG_TAGS, $out ); + } } diff --git a/tests/test-zz-groovy-menu.php b/tests/test-zz-groovy-menu.php index a6790cc3..0ea900dc 100644 --- a/tests/test-zz-groovy-menu.php +++ b/tests/test-zz-groovy-menu.php @@ -78,6 +78,9 @@ public function setUp(): void { add_filter( 'groovy_menu_final_output', 'groovy_menu_add_after_body' ); $this->compatibility = new Optml_groovy_menu(); + // The compatibility acts only with the shutdown capture opt-in. + add_filter( 'optml_capture_at_shutdown', '__return_true' ); + $this->reset_buffer_state(); $this->base_level = ob_get_level(); } @@ -95,6 +98,7 @@ public function tearDown(): void { remove_filter( 'groovy_menu_final_output', 'groovy_menu_add_after_body' ); remove_filter( 'optml_captured_page_html', [ $this->compatibility, 'insert_menu' ] ); remove_filter( 'optml_capture_at_shutdown', '__return_false' ); + remove_filter( 'optml_capture_at_shutdown', '__return_true' ); parent::tearDown(); } @@ -167,10 +171,10 @@ public function test_menu_inserted_when_groovy_buffer_is_above_ours() { } /** - * In legacy in-handler mode Groovy Menu keeps its own shutdown step and still works. + * In the default in-handler mode Groovy Menu keeps its own shutdown step and still works. */ - public function test_legacy_mode_keeps_groovy_shutdown_step() { - add_filter( 'optml_capture_at_shutdown', '__return_false' ); + public function test_default_mode_keeps_groovy_shutdown_step() { + remove_filter( 'optml_capture_at_shutdown', '__return_true' ); $this->compatibility->register(); $manager = Optml_Manager::instance(); ob_start();