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

Ensure optimization is performed in the wp-env local environment and log debug messages to console when disabled #1822

Merged
merged 6 commits into from
Jan 24, 2025

Conversation

westonruter
Copy link
Member

I was testing Optimization Detective and I was confused why it wasn't working. Pages which normally had been optimized were being served as if Optimization Detective wasn't even active. It turns out that this is a side effect of #1762 where we prevent any page processing when the REST API for storing URL Metrics is unavailable. Since loopback requests don't work in wp-env (WordPress/gutenberg#20569), neither does the Site Health test for checking whether the REST API is available.

Without the patch in this PR, a plugin can force optimization to apply even when the REST API isn't available via code like the following:

// The wp-env environment does not support loopback requests, so we need to pretend that everything is good.
add_filter(
	'pre_option_od_rest_api_unavailable',
	static function ( $pre ): string {
		if ( ! is_admin() ) {
			return '0';
		}
		return $pre;
	}
);

This could be added to the od-dev-mode plugin. But this plugin shouldn't have to be installed in order to test Optimization Detective in wp-env.

So this PR adds a condition (a7cedce) so that od_is_rest_api_unavailable() is checked alongside ( wp_get_environment_type() !== 'local' || class_exists( 'Test_OD_Optimization' ) ). Only if both are true will it short-circuit.

Additionally, to assist with helping users understand why Optimization Detective isn't doing anything, this PR includes the reasons for optimization being disabled in the console when WP_DEBUG is enabled.

Copy link

github-actions bot commented Jan 23, 2025

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 <[email protected]>
Co-authored-by: felixarntz <[email protected]>

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

@westonruter westonruter added the [Plugin] Optimization Detective Issues for the Optimization Detective plugin label Jan 23, 2025
@westonruter westonruter added the [Type] Bug An existing feature is broken label Jan 23, 2025
'reason' => __( 'Page is not optimized because od_can_optimize_response() returned false. This can be overridden with the od_can_optimize_response filter.', 'optimization-detective' ),
),
array(
'test' => ! ( od_is_rest_api_unavailable() && ( wp_get_environment_type() !== 'local' || class_exists( 'Test_OD_Optimization' ) ) ),
Copy link
Member Author

Choose a reason for hiding this comment

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

I wish the class_exists( 'Test_OD_Optimization' ) check weren't needed, but there's apparently no way to override the return value of wp_get_environment_type() in tests.

Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker, but is there a more generic way to check if tests are being run? Maybe a constant that the WP core unit test suite always sets?

}
}
if ( count( $reasons ) > 0 ) {
if ( WP_DEBUG ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Test coverage for this condition can't be added due to #1271.

Copy link

codecov bot commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 82.35294% with 6 lines in your changes missing coverage. Please review.

Project coverage is 65.96%. Comparing base (b7c4475) to head (e6135b8).
Report is 29 commits behind head on trunk.

Files with missing lines Patch % Lines
plugins/optimization-detective/optimization.php 82.35% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk    #1822      +/-   ##
==========================================
+ Coverage   65.47%   65.96%   +0.49%     
==========================================
  Files          85       88       +3     
  Lines        6748     6876     +128     
==========================================
+ Hits         4418     4536     +118     
- Misses       2330     2340      +10     
Flag Coverage Δ
multisite 65.96% <82.35%> (+0.49%) ⬆️
single 38.64% <82.35%> (+0.76%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@westonruter westonruter force-pushed the fix/short-circuit-rest-api-unavailable branch from 0cb53df to e9c0618 Compare January 23, 2025 22:39
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@westonruter LGTM, just two minor notes.

'reason' => __( 'Page is not optimized because od_can_optimize_response() returned false. This can be overridden with the od_can_optimize_response filter.', 'optimization-detective' ),
),
array(
'test' => ! ( od_is_rest_api_unavailable() && ( wp_get_environment_type() !== 'local' || class_exists( 'Test_OD_Optimization' ) ) ),
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker, but is there a more generic way to check if tests are being run? Maybe a constant that the WP core unit test suite always sets?

plugins/optimization-detective/optimization.php Outdated Show resolved Hide resolved
Co-authored-by: Felix Arntz <[email protected]>
@westonruter westonruter enabled auto-merge January 24, 2025 01:15
@westonruter westonruter merged commit 0615c18 into trunk Jan 24, 2025
13 checks passed
@westonruter westonruter deleted the fix/short-circuit-rest-api-unavailable branch January 24, 2025 01:15
@westonruter westonruter mentioned this pull request Jan 26, 2025
5 tasks
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
Status: Done 😃
Development

Successfully merging this pull request may close these issues.

2 participants