-
Notifications
You must be signed in to change notification settings - Fork 9
Implement server-side registry for Abilities API #3
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
Conversation
The proposed initial implementation for server-side registry with testing code coverage is ready to receive the initial feedback. What's still missing is the unit tests configuration in a standalone repository, as I developed and tested everything with the WordPress core testing infrastructure available. |
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 implements a server-side registry for the WordPress Abilities API, introducing the core infrastructure for registering and managing abilities with validation, permissions, and execution capabilities.
- Introduces a complete abilities registry system with registration, validation, and management functionality
- Implements ability classes with input/output schema validation and permission handling
- Provides global functions for registering, unregistering, and retrieving abilities
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/abilities-api.php | Main API functions for registering, unregistering, and retrieving abilities |
src/class-wp-abilities-registry.php | Registry class for managing ability registration and lookup with validation |
src/class-wp-ability.php | Core ability class with validation, permission checking, and execution methods |
tests/unit/AbilitiesAPITest.php | Comprehensive unit tests for the global API functions |
tests/unit/WPAbilitiesRegistryTest.php | Unit tests for registry validation and management functionality |
phpunit.xml.dist | PHPUnit configuration for running unit tests |
.editorconfig | Editor configuration for consistent code formatting |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
To better illustrate that I opened a PR in WordPress Core to ensure all the CI checks pass and all unit tests work: CI for WP core still validates against PHP 7.2 and 7.3, so that might enforce some syntax changes as these jobs fail ... It looks like unit tests pass on 7.4 and higher as expected. PHPCS doesn't like the followign things:
I'm inclined to address these issues to avoid a compatibility mismatch. |
IMHO dont waste the time rn PHP 7.4 is coming to core in either 6.9 or 7.0, so either way in time for this to be merged, but more important once I set up vipcs locally most these code standard fails will autofix. |
src/abilities-api.php
Outdated
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.
Would recommend using inc/
instead of src/
. I've got a feeling we're gonna need use some @wordpress/*
packages that need to be built down the road.
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.
Initially, I wanted to replicate https://github.com/WordPress/php-ai-client and went with src
for PHP, but I agree that it's hard to use one pattern because we expect both PHP and JS code in this repo. Gutenberg uses packages
and lib
. Here we also need to take into account a need for a composer package, and an npm package. It isn't so simple to pick right folder names 😅
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.
Naming things is always the hardest part 😬
I don’t think GB (JS-first monorepo) or php-ai-client (standalone Composer lib) are quite the right references for a feature (or canonical) plugin.
FWIW, heres what other official WordPress/* projects are doing:
- WordPress/TwoFactor and Performance Plugins (e.g. use
includes
for PHP and scatter JS (assets
, root-levelcss/js
folders,src
) - Health Check: PHP in
HealthCheck
namespace, build-step assets insrc
. - PCP uses
includes
for PHP andassets
for JS/CSS`. - WordPress Importer uses
src
for PHP but it's legacy and has no js/css.
Semantically it seems the general pattern is
includes
(orinc
but that might be more of an a8c thing, havn't seen it inWordPress/*
) = core PHP (no build step)src
are plugin assets that need a build step. OR sometimes they're co-locatedassets
. (OR sometimes non-buildable PHP)packages
reusable (and usually releasable) JS/TS libraries.- havnt found any consistent use of
lib
in the org.
tl;dr I think includes
or inc
is best for clarity (unlike src
we know PHP is expected to be there). Then, if/when we need buildable assets for the admin we can see whether assets
vs buildable src
(or a ts package
, but that seems unlikely in this repo) make sense without being boxed in by semantics.
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.
Interesting find is that there are best practices for WP plugins at https://developer.wordpress.org/plugins/plugin-basics/best-practices/#folder-structure. I see includes
there. src
for JavaScript code makes sense, too. The remaining question is where we locate the composer package and npm package that we will need here allowing early access to server-side registry, REST API controller, and the client-side package with the client-side registry that also integrates the server-side registry through REST API call.
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 implemented previously wp-scripts plugin-zip
command and this is what I found there today:
Let's go with includes
to have an easy way to integrate that. For JS output, we can use either build
or public
.
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 figured out it's best to tackle it in #4 after we land the initial code. This way it's going to be much easier to handle every code change while the CI is running.
Should we use a namespace My gut says we should, because like GB our goal here is for experimentation/extenders and we don't want to have to worry about breaking back-compat when we merge into core. (Unlike individual performance plugins which arent meant to be extended or rly used in 3rd-party code |
Gutenberg doesn't use namespaces, so I'm a little bit confused. Did you mean we shouldn't? Overall, in WP core you can find some libs like SimplePie, PhpMailer, Sodium that use namespaces, but they were forked from preexisting projects. I don't know whether there is enough support to use namespaces for other parts of codebase in WP core. From the composer package perspective, using namespaces would only help, but there is the backward compatibility question as part of that once this code gets included in WP core. |
FML This is why I shouldn't do code review after midnight 🤦 I meant to ask if we want to prefix or suffix. For example: Although I guess my subconscious mind was suggesting that namespaces could solve the same problem, but make the eventual migration even easier, e.g: // If the final API doesnt change, then the user only needs to remove the `use` statement
- use SomeNamespace\wp_get_ability;
...
wp_get_ability( ...$args );
/// versus search-replace in the code logic
- some_prefix_get_ability( ...$args );
+ wp_get_ability( ...$args ); |
This is used that way only when Gutenberg wants to override the preexisting class in WP Core. If the class or function doesn't exist then in Gutenberg the strategy is to polyfiil the functionality using check if such thing exists to prevent double declaration. |
For the sake of illustration, I fixed all issues reported in WordPress/wordpress-develop#9410 regarding PHP 7.2 and coding standards compatibility with 1975276. In particular, the changes around types might need to be reverted when we opt to go with PHP 7.4 as the minimum required version, but the rest might be fine to leave as is. |
I know better than to attempt to Gutenberg-splain to you 🙇 Thanks for the correction, I must be having some mandela effect moment about not needing to worry about my Fonts API or Interactivity API experiments clashing on merge until I renamed them 🤦 So with absolutely 0 WordPress prior art the question becomes more generically: "do we want to take some preventative measures (prefixing or namespacing) to allow for immediate 3rd party adoption/use of the Abilities API without worrying about carrying tech-debt into core with us or breaking the early-ecosystem (both of which I think we can agree do have prior art in WordPress 😅). |
We definitely should provide some recommendations on how to safely use the package in a way that will eventually get replaced by the version that ships in WP core. I'm not entirely sure what that will be, but that's part of the story we need to design. @jonathanbossenger plans to work on the documentation as outlined in #5, so we definitely need to coordinate with him. |
Both #4 and #6 depend on this PR, so let's land it first. I tested everything against WordPress core CI, see WordPress/wordpress-develop#9410. We can follow up with the tooling afterwards, while in parallel extend the logic with REST API controllers. |
Closes #1.
PR in WordPress core, which validates compatibility and whether tests pass:
Proposed API
Registering an ability
Unregistering an ability
Retrieving a specific ability
Retrieving all abilities
Operations on abilities