-
Notifications
You must be signed in to change notification settings - Fork 16
dev: add wp_ability_args
filter [Proposal]
#74
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
base: trunk
Are you sure you want to change the base?
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 introduces the wp_ability_args
filter to the Abilities API, allowing developers to modify ability arguments before validation and instantiation. This provides a centralized mechanism for extending and customizing ability behavior.
- Adds a filter hook in the registry's register method to modify ability arguments
- Includes comprehensive documentation with usage examples
- Uses wp_ prefix to avoid naming collisions before core integration
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
includes/abilities-api/class-wp-abilities-registry.php | Adds the wp_ability_args filter in the register method before validation |
docs/5.hooks.md | Documents the new filter with detailed usage examples and parameters |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
docs/5.hooks.md
Outdated
$args['has_permissions' ] = static function ( $input = null ) use ( $args ) { | ||
$previous_check = is_callable( $args['has_permissions'] ) ? $args['has_permissions']( $input ) : true; | ||
|
||
// If we already failed, no need for stricter checks. | ||
if ( ! $previous_check || is_wp_error( $previous_check ) ) { | ||
return $previous_check; | ||
} | ||
|
||
return current_user_can( 'my_custom_ability_cap', $args['name'] ) | ||
} |
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.
This creates a circular reference issue. The closure captures $args
by reference, but then tries to access $args['has_permissions']
which would be the closure itself, causing infinite recursion. The original callback should be stored in a separate variable before reassigning.
$args['has_permissions' ] = static function ( $input = null ) use ( $args ) { | |
$previous_check = is_callable( $args['has_permissions'] ) ? $args['has_permissions']( $input ) : true; | |
// If we already failed, no need for stricter checks. | |
if ( ! $previous_check || is_wp_error( $previous_check ) ) { | |
return $previous_check; | |
} | |
return current_user_can( 'my_custom_ability_cap', $args['name'] ) | |
} | |
$previous_has_permissions = $args['has_permissions'] ?? null; | |
$args['has_permissions'] = static function ( $input = null ) use ( $args, $previous_has_permissions ) { | |
$previous_check = is_callable( $previous_has_permissions ) ? $previous_has_permissions( $input ) : true; | |
// If we already failed, no need for stricter checks. | |
if ( ! $previous_check || is_wp_error( $previous_check ) ) { | |
return $previous_check; | |
} | |
return current_user_can( 'my_custom_ability_cap', $args['name'] ); | |
}; |
Copilot uses AI. Check for mistakes.
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 dont think this is true 🤔 we're only calling inside the function, not reassigning.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #74 +/- ##
============================================
+ Coverage 83.33% 83.36% +0.03%
Complexity 96 96
============================================
Files 8 8
Lines 516 517 +1
============================================
+ Hits 430 431 +1
Misses 86 86
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:
|
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.
I left some minor feedback. Overall, I'm very much in favor of the proposed extensibility option.
* @param array<string,mixed> $args The arguments used to instantiate the ability. | ||
* @param string $name The name of the ability, with its namespace. | ||
*/ | ||
$args = apply_filters( 'wp_ability_args', $args, $name ); |
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.
Minor note, this could get moved after all checks against $name
to ensure it doesn't get processed on abilities that have an incorrect name.
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.
My intent here is intentionally the opposite, since it allows changes/recoverability of the $name
while still ensuring it passes the necessary validation for downstream checks.
IMO (pseudocode):
add_filter( 'register_ability_args',
// Or whatever prefixes/missing-namespaces are enforced by validation.
static fn ( &$args, $original_name ) => $args['name'] = sprintf( '!$#@#_%s', str_replace( '/', '_', $original_name ),
10, 2 );
should not be allowed since it means adapters/downstream cant reliably depend on the namespace/shape for their needs.
Meanwhile, I'm believe that e.g.
add_filter( 'register_ability_args',
static function( $args,) {
if ( str_starts_with( 'my_legacy_namespace/', $args['name' ) ) {
$args['name'] = str_replace( 'my_legacy_namespace/', 'my_new_namespace/', $args['name'] );
}
}
);
should still be flexible enough for any downstream/back-compat considerations without making the expected shape of the Ability API unreliable, but I'm happy to evaluate other use cases (the only one I can think of is if a user intentionally registers a namespace wrong initially, but the immediate error they face should prevent that from ever shipping and thus preempting the need for a downstream filter)
(PS: moving this to after the checks after a 6.9 merge is a nonbreaking change. moving it up to enforce name validation
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.
$args
is not used to read the name. It's a separate variable $name
which can't be modified by the hook.
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.
Indeed 🤦 too much task switching that even with the function in front of me I got our $name
handling confused with $ability_class
. Sorry for wasting your time 😬
I'll move it down to after the ->is_registered()
check. When that and the other comments are handled I'll ping you for re-review 🙇
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.
No worries, it's why additional eyes are helpful to the implementation from a different perspective 👍🏻
> [!IMPORTANT] | ||
> This filter is prefixed with `wp_` to avoid potential naming collisions. | ||
> Once merged into WordPress core, the prefix will likely be removed to preserve backward-compatibility. |
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 would go with the final name from the start to avoid the hassle. register_ability_args
should be distinct enough.
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.
register_ability_args
is better than ability_args
I had in mind too as it aligns with register_{post_type|taxonomy}
args.
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'm closely familiar with register_block_type_args
, which was likely inspired by these you listed.
); | ||
|
||
// Even if they're callbacks. | ||
$args['has_permissions' ] = static function ( $input = null ) use ( $args, $ability_name ) { |
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.
This should be permission_callback
rather than has_permissions
.
We will also need some basic unit test coverage to illustrate the expected behavior better and prevent future regressions. I plan to add some tests today to: We can coordinate how to ensure we avoid more complex merge conflicts. |
What
This PR exposes the
ability_args
wp_ability_args
filter as a way to centrally extend and change the behavior of registered abilities.Why
Part of Proposal: Provide extensibility for individual abilities #39
This is a counterproposal to Propose filters to use with the registered ability #37 :
WP_Abilities_Registry::register()
makes behavior robust againstWP_Ability
class overloads and keeps the lifecycle + validation intact. In other words: filtering abilities remains idempotent to their non-filtered versions.How
wp_ability_args
is used before the merge to core, but it should be changed toabilities_api
as part of 6.9 IMO.Once merged into core, we can just
add_filter( 'ability_args', function ( ... ) { return apply_filters_deprecated( 'wp_ability_args' );
to allow anyone who builds something between now and December time to transition.