Make remaining modules (all Site Health) part of Performance Lab core

[felixarntz]

As recently discussed (also see #911 (comment)), the concept of modules will be entirely removed as part of the 3.0.0 release.

At this point, all features are already moved to standalone plugins exclusively, except for the Site Health modules where this was an explicit design decision, to keep features that don't directly enhance performance part of Performance Lab. This philosophy remains, with a slight adjustment of no longer using modules:

Any features that directly enhance performance should be standalone plugins.
Any features that don't directly enhance performance (such as measurement features or recommendations like Site Health) should be part of Performance Lab core.

Site Health checks should continue to remain part of Performance Lab. However, going forward, this should no longer be in the form of modules, but simply as part of Performance Lab core. This means:

• They'll be permanently active, as long as Performance Lab is active.
• There won't be a UI to control them. Their only UI will be the respective check itself within Site Health.
• They should still be implemented in distinct folders, in a standalone manner.
• They can still be ported over to WordPress core, once deemed eligible. At that point, they would be removed from Performance Lab.

[felixarntz]

I would propose the following implementation:

• Move the 3 remaining Site Health module directories into a new top level site-health directory.
• Remove the module headers from the 3 load.php files, but otherwise keep the module code as is.
• Move the tests for these 3 modules out of tests/modules, to a new tests/site-health location.
• In all cases, discard the so-called "focus" directories entirely. The Site Health check directories should be directly under the site-health directory.
• In the main Performance Lab load.php file, directly require_once the 3 Site Health check load.php files.
• Remove the mapping references of "legacy module slugs" in the perflab_get_module_settings() which reference the Site Health modules.

@mukeshpanchal27 @joemcgill @thelovekesh Let me know what you think.

[thelovekesh]

@felixarntz Makes sense to me. The only thing I would like to request is to limit the PL core code to a specific directory maybe the top level src or includes directory, to better organize the code. Aftert this the repo structure will look like:

- includes(or src)
    - admin
    - site-health
    - server-timing
- plugins
- tests

//...other config files/dirs

[felixarntz]

@thelovekesh I like that idea, let's do it. I'd say includes works best, as src is commonly used for JS code underlying a build process (e.g. blocks) as well, so we can keep that reserved for when we need it.

[joemcgill]

I agree with the folder structure @thelovekesh is proposing here. Here are the 3 directories in modules with site health checks:

• https://github.com/WordPress/performance/tree/trunk/modules/database/audit-autoloaded-options
• https://github.com/WordPress/performance/tree/trunk/modules/images/webp-support
• https://github.com/WordPress/performance/tree/trunk/modules/js-and-css/audit-enqueued-assets

I don't think we need to have separate files folders for each check, nor do we need 4 files for each check. I would do something like this:

- includes/
    - site-health/
        - load.php – This should be the entry point for all health checks which includes all of the necessary files for each check
        - audit-autoloaded-options.php – All code related to what is currently in this folder (helpers, hooks, etc)
        - webp-support.php - Similar to the above
        - audit-enqueued-assets – Similar to the above

[felixarntz]

@joemcgill I disagree with putting the Site Health checks all in one folder. They should still be kept standalone, and separate folders encourage that. We may merge individual checks to core, not all at once. Putting them in distinct files limits us to only using one file per check, which is not always a good idea, particularly for more complex checks.

For those reasons, I think separate folders per check is a more maintainable and scalable option. We can handle the files within however we want, but I think separate folders and a single entry point file to load for each check are crucial.

[joemcgill]

We can keep them in separate folders, if you prefer. I mainly am suggesting that we simplify the structure of the files now that we don't need to follow the previous architecture intended for more complex modules.

For example, in each of our current health checks, the functionality from the can-load.php, helper.php, and hooks.php can all be part of the same file. Given that each check at that point will be a single file per folder, each of them can be loaded by the main site-health entry file, includes/site-health/load.php, so we probably don't need separate load.php files for each heath check either.

If part of the goal is to write these health checks in a way that can easily be ported to WP Core, then it's also useful to keep in mind that most health checks in core are simply executed via a method on the WP_Site_Health class, or as a set of methods on a separate class, like WP_Site_Health_Auto_Updates, so keeping functionality for checks contained to one file that contains the main check callback that will be run is not an entirely bad thing.

[felixarntz]

Right, I'm not opposed to combining logic into a single file per check, where that makes sense. I just think we should not confine ourselves to having to do that, as some checks may require supplemental functionality - for instance the audit-enqueued-assets check is more complex as it also interacts with the frontend. That's something that even in core would probably not be part of the actual Site Health class, even though it facilitates a Site Health check.

In other words, as long as we put the Site Health checks each into their own folder, the rest can be decided on a case by case basis I think.