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

The method signature for ActionScheduler_DBStore::bulk_cancel_actions() is different than its parent class #998

Open
coreymckrill opened this issue Sep 29, 2023 · 3 comments
Labels
priority: normal The issue/PR is normal priority—not many people are affected or there’s a workaround, etc. type: enhancement The issue is a request for an enhancement.

Comments

@coreymckrill
Copy link
Contributor

In ActionScheduler_DBStore::bulk_cancel_actions() the method takes one argument, $query_args, described like this in the doc block:

@param array $query_args Query parameters.

In the parent class that ActionScheduler_DBStore extends, ActionScheduler_Store, the same method also takes one argument, $action_ids, described as:

@param array $action_ids List of action IDs.

Now, since in both cases there is one argument and it is expected to be an array, I'm not sure if there are compatibility issues with newer versions of PHP (8.1+). However, this still seems problematic because the contents of the arrays are very different. In one we have an associative array of key/value pairs that define the arguments for a query. In the other we have an indexed array of integers (action IDs) that are the result of already having run that query. At the very least this is confusing to anyone who is trying to understand what ActionScheduler_DBStore inherits from its parent class, and how they should extend that parent class if they were to create another custom store.

@barryhughes barryhughes added type: enhancement The issue is a request for an enhancement. priority: normal The issue/PR is normal priority—not many people are affected or there’s a workaround, etc. labels Oct 3, 2023
@barryhughes
Copy link
Member

Renaming the param in the base method (which is private) would seem like the safest solution, if we want to limit the potential for future confusion.

@coreymckrill
Copy link
Contributor Author

Probably need to rename, and refactor to make use of query params instead of a list of IDs. But agreed that changing the private method is probably the best option.

@barryhughes
Copy link
Member

Right, I mean since it is private and not inherited it could simply change from:

ActionScheduler_Store::bulk_cancel_actions( $action_ids )

To:

ActionScheduler_Store::bulk_cancel_actions_by_id( $action_ids )

The logic could remain the same. Though, of course, there are several ways to solve it and getting into the weeds as I'm doing probably isn't useful 🍃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: normal The issue/PR is normal priority—not many people are affected or there’s a workaround, etc. type: enhancement The issue is a request for an enhancement.
Projects
None yet
Development

No branches or pull requests

2 participants