Skip to content

CLI Runtime environment setup/cleanup#100

Merged
felixarntz merged 16 commits into
trunkfrom
feature/cli-runtime-environment-setup-cleanup
Mar 27, 2023
Merged

CLI Runtime environment setup/cleanup#100
felixarntz merged 16 commits into
trunkfrom
feature/cli-runtime-environment-setup-cleanup

Conversation

@jjgrainger
Copy link
Copy Markdown
Contributor

Addresses acceptance criteria for #87

Closes #87

@jjgrainger jjgrainger requested a review from felixarntz March 16, 2023 17:32
@jjgrainger
Copy link
Copy Markdown
Contributor Author

Hi @felixarntz

I've setup a draft PR to review the changes here. I have the command working with the Runtime setup, however I think there is an opportunity to refactor some elements of the code here, in particular the latest additions to the Plugin_Check_Command.

Currently this uses the same logic found in the Admin_AJAX::get_checks_to_run() method to determine the checks to be run, if those checks can be run and filtering out runtime checks if the plugin is inactive.

I wasn't sure how best to solve this but my current thinking is we could potentially move a lot of this logic over to the Abstract_Check_Runner?

  • The get_checks_to_run method could handle the additional logic to either remove runtime checks from the array or throw an exception if the plugin is inactive based on the checks requested.
  • Rename the requires_universal_preparations method to has_runtime_check and make it public so it can be used in the Plugin_Check_Command to determine if we need to use the Runtime_Environment_Setup
  • Update the logic for the Admin_AJAX to use a AJAX_Runner instance to handle returning the check slugs to run in the get_checks_to_run method.

Let me know your thoughts here. Thanks

Comment thread includes/CLI/Plugin_Check_Command.php Outdated
Comment thread includes/CLI/Plugin_Check_Command.php Outdated
@felixarntz
Copy link
Copy Markdown
Member

@jjgrainger Thanks for pointing those out!

I agree with your points 1 and 3, it would be great if you could incorporate those changes.

Regarding point 2, I'm not sure about that one yet, it doesn't seem as appropriate, although I also agree with you we should avoid the duplicate logic. Maybe for now let's keep that one as is (i.e. acknowledge that we'll for now have the duplicate logic for the "has runtime check" concept), and we can revisit this as part of the architecture review.

@jjgrainger jjgrainger marked this pull request as ready for review March 20, 2023 19:00
@jjgrainger
Copy link
Copy Markdown
Contributor Author

Thanks @felixarntz I've made the updates for 1 and 3. This is now ready for review.

Copy link
Copy Markdown
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@jjgrainger Mostly looks good including the refactoring, however a few things need to be fixed.

Comment thread cli.php Outdated
Comment thread cli.php Outdated
Comment thread cli.php Outdated
Comment thread cli.php Outdated
Comment thread includes/Admin/Admin_AJAX.php Outdated
Comment thread includes/Checker/Abstract_Check_Runner.php Outdated
Comment thread includes/Traits/URL_Aware.php
Comment thread includes/Checker/Abstract_Check_Runner.php
Comment thread includes/Checker/Abstract_Check_Runner.php Outdated
Comment thread includes/Plugin_Main.php Outdated
Copy link
Copy Markdown
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Thanks @jjgrainger, LGTM!

@felixarntz felixarntz requested a review from spacedmonkey March 21, 2023 19:36
Comment thread cli.php Outdated
Copy link
Copy Markdown
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Thanks @jjgrainger, Left nit-pick.

Comment thread includes/CLI/Plugin_Check_Command.php Outdated
}

/**
* Check for a Runtime_Check in a list of checks
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
* Check for a Runtime_Check in a list of checks
* Checks for a Runtime_Check in a list of checks.

Comment thread cli.php Outdated
Comment thread cli.php Outdated
Comment thread tests/Utilities/Plugin_Request_Utility_Tests.php
Comment thread tests/Utilities/Plugin_Request_Utility_Tests.php
Comment thread includes/Checker/CLI_Runner.php
Comment thread cli.php Outdated
Comment thread includes/Admin/Admin_AJAX.php
Comment thread includes/Checker/Abstract_Check_Runner.php
Comment thread tests/Checker/AJAX_Runner_Tests.php
@spacedmonkey
Copy link
Copy Markdown
Member

I think this is close, I have some nicpics, otherwise this is looking good.

Comment thread cli.php
Copy link
Copy Markdown
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

Created a number of follow on issues. But this is good to merge now.

@felixarntz
Copy link
Copy Markdown
Member

Thanks @spacedmonkey!

@felixarntz felixarntz merged commit ba29045 into trunk Mar 27, 2023
@felixarntz felixarntz deleted the feature/cli-runtime-environment-setup-cleanup branch March 27, 2023 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI Runtime Environment Setup/Cleanup

4 participants