Created the site:remove-atlantis-legacy-modules command#103
Created the site:remove-atlantis-legacy-modules command#103bernattorras wants to merge 8 commits into
Conversation
This is a temporary command designed to bulk remove the legacy modules replaced by Atlantis (Colophon and the Plugin Autoupdate Filter) automatically.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Symfony CLI command class 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
commands/Site_Remove_Atlantis_Legacy_Modules.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ahegyes
Repo: a8cteam51/team51-cli PR: 97
File: commands/WPCOM_Site_WP_User_Delete.php:180-199
Timestamp: 2025-08-06T12:00:14.638Z
Learning: In the WPCOM_Site_WP_User_Delete command, when no administrators are found on a site, the process intentionally exits entirely rather than skipping that site, as this is considered an anomaly that should be investigated before attempting the deletion operation again.
🧬 Code graph analysis (1)
commands/Site_Remove_Atlantis_Legacy_Modules.php (4)
includes/functions.php (4)
get_enum_input(318-330)run_system_command(233-249)validate_user_choice(459-465)get_string_input(298-305)includes/functions-pressable.php (3)
get_pressable_site(104-108)get_pressable_site_deployhq_config(168-180)run_pressable_site_wp_cli_command(612-623)includes/functions-wpcom.php (3)
get_wpcom_site(69-71)get_wpcom_site_code_deployments(582-584)run_wpcom_site_wp_cli_command(631-642)includes/functions-github.php (3)
get_github_repository_from_deployhq_project(245-257)get_github_repository_branches(82-84)get_github_repository(32-34)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@commands/Site_Remove_Atlantis_Legacy_Modules.php`:
- Around line 328-360: The progress messages use the CSV row index ($index)
instead of a sequential site counter, so create a separate counter (e.g.
$site_count) initialized before the foreach over $csv_data and increment it at
the start of each iteration, then replace uses of $index in the output->writeln
calls (and in update_csv_row note messages if they should reflect sequential
position) with the new $site_count variable; update all occurrences inside the
loop (the Processing, Skipping, and Already processed messages around the
foreach handling) and keep $index for CSV-key lookups and update_csv_row calls
that rely on the original CSV row index.
- Around line 485-500: The condition is using the hardcoded
$this->legacy_modules (always >0); change the check to use the runtime-detected
list (e.g. $found_modules or $this->removed_modules) so the prompt only appears
when plugins were actually found, and pass that specific list to
uninstall_plugins_from_wordpress; update the $plugins_list construction and the
ConfirmationQuestion to use the chosen runtime array (and ensure
uninstall_plugins_from_wordpress accepts/receives that array instead of relying
on $this->legacy_modules).
🧹 Nitpick comments (3)
commands/Site_Remove_Atlantis_Legacy_Modules.php (3)
765-773: Redundant empty check and unsafe continuation path.The
get_wpcom_repository()method is only called frominitialize_repository()at line 624, after the empty repositories check at line 607. The check at line 765 is therefore redundant. Additionally, if somehow reached with empty repositories and the user continues,$this->gh_repositoryremains null.Consider removing this redundant check since the caller already handles the empty case:
♻️ Suggested refactor
private function get_wpcom_repository( InputInterface $input, OutputInterface $output ): void { $wpcom_gh_repositories = get_wpcom_site_code_deployments( $this->site->ID ); $gh_repository_name = null; - if ( empty( $wpcom_gh_repositories ) ) { - $output->writeln( '<error>Unable to find a WPCOM GitHub Deployments for the site.</error>' ); - - $question = new ConfirmationQuestion( '<question>Do you want to continue anyway? [y/N]</question> ', false ); - if ( true !== $this->getHelper( 'question' )->ask( $input, $output, $question ) ) { - $output->writeln( '<comment>Command aborted by user.</comment>' ); - exit( 1 ); - } - } - - if ( $wpcom_gh_repositories && 1 < count( $wpcom_gh_repositories ) ) { + if ( 1 < count( $wpcom_gh_repositories ) ) {
1062-1063: Consider making the Atlantis plugin version configurable.The plugin URL contains a hardcoded version (
v1.0.2). If this command lives beyond its temporary purpose, consider extracting this to a constant or configuration option for easier updates.
1107-1131: Uninstalls all legacy modules regardless of what was found.
uninstall_plugins_from_wordpress()uses$this->legacy_modulesto build the plugin list, which attempts to uninstall both legacy plugins even if only one (or neither) was found in the repository. This is related to the issue at line 486.♻️ Consider passing the found plugins explicitly
- private function uninstall_plugins_from_wordpress( OutputInterface $output ): void { + private function uninstall_plugins_from_wordpress( OutputInterface $output, array $plugins_to_uninstall = null ): void { $site_identifier = 'atomic' === $this->host ? $this->site->ID : $this->site->id; - $plugins_list = implode( ' ', $this->legacy_modules ); + $plugins_to_uninstall = $plugins_to_uninstall ?? $this->legacy_modules; + $plugins_list = implode( ' ', $plugins_to_uninstall );Then call it with the specific plugins found:
$this->uninstall_plugins_from_wordpress( $output, array_keys( $found_modules ) );
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@commands/Site_Remove_Atlantis_Legacy_Modules.php`:
- Around line 1124-1139: The code is passing the integer exit code returned by
run_pressable_site_wp_cli_command/run_wpcom_site_wp_cli_command into
json_decode; instead check the exit code stored in $result, return with
$this->write_output(...) if $result !== 0, then read the actual WP-CLI output
from $GLOBALS['wp_cli_output'] and pass that to json_decode; keep the existing
validation that json_decode($GLOBALS['wp_cli_output'], true) yields an array and
emit the same "Could not parse plugin list." message and return if it does not.
🧹 Nitpick comments (2)
commands/Site_Remove_Atlantis_Legacy_Modules.php (2)
765-777: Redundant API call for WPCOM code deployments.
get_wpcom_site_code_deployments()is already called at line 609 ininitialize_repository()before calling this method. Calling it again here at line 766 is redundant.♻️ Suggested optimization
Pass the already-fetched repositories as a parameter:
- private function get_wpcom_repository( InputInterface $input, OutputInterface $output ): void { - $wpcom_gh_repositories = get_wpcom_site_code_deployments( $this->site->ID ); + private function get_wpcom_repository( InputInterface $input, OutputInterface $output, array $wpcom_gh_repositories ): void {Then update the call site at line 628:
- $this->get_wpcom_repository( $input, $output ); + $this->get_wpcom_repository( $input, $output, $wpcom_gh_repositories );
1195-1219:uninstall_plugins_from_wordpressalways tries to uninstall all legacy modules.This method uses
$this->legacy_modules(line 1197) which includes all hardcoded legacy plugins, regardless of which were actually found on the site. This is related to the earlier issue at lines 489-504.Consider passing the list of found plugins as a parameter to avoid attempting to uninstall plugins that don't exist on the site (though WP-CLI handles this gracefully with warnings).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@commands/Site_Remove_Atlantis_Legacy_Modules.php`:
- Around line 430-436: The deployment error handling references an undefined
variable $site_host causing a warning; update the code in the error handling
block that computes $is_deployment_error and sets $deployment_type/$note to use
$this->host (or the appropriate property on the current class) instead of
$site_host so the check and message use the defined host value (replace all
occurrences of $site_host in that block with $this->host, e.g., where
$is_deployment_error is determined and when assigning $deployment_type).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commands/Site_Remove_Atlantis_Legacy_Modules.php`:
- Around line 454-466: When running in quiet/automation mode ($this->quiet),
avoid creating a ChoiceQuestion and prompting; instead default to a safe
non-interactive choice (e.g., set $deployment_choice = 'skip') and skip calling
$this->getHelper('question')->ask. Locate the block that builds the $choices
array and creates the ChoiceQuestion (uses ChoiceQuestion and
$deployment_choice) and add a conditional: if ($this->quiet) assign the default
string ('skip') to $deployment_choice and only construct/ask the ChoiceQuestion
when not quiet.
- Around line 1440-1459: In uninstall_plugins_from_wordpress, capture the
integer return value from run_pressable_site_wp_cli_command or
run_wpcom_site_wp_cli_command (depending on host) instead of ignoring it, then
check if it equals 0 before printing the success message; if non-zero, write an
error via $this->write_output (including the exit code and the original $command
for context) and throw or return to avoid reporting success erroneously. Ensure
you reference the $command variable and use the same write_output pattern
currently used for success/failure messages.
---
Duplicate comments:
In `@commands/Site_Remove_Atlantis_Legacy_Modules.php`:
- Around line 956-969: In clone_repo, stop pre-creating the destination folder
($this->repo_dir) before calling \run_system_command('git clone ...') because
git fails if the target directory already exists; instead only ensure the parent
repos folder ($this->repos_dir) exists, remove the mkdir of $this->repo_dir,
call
\run_system_command(array('git','clone',$this->gh_repository->clone_url,$this->repo_dir))
to let git create the directory, and move the success write_output for the clone
to after the command succeeds; also add a guard to detect if $this->repo_dir
already exists and fail with a clear message (or skip) before attempting clone.
- Around line 774-788: The early-return when $wpcom_gh_repositories is empty
leaves $this->gh_repository null and later dereferences
$this->gh_repository->name; instead of returning after the interactive
confirmation, abort the command consistently (throw an exception or exit with
non-zero status) so execution cannot continue with a null $this->gh_repository;
update the block that currently does "return;" (after the
ConfirmationQuestion/exit(1) path) to throw new \Exception('No WPCOM GitHub
repository available') or call exit(1) so downstream references to
$this->gh_repository->name are never reached.
2 participants
This is a temporary command designed to bulk remove the legacy modules replaced by Atlantis (Colophon and the Plugin Autoupdate Filter) automatically.
I'm still testing it, so adjustments may be added.
Summary by CodeRabbit