Skip to content

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Aug 21, 2025

Closes #39.

This PR introduces filter support for the WP_Ability class, allowing developers to customize ability behavior through WordPress hook filters. The main purpose is to provide extensibility points for input/output schemas, permission checks, and execution results.

  • Adds four new WordPress filters: ability_input_schema, ability_output_schema, ability_permission_result, and ability_execute_result.
  • Includes comprehensive test coverage for all filter integration scenarios.
  • Updates phpcs configuration to accommodate the new hook prefixes.

Example for how to revoke access to the ability:

$filter_cb = static function ( $permission, $ability_name ) {
	if ( 'test/filter-ability' !== $ability_name ) {
		return $permission;
	}

	return false;
};

add_filter( 'ability_permission_result', $filter_cb, 10, 2 );

Example for how to change the output value:

$filter_cb = static function ( $result, $ability_name ) {
	if ( 'test/filter-ability' !== $ability_name ) {
		return $result;
	}

	return 'modified-' . $result;
};

add_filter( 'ability_execute_result', $filter_cb, 10, 2 );

Testing instructions

6 new unit tests were added to cover possible scenarios. You can run them with

npm run test:php

@gziolo gziolo self-assigned this Aug 21, 2025
@gziolo gziolo added [Type] Enhancement New feature or request [Status] In Progress Assigned work scheduled labels Aug 21, 2025
@gziolo gziolo force-pushed the update/ability-filter-values branch 2 times, most recently from 80c0d3f to 7dca275 Compare August 21, 2025 18:37
@gziolo gziolo requested a review from Copilot August 22, 2025 04:55
Copy link
Contributor

