Documentation of PCP check process flows in all scenarios

[felixarntz]

The process flows in the different scenarios in which PCP can be used are quite complex to understand, most notably when runtime checks are involved. The complexities for runtime checks mostly stem from the fact that part of the functionality needs to be initialized very early, before plugins are loaded, which is achieved by temporarily placing an object-cache.php drop-in (if possible).

This issue is about documenting those flows to help contributors better understand these flows and potentially uncover current bugs.

Since I was heavily involved in the architecture of the plugin and making runtime checks possible in safe way, I went over the code in depth again (which was also very helpful to refresh my memory!) and documented the current flows individually. For now, I put it all in this public document, for easy discussion via contextual comments: https://docs.google.com/document/d/1wDGZBwWB2WAxfbHE3lygIzQFK8IssCa5apOyaBolukQ/edit
Please request comment access if you'd like to leave comments.

Note that I did encounter some problems while documenting the flows, particularly about supporting checks added via other plugins ("addon checks"). I added comments on the doc for all of those problems. I would like to use this documentation issue also as an opportunity to discuss solutions to those problems.

For reference, listing the problems found here (roughly ordered by severity):

• Force_Single_Plugin_Preparation prevents plugins other than PCP and the checked plugin from being active when any runtime checks are included, preventing addon checks to work in that context. See doc comment. and Improve active_plugins filter #608 for a PR that fixes this.
• When running runtime checks, the database table prefix is only modified when actually running the checks, but active plugins are still filtered. This can lead to unexpected modification of the actual site database. A solution would be to modify the database table prefix more or less throughout the entire request. See doc comment. and Always use prefixed tables during runtime check requests #768 for a PR that fixes this.
• In WP-CLI context, runtime checks may be run when they shouldn't, which could lead to unexpected results with other plugins active. See Prevent that runtime checks are allowed in CLI context when they cannot be used #598 for a PR that fixes this.
• Abstract_Check_Runner::get_checks_to_run() is called before plugins are loaded when initialized early, just to determine whether runtime checks are included or not. This means addon checks will not be considered, even though they may include the only runtime check requested. See doc comment. and Allow runtime initialization even when only addon checks are requested #612 for a PR that fixes this.

But first and foremost: Please review the doc if you're interested, and leave questions wherever something may still be unclear.

[felixarntz]

Gathering some proposals for solving the remaining 3 bugs:

1. Force_Single_Plugin_Preparation prevents plugins other than PCP and the checked plugin from being active when any runtime checks are included, preventing addon checks to work in that context.
   • Instead of only keeping PCP and the checked plugin, the logic should scan the currently active plugins to find those that have plugin-check as a dependency. Those plugins should then remain active too.
   • This means any plugin with PCP checks will be required to formally mark PCP as a dependency, but that's probably a reasonable requirement.
   • Props @swissspidy for suggesting this on the doc.
2. When running runtime checks, the database table prefix is only modified when actually running the checks, but active plugins are still filtered.
   • We should introduce a new preparation class, e.g. Use_Separate_Tables_Preparation, which gets invoked via Universal_Runtime_Preparation::prepare().
   • This preparation would switch the database table prefix to the custom prefix, and its cleanup would switch back.
   • It would only perform the switch conditionally after checking that the separate database tables are already present. This is necessary since in WP-CLI the tables are only created after Universal_Runtime_Preparation::prepare(), via Runtime_Environment_Setup::set_up(). Since that call is made directly by the WP-CLI command, that command could also manually handle the new Use_Separate_Tables_Preparation, invoking it right after Runtime_Environment_Setup::set_up(), and invoking its cleanup right before Runtime_Environment_Setup::clean_up().
   • The logic to set and reset the database table prefix in Abstract_Check_Runner::run() should be removed in favor of the aforementioned new preparation.
