Merge Optimization Detective into trunk #1079

westonruter · 2024-03-22T00:44:20Z

Since Optimization Detective has been approved by the Plugin Review team, it is ready to be committed to WordPress.org. Therefore, it should be merged into trunk and the feature branch should be retired.

Blocked by:

Rely on server-side breadcrumbs for detection and optimization

@joemcgill

…ge-loading-optimization * origin/trunk: (149 commits) Manually fix remaining phpcs issues Run php-format Discontinue excluding all WordPress-Extra from tests so phpcbf can format Add @joemcgill to images and measurement focuses. Update codeowners, removing inactive contributors. Introduce perflab-plugin-management.js Load migration script in settings page Update function name Apply suggestions from code review Address slow query feedbacks Revise migration pointer un-dismissal to work for all admins Rename variable. improve code Require Node v20 un-dismiss the migration pointer Minor update to return doc. Move inline script in to JS Fix PHP Documentation Standards Remove duplicate admin notice call Address review feedback ...

… merge/trunk-into-image-loading-optimization * 'trunk' of https://github.com/WordPress/performance: Remove scheduler from globals since not yet used Fix or ignore eslint rules Remove modules/images/webp-uploads/fallback.js from ignorePatterns Run format-js on JS files Cherry pick fixes to JS linting from feature/image-loading-optimization Prevent sending header during test

…-optimization Merge trunk into feature/image-loading-optimization

@joemcgill

…om/WordPress/performance into add/ilo-tests * 'feature/image-loading-optimization' of https://github.com/WordPress/performance: (155 commits) Remove scheduler from globals since not yet used Fix or ignore eslint rules Remove modules/images/webp-uploads/fallback.js from ignorePatterns Run format-js on JS files Cherry pick fixes to JS linting from feature/image-loading-optimization Prevent sending header during test Manually fix remaining phpcs issues Run php-format Discontinue excluding all WordPress-Extra from tests so phpcbf can format Add @joemcgill to images and measurement focuses. Update codeowners, removing inactive contributors. Introduce perflab-plugin-management.js Load migration script in settings page Update function name Apply suggestions from code review Address slow query feedbacks Revise migration pointer un-dismissal to work for all admins Rename variable. improve code Require Node v20 ...

… add/ilo-tests * 'trunk' of https://github.com/WordPress/performance: Re-run composer update with PHP 8.1 Run composer update Fix PHPStan errors in tests

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

…purposes

This reverts commit 4656cbf.

This reverts commit 767c895.

…env config

Update Optimization Detective plugin build process Co-authored-by: thelovekesh <thelovekesh@git.wordpress.org> Co-authored-by: westonruter <westonruter@git.wordpress.org> Co-authored-by: felixarntz <flixos90@git.wordpress.org>

…ation-detective-readme

…-readme Update readme and phpdoc for initial Optimization Detective release Co-authored-by: westonruter <westonruter@git.wordpress.org> Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org> Co-authored-by: felixarntz <flixos90@git.wordpress.org>

github-actions · 2024-03-22T21:43:33Z

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: thelovekesh <thelovekesh@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@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.

felixarntz

@westonruter Almost looks good, a few points of feedback. Please take a look, though we could also consider addressing some of them afterwards.

felixarntz · 2024-03-22T21:52:59Z

.gitignore

@@ -32,6 +33,7 @@ build-cs/composer.lock

 node_modules/
 vendor/
+plugins/optimization-detective/detection/web-vitals*


I think it would be a good idea to use build directories within the individual standalone plugins for things like this. If we want to exclude built resources, it shouldn't be necessary to add exclusions for every individual one.

Sounds good. Let's do that in another PR to trunk.

felixarntz · 2024-03-22T21:55:37Z

includes/admin/server-timing.php

-	/*
-	 * This settings section technically includes a field, however it is directly rendered as part of the section
-	 * callback due to requiring custom markup.
-	 */
-	add_settings_section(
-		'output-buffering',
-		__( 'Output Buffering', 'performance-lab' ),
-		'perflab_render_server_timing_page_output_buffering_section',
-		PERFLAB_SERVER_TIMING_SCREEN
-	);
+	if ( ! has_filter( 'template_include', 'od_buffer_output' ) ) {
+		/*
+		 * This settings section technically includes a field, however it is directly rendered as part of the section
+		 * callback due to requiring custom markup.
+		 */
+		add_settings_section(
+			'output-buffering',
+			__( 'Output Buffering', 'performance-lab' ),
+			'perflab_render_server_timing_page_output_buffering_section',
+			PERFLAB_SERVER_TIMING_SCREEN
+		);
+	}


