Feature: adding the --ignore-group CLI option #784
Conversation
…tiple group definitions
ovidiul
changed the title
|
Sorry for the delay, here.
We are a little pressed for bandwidth right now, so can't act on the above immediately, but this looks good in general. Thank you! |
| // removing the ignored group prefix from slug if present. | ||
| $slug = preg_replace( '/(' . preg_quote( self::IGNORE_GROUP_FLAG, null ) . ')?(.*)/m', '$2', $slug ); | ||
|
|
||
| // further sanitisation here with `sanitize_title` maybe? | ||
| return $slug; |
There was a problem hiding this comment.
Perhaps we should also anchor ^the regex? Re sanitization; I wonder if esc_sql would (also) make sense?
There was a problem hiding this comment.
Added the ^ regex change as well as the esc_sql filtering
| $params[] = $group_id; | ||
| if ( count( $include_group_ids ) ) { | ||
| $where .= ' AND group_id IN (%s)'; | ||
| $params[] = implode( ',', $include_group_ids ); |
There was a problem hiding this comment.
Asking this without actually testing 😅 ... but, won't the group ID arrays at this point be comma separated strings without quotation? That is, we will get:
( foo, bar, baz )
Instead of:
( 'foo', 'bar', 'baz' )
If so, we should add quoting at some suitable point.
There was a problem hiding this comment.
The $include_group_ids will actually contain the numerical ID's of each group found in the DB, taken from above line
$group_id = $this->get_group_id( $sanitized_group_name, false );
so I don't think we need to add any quotes here...
Co-authored-by: Barry Hughes <[email protected]>
Co-authored-by: Barry Hughes <[email protected]>
Co-authored-by: Barry Hughes <[email protected]>
Co-authored-by: Barry Hughes <[email protected]>
Co-authored-by: Barry Hughes <[email protected]>
|
Thanks for the suggestions @barryhughes , I've addressed the mentioned issues. 👍 |
| @@ -29,7 +29,8 @@ These are the commands available to use with Action Scheduler: | |||
| * `--batch-size` - This is the number of actions to run in a single batch. The default is `100`. | |||
| * `--batches` - This is the number of batches to run. Using 0 means that batches will continue running until there are no more actions to run. | |||
| * `--hooks` - Process only actions with specific hook or hooks, like `'woocommerce_scheduled_subscription_payment'`. By default, actions with any hook will be processed. Define multiple hooks as a comma separated string (without spaces), e.g. `--hooks=woocommerce_scheduled_subscription_trial_end,woocommerce_scheduled_subscription_payment,woocommerce_scheduled_subscription_expiration` | |||
| * `--group` - Process only actions in a specific group, like `'woocommerce-memberships'`. By default, actions in any group (or no group) will be processed. | |||
| * `--group` - Process only actions in a specific group, like `'woocommerce-memberships'`. By default, actions in any group (or no group) will be processed. Accepts comma-separated list of groups to filter by. Adding --*IGNORE*-- in front of a group will cause it to be ignored. | |||
| * `--ignore-group` - Ignore processing actions from groups found inside the comma separated list of values. | |||
|
Thanks for all the work here, and for pioneering a way forward! Closing, since most of the proposed functionality has been introduced via #905 and #918. I want to also note that one thing those PRs do not add support for is processing multiple groups in a single queue runner, which there may still be value in. |
This PR fixes #736.
As suggested in the issue, a
--ignore-groupoption is the desired approach to name this.The PR also adds support for multiple comma-separated groups, so
--ignore-group=group1,group2could be used as well. This implementation would also allow for the--groupoption to take comma-separated values as--group=group1,group2.Why this approach?
Looking at the
$runner->setup( $batch, $hooks, $group, $force, $ignore_group );alternative, where we would have to pass the$ignore_groupadditional parameter, the$groupand$ignore_groupare very similar, so maybe using a flag to specify which group to ignore as proposed in this PR might make the code a little bit simpler?Tests - TBD
Happy to add any related tests if this approach is considered valid.