-
Notifications
You must be signed in to change notification settings - Fork 31
Introduce invoke_callback helper method in WP_Ability class
#88
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #88 +/- ##
============================================
+ Coverage 86.24% 86.26% +0.01%
Complexity 110 110
============================================
Files 16 16
Lines 807 808 +1
Branches 86 86
============================================
+ Hits 696 697 +1
Misses 111 111
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e0d09cb to
53233b4
Compare
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.
Pull Request Overview
This PR introduces a new invoke_callback helper method in the WP_Ability class to standardize callback invocation logic. The method handles whether to pass input parameters based on the presence of an input schema, replacing duplicated logic in the permission and execution callback methods.
- Adds a new protected
invoke_callbackhelper method to centralize callback invocation logic - Refactors
check_permissionsanddo_executemethods to use the new helper - Adds comprehensive test coverage for different callback types (functions, closures, static methods, instance methods)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| includes/abilities-api/class-wp-ability.php | Introduces invoke_callback helper method and refactors existing methods to use it |
| tests/unit/abilities-api/wpAbility.php | Adds test methods and data provider to verify callback handling with different callable types |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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.
Nice little refactor! I made a minor suggestion, but it's small enough where I'll approve regardless of your decision.
| */ | ||
| protected function invoke_callback( callable $callback, $input = null ) { | ||
| $args = array(); | ||
| if ( ! empty( $this->get_input_schema() ) ) { |
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.
While we're cleaning things up, what do you think about adding a $this->has_input_schema() method to make this sort of check more declarative?
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.
Sounds like a good idea 👍
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 will play with this, and the other method idea #104 (comment) in a follow-up PR 😄
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.
Would it make sense to have a similar method for the output schema? Given all the other tasks necessary for WP 6.9, this one is nice to have we can introduce later.
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.
@gziolo Good idea, LGTM!
This PR introduces a new
invoke_callbackhelper method in theWP_Abilityclass to standardize callback invocation logic. The method handles whether to pass input parameters based on the presence of an input schema, replacing duplicated logic in the permission and execution callback methods. Additional test coverage was added to ensure that functionality continues to work the same way.You can run tests locally with: