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

CLI: Fix confusing runtime environment setup order #749

Merged
merged 14 commits into from
Oct 26, 2024

Conversation

felixarntz
Copy link
Member

@felixarntz felixarntz commented Oct 25, 2024

Related to #597.

As of today, in WP-CLI the Runtime_Environment_Setup sets up the separate database tables after all the regular preparations have run (i.e. Universal_Runtime_Preparation). This is confusing and inconsistent with how it works in the AJAX based workflow, where the Runtime_Environment_Setup is executed in a previous request. This is how it was intended, as ideally setting up the separate database tables to run runtime checks against needs to be the earliest step. However, in WP-CLI this has not been happening so far, because it was invoked by the command implementation itself, which however is executed after the early initialization process.

For example, today the option_active_plugins filter is applied on the real database tables for WP-CLI, which means the separate database tables will only contain the filtered plugins. In the AJAX based flow on the other hand, all active plugins from the real database tables will be added to the separate database tables, and the option_active_plugins filter is only applied then. This is, to my knowledge, not causing an actual problem today, but it's just strange and error-prone to have to consider both execution orders.

I had mentioned in this comment that it's "just a technical nuance", but it's still weird and inconsistent, and it doesn't have to be.

This PR fixes it:

  • Abstract relevant runtime initialization in the Abstract_Check_Runner class to a new protected method initialize_runtime().
  • In CLI_Runner, override that method to first execute Runtime_Environment_Setup, and of course also add its cleanup callback to the list of callbacks.
  • This furthermore allows to simplify the Plugin_Check_Command class:
    • Remove the separate use of Runtime_Environment_Setup since it's now handled by CLI_Runner.
    • By doing that, we can also remove the private (and somewhat duplicated) has_runtime_check() method, as well as an extra call to the get_checks_to_run() method which only existed for that purpose.

It now all works through the CLI_Runner, i.e. less technical and cognitive overhead for the Plugin_Check_Command implementation.

…cution for WP-CLI and simplify WP-CLI command implementation accordingly.
@felixarntz felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall plugin infrastructure WP-CLI Issues related to WP-CLI labels Oct 25, 2024
@felixarntz felixarntz added this to the 1.3.0 milestone Oct 25, 2024
Copy link

github-actions bot commented Oct 25, 2024

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

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

@felixarntz
Copy link
Member Author

It took me a while to get the wp_install() call to work early, but I got it resolved now. So this PR is good for review!

@swissspidy swissspidy changed the title Fix inconsistent and confusing order of runtime environment setup execution for WP-CLI and simplify WP-CLI command implementation accordingly CLI: Fix confusing runtime environment setup order Oct 25, 2024
Copy link
Member

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

Appreciate all the digging into this! 👏

includes/Checker/Abstract_Check_Runner.php Outdated Show resolved Hide resolved
includes/Checker/CLI_Runner.php Outdated Show resolved Hide resolved
@ernilambar ernilambar merged commit afbe0a7 into trunk Oct 26, 2024
30 checks passed
@ernilambar ernilambar deleted the fix/inconsistent-runtime-setup branch October 26, 2024 00:21
@davidperezgar
Copy link
Member

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Issues for the overall plugin infrastructure [Type] Enhancement A suggestion for improvement of an existing feature WP-CLI Issues related to WP-CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants