docs: fix wp_register_ability() args not marked as required. #97
Conversation
|
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.
Pull Request Overview
This PR fixes PHPStan type annotations for the wp_register_ability() function by marking the label, description, execute_callback, and permission_callback parameters as required instead of optional in the $args array.
- Updates PHPStan parameter type annotations to reflect required fields
- Improves static analysis accuracy for developers using PHPStan
- Maintains existing runtime validation while providing better IDE/tooling support
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 #97 +/- ##
=========================================
Coverage 85.69% 85.69%
Complexity 103 103
=========================================
Files 16 16
Lines 776 776
Branches 86 86
=========================================
Hits 665 665
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:
|
Overall, I'm in favor of making these args required with a small remark. There are possible paths where the developer might omit these args, for example when using wp_register_ability(
'wp-ai-sdk-chatbot-demo/get-post',
array(
'label' => __( 'Get Post', 'wp-ai-sdk-chatbot-demo' ),
'ability_class' => Get_Post_Ability::class,
)
);An important note, the developer still must provide these properties, which is satisfied with this code: Another, edge case scenario that I can think of would be with the new WP filter Could we use a union like the one below, or would it make it too complicated? /**
* @phpstan-param
* (array{
* label: string,
* description: string,
* execute_callback: callable(mixed $input=): (mixed|\WP_Error),
* permission_callback: callable(mixed $input=): (bool|\WP_Error),
* input_schema?: array<string,mixed>,
* output_schema?: array<string,mixed>,
* meta?: array<string,mixed>,
* ability_class?: class-string<\WP_Ability>,
* ...<string, mixed>
* }
* |
* array{
* ability_class: class-string<\WP_Ability>,
* label?: string,
* description?: string,
* execute_callback?: callable(mixed $input=): (mixed|\WP_Error),
* permission_callback?: callable(mixed $input=): (bool|\WP_Error),
* input_schema?: array<string,mixed>,
* output_schema?: array<string,mixed>,
* meta?: array<string,mixed>,
* ...<string, mixed>
* })
* $args
*/Perhaps we could also include a unit test that illustrates the usage observed in the WC US demo plugin, providing internal PHPStan verification. |
gziolo
added
the
[Type] Developer Documentation
|
+1 to what @gziolo suggested above: Neither the current nor the suggested shape in this PR are ideal IMO, because these arguments are only required if Something like this union approach would address the actual requirements properly. |
@felixarntz nit: they are only not required if The fact that IMO as I've said elsewhere, removing the predictability of required args is a huge footgun in general, but way worse to introduce into an initial release in v6.9 where it becomes permanent tech debt In the interim a downstream dev (using PHPStan level 8) really thinks there's a reason for their polymorphism, then I imo a one-line |
Fair point.
Where was that decided? I disagree with that assessment. Extending |
Agreed that extending |
There was a problem hiding this comment.
In the interim a downstream dev (using PHPStan level 8) really thinks there's a reason for their polymorphism, then I imo a one-line // @phpstan-ignore argument.type is an encouraging bit of friction to have.
I tend to agree we can optimize for tha majority of devs using the default way to register abilities 👍🏻
@felixarntz @gziolo not the ability to extend the class altogether, the ability to make those required args optional when extending (unlike Woo in that last class which a. respects the shape and b. doesn't need a class even Iirc the discussion started on #21 and moved to #53 + #54. The broader argument against polymorphism also came up in #61 and some other tangental PRs. |
|
Right, I know we talked about it in several places, but I don't recall any decision that this was discouraged / not recommended or anything along these lines. If you implement your own When no |
There was a problem hiding this comment.
Based on the conversation so far, I continue to question this approach: What is the concrete argument against going with @gziolo's suggestion from #97 (comment) instead?
Correct. No "decision" (if I'm now understanding your emphasis) has been made about actively promoting polymorphism in the initial core merge of this API. To make progress without such a decision we've been mostly avoiding polymorphism whenever it would be a breaking change to revert (because going from a single type to a union is a nonbreaking). So when I say that #21 isn't an endorsement on polymorphism, I mean it was explicitly merged without a decision to endorse polymorphism having been made.
WordPress documentation has always been prescriptive, not descriptive. It's why most types in the codebase aren't unioned to
The correlary here is that for <5% usage we're introducing a problematic footgun into an API we're navelgazing into existence isolated from core review, and where forward incompatible changes remain as permanent tech debt. We can always explicitly document the polymorphism in the future if/when we decide that it's something we want holistically and have it be nonbreaking.
If you mean "Overall, I'm in favor of making these args required with a small remark.", then yes that's the approach I think we should take ;) If you mean "Could we use a union like the one below, or would it make it too complicated?" then a quick tl;Dr would be:
@felixarntz what do you believe are the reasons:
|
What
This PR fixes the
@phpstan-paramtype annotations for the requiredlabel,description,execute_callback, andpermission_callback$argsonwp_register_ability()This PR was authored by @johnbillion on justlevine#6, but I didn't see it before the upstream PR got merged. All I did was cherrypick it onto truck.
Why
Per John