Make WordPress Core

Opened 3 months ago

Closed 3 months ago

Last modified 3 months ago

#64126 closed defect (bug) (fixed)

Sending late HTTP headers not facilitated by template enhancement output buffer

Reported by: westonruter's profile westonruter Owned by: westonruter's profile westonruter
Milestone: 6.9 Priority: normal
Severity: normal Version: 6.9
Component: General Keywords: has-patch has-unit-tests
Focuses: performance Cc:

Description (last modified by westonruter)

With #43258 there is now an opt-in way for core to output buffer a rendered template. Output buffers are useful not only for modifying the response body, but also for sending late headers, such as Server-Timing:

The former is currently facilitated via the wp_template_enhancement_output_buffer filter, but the latter is not. There is no dedicated action to fire after the filters have applied and right before the output buffer callback returns to send the HTTP response body.

Plugins which need to send a Server-Timing header after the template has been rendered shouldn't have to resort to hacking the wp_template_enhancement_output_buffer filter to run at PHP_INT_MAX while passing through the $buffer unchanged.

Related: #64108

Change History (17)

This ticket was mentioned in Slack in #core-editor by westonruter. View the logs.


3 months ago

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


3 months ago
#2

  • Keywords has-patch added

#3 @westonruter
3 months ago

  • Description modified (diff)

@westonruter commented on PR #10381:


3 months ago
#4

@dmsnell @johnbillion Here's two example use cases for this action.

# Sending Server-Timing

This shows how to measure how long it takes a template to render and to send that as a Server-Timing header. Note that if there is an wp_send_late_headers action added, then the template enhancement buffer will automatically be opened, the same as if there are any wp_template_enhancement_output_buffer filters added:

add_action(
        'wp_before_include_template',
        static function (): void {
                $start_time = hrtime( true );

                add_action(
                        'wp_send_late_headers',
                        static function () use ( $start_time ): void {
                                $server_timing_header = sprintf(
                                        'Server-Timing: wpTemplate; desc="Template render"; dur=%f',
                                        ( hrtime( true ) - $start_time ) / 1000000
                                );
                                header( $server_timing_header, false );
                        }
                );
        }
);

The wp_before_include_template action was newly introduced with the template enhancement output buffer.

# HTTP Conditional Requests

The wp_send_late_headers action can be used to implement conditional requests. Since the wp_send_late_headers action proposed in this PR sends the output buffer as an argument this can be used to compute an ETag to send as a header. In the same action, it can check to see if the client has sent that same ETag in an If-None-Match request header, and of so, send back a 304 Not Modified response:

add_action(
        'wp_send_late_headers',
        static function ( string $response ) {
                $etag = '"' . md5( $response ) . '"';
                header( "ETag: $etag" );
                header( 'Cache-Control: public, max-age=3600' );

                if ( isset( $_SERVER['HTTP_IF_NONE_MATCH'] ) ) {
                        $client_etag = wp_unslash( $_SERVER['HTTP_IF_NONE_MATCH'] );
                        if ( $client_etag === $etag ) {
                                status_header( 304 );
                        }
                }
        }
);

I find that if PHP sends a 304 response header, that it also omits sending any response body. This prevents the need to try to use the wp_before_include_template filter to empty out the response. But if need be, this could be made explicit:

  • src/wp-includes/template.php

    diff --git a/src/wp-includes/template.php b/src/wp-includes/template.php
    index 035b7e16b9..19984ca9ab 100644
    a b function wp_finalize_template_enhancement_output_buffer( string $output, int $ph 
    998998
    999999        $do_send_late_headers( $filtered_output );
    10001000
     1001        if ( 304 === http_response_code() ) {
     1002                $filtered_output = '';
     1003        }
     1004
    10011005        return $filtered_output;
    10021006}

#5 @westonruter
3 months ago

  • Keywords has-unit-tests added

In PR #10381:

  • Introduce wp_send_late_headers action which fires right right after wp_template_enhancement_output_buffer filters have applied, and right before the output buffer is flushed. The output buffer is passed as an argument to the function so that plugins may do things like send an ETag header calculated from the content.
  • The template enhancement output buffer now is enabled by default if _either_ there is a wp_template_enhancement_output_buffer filter added _or_ there is a wp_send_late_headers action added.
  • The wp_start_template_enhancement_output_buffer() callback for the wp_before_include_template action is increased from the default 10 to 1000. This goes with the previous point, so that plugins can add those filters and actions during the wp_before_include_template action without having to worry about adding them too late, that is, after wp_start_template_enhancement_output_buffer() has run.
  • The wp_send_late_headers action fires regardless of whether the buffered response is HTML.

@westonruter commented on PR #10381:


3 months ago
#6

I find that if PHP sends a 304 response header, that it also omits sending any response body. This prevents the need to try to use the wp_before_include_template filter to empty out the response. But if need be, this could be made explicit:

However, this I envision would wreak havoc on caching plugins which have output buffers started much much earlier, e.g. right before plugins_loaded:

https://github.com/WordPress/wordpress-develop/blob/9d7752ecb8e1f5641b2360052f01dd29617e1e38/src/wp-settings.php#L581-L584

WP Super Cache defines `wp_cache_postload()` to by default call `wp_cache_phase2()` which does `ob_start( 'wp_cache_ob_callback' );`. So it is necessary for the output buffer from WP Super Cache to have access to the response body, even in the case of a 304 Not Modified response. I verified this is the case with another plugin that does this, to wrap the template enhancement output buffer with the plugin code above that sense 304:

ob_start( function ( $buffer ) {
        error_log( strlen( $buffer ) ); // => 89069
        return $buffer;
} );

Even though the HTTP status may be 304, the response body is still available internally before PHP sends the response with the body omitted.

Nevertheless, this will still have the effect of causing WP Super Cache to not cache the response:

} elseif ( isset( $wp_super_cache_query['is_304'] ) ) {
                wp_cache_debug( 'HTTP 304 (Not Modified) sent. Caching disabled.' );
                $cache_this_page = false;

But that would be something which could be addressed with caching plugins as need be, if someone wants to add a plugin that opts in to sending 304 responses.

This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.


3 months ago

#8 @westonruter
3 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 61088:

General: Add wp_send_late_headers action which fires right before the template enhancement output buffer is flushed.

This adds a (missing) wp_send_late_headers action which fires right after the wp_template_enhancement_output_buffer filters have applied and right before the output buffer is flushed. The filtered output buffer is passed as an argument to the action so that plugins may do things like send an ETag header which is calculated from the content. This action eliminates the need for plugins to hack the wp_template_enhancement_output_buffer filter with a high priority to send a late response header. This action compliments the send_headers action which is commonly used to send HTTP headers before the template is rendered. Furthermore:

  • The template enhancement output buffer is now enabled by default if there is a callback added to either the wp_template_enhancement_output_buffer filter or the wp_send_late_headers action.
  • The wp_start_template_enhancement_output_buffer() callback for the wp_before_include_template action is increased from the default of 10 to 1000. This goes with the previous point, so that plugins can add those filters and actions during the wp_before_include_template action without having to worry about adding them too late, that is, after wp_start_template_enhancement_output_buffer() has run.
  • The wp_send_late_headers action fires regardless of whether the buffered response is HTML.

Developed in https://github.com/WordPress/wordpress-develop/pull/10381

Follow-up to [60936].

Props westonruter, peterwilsoncc, johnbillion.
See #43258.
Fixes #64126.

@westonruter commented on PR #10381:


3 months ago
#9

FYI: I further improved the action's phpdoc in r61088 versus what I had pushed to this PR:

  • src/wp-includes/template.php

    a b function wp_finalize_template_enhancement_output_buffer( string $output, int $ph 
    987987         *
    988988         * This happens immediately before the template enhancement output buffer is flushed. This is in contrast with
    989989         * the {@see 'send_headers'} action which fires after the initial headers have been sent before the template
    990          * has begun rendering, and thus does not depend on output buffering.
     990         * has begun rendering, and thus does not depend on output buffering. This action does not fire if the "template
     991         * enhancement output buffer" was not started. This output buffer is automatically started if this action is added
     992         * before {@see wp_start_template_enhancement_output_buffer()} runs at the {@see 'wp_before_include_template'}
     993         * action with priority 1000. Before this point, the output buffer will also be started automatically if there was a
     994         * {@see 'wp_template_enhancement_output_buffer'} filter added, or if the
     995         * {@see 'wp_should_output_buffer_template_for_enhancement'} filter is made to return `true`.
    991996         *
    992997         * @since 6.9.0
    993998         *

@westonruter commented on PR #10381:


3 months ago
#10

@dmsnell replying to your https://github.com/WordPress/wordpress-develop/pull/10381#discussion_r2476656773 outside of an inline comment for better visibility:

is this supposed to be for 6.9 still, or for 7.0? it’s probably great and I am too slow to recognize it, but I worry about haste in choosing the name.

I intended it for 6.9, yes. I'm not dead set on the name, though. If you think it is not ideal, I'm happy to revisit for beta3.

Part of the inspiration for the name also comes from the print_late_styles filter (and print_late_styles() function) which handles printing stylesheets at wp_footer if a stylesheet was enqueued late (after wp_print_styles() runs in wp_head). In this way, there are two places where stylesheets can be printed: standard or late. With Core-64099, these late-printed styles are also hoisted up to wp_head. Likewise, the template enhancement output buffer also enables the same thing for sending late HTTP headers: if the output buffer is open, plugins can continue sending headers up until the wp_send_late_headers action fires in the same way they can continue enqueueing styles up until the wp_print_styles() function runs in wp_footer.

However the action is named, having _some_ action that fires after the filters have applied seems like something important we missed in the original commit for adding the template enhancement output buffer. I want to avoid devs having to hack the wp_template_enhancement_output_buffer filter to send a header (as is done today with the template_include filter) without otherwise modifying the content. Consider a plugin that adds an ETag header but only has the filter:

// Plugin that adds an ETag header.
add_filter(
        'wp_template_enhancement_output_buffer',
        static function ( $buffer ) {
                header( sprintf( 'ETag: "%s"', md5( $buffer ) ) );
                return $buffer; // Not touched!
        },
        1000 // (Nothing could possibly come after this, right?)
);

But then consider another plugin which modifies the output buffer even later:

add_filter(
        'wp_template_enhancement_output_buffer',
        static function ( $buffer ) {
                // Don't do this!
                return str_replace(
                        'Wordpress', // phpcs:ignore WordPress.WP.CapitalPDangit.MisspelledInText -- Hi Dennis!
                        'WordPress',
                        $buffer
                );
        },
        99999
);

I want to avoid this.

maybe you had already worked on this for a while, in which case I trust you.

Originally I had thought of send_late_headers but @johnbillion suggested adding the wp_ prefix. I didn't work on the name for _that_ long, but the rationale above shows how I arrived at it.

Since the output is about to be sent to the client, it is essential that _no output_ be emitted from callbacks for this action. This would seem to make mentioning "headers" in the name a worthwhile reminder of that, but this could be made explicit in phpdoc instead.

it sounds like send_headers but as you point out, it’s something different.

How?

what I started looking at when I saw it was whether we could use those streaming buffers to capture the first output and send late headers. that is, Core could actually _always_ buffer the output, but only lock that buffer when plugins demand it.

what I started looking at when I saw it was whether we could use those streaming buffers to capture the first output and send late headers. that is, Core could actually _always_ buffer the output, but only lock that buffer when plugins demand it.

Interesting. This is somewhat different than the model I was using relating sending HTTP headers to enqueueing stylesheets.

As I am understanding, what you're suggesting is that the output buffer could always start, but the chunk size would only be set to zero if there is a wp_template_enhancement_output_buffer filter added. This would ensure that the entire buffer can be passed into the filter. Otherwise, if there is no wp_template_enhancement_output_buffer, then an alternate chunk size could be used, like 4KB, and then the first time the output buffer callback is called, there could be an action that fires which allows plugins to send "later" headers based on what was found in the HTML for the initial chunk. Is that right? What headers do you envision being sent here? Perhaps elevating some dns-prefetch/preconnect/preload links?

So you're talking about sending "later" headers as opposed the "latest" headers I'm targeting.

if template-loader immediately prints to STDOUT this is moot, of course. if there’s _always_ an immediate flush after send_headers then I see no critique of this filter.

Well, it does print to STDOUT right away by virtue of including the template file. So there would have to be an output buffer opened to be able to send a header after the template has been included. But the chunk size could be set to 4KB as mentioned above, to ensure that the response is streamed.

  • before_flushing_output_enhancement_buffer

there are plenty of things one might do there, I suppose. are headers the only thing?

Headers wouldn't have to be the only thing done at this action, no. But sending headers would be the only _output_ which could be done at this point. No printing would be allowed. I would think that the shutdown action would be more preferable to use than this, however.

If we didn't want to specifically refer to headers in the action name, since the action runs in the wp_finalize_template_enhancement_output_buffer() action, and given the other hooks wp_template_enhancement_output_buffer and wp_should_output_buffer_template_for_enhancement, some alternative ideas to before_flushing_output_enhancement_buffer:

  • wp_finalized_template_enhancement_output_buffer (cf. deleted_post, added_option, activated_plugin, and many more)
  • wp_flush_template_enhancement_output_buffer
  • wp_before_flush_template_enhancement_output_buffer
  • wp_before_template_enhancement_output_buffer_flush (cf. wp_before_admin_bar_render, before_delete_post)

also I thought that when I reviewed the other work that the template buffer trapped the template generation, but didn’t also include the <!DOCTYPE html><html…> content in the HEAD…? I am mistaken, right?

Actually, the template enhancement buffer _does_ trap the entire rendered template, including <!DOCTYPE html><html…>. I think you may be thinking about a block theme's `template-canvas.php` which which buffers the rendered blocks in memory, and then proceeds with printing the root element.


Aside: I just realized the phpdoc for this action and the wp_template_enhancement_output_buffer filter need to specifically call out that callbacks _cannot_ do any calls to ob_start() or else they'll get a fatal error:

PHP Fatal error: ob_start(): Cannot use output buffering in output buffering display handlers in /var/www/src/wp-content/plugins/try-ob-start-during-finalize-template-output-buffer.php on line 9

Example:

add_filter(
        'wp_template_enhancement_output_buffer',
        static function ( $buffer ) {
                // Note: DO NOT DO THIS!!!! ❌
                ob_start();
                ?>
                <script>
                        document.write('Hello World!!!');
                </script>
                <?php
                $script = wp_remove_surrounding_empty_script_tags( ob_get_clean() );

                return $buffer . wp_get_inline_script_tag( $script );
        }
);

#11 @westonruter
3 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening based on post-commit PR conversation, to revise the action name.

@westonruter commented on PR #10443:


3 months ago
#14

Without any strong feelings about the names, including the previous one, _after_ stands out more clearly to me than _finalized_, and I think we can see some natural associations with it based on the way the documentation for the filter is worded.

@dmsnell Do you mean wp_finalized_template_enhancement_output_buffer is fine but you would prefer something that incorporated "after"?

There's wp_after_insert_post, wp_after_execute_ability, wp_after_admin_bar_render, and wp_after_load_template as precedent for that but there should then be a verb added after after, so like wp_after_finalize_template_enhancement_output_buffer? Oof. That's getting long.

#15 @westonruter
3 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 61111:

General: Rename wp_send_late_headers action to wp_finalized_template_enhancement_output_buffer.

Also update docs for wp_finalized_template_enhancement_output_buffer action and wp_template_enhancement_output_buffer filter to warn against attempting to open an output buffer in callbacks or else a PHP fatal error will occur.

Developed in https://github.com/WordPress/wordpress-develop/pull/10443

Follow-up to [61088], [60936].

Props westonruter, dmsnell.
See #43258.
Fixes #64126.

@westonruter commented on PR #10443:


3 months ago
#16

Committed in r61111.

#17 @westonruter
3 months ago

In 61120:

General: Ensure errors can be displayed when triggered during finalization of the template enhancement output buffer.

When display_errors (WP_DEBUG_DISPLAY) is enabled, errors (including notices, warnings, and deprecations) that are triggered during the wp_template_enhancement_output_buffer filter or the wp_finalized_template_enhancement_output_buffer action have not been displayed on the frontend since they are emitted in an output buffer callback. Furthermore, as of PHP 8.5 attempting to print anything in an output buffer callback causes a deprecation notice. This introduces an error handler and try/catch block to capture any errors and exceptions that occur during these hooks. If display_errors is enabled, these captured errors are then appended to the output buffer so they are visible on the frontend, using the same internal format PHP uses for printing errors. Any exceptions or user errors are converted to warnings so that the template enhancement buffer is not prevented from being flushed.

Developed in https://github.com/WordPress/wordpress-develop/pull/10310

Follow-up to [61111], [61088], [60936].

Props westonruter, dmsnell.
See #43258, #64126.
Fixes #64108.

Note: See TracTickets for help on using tickets.