-
Notifications
You must be signed in to change notification settings - Fork 47
Feature: add categories system #102
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
Feature: add categories system #102
Conversation
* Add functions to register, unregister, and retrieve ability categories. * Introduce WP_Ability_Category and WP_Abilities_Category_Registry classes for managing categories. * Update WP_Ability class to support categories and modify the abilities retrieval process to filter by category. * Enhance REST API to allow filtering abilities by category and include category information in responses. * Bump version to 0.3.0 to reflect new features.
* Update the `register` method in `WP_Abilities_Category_Registry` to check for existing slugs before validating format. * Modify the `WP_Abilities_Registry` class to return abilities as an associative array keyed by ability name. * Enhance the `WP_Ability_Category` constructor to throw an exception for empty slugs and streamline property assignment.
…e required string
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #102 +/- ##
============================================
+ Coverage 86.26% 86.48% +0.21%
- Complexity 110 148 +38
============================================
Files 16 18 +2
Lines 808 969 +161
Branches 86 85 -1
============================================
+ Hits 697 838 +141
- Misses 111 131 +20
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:
|
Move the action hook validation for ability category registration from the public API function into the registry class itself. This change centralizes the validation logic, ensuring it's consistently applied. The validation is also made more specific, now only permitting registration during the `abilities_api_category_registry_init` action to enforce a stricter and more predictable initialization order.
Update unit tests to register ability categories using the `abilities_api_category_registry_init` action hook. Previously, tests registered categories after this hook had already fired, which does not reflect the intended API usage. This change ensures that the test setup accurately simulates how categories should be registered, making the test suite more robust and reliable. A helper method has also been introduced in the `wpAbilityCategory` test class to streamline this process and reduce code duplication.
The `abilities_api_category_registry_init` action hook is renamed to the more concise and intuitive `abilities_api_categories_init`. This change improves developer experience by making the hook's purpose clearer and aligning it more closely with standard WordPress naming conventions. All related code, documentation, and tests have been updated to use the new hook name.
gziolo
left a comment
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 a few minor notes for the production logic. This looks solid and I'm planning to approve the PR as soon as I review unit tests. Some of my feedback is perfectly suited as follow-up work as it focuses on code quality which might be even easier to review and discuss seperately. @galatanovidiu, can you collect the discussed code quality improvements so we have a good overview of what's left when making the final call? Again, I didn't disovered any blockers so far.
includes/abilities-api/class-wp-abilities-category-registry.php
Outdated
Show resolved
Hide resolved
includes/abilities-api/class-wp-abilities-category-registry.php
Outdated
Show resolved
Hide resolved
| // Validate category exists if provided (will be validated as required in WP_Ability). | ||
| if ( isset( $args['category'] ) ) { | ||
| $category_registry = WP_Abilities_Category_Registry::get_instance(); | ||
| if ( ! $category_registry->is_registered( $args['category'] ) ) { | ||
| _doing_it_wrong( | ||
| __METHOD__, | ||
| sprintf( | ||
| /* translators: %1$s: category slug, %2$s: ability name */ | ||
| esc_html__( 'Category "%1$s" is not registered. Please register the category before assigning it to ability "%2$s".' ), | ||
| esc_attr( $args['category'] ), | ||
| esc_attr( $name ) | ||
| ), | ||
| 'n.e.x.t' | ||
| ); | ||
| return null; | ||
| } | ||
| } | ||
|
|
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.
So it moves most of the validation to the category registry, as the existence of the category name there is the strongest indicator that it has already been validated. No need for additional checks here. There is strong coupling with the category registry, which is fine to have at both levels. @galatanovidiu explained that the basic check for existence still happens inside prepare_properties(), so all checks necessary are covered. I'm fine keeping it here.
| @@ -0,0 +1,80 @@ | |||
| # 7. Registering Categories | |||
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.
@jonathanbossenger, we really need to remove these numbers, as I would put this one next to registering categories as it fits better there. It's another instance where this ordering causes trouble 😅
Let's also make sure to list this new document in README somwhere next to:
Line 23 in 4dc57b3
| - [Registering Abilities](docs/3.registering-abilities.md) |
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.
Moved the ordering aspect to a new issue here.
Co-authored-by: Greg Ziółkowski <[email protected]>
…r clearer context during translation. Co-authored-by: Greg Ziółkowski <[email protected]>
…r clearer context during translation. Co-authored-by: Greg Ziółkowski <[email protected]>
…r clearer context during translation. Co-authored-by: Greg Ziółkowski <[email protected]>
This parts is coverded in Hooks documentation
Corrects the PHPStan type annotations for wp_register_ability_category() and WP_Abilities_Category_Registry::register() to accurately reflect the actual implementation: - Mark `label` and `description` as required fields (removed optional `?`) - Add `meta` as an optional property (array<string,mixed>) - Update docblock to mention `meta` parameter The label and description fields are validated as required in the WP_Ability_Category::prepare_properties() method, while meta is truly optional. The annotations now match the runtime behavior.
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 a couple of additional nitpicks regarding the implementation of tests to consider before landing this PR.
Overall, this is looking excellent functionality-wise from my perspective. Let's make sure that other folks who left feedback are happy with the current shape and plan for merging. I would like to start the process of syncing the abilities registry and REST API layer to WordPress core, and this is the last missing piece of the puzzle I expected 🎉
| $this->assertInstanceOf( WP_Ability::class, $result ); | ||
| $this->assertSame( 'test-math', $result->get_category() ); | ||
|
|
||
| // Cleanup. | ||
| wp_unregister_ability( 'test/calculator' ); |
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.
Nit, I would do the cleanup before assertions to ensure it always happens in case assertions fail for some reason during development.
| $this->assertInstanceOf( WP_Ability::class, $result ); | |
| $this->assertSame( 'test-math', $result->get_category() ); | |
| // Cleanup. | |
| wp_unregister_ability( 'test/calculator' ); | |
| // Cleanup. | |
| wp_unregister_ability( 'test/calculator' ); | |
| $this->assertInstanceOf( WP_Ability::class, $result ); | |
| $this->assertSame( 'test-math', $result->get_category() ); |
Co-authored-by: Greg Ziółkowski <[email protected]>
Replace loop-based slug validation tests with PHPUnit data providers for better test isolation and clearer failure reporting. - Add valid_slug_provider() for valid slug format tests - Add invalid_slug_provider() for invalid slug format tests
Simplify the test by replacing the callback function with direct calls to register_category_during_hook
…fields to include 'meta'
…and 'output_schema'
- Assert the count of properties in the schema to ensure it matches expected values. - Verify the existence and details of the 'category' property, including its type and readonly status. - Confirm that 'category' is included in the required fields of the schema. - Remove redundant test method for category schema validation.
JasonTheAdams
left a comment
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.
Great work, @galatanovidiu! And great discussions, everyone! 😄
Summary
Implements a comprehensive category system for organizing abilities. Each ability must now belong to exactly one category, improving discoverability and enabling filtering of abilities by their purpose.
Required
categoryParameterAll abilities must now specify a
categorywhen registering.Before:
After:
What's Changed
New Classes
WP_Ability_CategoryEncapsulates category properties (slug, label, description).
Location:
includes/abilities-api/class-wp-ability-category.phpWP_Abilities_Category_RegistrySingleton registry managing all registered categories.
Location:
includes/abilities-api/class-wp-abilities-category-registry.phpFeatures:
labelanddescriptionfor all categoriesabilities_api_category_registry_inithook on initializationregister_ability_category_argsfilter before registrationCore Changes to
WP_AbilityFile:
includes/abilities-api/class-wp-ability.php$categoryproperty (required string)get_category()method^[a-z0-9]+(-[a-z0-9]+)*$Changes to
WP_Abilities_RegistryFile:
includes/abilities-api/class-wp-abilities-registry.phpget_abilities_by_category( string $category )method_doing_it_wrong()if category not foundNew API Functions
File:
includes/abilities-api.phpCategory Management
Ability Filtering
New Hooks
Action:
abilities_api_category_registry_initFires when the category registry is initialized. This is the required hook for registering categories.
Parameters:
$registry(WP_Abilities_Category_Registry) - The category registry instanceFilter:
register_ability_category_argsAllows modification of category arguments before validation.
Parameters:
$args(array) - Category arguments (label, description)$slug(string) - Category slug being registeredREST API Updates
File:
includes/rest-api/endpoints/class-wp-rest-abilities-list-controller.phpNew Features
Category field in responses:
{ "name": "my-plugin/get-data", "label": "Get Data", "description": "Retrieves data", "category": "data-retrieval", "input_schema": {}, "output_schema": {}, "meta": {} }Category filtering parameter:
GET /wp/v2/abilities?category=data-retrievalSchema updates:
categoryadded to ability schema as required fieldcategorymarked as readonlyTest Coverage
New Test File:
tests/unit/abilities-api/wpAbilityCategory.php(550 lines)Updated Test Files:
wpAbilitiesRegistry.php- Added category setup/teardownwpAbility.php- Added category property to testswpRegisterAbility.php- Added category validation testswpRestAbilitiesListController.php- Added category filtering tests (125+ lines)wpRestAbilitiesRunController.php- Updated for category support (56+ lines)Documentation Updates
1.
docs/1.intro.md2.
docs/3.registering-abilities.md(59 new lines)categoryas Required parameterwp_register_ability_category()function signaturecategoryfield3.
docs/5.rest-api.mdcategoryfield to Ability Object schemacategoryfilter parameter to List Abilities endpoint4.
docs/6.hooks.md(70 new lines)abilities_api_category_registry_initaction documentationregister_ability_category_argsfilter documentationMigration Guide
For Plugin Developers
Step 1: Register Categories
Create categories before registering abilities:
Step 2: Update Ability Registrations
Add the
categoryparameter to allwp_register_ability()calls:Category Slug Naming Conventions
Valid Slugs:
data-retrievaluser-managementecommerceanalytics-123Invalid Slugs:
Data-Retrieval(uppercase)data_retrieval(underscores)data.retrieval(dots)data/retrieval(slashes)-data-retrieval(leading dash)data-retrieval-(trailing dash)Related Issues
This PR implements the category system from issue #101.