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.

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

Fix Optimization Detective compatibility with WooCommerce when Coming Soon page is served #1565

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

westonruter
Copy link
Member

When testing WooCommerce, I discovered an unexpected result when attempting to access the Shop page:

image

In my local testing site, I imported some sample products but didn't "launch" the store yet, so WooCommerce is attempting to serve the Coming Soon page to a logged-out user. The relevant code is ComingSoonRequestHandler::handle_template_include() which filters template_include to conditionally return null on FSE themes:

		if ( $is_fse_theme ) {
			// Since we've already rendered a template, return null to ensure no other template is rendered.
			return null;
		} else {
			// In non-FSE themes, other templates will still be rendered.
			// We need to exit to prevent further processing.
			exit();
		}

This causes a fatal error in Optimization Detective:

PHP Fatal error:  Uncaught TypeError: od_buffer_output(): Argument #1 ($passthrough) must be of type string, null given, called in /var/www/html/wp-includes/class-wp-hook.php on line 324 and defined in /var/www/html/wp-content/plugins/optimization-detective/optimization.php:33
Stack trace:
#0 /var/www/html/wp-includes/class-wp-hook.php(324): od_buffer_output(NULL)
#1 /var/www/html/wp-includes/plugin.php(205): WP_Hook->apply_filters(NULL, Array)
#2 /var/www/html/wp-includes/template-loader.php(104): apply_filters('template_includ...', '/var/www/html/w...')
#3 /var/www/html/wp-blog-header.php(19): require_once('/var/www/html/w...')
#4 /var/www/html/index.php(17): require('/var/www/html/w...')
#5 {main}
  thrown in /var/www/html/wp-content/plugins/optimization-detective/optimization.php on line 33

This is because od_buffer_output() is erroneously defined with a strict string type for the method parameter. Well, I suppose WooCommerce is also erroneously returning null when it should instead return an empty string (which has the same effect) since the template_include filter is defined as passing a string and not string|null.

In any case, since we don't actually do anything with the variable we can just remove the typing. The result is the page as expected:

image

@westonruter westonruter added [Type] Bug An existing feature is broken [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels Sep 28, 2024
Copy link

github-actions bot commented Sep 28, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@westonruter
Copy link
Member Author

Note that Optimization Detective does not operate on this Coming Soon page because WooCommerce is outputting it inside the template_include filter at priority 10, whereas Optimization Detective starts output buffering in the template_include filter at PHP_INT_MAX. If it were changed to PHP_INT_MIN then it would apply optimizations on this Coming Soon page as well. Something to consider for the future. I had chosen PHP_INT_MAX because I expected that when merged into core it would call ob_start() after the following line in wp-includes/template-loader.php:

$template = apply_filters( 'template_include', $template );

WooCommerce's injecting a template override in the template_include filter as well as Optimization Detective starting output buffering in the template_include filter are both somewhat abuses of this filter.

@westonruter
Copy link
Member Author

I also opened a Woo PR to fix this issue as it could cause problems with other plugins: woocommerce/woocommerce#51751

@@ -27,10 +27,10 @@
* @access private
* @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.
* @param string|mixed $passthrough Value for the template_include filter which is passed through.
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to include a note here explaining why that's not always a string.

Copy link
Member Author

@westonruter westonruter Sep 30, 2024

Choose a reason for hiding this comment

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

We do the |mixed type elsewhere for callbacks for filters. But in general the reason is that other plugins may return anything in filter callbacks, so there is no guarantee for the type.

So you want something like the following?

Suggested change
* @param string|mixed $passthrough Value for the template_include filter which is passed through.
* @param string|mixed $passthrough Value for the template_include filter which is passed through. (A plugin may have filtered this to return `null` or any other value type.)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, nevermind then. I somehow was confused thinking this was a callback to a PHP function like ob_start(), but since it's a filter callback, it's fine without a comment.

@westonruter westonruter merged commit 76685b0 into trunk Sep 30, 2024
22 of 24 checks passed
@westonruter westonruter deleted the fix/od-woo-coming-soon-page-compat branch September 30, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Bug An existing feature is broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants