dev!: handle property registration inside WP_Ability #54
Conversation
There was a problem hiding this comment.
Pull Request Overview
Refactors the WP_Ability class to handle its own property validation, moving validation logic from WP_Abilities_Registry into the ability class itself. This improves separation of concerns and simplifies the developer experience when extending the WP_Ability class.
- Moved property validation from
WP_Abilities_Registry::register()toWP_Ability::validate_properties() - Added exception handling in the registry to catch validation errors and convert them to
_doing_it_wrong()calls - Added test coverage for direct instantiation of
WP_Abilitywith invalid properties
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| includes/abilities-api/class-wp-ability.php | Added validate_properties() method and validation call in constructor |
| includes/abilities-api/class-wp-abilities-registry.php | Removed validation logic and added try-catch block for ability instantiation |
| tests/unit/abilities-api/wpAbilitiesRegistry.php | Added test for exception throwing when WP_Ability is instantiated with invalid properties |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #54 +/- ##
============================================
- Coverage 88.43% 84.61% -3.83%
- Complexity 94 96 +2
============================================
Files 8 8
Lines 519 507 -12
============================================
- Hits 459 429 -30
- Misses 60 78 +18
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:
|
| * ...<string, mixed>, | ||
| * } $properties | ||
| */ | ||
| public function __construct( string $name, array $properties ) { |
There was a problem hiding this comment.
A small part of me thinks that we should change all the references of properties to args to bring it inline with other WP core naming, now that we've broken the direct dependency to add a validator. 🤷
I could add it in this PR but i didnt want to obfuscate the discussion on #53
There was a problem hiding this comment.
We have now ability_class which isn't strictly a property of the object, so I don't mind changing to args at this point. If you want to take care of it, let's put it into a separate PR to keep the current refactor lean.
There was a problem hiding this comment.
+1 to changing this to use $args.
| $this->validate_properties( $properties ); | ||
|
|
||
| foreach ( $properties as $property_name => $property_value ) { |
There was a problem hiding this comment.
An alternative approach is to replace the validate_*() pattern with the prepare_*(): array|WP_Error() , and then throw in the constructor.
This would improve the DX both mapping and validation in the same step, but I'm not sure how flexible we want things to be at this initial stage vs once we've given the initial API + round of feedback time to percolate, so I went with the approach in the diff.
e.g. (pseudocode)
$properties = $this->prepare_args( $args );
if ( is_wp_error( $properties ) ) {
throw \InvalidArgumentException( $properties->getMessage() );
}
// This is still outside the function so extenders don't need to reimplement it.
foreach( $properties as $name => $value ) {
...
}There was a problem hiding this comment.
@gziolo / @felixarntz would love some specific thoughts on this if you have them. The more I think about it the more I feel like protected function prepare_properties( array<string,mixed> ): array<validated-shape>|WP_Error is the better approach 🤔
There was a problem hiding this comment.
Ok, so you prefer to have prepare_properties that returns WP_Error as soon as something goes wrong, and it gets translated to an exception, or the properties returned gets passed to the loop. That sounds good to me, to avoid having a method that only throws an exception when someting goes wrong. In fact, it might be simpler to ever introduce a filter if folks want to change this properties as an alternative to what I proposed here:
The filter used with block types register_block_type_args comes to my find as a good reference:
There was a problem hiding this comment.
My thoughts exactly. After the chat, I'll push a change with that approach.
There was a problem hiding this comment.
I'm not strongly opposed, but I also don't see the benefit of using prepare_properties and returning a WP_Error from it if something is wrong. Can you clarify why that's better?
To me it seems unnecessarily complex to rely on a method that can return WP_Error, only to turn that into an InvalidArgumentException, only to catch that and turn it into a _doing_it_wrong().
Why not simply throw the exceptions?
There was a problem hiding this comment.
Clarifying that my focus is on returning the array of (prepared) args instead of void. 😅
@@felixarntz I agree with you from a code quality POV
From an implementer POV my thought was less "exceptions should be exceptional" and more "let's do it the WP™️ way 💪" :
- most the other parts of the exposed API return WP_Errors to be handled instead of throwing.
- In general WordPress developers are more comfortable returning error objects than throwing.
- most importantly: it gives me a justification to leave the return type off the method signature (since php7.4 doesn't support union return types) since I didn't want to argue about why a legacy project wanting to migrate to this API but keep their existing DTO object isn't a justifiable use case for polymorphic tech debt at this early stage. 😅
(I drafted this before you both aligned yesterday on #53 and only saw after I pushed, so in my head this was still very much a proposal ).
My takeaway from this conversation is that we're good not caring about that last point (between overloading and the filter if someone really, realy thinks shimming a DTO or VO into this api is a good idea, they have ways), so if y'all don't think it's outweighed by the first two (I don't), I'll use prepare_*( array<string,mixed> ): array<valid-shape> and just throw
| * ...<string, mixed>, | ||
| * } $properties | ||
| */ | ||
| protected function validate_properties( array $properties ) { |
There was a problem hiding this comment.
The only changes here are that they're now InvalidArgumentExceptions() instead of _doing_it_wrong(). The conditionals and error message are identical to what was in WP_Abilities_API::register()
There was a problem hiding this comment.
I double-checked with the implementation @felixarntz used in the demo plugin and it seems to be compatible even with the strict checks for madatory properties: label, description and execute callback.
|
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.
I have just realized that $name is passed as a first argument so it won't be possible to make it optional with the intent that implementer extending WP_Abilities defines it similar to label, description, and the execute callback. That's perfectly fine. With the changes included, the code from the demo plugin will be simplified nicely:
wp_register_ability(
'wp-ai-sdk-chatbot-demo/get-post',
array(
- 'label' => __( 'Get Post', 'wp-ai-sdk-chatbot-demo' ),
- 'description' => 'Mock description.',
- 'execute_callback' => $mock_execute_callback,
'ability_class' => Get_Post_Ability::class,
)
);
|
Let's see what feedback @felixarntz has to share before landing these changes. |
There was a problem hiding this comment.
@justlevine This looks great, thank you! I left a few comments on the open discussion points (I'm slightly favoring simply throwing exceptions and keeping the code as it is now), but I'll preemptively approve, since either way it's not a blocker.
|
Once this is merged and available in a release, I'll update the demo plugin :) |
|
As I start my day, I'll draft the refactoring described in #54 (comment). I landed this PR so all the essential changes are included in the next package release as soon as possible. |
|
@gziolo feel free to lift https://github.com/justlevine/abilities-api/tree/dev/wp_ability-prepare_args I didnt have time to review it before passing out last night, but might save you a few minutes |
|
Oh, nice. I see you started the process of renaming to |
I continue based on the commit from your branch in #59. |
What
Refactors
WP_Abilityto validate its own properties.How
WP_Ability::validate_properties()throws an exception, which is caught and translated into a_doing_it_wrong()byWP_Abilities_API::register()which was previously handled registration and validation.More specific implementation notes are in the diff.
Why
ability_classduring registration #53This approach improves separation of concerns and SRP between
WP_AbilityandWP_Ability_Registryin a way that also simplifies the DX for users needing to extend theWP_Abilityclass. Instead of needing to shim instantiation to comply or circumventWP_Ability_Registry, developers can overloadWP_Ability::validate_properties()colocated with the other changes that justify extending the class, or even choose to bypass it entirely in their overloaded constructor.Beyond the Scope
propertiestoargsnow that there's no semantic implication from the distinction._doing_it_wrong()over other flavors of WordPress error handling