@Copilot Copilot AI left a 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 introduces filter support for the WP_Ability class, allowing developers to customize ability behavior through WordPress hook filters. The main purpose is to provide extensibility points for input/output schemas, permission checks, and execution results.

  • Adds four new WordPress filters: ability_input_schema, ability_output_schema, ability_permission_result, and ability_execute_result
  • Includes comprehensive test coverage for all filter integration scenarios
  • Updates phpcs configuration to accommodate the new hook prefixes

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
includes/abilities-api/class-wp-ability.php Implements filter integration in schema getters, permission checks, and execution methods
tests/unit/abilities-api/wpAbilityFilters.php Adds comprehensive test suite for all filter functionality
phpcs.xml.dist Updates coding standards configuration to allow new hook prefixes
includes/abilities-api.php Adds phpcs disable comment for global naming conventions
tests/bootstrap.php Removes unnecessary phpcs disable comment
tests/unit/rest-api/*.php Adds descriptive comments to test files
tests/unit/abilities-api/wpAbilitiesRegistry.php Adds descriptive comment to test file

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@gziolo gziolo removed the [Status] In Progress Assigned work scheduled label Aug 22, 2025
@gziolo gziolo marked this pull request as ready for review August 22, 2025 05:26
@gziolo
Copy link
Member Author

gziolo commented Aug 22, 2025

@jonathanbossenger, this is something to keep on the radar for inclusion in the docs in case we agree that this level of extensibility is expected.

@justlevine
Copy link
Contributor

@gziolo is there an expectation for people to be calling public WP_Ability methods more than once per lifecycle?

My two concerns are

  1. Because this breaks SRP, folks extending the class (e.g. dev!: require string $name for registration #21 ) now either need more boilerplate (or not implement the hooks).
  2. The possibility of hooks being triggered multiple times introduces perf and behavior unpredictability.

@gziolo
Copy link
Member Author

gziolo commented Aug 22, 2025

The possibility of hooks being triggered multiple times introduces perf and behavior unpredictability.

That's the reality in the WordPress land:

"With the great power (of extensibility) comes great responsibility"

Some recent examples:

https://github.com/WordPress/wordpress-develop/blob/55644dc608d3a9c9d2541379d8a5b48d4b2c3d99/src/wp-includes/class-wp-block-type.php#L600-L617

https://github.com/WordPress/wordpress-develop/blob/55644dc608d3a9c9d2541379d8a5b48d4b2c3d99/src/wp-includes/class-wp-block-type.php#L619-L636

Because this breaks SRP, folks extending the class (e.g. #21 ) now either need more boilerplate (or not implement the hooks).

There are different audiences here:

  • developers implementing abilities
  • plugin developers and site admins who want to customize these abilities

If the author of the ability uses a custom class that extends WP_Ability, it's their responsibility to use parent methods to keep these hooks. However, they might not want that for some reason that we can't anticipate. This works both ways, so I think it's fine to leave these considerations to implementers. WordPress core will never register abilities that skip these hooks, making things unpredictable.

@justlevine
Copy link
Contributor

@gziolo my question wasn't critique, I was looking for some direction for code review 🙇


I don't think we disagree in premise. The nuance here is our extremely short timetable before merging. To use your example of get_variations()

Just because we don't have time to evolve it holistically doesn't mean that we can't design our initial API with intention and around specific use cases. And if we don't want to wait to merge these until we have a specific use case from e.g. MCP Adapter or AI Experiments, then we should at least take a bit here to consider the use cases/ code flows for these proposed hooks (instead of designing code to conform with non-rubberducked hooks after the fact).


So with that context, and putting yourself into the mind of an "implementer" for a minute - how do you envision these hooks to be used?

  • Are there public * methods that are obviously gonna be called multiple times (where if the filter is repeated there will be detrimental).
  • Are the methods that contain these hooks the most likely going to be overloaded.
  • And conversely, are there hooks that we want to be triggered every time people use, or consider it a good think if they need to actively take steps to reinclude the hook in their downstream extends *.

@gziolo
Copy link
Member Author

gziolo commented Aug 26, 2025

So with that context, and putting yourself into the mind of an "implementer" for a minute - how do you envision these hooks to be used?

I'm mostly concerned with this proposal about making it possible to customize the abilities that will get registered through WordPress Core.

Are there public * methods that are obviously gonna be called multiple times (where if the filter is repeated there will be detrimental).

Can you provide a real-life example of how often you anticipate the same hooks being called multiple times?

In the frontend context, WordPress core won't even process individual abilities because they are never consumed by default, so only the abilitis_api_init hooks get registered.

Inside REST API context, they should be called once per ability, depending on the endpoint type:

  • get all would call input and out schema hooks to produce the result
  • get single would call input and output schema hooks to produce the result
  • execute, would call all 4 hooks, it could be that input and output schema hooks, when defined, would get called twice - during permissions check and during the execution

WP Admin is less predictable at this point, as we don't know how wide the usage will become. That said, WP hooks are everywhere in the codebase. I would be surprised learning that the abilities registry would have a larger impact than the rest of the codebase. What's the part that worries you the most?

@justlevine
Copy link
Contributor

Are there public * methods that are obviously gonna be called multiple times (where if the filter is repeated there will be detrimental).

Can you provide a real-life example of how often you anticipate the same hooks being called multiple times?

The one that prompted my question is $ability->has_permissions() e.g

add_filter( 'ability_permission_result', static function( $result ) {
  if ( $result instanceof \WP_Error ) {
    return $result;
  }
  
  return MyCustom::check_user_permissions_in_enterprise_sso( wp_get_current_user() );
}

... 

add_filter( 'ability_execute_result', static function ( $result, string $ability_name, array $input ) {
  if ( 'my/complex-tool-with-state-and-context' !== $ability_name ) {
    return $result;
  }
  
  $prev_ability = wp_get_ability( $input['prevAbilityId'] );
  if ( $prev_ability->has_permissions() ) {
    ...
  }
}

What's the part that worries you the most?

Solely the timeline to core merge has us theorycrafting these, nothing blocking.

The combination of (a.) public getters that (b.) don't just "get" but break SRP to do something, while (c.) the filters run on each call instead of caching and (d) we already have people hankering to extends WP_Ability makes this class a recipe for refactor (IMO), but it's hard to justify the extra complexity of "doing it right" ( e.g. separate prepare_*() methods, applying filters once etc ) in an initial release.

@gziolo
Copy link
Member Author

gziolo commented Aug 27, 2025

All valid concerns. We don’t have to rush introducing these filters, and wait for feedback from folks what’s limiting them.

Aside, caching and filters is often incompatible in WordPress reality because there are two primary challenges:

  • filters can be added or removed at different stages of page rendering, I saw many examples where filter gets added before executing the function and immediately removed after
  • result from filters and functions are not guaranteed to be idempotent as they often depend on thr current global state

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Provide extensibility for individual abilities
2 participants