Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
westonruter merged 16 commits into from
Aug 8, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b3ce59e
Try considering PHP_OUTPUT_HANDLER_FINAL in output buffer callback
westonruter Jun 24, 2024
13e068d
Iterate on output buffer handling
westonruter Aug 2, 2024
523fc7a
Update test-optimization.php
nextend Aug 2, 2024
3235f3b
Merge pull request #1428 from nextend/update/ob-handling
westonruter Aug 6, 2024
15ca2f0
Remove final ob_end_clean() and add ob_get_level() assertions
westonruter Aug 6, 2024
78967f5
Merge branch 'trunk' of https://github.com/WordPress/performance into…
westonruter Aug 6, 2024
f218dc8
Allow output buffer to be cleaned but not flushed
westonruter Aug 6, 2024
0b4f2cd
Allow output buffer to be removable
westonruter Aug 6, 2024
2c6ec0c
Use bitwise XOR on the standard flags to remove the flushable flag
westonruter Aug 6, 2024
35085e8
Ensure only HTML documents are processed by Optimization Detective
westonruter Aug 6, 2024
364fa43
Remove assert() checks
westonruter Aug 6, 2024
1b62c1a
Avoid sending Server-Timing header when buffer is being cleaned
westonruter Aug 6, 2024
26e859d
Fix passing arg name to _doing_it_wrong()
westonruter Aug 6, 2024
21ebb97
Improve escaping output by wrapping entire string in esc_html()
westonruter Aug 6, 2024
8cbcbd6
Merge pull request #1443 from WordPress/fix/server-timing-after-ob-clean
Aug 7, 2024
85e9c2b
Merge pull request #1442 from WordPress/fix/od-processing-non-html-re…
Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Iterate on output buffer handling
  • Loading branch information
@westonruter
westonruter committed Aug 2, 2024
commit 13e068d2673bad734dadf622a1fc3c1eb1d9736d
2 changes: 1 addition & 1 deletion plugins/optimization-detective/optimization.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
function od_buffer_output( string $passthrough ): string {
ob_start(
static function ( string $output, ?int $phase ): string {
if ( $phase & PHP_OUTPUT_HANDLER_FINAL ) {
if ( ( $phase & PHP_OUTPUT_HANDLER_FINAL ) > 0 ) {

@felixarntz felixarntz Aug 6, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 westonruter Aug 6, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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 felixarntz Aug 6, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 westonruter Aug 6, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 felixarntz Aug 6, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 westonruter Aug 6, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/**
* Filters the template output buffer prior to sending to the client.
*
Expand Down
28 changes: 16 additions & 12 deletions plugins/optimization-detective/tests/test-optimization.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,40 +77,44 @@ function ( $buffer ) use ( $original, $expected, &$filter_invoked ) {
}

/**
* Make output is buffered and that it is also filtered.
* Test that calling ob_clean() will discard previous buffer and never send it into the od_template_output_buffer filter.
*
* @covers ::od_buffer_output
*/
public function test_od_buffer_output_not_finalized(): void {
$original = 'Hello My World!';
$override = 'Ciao mondo!';
$original = 'Hello My World!';
$template_override = 'Ciao mondo!';
$filter_override = '¡Hola Mi Mundo!';

// In order to test, a wrapping output buffer is required because ob_get_clean() does not invoke the output
// buffer callback. See <https://stackoverflow.com/a/61439514/93579>.
ob_start();

$filter_invoked = false;
$filter_count = 0;
add_filter(
'od_template_output_buffer',
function ( $buffer ) use ( $original, &$filter_invoked ) {
$this->assertSame( $original, $buffer );
$filter_invoked = true;
return '¡Hola Mi Mundo!';
function ( $buffer ) use ( $template_override, $filter_override, &$filter_count ) {
$this->assertSame( $template_override, $buffer, 'Expected the original template output to never get passed into the buffer callback since ob_clean() was called after the original was printed.' );
$filter_count++;
return $filter_override;
}
);

$original_ob_level = ob_get_level();
od_buffer_output( '' );
$this->assertSame( $original_ob_level + 1, ob_get_level(), 'Expected call to ob_start().' );
echo $original;
echo $original; // This should never be passed into the od_template_output_buffer filter.

// Abort the original content printed above.
ob_clean(); // Note the lack of flush here.
echo $override;
echo $template_override; // This should get passed into the od_template_output_buffer filter.

$buffer = ob_get_clean(); // Get the buffer from our wrapper output buffer.
Comment thread
westonruter marked this conversation as resolved.
Outdated

$this->assertFalse( $filter_invoked );
$this->assertSame( $override, $buffer );
$this->assertSame( 1, $filter_count, 'Expected filter to be called once.' );
$this->assertSame( $filter_override, $buffer, 'Excepted return value of filter to be the resulting value for the buffer.' ); // TODO: The output buffer callback returns $filter_override, but the $buffer for some reason is actually equal to $template_override. Why?
Comment thread
westonruter marked this conversation as resolved.
Outdated

ob_end_clean(); // Close the wrapper buffer.
}

/**
Expand Down