Migrate Site Health checks from being modules to becoming part of the plugin core #1042

thelovekesh · 2024-03-09T13:16:42Z

Summary

Relevant technical choices

Checklist

PR has either [Focus] or Infrastructure label.
PR has a [Type] label.
PR has a milestone or the no milestone label.

felixarntz

@thelovekesh This mostly looks good. Please check the below comments.

Looking at the unit test failures, only the 5th failure is actually valid. The other tests just fail because there aren't any modules anymore, which is fine. Please remove those tests.

includes/site-health/load.php

load.php

tests/load-tests.php

github-actions · 2024-03-11T21:03:38Z

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

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

felixarntz · 2024-03-11T21:28:02Z

includes/server-timing/object-cache.copy.php

-		if ( ! file_exists( $plugin_dir . 'server-timing/load.php' ) ) {
+		if ( ! file_exists( $plugin_dir . 'includes/server-timing/load.php' ) ) {
 			$plugin_dir = $plugins_dir . '/performance/';
-			if ( ! file_exists( $plugin_dir . 'server-timing/load.php' ) ) {
+			if ( ! file_exists( $plugin_dir . 'includes/server-timing/load.php' ) ) {
 				return;
 			}
 		}

-		require_once $plugin_dir . 'server-timing/class-perflab-server-timing-metric.php';
-		require_once $plugin_dir . 'server-timing/class-perflab-server-timing.php';
-		require_once $plugin_dir . 'server-timing/load.php';
-		require_once $plugin_dir . 'server-timing/defaults.php';
+		require_once $plugin_dir . 'includes/server-timing/class-perflab-server-timing-metric.php';
+		require_once $plugin_dir . 'includes/server-timing/class-perflab-server-timing.php';
+		require_once $plugin_dir . 'includes/server-timing/load.php';
+		require_once $plugin_dir . 'includes/server-timing/defaults.php';


@thelovekesh Now that I see this change, moving the files for the Server-Timing API is tricky from a backward compatibility perspective. Here's why:

The wp-content/object-cache.php drop-in may already be placed on some sites, from an older Performance Lab version. As such, it would try to load the old paths, which wouldn't work. It wouldn't cause a fatal error because of the checks above, but it just wouldn't load the API.

In order to fix that, we'll have to enhance the logic that places that drop-in file (i.e. copies from object-cache.copy.php to the actual drop-in) so that it detects whether the current file is outdated.

I'm happy to open another PR for that. However, at a minimum, let's update the drop-in version number in this PR from 2 to 3 (in both the file header and constant).

@thelovekesh I've opened #1047 for the purpose of updating an older version of the drop-in, please take a look. Depending which of the two PRs gets merged first, the other one will require a minor update (to sync the version numbers and file paths).

mukeshpanchal27

Thanks for the PR! can we remove https://github.com/WordPress/performance/tree/enhancement/move-modules-to-pl-core/tests/testdata/demo-modules directory.

includes/site-health/load.php

felixarntz

@thelovekesh Thank you for the updates. Other than the remaining feedback from @mukeshpanchal27, LGTM!

thelovekesh · 2024-03-12T20:10:11Z

Thanks for the PR! can we remove https://github.com/WordPress/performance/tree/enhancement/move-modules-to-pl-core/tests/testdata/demo-modules directory.

@mukeshpanchal27 As discussed in today's performance meeting, we will remove modules in a separate PR.

thelovekesh · 2024-03-13T12:33:39Z

@mukeshpanchal27 Can you please re-review this PR?

…ml.dist

thelovekesh added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure no milestone PRs that do not have a defined milestone for release Creating standalone plugins labels Mar 9, 2024

felixarntz reviewed Mar 11, 2024

View reviewed changes

includes/site-health/load.php Outdated Show resolved Hide resolved

includes/site-health/load.php Outdated Show resolved Hide resolved

load.php Outdated Show resolved Hide resolved

load.php Outdated Show resolved Hide resolved

tests/load-tests.php Show resolved Hide resolved

thelovekesh marked this pull request as ready for review March 11, 2024 21:03

thelovekesh requested review from manuelRod and adamsilverstein as code owners March 11, 2024 21:03

thelovekesh requested a review from felixarntz March 11, 2024 21:03

thelovekesh requested review from mukeshpanchal27 and joemcgill March 11, 2024 21:03

felixarntz reviewed Mar 11, 2024

View reviewed changes

felixarntz mentioned this pull request Mar 11, 2024

Enhance object-cache.php drop-in placement logic to support updating to a newer version of the file #1047

Merged

3 tasks

mukeshpanchal27 reviewed Mar 12, 2024

View reviewed changes

includes/site-health/load.php Outdated Show resolved Hide resolved

felixarntz approved these changes Mar 12, 2024

View reviewed changes

felixarntz requested review from mukeshpanchal27 and swissspidy March 12, 2024 17:37

thelovekesh force-pushed the enhancement/move-modules-to-pl-core branch from 78f4e0e to 88cd219 Compare March 12, 2024 20:13

thelovekesh added 9 commits March 13, 2024 18:04

Move server-timing to top-level includes directory

0fdf37e

Move site health tests from various focus areas to includes directory

6a3c9f9

Add loader file for site health tests

4e739af

Remove module based loading from site health tests

3e27c87

Update phpstan.neon.dist

29e549b

Move server-timing and site health tests to tests/includes directory

134cb63

Remove ref for legacy module slugs from perflab_get_module_settings()

71d4aeb

Fix webp support site health test name

c303628

Add site health tests to be loaded with plugin

2fb4e7a

thelovekesh added 9 commits March 13, 2024 18:04

Update unit tests after removing site health tests from modules

d7c36a5

Update site health checks loader

8604b65

Fix namings for PL core APIs

80f8a0a

Remove failing module test cases from tests

a06213d

Fix server-timing API path

6d8d231

Update server-timing object-cache path and exclude pattern in phpcs.x…

29dfb4c

…ml.dist

Bump object cache drop-in from 2 to 3

7ef492a

Bump object cache drop-in plugin version from 2 to 3

686a8d9

Remove site health caps safeguard

86ff88b

thelovekesh force-pushed the enhancement/move-modules-to-pl-core branch from 88cd219 to 86ff88b Compare March 13, 2024 12:34

swissspidy approved these changes Mar 13, 2024

View reviewed changes

mukeshpanchal27 approved these changes Mar 13, 2024

View reviewed changes

mukeshpanchal27 merged commit b928061 into trunk Mar 13, 2024
28 checks passed

mukeshpanchal27 deleted the enhancement/move-modules-to-pl-core branch March 13, 2024 14:24

felixarntz changed the title ~~Move site health module to PL core~~ Migrate Site Health checks from being modules to becoming part of the plugin core Mar 13, 2024

felixarntz added this to the PL Plugin 3.0.0 milestone Mar 13, 2024

felixarntz removed the no milestone PRs that do not have a defined milestone for release label Mar 13, 2024

felixarntz mentioned this pull request Mar 13, 2024

Remove modules infrastructure #1030

Closed

thelovekesh mentioned this pull request Mar 21, 2024

Move admin to includes directory #1072

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Site Health checks from being modules to becoming part of the plugin core #1042

Migrate Site Health checks from being modules to becoming part of the plugin core #1042

thelovekesh commented Mar 9, 2024

felixarntz left a comment

github-actions bot commented Mar 11, 2024 •

edited

Loading

felixarntz Mar 11, 2024

felixarntz Mar 11, 2024 •

edited

Loading

mukeshpanchal27 left a comment

felixarntz left a comment

thelovekesh commented Mar 12, 2024

thelovekesh commented Mar 13, 2024

Migrate Site Health checks from being modules to becoming part of the plugin core #1042

Migrate Site Health checks from being modules to becoming part of the plugin core #1042

Conversation

thelovekesh commented Mar 9, 2024

Summary

Relevant technical choices

Checklist

felixarntz left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 11, 2024 • edited Loading

felixarntz Mar 11, 2024

Choose a reason for hiding this comment

felixarntz Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

thelovekesh commented Mar 12, 2024

thelovekesh commented Mar 13, 2024

github-actions bot commented Mar 11, 2024 •

edited

Loading

felixarntz Mar 11, 2024 •

edited

Loading