Add commands for managing signups on multisite by ernilambar · Pull Request #489

Add commands for managing signups on multisite by ernilambar · Pull Request #489 · wp-cli/entity-command

Choose a reason for hiding this comment

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

Thank you very much for working on this! I left some nit-picky comments that try aligning wording as close to existing commands as possible.
If you agree with my comments in README.md, please also apply them to src/Signup_Command.php. Thank you!
Also, I would have expected that there would be WordPress APIs (PHP functions) for fetching and deleting signups, but I couldn't find anything. 🤷🏽

README.md Outdated Show resolved Hide resolved

Note:

Fetcher should be probably moved to https://github.com/wp-cli/wp-cli/tree/main/php/WP_CLI/Fetchers

Choose a reason for hiding this comment

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

Thanks for incorporating all the changes. 🙏🏽
Would it be possible to add tests for failed activating and deleting signups to ensure the presence of the "Failed (activating|deleting) signup 123" message?

Fetcher should be probably moved to https://github.com/wp-cli/wp-cli/tree/main/php/WP_CLI/Fetchers

Does it make sense to create a separate PR there, have it merged, and then continue here?

README.md Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

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

I left another round of nit-picky wording-related comments. However, these don't stop me from approving.
Thank you for working on this! It's a small feature, but we have desperately wanted it at times. 💚

Choose a reason for hiding this comment

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

Great start! I left some comments.

I don't understand the test failures, they look unrelated 🤔

I don't understand the test failures, they look unrelated 🤔

Yah, there are no changes related to taxonomies here.

Ah, when I try locally I get this:

$ WP_VERSION=trunk composer behat features/taxonomy.feature:7
> run-behat-tests 'features/taxonomy.feature:7'
-----

--- Failed hooks:

    BeforeSuite # WP_CLI\Tests\Context\FeatureContext::prepare()
      $ wp cli info
      
      Error: Callable "Signup_Command" does not exist, and cannot be registered as `wp user signup`.
      cwd: 
      run time: 0.16830706596375
      exit status: 1 (RuntimeException)

Edit: just needed to run composer update

As just discussed on the Zoom call, it's because of wp-cli/wp-cli#5928

We'll fix this separately.

Choose a reason for hiding this comment

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

@ernilambar Can you:

Credit the original package in the PR description?
Highlight any differences between this implementation and the original package?

danielbachhuber changed the title ~~Add signup command class~~ Add commands for managing signups on multisite

Apr 27, 2024