3. Abstract_Check_Runner::get_checks_to_run() is called before plugins are loaded when initialized early, just to determine whether runtime checks are included or not (and thus whether or not to execute the runtime check specific preparations).
   • This bug is only relevant for requests where the only runtime check(s) to run comes from an addon plugin. In other words, this only potentially matters for specific WP Admin AJAX requests or for WP-CLI requests where checks are explicitly limited by the --checks argument.
   • We can't fully fix this, but we can find a reasonable workaround: When getting checks early (prior to plugins loaded), if there are any checks which we don't know (meaning they could be from an addon plugin), we assume that they include a runtime check. Only if all checks to run are already loaded and are all static checks, we know for certain that no runtime checks are needed in the request.
   • Specifically, this could be implemented by modifying Abstract_Check_Runner::prepare(): If this is run prior to plugins_loaded, we wrap the Abstract_Check_Runner::get_checks_to_run() call in a try ... catch. If at this point the exception from Default_Check_Collection::require() is thrown, it is likely that this is because one of the given slugs refers to a check that's not loaded yet because it's from an addon plugin. In this case, we simply assume it is a runtime check and initialize the relevant logic. If the exception is not thrown, it means that all checks to run are already loaded, or that the checks to run are not limited to specific checks, in which case there'll be runtime checks present anyway.

@swissspidy LMKWYT or if you have any questions regarding those ideas.

[swissspidy]

Force_Single_Plugin_Preparation prevents plugins other than PCP and the checked plugin from being active when any runtime checks are included, preventing addon checks to work in that context.

• Instead of only keeping PCP and the checked plugin, the logic should scan the currently active plugins to find those that have plugin-check as a dependency. Those plugins should then remain active too.

As just brought up by @felixarntz, we will also need to include any dependencies of the plugin being tested.

[swissspidy]

• This bug is only relevant for requests where the only runtime check(s) to run comes from an addon plugin. In other words, this only potentially matters for specific WP Admin AJAX requests or for WP-CLI requests where checks are explicitly limited by the --checks argument.

AFAICT it is always relevant, even when not specifying --checks.

When initializing early, we need to manually load any plugins depending on PCP. Question is, where do we do that?

[felixarntz]

I think the only reason that Abstract_Check_Runner::get_checks_to_run() is called early is to determine whether or not to initialize the runtime environment related preparations (which should only happen if at least one runtime check is run).

The actual checks that are run are only retrieved later when plugins are already loaded, so with #608 implemented it should no longer be a problem.

I think a better solution than loading all relevant plugins early (which has lots of other potential side effects/risks), I think we should rather change the logic to be aware that this is running too early for plugins to be loaded. For example:

• Catch exception that is thrown because an invalid slug is given, since at this point we don't know whether the slug was actually invalid or whether it was for an addon check that isn't loaded yet.
• If the exception is thrown, we proceed as if we knew there were runtime checks included. We don't know, but in this scenario it's better to initialize that logic because initializing it when needed is only good for performance while not initializing it when needed breaks things. :)

[felixarntz]

FYI I believe #768 is the last crucial problem to resolve related to the process flows and infrastructure.

Once that is completed, I plan to revise the doc based on the updated/fixed flows, and after that we can see how we want to best integrate that documentation into this repo itself.

cc @swissspidy @ernilambar @davidperezgar

[davidperezgar]

Perfect!

[felixarntz]

I have updated the process workflows doc based on how everything behaves now. With the additional fixes made, it's a lot more consistent - still different due to the different implications between AJAX and CLI, but far better than before. Please have a look.

The most important revision is obviously the tables, which especially for the CLI workflow has now been simplified.

Additionally, I've added two paragraphs on top about the most important differences, as a high-level summary:

The most significant difference between WP Admin and WP-CLI workflows is that in WP Admin, a plugin is checked via multiple subsequent AJAX requests, one request for each check, whereas with WP-CLI the entire plugin is checked within a single process.

The most significant difference between only static checks and with runtime checks is that runtime checks require the setup of a separate WordPress installation (under a different database table prefix), as well as the presence of a specific object-cache.php drop-in to hijack certain WordPress functionality early on.

With this being in a decent state now, let's discuss what we want to do with this? Should we move the doc over to a Markdown file in e.g. a docs folder? Should we use any of the tools mentioned in this comment to create flow chart diagrams? If so, which one?

[swissspidy]

A Markdown file with Mermaid diagrams would be great!