Update logic when deleting and uninstalling plugins

[dkoston]

Fixes: #437

1. Add missing hooks for delete_plugin and deleted_plugin to delete_plugin()
2. Add function comment to delete_plugin()
3. Also remove .l10n.php files when removing language files as per https://github.com/WordPress/WordPress/blob/e8b5d1a702e9fbf6a1f225df6501ea2e517938ec/wp-admin/includes/plugin.php#L1022
4. Add additional logic branch for a plugin which was uninstalled successfully but not deleted.
5. Remove any successfully deleted plugins from plugin update list.

Excuse my lack of tests and general ignorance of how to code on the WP CLI codebase. This is my first PR and I'm not seeing any testing like I would normally do for production projects:

1. I'd expect a set of functional tests which would test conditional logic branches here (ensure that --skip-delete doesn't actually call $this->delete_plugin by using a spy)
2. Typically I'd unit test and create smaller functions from the logic here as well and since these functions are essentially all calling WordPress functions, I'd do this:
   a. Would update WordPress core to take any logic that's being copy/pasted into WP CLI and extract it into functions that can be called, rather than duplicating the logic as it would mean only 1 set of functions to maintain.
   b. Would be create a set of table-driven tests to test all the logic branches of uninstall() which have spies to make sure that the appropriate WordPress core functions are called (or not) depending on the flags passed

However, I don't know the logic and history behind why this codebase has duplicate logic. I'm not sure if there are stability concerns relying on core directly like that or other reason.

Let me know if there's ways I can improve the contribution and what levels of testing I can apply to make sure we have confidence in the current changes and to prevent regressions.

Thanks!

[dkoston]

Not sure what to do about the hook name linter which is not compatible with the legacy WordPress hook names:

✗ vendor/bin/phpcs -q --report=full --report-checkstyle=/tmp/phpcs-checkstyle-report.xml

FILE: src/Plugin_Command.php
-------------------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
-------------------------------------------------------------------------------------------------------------------------
 1510 | ERROR | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "delete_plugin".
 1534 | ERROR | Hook names invoked by a theme/plugin should start with the theme/plugin prefix. Found: "deleted_plugin".
-------------------------------------------------------------------------------------------------------------------------

[swissspidy]

Thanks for your PR, it's much appreciated!

Excuse my lack of tests and general ignorance of how to code on the WP CLI codebase. This is my first PR and I'm not seeing any testing like I would normally do for production projects:

WP-CLI uses Behat as its functional test suite. Functional tests are different than unit tests in that they execute the entire WP-CLI command, and ensure they always work as expected. Every repository has a features/ directory with one or more YAML-formatted *.feature files. Here, plugin-delete.feature and plugin-uninstall.feature are relevant.

See https://make.wordpress.org/cli/handbook/contributions/pull-requests/#running-and-writing-tests and https://www.youtube.com/playlist?list=PL_B8Y6K6MH2d6T7pYa6dloUgk67mfBT4K

One example test would be to install a plugin, install its translations, then run wp plugin delete and verify that the translations were properly deleted.

[dkoston]

Thanks for your PR, it's much appreciated!

Excuse my lack of tests and general ignorance of how to code on the WP CLI codebase. This is my first PR and I'm not seeing any testing like I would normally do for production projects:

WP-CLI uses Behat as its functional test suite. Functional tests are different than unit tests in that they execute the entire WP-CLI command, and ensure they always work as expected. Every repository has a features/ directory with one or more YAML-formatted *.feature files. Here, plugin-delete.feature and plugin-uninstall.feature are relevant.

See https://make.wordpress.org/cli/handbook/contributions/pull-requests/#running-and-writing-tests and https://www.youtube.com/playlist?list=PL_B8Y6K6MH2d6T7pYa6dloUgk67mfBT4K

One example test would be to install a plugin, install its translations, then run wp plugin delete and verify that the translations were properly deleted.

Thanks @swissspidy

Sorry, I think I was unintentionally using language around testing framework / style vs discussing what is actually tested. BDD is a totally fine approach.