Is this change really needed? Is it harmful to have both output buffers active?

I'm asking since I don't like the idea of excluding PL specific logic because of another standalone plugin. There are lots of WordPress plugins that have output buffers, so it's a bit random we would cater for the one from Optimization Detective here just because it's also our plugin. What would be the problem of having the Server-Timing output buffering active in combination with the one from Optimization Detective?

The thinking was that if Optimization Detective introduces an official way to do output buffering (for Core-43258), that this can be reused.

And that if a PL standalone plugin is already doing output buffering, it's kinda unnecessary to do yet more output buffering on top.

In other words, it's the Performance Lab plugin taking advantage of the capabilities of one of its own feature plugins.

If there was a standard way to do this, that would be great indeed. However, I'm not sure there's good value in connecting the plugins in this way. It comes with its own set of questions, such as:

Which of the two plugins should be the "source of truth" for output buffering when both are active?

If PL uses OD's output buffering capabilities, how do we deal with the somewhat confusing implications of that for the Server-Timing screen? It's strange to just hide the control without any explanation.

I'm not sure it's worth discussing those questions given the two plugins could just work independently. And again, it's somewhat arbitrary anyway. If any other plugin can have its own output buffering, I don't think it's important that these two plugins don't both add their own.

For those reasons I'd prefer to either remove this integration to simplify things, or alternatively to polish it. Could be done in a follow up PR though.

Since it doesn't impact the Optimization Detective plugin's codebase, let's do in another PR.

felixarntz · 2024-03-22T21:55:47Z

includes/admin/server-timing.php

@@ -93,7 +95,7 @@ static function () {
 				);
 				?>
 			</p>
-			<?php if ( ! perflab_server_timing_use_output_buffer() ) : ?>
+			<?php if ( ! perflab_server_timing_use_output_buffer() && ! has_filter( 'template_include', 'od_buffer_output' ) ) : ?>


felixarntz · 2024-03-22T21:55:57Z

includes/server-timing/class-perflab-server-timing.php

+		// Skip doing anything with output buffering if it is already enabled via Optimization Detective.
+		if ( has_filter( 'template_include', 'od_buffer_output' ) ) {
+			// It feels better if this could rather be replaced with add_action( 'shutdown', [ $this, 'send_header' ] )
+			// However, this does not work because the buffer is sent before the shutdown callback is executed.
+			add_filter(
+				'od_template_output_buffer',
+				function ( $buffer ) {
+					$this->send_header();
+					return $buffer;
+				},
+				PHP_INT_MAX
+			);
+			return $passthrough;
+		}
+


phpcs.xml.dist

felixarntz · 2024-03-22T21:57:25Z

.gitignore

@@ -23,6 +23,7 @@ nbproject/

 build
 .wp-env.override.json
+*.asset.php


See my other comment below, if we used a build directory, we wouldn't need to explicitly exclude this from here and a few other places.

felixarntz · 2024-03-22T21:58:43Z

