-
Notifications
You must be signed in to change notification settings - Fork 90
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
Add commands for managing signups on multisite #489
Conversation
There was a problem hiding this comment.
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. 🤷🏽
Note:
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. 💚
Hi @danielbachhuber, to continue our conversation from wp-cli/wp-cli#1911, would you consider this PR ready? Thanks! |
There was a problem hiding this comment.
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.
@ernilambar I just merged wp-cli/wp-cli#5926 now To make the PR work now, update |
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:
Edit: just needed to run |
As just discussed on the Zoom call, it's because of wp-cli/wp-cli#5928 We'll fix this separately. |
wp-cli/wp-cli#5928 has been reverted in the meantime |
See also wp-cli/ideas#99 |
There was a problem hiding this comment.
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?
Adds signup command and subcommands
Command details: https://github.com/ernilambar/entity-command/tree/feat-signup#wp-signup
Related: wp-cli/wp-cli#1911
PR inspired from https://github.com/trepmal/wp-cli-unconfirmed-users
Highlights:
$this->fetcher->get_check()
CommandWithDBObject
which provides several helpful methods