When I look at: https://github.com/wp-cli/extension-command/blob/main/features/plugin-uninstall.feature

The only thing that checks something other than STDERR/STDOUT and the exit code is this test:

https://github.com/wp-cli/extension-command/blob/main/features/plugin-uninstall.feature#L94-L114

There's no actual validation that any plugins have been installed, removed, what logic branches have been exercised, etc.

Its definitely fine to take a functional style and agreed that one should focus on testing the interfaces that people interact with but what I was missing is a way to understand how to exercise the various logic branches in the changes I've made and to validate that things are "actually" happening (other than the files being present or not) because IMO, the presence of text on STDOUT/STDERR doesn't indicate anything other than a logic branch with the WPCLI::log() function was executed.

Not trying to be contrarian, just trying to make sure that I can be confident in the changes that I'm bringing to the project and help future contributors from not having to manually run WP CLI with all permutations to validate success.

I'll dig more into Behat to see if there are ways to validate the underlying logic branches and confirm that they are being exercised.

[dkoston]

I didn't see any tooling to run tests locally. Really sorry about using all these compute cycles on GitHub Actions to get these tests passing. Please let me know if there's tooling for local test runs so I can avoid wasting all this compute. Thanks!

[dkoston]

Seeing failed tests outside the scope of the PR:

https://github.com/wp-cli/extension-command/blob/main/features/plugin-list-wporg-status.feature#L29

001 Scenario: Install plugins and check the status on wp.org. # features/plugin-list-wporg-status.feature:4
      Then STDOUT should be a table containing rows:          # features/plugin-list-wporg-status.feature:30
        name	wporg_last_updated
        akismet	2024-12-09
        hello	
        no-longer-in-directory	2017-11-13
        never-wporg	
        wordpress-importer	
        no-mail	
        polyfills	
         (Exception)

This seems odd as the test above has a different set of outputs for wp plugin list:

    When I run `wp plugin list --fields=name,wporg_status`
    Then STDOUT should be a table containing rows:
      | name                   | wporg_status    |
      | wordpress-importer     | active          |
      | no-longer-in-directory | closed          |
      | never-wporg            |                 |

Not sure how to proceed

[dkoston]

Looks like Behat is 3.7.0 which doesn't contain diff tooling from the latest versions.

Script run-behat-tests handling the behat event returned with error code 1
> rerun-behat-tests
..............F---

--- Failed steps:

001 Scenario: Missing required inputs  # features/plugin-uninstall.feature:40
      When I run `wp plugin uninstall` # features/plugin-uninstall.feature:41
        $ wp plugin uninstall
        
        Error: Please specify one or more plugins, or use --all.
        cwd: /tmp/wp-cli-test-run--6758746b99ccc2.38442757/
        run time: 0.3444[73](https://github.com/wp-cli/extension-command/actions/runs/12260869416/job/34206558224?pr=438#step:11:74)83880615
        exit status: 1 (RuntimeException)

^ Is this a whitespace issue? Is there a technique to understand how this doesn't match the test?

These seem the same: https://github.com/wp-cli/extension-command/pull/438/files#diff-9817a156be3e1731243e8c1aafb2f74ce2d0b4e10a731cb4d4056273e9f7d550R44

[swissspidy]

There's no actual validation that any plugins have been installed, removed, what logic branches have been exercised, etc.
to validate that things are "actually" happening

You can also use other commands to verify that a plugin is no longer present, e.g. by verifying whether wp plugin list includes the plugin, or if the directory no longer exists.

I didn't see any tooling to run tests locally. Really sorry about using all these compute cycles on GitHub Actions to get these tests passing. Please let me know if there's tooling for local test runs so I can avoid wasting all this compute. Thanks!

Please check the https://make.wordpress.org/cli/handbook/contributions/pull-requests/#running-and-writing-tests link I shared before.

Basically:

Run composer prepare-tests once to set up the MySQL database, and then composer behat to run the tests. You can also run only a specific test, see https://make.wordpress.org/cli/handbook/contributions/pull-requests/#running-the-test-suite

Alternatively: use WP_CLI_TEST_DBTYPE=sqlite composer behat to run tests with SQLite. Then you don't need the prepare-tests step.

Seeing failed tests outside the scope of the PR:

Yes, looks like a newly failing test even on trunk, see https://github.com/wp-cli/extension-command/actions/runs/12247875028/job/34166552233#step:11:91

Please ignore it.

Looks like Behat is 3.7.0 which doesn't contain diff tooling from the latest versions.

They have new diff features? Do you have a link? Because in wp-cli/wp-cli-tests#17 many years ago we were looking for that. Would be nice to finally fix this.

^ Is this a whitespace issue? Is there a technique to understand how this doesn't match the test?

When you are expecting an issue, use try instead of run

Scenario: Missing required inputs
-  When I run `wp plugin uninstall`
+  When I try `wp plugin uninstall`

[dkoston]

There's no actual validation that any plugins have been installed, removed, what logic branches have been exercised, etc.
to validate that things are "actually" happening

You can also use other commands to verify that a plugin is no longer present, e.g. by verifying whether wp plugin list includes the plugin, or if the directory no longer exists.

That makes sense. I'll add some additional testing to validate these scenarios using other wp cli commands

I didn't see any tooling to run tests locally. Really sorry about using all these compute cycles on GitHub Actions to get these tests passing. Please let me know if there's tooling for local test runs so I can avoid wasting all this compute. Thanks!

Please check the https://make.wordpress.org/cli/handbook/contributions/pull-requests/#running-and-writing-tests link I shared before.

Basically:

Run composer prepare-tests once to set up the MySQL database, and then composer behat to run the tests. You can also run only a specific test, see https://make.wordpress.org/cli/handbook/contributions/pull-requests/#running-the-test-suite

Alternatively: use WP_CLI_TEST_DBTYPE=sqlite composer behat to run tests with SQLite. Then you don't need the prepare-tests step.

Ah thanks, sorry, I stopped reading at the Behat section. Apologies

Seeing failed tests outside the scope of the PR:

Yes, looks like a newly failing test even on trunk, see https://github.com/wp-cli/extension-command/actions/runs/12247875028/job/34166552233#step:11:91

Please ignore it.

10-4.

Looks like Behat is 3.7.0 which doesn't contain diff tooling from the latest versions.

They have new diff features? Do you have a link? Because in wp-cli/wp-cli-tests#17 many years ago we were looking for that. Would be nice to finally fix this.

Yea, it was added in Behat/Behat#1532

It looks like there's a PR to introduce newer Behat that's blocked due to php version support: wp-cli/wp-cli-tests#228

^ Is this a whitespace issue? Is there a technique to understand how this doesn't match the test?

When you are expecting an issue, use try instead of run

Scenario: Missing required inputs
-  When I run `wp plugin uninstall`
+  When I try `wp plugin uninstall`

Ah, thanks. Apologies again for being a newbie to all this stuff.

[dkoston]

@swissspidy this should be good to go. Thanks for all the help.

I couldn't find a way to populate site transients so I removed that assertion:

002 Scenario: Uninstall an installed plugin should uninstall, delete files, and remove from transient update_plugins # features/plugin-uninstall.feature:6
      When I run `wp transient get --network update_plugins`                                                         # features/plugin-uninstall.feature:17
        $ wp transient get --network update_plugins

        Warning: Transient with key "update_plugins" is not set.
        cwd: /var/folders/lm/gzk1q0b95491ctlw16p4_4_m0000gn/T/wp-cli-test-run--6758a46b022fc0.22076735/
        run time: 0.23778200149536
        exit status: 0 (RuntimeException)

but otherwise added some checks around more logic branches and file presence

[swissspidy]

Yea, it was added in Behat/Behat#1532

Looks like that's not in a tagged release yet, but maybe we can manually add these changes to our tooling or so. Thanks for pointing it out!

this should be good to go. Thanks for all the help.

Amazing! Thank you for working on this!

I couldn't find a way to populate site transients

wp plugin status should do that. I'll give it a try.