phpcs.xml.dist

 	<file>.</file>
 	<exclude-pattern>./build/*</exclude-pattern>
 	<exclude-pattern>./node_modules/*</exclude-pattern>
 	<exclude-pattern>./plugins/*</exclude-pattern>
 	<exclude-pattern>./vendor/*</exclude-pattern>
+	<exclude-pattern>*.asset.php</exclude-pattern>


FWIW, Gutenberg does this as well: https://github.com/WordPress/gutenberg/blob/4056660341852e2b627127d2dacbbedd38cb6e3a/phpcs.xml.dist#L80

adamsilverstein

Looks good, maybe we can address additional feedback in separate PR?

felixarntz

Good to merge, though let's address the two points from my previous review separately.

westonruter · 2024-03-22T23:21:34Z

let's address the two points from my previous review separately.

See #1082

westonruter and others added 30 commits December 14, 2023 10:31

Use ILO prefix for test classes

7324ce2

Detect background-image inline style on elements when iterating

d95c585

Implement initial support for preloading LCP background images

0942fa5

Add E2E test for background-image optimization

19459d8

Move XPath pattern definition to ILO_HTML_Tag_Processor

2df915d

Use ILO prefix for test classes

966c5b3

Merge pull request #892 from WordPress/add/server-side-breadcrumbs

7c0eb74

Rely on server-side breadcrumbs for detection and optimization

Merge branch 'feature/image-loading-optimization' into add/ilo-tests

7455a2e

Add server timing for image-loading-optimization

9d64828

Remove obsolete link preload workaround for responsive images

c6ff3c0

Run composer update

28db827

Fix Generic.CodeAnalysis.UnusedFunctionParameter

3709688

Fix filter return type for PHPStan static analysis

1b3b859

Fix sending header() in test_perflab_aea_clean_aea_audit_action

fd43aaa

Add since tags to ILO_HTML_Tag_Processor methods

df0d09e

Use test method prefix consistenty instead of redundant tag

1fa877c

Remove unnecessary ilo_needs_url_metric_for_breakpoint()

3da1363

Revert test changes moved to #924

2181d88

Unrevert 2181d88 for server-timing-tests.php

930d4ed

Unrevert 2181d88 for audit-enqueued-assets-test.php

8dd3283

Reset SERVER global after each test

bf1bd17

Unset REQUEST_URI in tests that call go_to

54396ec

Merge pull request #929 from WordPress/merge/trunk-into-image-loading…

f4b13ad

…-optimization Merge trunk into feature/image-loading-optimization

Update ilo_verify_url_metrics_storage_nonce() to return bool

d865bac

Move ilo_can_optimize_response() to optimization.php

ed305a9

Merge branch 'trunk' of https://github.com/WordPress/performance into…

8874d07

… add/ilo-tests * 'trunk' of https://github.com/WordPress/performance: Re-run composer update with PHP 8.1 Run composer update Fix PHPStan errors in tests

Add missing since tag

06ac5fe

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

thelovekesh and others added 18 commits March 22, 2024 10:13

Move utils from webpack config to bin/webpack

381ba0f

Update optimization-detective plugin build step

c2815e3

Move webpack utils import to Internal dependencies

d0cb266

Add getPluginRootPath() to exports

2f3605a

Add build:plugin command for embed-optimizer plugin

b246546

Decouple plugins asset building from main Webpack config

767c895

Fix doing_it_wrong to allow freshness ttl to be zero for development …

2052292

…purposes

Add console error when web-vitals.js is absent

4656cbf

Revert "Add console error when web-vitals.js is absent"

6ae5fb0

This reverts commit 4656cbf.

Revert "Decouple plugins asset building from main Webpack config"

0e07d11

This reverts commit 767c895.

Add web-vitals file exists check to load.php

3aae425

Only do file exists check if in admin or WP-CLI

ae07548

Update PL plugin tests workflow to remove standlaone plugins from wp-…

bc906ae

…env config

Merge branch 'feature/image-loading-optimization' into update/optimiz…

a8fc1d8

…ation-detective-readme

Update readme from Google Doc

48eca75

Fix typo

8dc0c8b

westonruter marked this pull request as ready for review March 22, 2024 21:43

westonruter requested review from felixarntz and mukeshpanchal27 as code owners March 22, 2024 21:43

Merge branch 'trunk' into feature/image-loading-optimization

4352e58

felixarntz reviewed Mar 22, 2024

View reviewed changes

adamsilverstein self-requested a review March 22, 2024 22:35

adamsilverstein approved these changes Mar 22, 2024

View reviewed changes

felixarntz approved these changes Mar 22, 2024

View reviewed changes

westonruter merged commit 747956b into trunk Mar 22, 2024
37 checks passed

westonruter mentioned this pull request Mar 22, 2024

Further refine Optimization Detective integration with build process and PL plugin #1082

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge Optimization Detective into trunk #1079

Merge Optimization Detective into trunk #1079

westonruter commented Mar 22, 2024 •

edited

github-actions bot commented Mar 22, 2024 •

edited

felixarntz left a comment

felixarntz Mar 22, 2024

westonruter Mar 22, 2024 •

edited

felixarntz Mar 22, 2024

westonruter Mar 22, 2024

westonruter Mar 22, 2024

westonruter Mar 22, 2024

felixarntz Mar 22, 2024

westonruter Mar 22, 2024

felixarntz Mar 22, 2024

felixarntz Mar 22, 2024

felixarntz Mar 22, 2024

felixarntz Mar 22, 2024

westonruter Mar 22, 2024

adamsilverstein left a comment

felixarntz left a comment

westonruter commented Mar 22, 2024

Merge Optimization Detective into trunk #1079

Merge Optimization Detective into trunk #1079

Conversation

westonruter commented Mar 22, 2024 • edited

github-actions bot commented Mar 22, 2024 • edited

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter Mar 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsilverstein left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

westonruter commented Mar 22, 2024

westonruter commented Mar 22, 2024 •

edited

github-actions bot commented Mar 22, 2024 •

edited

westonruter Mar 22, 2024 •

edited