Skip to content
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

WP-CLI #1071

Open
wants to merge 72 commits into
base: trunk
Choose a base branch
from
Open

WP-CLI #1071

wants to merge 72 commits into from

Conversation

crstauf
Copy link
Contributor

@crstauf crstauf commented Jun 19, 2024

Port of crstauf/action-scheduler-cli into Action Scheduler.

Closes #255.

Hope I'm not disappointed.

@barryhughes barryhughes requested review from a team and vedanshujain and removed request for a team June 24, 2024 21:00
Copy link
Contributor

@vedanshujain vedanshujain left a comment

Choose a reason for hiding this comment

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

Great work! I posted some initial thoughts as I work my way through the PR, let me know what you think.

classes/WP_CLI/Action/Create.php Outdated Show resolved Hide resolved
classes/WP_CLI/Action/Create.php Outdated Show resolved Hide resolved
try {
call_user_func( $function_name, $hook, $callback_args, $group );
} catch ( \Exception $e ) {
$this->print_error( $e, $multiple );
Copy link
Contributor

Choose a reason for hiding this comment

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

note that it won't throw an exception necessarily even if cancelling failed, there could be an error caught by the try_catch in as_unschedule_action. It would be more reliable to either

  1. check the logs for failed action, or
  2. verify that the action is indeed cancelled, or
  3. modify the as_unschedule_action to return the result value so that we can display it reliably.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to rework this command a bit: realized in its current state actions could not be unscheduled by group only.

classes/WP_CLI/Action/Delete.php Outdated Show resolved Hide resolved
classes/WP_CLI/Action/Delete.php Outdated Show resolved Hide resolved
classes/WP_CLI/Action/Get.php Outdated Show resolved Hide resolved
classes/WP_CLI/Action/List.php Outdated Show resolved Hide resolved
@crstauf crstauf requested a review from vedanshujain July 3, 2024 23:53
@vedanshujain
Copy link
Contributor

vedanshujain commented Jul 5, 2024

The code looks good, going to test around these flows in general:

  • Create - async/single/cron/recurring/error
  • Generate - testing for bulk count
  • Get - error in action.
  • Run
  • List - listing in bulk
  • Cancel - check for error flows too
  • Delete - check for deleting in bulk + error flows

Copy link
Contributor

@vedanshujain vedanshujain left a comment

Choose a reason for hiding this comment

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

Posting feedback for some initial testing I was able to get around to today. Seems like we need to sprinkle in some more defensive checks

classes/WP_CLI/Action/Delete.php Outdated Show resolved Hide resolved
classes/WP_CLI/Action/Get.php Outdated Show resolved Hide resolved
classes/WP_CLI/Action/List.php Outdated Show resolved Hide resolved
classes/WP_CLI/Action/Create.php Outdated Show resolved Hide resolved
classes/WP_CLI/Action/Create.php Outdated Show resolved Hide resolved
classes/WP_CLI/Action/Generate.php Outdated Show resolved Hide resolved
@crstauf crstauf requested a review from vedanshujain July 6, 2024 01:37
@crstauf
Copy link
Contributor Author

crstauf commented Sep 14, 2024

@barryhughes Stalled again. 😞

@barryhughes
Copy link
Member

Yep, I'm sorry. We've been focusing on other tasks and responsibilities that have taken priority. This is looking great, though, so bear with us a little longer. Probably, if any further changes are needed, they'll be minor/small pieces of clean-up, and so we can likely take care of those if required (and can similarly update the branch if it falls out of sync in the meantime).

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.

WP CLI: Schedule new action(s)
3 participants