-
Couldn't load subscription status.
- Fork 32
Add Filtering Capabilities for Registered Abilities #115
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
|
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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #115 +/- ##
============================================
+ Coverage 86.65% 87.66% +1.00%
- Complexity 148 211 +63
============================================
Files 18 19 +1
Lines 982 1143 +161
Branches 92 92
============================================
+ Hits 851 1002 +151
- Misses 131 141 +10
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:
|
Public constants are converted to private static properties Additionally, validation methods are updated to use guard clauses, and some boolean logic is simplified.
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! Left a few suggestions!
| * @since n.e.x.t | ||
| * @var int | ||
| */ | ||
| private static $no_limit = -1; |
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.
Why is this a static property instead of a constant? Same for the static properties below.
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.
Here should be fine to use const like here:
| protected const DEFAULT_SHOW_IN_REST = false; |
For arrays, PHP 7.2 is unhappy, so here:
| protected static $default_annotations = array( |
It's a complex env to support WP core 😅
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 not sure what you mean. Why would PHP 7.2 be unhappy for arrays as constant values? I believe arrays as scalar values was introduced in PHP 5.6.
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.
In that case, it’s linter that is very strict and prohibits usage. That would be similar case to array short syntax.
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.
Can we update the linter? const FOO = array( 'bar' => 123 ); is supported, and reducing our architecture quality to appease linting feels lame. Hahah!
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.
As far as I’m aware that was discussed at WC US, and someone has to champion the proposal and adjustments to the codebase.
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.
Good point! I initially implemented these as constants, but our PHP linter was flagging them with errors. To maintain compatibility with our linting rules (which appear to be stricter than PHP 7.2's actual capabilities), I converted them to private static properties.
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.
That's unfortunate. Is this the right place to propose the change? https://github.com/WordPress/WordPress-Coding-Standards?
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 found the Core Committer meeting notes, which include an entire section on the need for changes in coding standards. You can try in that GitHub repository, or raise it as a topic for the weekly dev chat (next one today).
| * @phpstan-param array{ | ||
| * category?: string|array<string>, | ||
| * namespace?: string|array<string>, | ||
| * search?: string, | ||
| * meta?: array<string,mixed>, | ||
| * orderby?: string, | ||
| * order?: string, | ||
| * limit?: int, | ||
| * offset?: int, | ||
| * ...<string, mixed> | ||
| * } $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.
Let's define this as a type in the class docblock, and then use @phpstan-import-type in the wp_get_abilities function. We can then set that as the @param type in this method, too.
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've tried this, but it looks like @phpstan-import-type doesn't work in function docblocks (only in class docblocks)
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.
Correct! It would go like this:
- Add an AbilityQueryArgs type in the class docblock
- Use it as the
@paramtype for this method - Import and use it in the function
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 is exactly what I tried and is not working.
@phpstan-import-type only works in class docblocks, not in function docblocks. PHPStan throws an error: "Parameter $args has invalid type AbilityQueryArgs".
The solution is to keep the full @phpstan-param array shape in both places:
- Define
@phpstan-type AbilityQueryArgsin the class docblock as the source of truth - Duplicate the full array shape using
@phpstan-paramin the function docblock
This reduces duplication within the class but we still need to duplicate it for standalone functions since @phpstan-import-type doesn't work outside of class contexts.
Please check d3275fd
| $this->validate_meta_arg(); | ||
| $this->validate_orderby(); | ||
| $this->validate_order(); | ||
| $this->validate_pagination_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.
Hmm, these validation methods are interesting. They "validate" in the sense that they check if the provided value is valid, but then fallback on a default instead of notifying of the issue in any way. Is this how the WP_Query class works, so we're doing this for consistency? I feel like we should at least be calling doing_it_wrong so people know why there's unexpected 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 should have used sanitize instead of validate
WP_Query also only sanitizes the args.
I can implement validation if you want, but I'd prefer to keep it simple.
Please check 68fea1b
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.
Ah, gotcha. I'm inclined to let people know when they're straight up doing things wrong (I've personally felt the consternation of figuring out that I have a typo in a query), but won't push it if it's not recommended otherwise.
| usort( | ||
| $abilities, | ||
| static function ( $a, $b ) use ( $getter_method, $order_multiplier ) { | ||
| return strcasecmp( $a->$getter_method(), $b->$getter_method() ) * $order_multiplier; |
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.
Ohh, I like the use of $order_multiplier here! Clever! 😃
WP_Abilities_Query already returns all abilities when args is empty, making the backward compatibility check unnecessary.
Updates type hints to specify arrays of strings for 'category' and 'namespace' parameters.
Updates method names and related docblocks to clarify that query argument processing focuses on sanitization rather than validation.
Improves documentation to specify use of the query class for retrieving and filtering abilities.
| * | ||
| * @since n.e.x.t | ||
| */ | ||
| class WP_Abilities_Query { |
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.
From the docs it seems this class isn't meant to be consumed/extended directly (which IMO makes sense from a 6.9 iterative POV).
If so, can we mark the class final and add an @internal + a note pointing to wp_get_abilities( $args ) in the doc-block? Would give us more freedom to iterate in future versions.
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.
Done
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 concerned about using the WP_*_Query when that pattern is
- used by core for DB lookups
- often encouraged to be used directly versus getters (e.g.
new WP_Term_Query()versusget_terms(). )
Neither afaik are true here and docs can only help so much.
From a code POV, we could also simplify implementation significantly if we weren't trying to mimic WP_Query. E.g. we don't need 011y, query getters, or pagination when we're just sorting/filtering an array in PHP.
| * @since n.e.x.t | ||
| * | ||
| * @param array<string,mixed> $args Query arguments. | ||
| * |
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'm assuming the blank end-lines in the comments throughout this file are unintentional
| * |
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 is a really fair point, @justlevine. Thinking on this, I wonder if this would be a good use for introducing an Abilities_Collection class, instead of a querying system like this. It would implement IteratorAggregate, ArrayAccess, and such, and be what the registry stores all the abilities into.
From there, it could have methods such as Abilities_Collection::with_category(...$categories) which returns an array (or another Abilities_Collection subset). If it's the latter, then folks can chain:
$abilities = wp_get_abilities()
->with_namespace('woo')
->with_category('orders');This way, to your point, we're treating it more like an in-memory array rather than querying against an external system. Constructing a query is most important for efficiently retrieving the data in one go. But for something like this it's not really necessary.
What do you think, including @galatanovidiu?
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.
If you feel this needs more time to define the API’s shape, we will have to postpone it to WordPress 7.0. This is a rather substantial change that needs some longer conversation, so contributors can have time to chime in and share their perspective on how the codebase evolves, so they can later replicate better patterns.
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 agree with all of you.
I know we are in a hurry to have this functionality out and ready for 6.9, but I also like the idea of the Abilities Collection a lot. I've used Laravel collections a lot, and I know how natural and DX-friendly that is.
Because of that, I've prepared a POC here: #119
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.
It's also important to consider performance. The proposal for collections will loop through once for the current set of abilities per filter. In contrast, in this PR, there is a single pass over the entire set, and all filters are applied to a single item at once. There is also an extensibility story that needs to be taken into account. WP_Query is at extreme with 44 occurrences of apply_filters() and apply_filters_ref_array(), but some sort of filtering will eventually need to be added, so custom filters can be executed, for example, when fetching abilities through the REST API.
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 is a rather substantial change that needs some longer conversation,
I second @gziolo, but probably take it further than he.
While I'm a big fan of fluent APIs in general (and love how much simpler #119 reads), I don't think that us introducing the concept of a *_Collection into WordPress on the cusp of 6.9 is a great idea. It's also possible that the WP_Query parrten will turn out to be ideal in 7.x after we find a reason to have abilities be backed by something in the DB (a la plugins/themes/fonts), in which case preemptively going with a Collection pattern would be hard to untangle.
I'm hoping we can take the simplified methods from @galatanovidiu 's #119 but move them to private functions inside WP_Abilities_Registry. That would give us all the surface area we need for MVP filtering support in 6.9 (albeit via passed $args), while still keeping our hands free for whatever pattern we ultimately decide is best (when we have some unpressured time to reach the decision holistically).
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.
Thanks for weighing in, folks! And for throwing together the draft so quickly, @galatanovidiu! 😄
Overall, I agree with the desire to not rush this. If we start with simple arrays in 6.9 with no built-in filtering (folks can always get the abilities and filter them themselves), I'm fine with that.
Regarding the introduction of the *_Collection concept, in my opinion, WordPress errs too much on the side of using "we haven't done that before" argument as a reason not to do things. Concepts like Collections are tried and true patterns across multiple systems. We're not creating a new concept entirely, we're porting one over. I do appreciate you raising the point, @justlevine, as it is a good one to be mindful of. I would love to see more tried-and-true practices and patterns brought tastefully into WordPress over time.
With regards to performance, @galatanovidiu, in this case I think it's negligible — absolutely nothing like performance from a database perspective. And even chaining things like with_x is working from a smaller. subset each time, so the iteration performance jumps each time. Frankly, this isn't too unlike how databases often filter things down.
In short, I'm in favor of closing this PR to explore a better API. I really like the draft (which I need to dig into more), and would like to explore that sort of direction further. 😄
| // Paginate results | ||
| $abilities = wp_get_abilities( array( | ||
| 'limit' => 10, | ||
| 'offset' => 0, | ||
| ) ); | ||
| ``` |
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.
Is there a specific need for pagination?
I see that you're trying to mirror WP_Query et al (versus adding sorting/filtering as registry methods) but abilities are in a PHP array, not a database. Do we really need a shortcut for what's essentially an array_slice()?
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.
Filtering abilities are the most crucial aspect from my perspective. Ordering and pagination are nice-to-haves, but I want to acknowledge that they fit well together in the proposed implementation, as they allow easy selection of a precise number of abilities that meet specific criteria, which might be useful when passing data to an LLM with limited context. @galatanovidiu might have a better overview of the real needs based on the work he has done for the MCP Adapter.
|
One thing I want to note is that some filters are hard to express in this array notation. For example, a |
Defines a new `@phpstan-type` called `AbilityQueryArgs` to represent the arguments for an abilities query. This centralizes the complex array shape definition, which was previously defined inline in the constructor's docblock. Using this shared type improves code readability and maintainability. The `wp_get_abilities` function documentation is also updated to reference this new type for better consistency.
Marks the `WP_Abilities_Query` class as `final` and `@internal` to clarify that it is not part of the public API. This change prevents the class from being extended or consumed directly, encouraging developers to use the stable `wp_get_abilities()` function instead. This helps ensure forward-compatibility and avoids potential breakage from future implementation changes.
Co-authored-by: Dovid Levine <[email protected]>
Summary
Closes #38.
Adds querying and filtering capabilities to the Abilities API, allowing developers to efficiently find and retrieve specific abilities from potentially thousands of registered abilities.
This PR introduces the WP_Abilities_Query class and extends wp_get_abilities() to accept filtering arguments, enabling server-side filtering by category, namespace, meta properties, search terms, and more.
Changes
New Features
WP_Abilities_Queryclass - WordPress-standard query interface for filtering abilitieswp_get_abilities()function - Now accepts optional$argsparameter for filteringImplementation Details
Documentation
docs/8.querying-filtering-abilities.mddocs/4.using-abilities.mdwith filtering examplesBackward Compatibility
Fully backward compatible - existing code calling
wp_get_abilities()without arguments continues to work unchanged.