-
Notifications
You must be signed in to change notification settings - Fork 16
Add execute actions to the ability object #56
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
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 adds extensibility hooks to the ability execution flow by introducing two action hooks that fire before and after ability execution. This allows developers to hook into the ability lifecycle for monitoring, logging, or extending functionality.
- Added
before_execute_ability
action hook that fires before ability execution - Added
executed_ability
action hook that fires after successful ability execution - Refactored the return logic to properly handle validation errors and execute the post-execution hook only on success
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #56 +/- ##
============================================
+ Coverage 83.33% 83.84% +0.51%
Complexity 96 96
============================================
Files 8 8
Lines 516 520 +4
============================================
+ Hits 430 436 +6
+ Misses 86 84 -2
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:
|
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 I know you marked as draft, but honestly this feels pretty close to mergeable.
(Unlike my filter feedback on #37 . an action should trigger every time ::do_execute()
is run vs 1x).
Only question is whether we want to pass the \WP_Error
to the filter as well to allow. Not necessary (WPError have their own built-in hooks), and doing this in the future is non-breaking, but it would allow for recoverability flows.
(e.g. if we're MCP and this is an input error, filter to try and coerce the shapes, but if its REST then you to rest_ensure_response( $error )
).
I will work on tests and documentation before marking it as fully ready for review. |
9cbf10c
to
485ac3b
Compare
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.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
LGTM, left some nonblocking doc nits. 🚀
I will wait for a reply about the docs before landing. |
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.
One small suggestion, otherwise it all looks good from a docs perspective.
Co-authored-by: Dovid Levine <[email protected]>
- Add before_execute_ability action that fires before ability execution - Add after_execute_ability action that fires after successful execution - Include comprehensive unit tests for both actions with edge cases - Add extensibility documentation with usage examples and best practices - Update README with new documentation link - Fix PHPDoc formatting for action parameters 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Co-authored-by: Dovid Levine <[email protected]>
2df0b39
to
3845992
Compare
Just to let you know, all feedback has been addressed in 3845992. I enabled auto-merge. |
Illustration for the ideas to make abilities more extensible:
This PR adds extensibility hooks to the ability execution flow by introducing two action hooks that fire before and after ability execution. This allows developers to hook into the ability lifecycle for monitoring, logging, or extending functionality.
before_execute_ability
action hook that fires before ability executionafter_execute_ability
action hook that fires after successful ability executionExample Usage
Basic Logging