Skip to content
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

Use output buffer and HTML tag processor to inject directives on BODY tag for full-page client-side navigation #61212

Merged
merged 5 commits into from
May 27, 2024
Merged

Use output buffer and HTML tag processor to inject directives on BODY tag for full-page client-side navigation #61212

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bc71592
Inject client-side navigation directives via Tag Processor and output…
westonruter Apr 29, 2024
8bd0a3c
Fix typing for render_block phpdoc
westonruter Apr 29, 2024
29eeed4
Eliminate use of render_block filter
westonruter Apr 29, 2024
36fa0d2
Remove copy-pasta since tag
westonruter Apr 29, 2024
300d9e3
Ensure client-side directives are only added to HTML response bodies
westonruter Apr 29, 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
60 changes: 47 additions & 13 deletions lib/experimental/full-page-client-side-navigation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,25 +33,59 @@ function _gutenberg_add_enhanced_pagination_to_query_block( $parsed_block ) {
add_filter( 'render_block_data', '_gutenberg_add_enhanced_pagination_to_query_block' );

/**
* Add directives to all links.
* Adds client-side navigation directives to BODY tag.
*
* Note: This should probably be done per site, not by default when this option is enabled.
*
* @param array $content The block content.
* @param string $html The rendered template.
*
* @return array The same block content with the directives needed.
* @return string The rendered template with modified BODY attributes.
*/
function _gutenberg_add_client_side_navigation_directives( $content ) {
$p = new WP_HTML_Tag_Processor( $content );
// Hack to add the necessary directives to the body tag.
// TODO: Find a proper way to add directives to the body tag.
static $body_interactive_added;
if ( ! $body_interactive_added ) {
$body_interactive_added = true;
return (string) $p . '<body data-wp-interactive="core/experimental" data-wp-context="{}">';
function _gutenberg_add_client_side_navigation_directives( $html ) {
$p = new WP_HTML_Tag_Processor( $html );
if ( $p->next_tag( array( 'tag_name' => 'BODY' ) ) ) {
$p->set_attribute( 'data-wp-interactive', 'core/experimental' );
$p->set_attribute( 'data-wp-context', '{}' );
$html = $p->get_updated_html();
}
return (string) $p;
return $html;
}

// TODO: Explore moving this to the server directive processing.
add_filter( 'render_block', '_gutenberg_add_client_side_navigation_directives' );
add_filter( 'gutenberg_template_output_buffer', '_gutenberg_add_client_side_navigation_directives' );

/**
* Starts output buffering at the end of the 'template_include' filter.
*
* This is to implement #43258 in core.
*
* This is a hack which would eventually be replaced with something like this in wp-includes/template-loader.php:
*
* $template = apply_filters( 'template_include', $template );
* + ob_start( 'wp_template_output_buffer_callback' );
* if ( $template ) {
* include $template;
* } elseif ( current_user_can( 'switch_themes' ) ) {
*
* @since 0.1.0
westonruter marked this conversation as resolved.
Show resolved Hide resolved
* @link https://core.trac.wordpress.org/ticket/43258
*
* @param string $passthrough Value for the template_include filter which is passed through.
*
* @return string Unmodified value of $passthrough.
*/
function _gutenberg_buffer_template_output( string $passthrough ): string {
ob_start(
static function ( string $output ): string {
/**
* Filters the template output buffer prior to sending to the client.
*
* @param string $output Output buffer.
* @return string Filtered output buffer.
*/
return (string) apply_filters( 'gutenberg_template_output_buffer', $output );
}
);
Comment on lines +88 to +98
Copy link
Member Author

Choose a reason for hiding this comment

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

Based on WordPress/performance#1317, I believe this should now rather be improved as follows:

Suggested change
ob_start(
static function ( string $output ): string {
/**
* Filters the template output buffer prior to sending to the client.
*
* @param string $output Output buffer.
* @return string Filtered output buffer.
*/
return (string) apply_filters( 'gutenberg_template_output_buffer', $output );
}
);
ob_start(
static function ( string $output ): string {
// When the output is being cleaned (e.g. pending template is replaced with error page), do not send it through the filter.
if ( ( $phase & PHP_OUTPUT_HANDLER_CLEAN ) !== 0 ) {
return $output;
}
/**
* Filters the template output buffer prior to sending to the client.
*
* @param string $output Output buffer.
* @return string Filtered output buffer.
*/
return (string) apply_filters( 'gutenberg_template_output_buffer', $output );
},
0,
PHP_OUTPUT_HANDLER_STDFLAGS ^ PHP_OUTPUT_HANDLER_FLUSHABLE
);

return $passthrough;
}
add_filter( 'template_include', '_gutenberg_buffer_template_output', PHP_INT_MAX );
Copy link
Member

Choose a reason for hiding this comment

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

isn't an action just a filter whose return value is ignored? we could eliminate the $passthrough if we made this add_action() couldn't we?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, add_action() defined as an alias:

function add_action( $hook_name, $callback, $priority = 10, $accepted_args = 1 ) {
	return add_filter( $hook_name, $callback, $priority, $accepted_args );
}

So if the callback doesn't return a value, then the filter is borked. I just tested:

diff --git a/lib/experimental/full-page-client-side-navigation.php b/lib/experimental/full-page-client-side-navigation.php
index 259517cfc84..452abee6c63 100644
--- a/lib/experimental/full-page-client-side-navigation.php
+++ b/lib/experimental/full-page-client-side-navigation.php
@@ -68,12 +68,8 @@ add_filter( 'gutenberg_template_output_buffer', '_gutenberg_add_client_side_navi
  *          } elseif ( current_user_can( 'switch_themes' ) ) {
  *
  * @link https://core.trac.wordpress.org/ticket/43258
- *
- * @param string $passthrough Value for the template_include filter which is passed through.
- *
- * @return string Unmodified value of $passthrough.
  */
-function _gutenberg_buffer_template_output( string $passthrough ): string {
+function _gutenberg_buffer_template_output() {
 	ob_start(
 		static function ( string $output ): string {
 			/**
@@ -85,6 +81,5 @@ function _gutenberg_buffer_template_output( string $passthrough ): string {
 			return (string) apply_filters( 'gutenberg_template_output_buffer', $output );
 		}
 	);
-	return $passthrough;
 }
-add_filter( 'template_include', '_gutenberg_buffer_template_output', PHP_INT_MAX );
+add_action( 'template_include', '_gutenberg_buffer_template_output', PHP_INT_MAX );

The result is an empty page because the if ( $template ) condition isn't entered here: https://github.com/WordPress/wordpress-develop/blob/204a1bbf4e5f22b07a93c1f4a0b12bdd65d6483f/src/wp-includes/template-loader.php#L105-L112

Copy link
Member

Choose a reason for hiding this comment

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

darn. you've shattered my understanding of this. I guess do_action() simply ignores the return value, but apply_filter() doesn't care if something was added as a filter or an action.

Loading