Improve the properties validation when using `ability_class` during registration

[gziolo]

I left a note here about making execute_callback optional when ability_class is passed as one of the properties:

invoke on Content_Rewrite_Tool would have to be static to fit here because execute_callback is mandatory at the moment during ability registration (label and description, too). If that is too limiting, we could revisit $properties validation. I also think that we should make $properties mandatory, as you still must provide these 3 properties, so validation passes before the ability instance gets created.

I have an example from @felixarntz of using ability_class from the AI chatbot demo using the PHP AI Client SDK presented at WC US:

https://github.com/felixarntz/wp-ai-sdk-chatbot-demo/blob/326266fd62fc805ceac0dfd4fe71cd3cc3e7cad3/includes/Abilities/Abilities_Registrar.php#L26-L44

It's where not only the execute_callback was mocked, but also the description.

Proposal

• Make $properties mandatory.
• Make execute_callback optional when ability_class gets passed.
• Consider making label and description optional when ability_class gets passed, with the assumption that the developer will set it in the custom class extending WP_Ability.

[felixarntz]

Thank you for raising this @gziolo ! I was going to open an issue for that, so you beat me to it.

From my perspective, all other properties should be optional when an ability_class is passed. When using classes, that makes the architecture more intuitive.

Regarding validation, maybe we could still validate the properties even then though. Two alternative ideas:

• We could instantiate the class without immediately assigning it to the property of registered abilities. Then we can call get_label() etc and ensure it's not empty. If it is, the developer did something wrong and we wouldn't register the class, the instance would be discarded.
• Or we could move all validation into the WP_Ability class. This is closer to the actual source of truth, and in there we could validate the properties as part of the constructor when they're passed. A child class can override or set its own properties (like I am doing in the AI client demo). In case of a validation error, we could throw an InvalidArgumentException, which we catch in the registration call and then "translate" into the error message to maintain the same format as we have now. The exception would also fundamentally prevent creating an invalid instance, so it keeps the code robust even if someone (for some reason) tried to instantiate a WP_Ability themselves.

The second idea would also have the benefit that it would "just work", regardless of whether or not an ability_class is passed.

[gziolo]

All great feedback, @felixarntz. It's similar to what I was thinking. In fact, in one of the previous versions (changed in #21) it was possible to pass an instance of WP_Ability and the validation for the name was executed based on the value from ->get_name() call.

The second idea would also have the benefit that it would "just work", regardless of whether or not an ability_class is passed.

Yes, we could, do that in all cases at least for ->get_name(), ->get_label() and ->get_description(). I think, this is possible in both the existing place and the WP_Ability, so it's mostly the question how to shortcircut the registration when the conditions are not met to get added to the registry depending on the approach taken.

[justlevine]

Or we could move all validation into the WP_Ability class.

I stand by the changes (and conversations ) made in #21, and think ☝️is the best approach to handle this. We shouldn't need to sacrifice on idempotency and other sorts of downstream predictability or on a small, forward-compatible API footprint to address this DX complication.

I'm mobile and cant find the nested code review conversation, but I'll come back later with a link + PR example where the registry is a try/catch and protected function validate( $props): void|throw can be overridden at the same level as whatever other changes justify a custom ability class.

[felixarntz]

@gziolo

The second idea would also have the benefit that it would "just work", regardless of whether or not an ability_class is passed.

Yes, we could, do that in all cases at least for ->get_name(), ->get_label() and ->get_description(). I think, this is possible in both the existing place and the WP_Ability, so it's mostly the question how to shortcircut the registration when the conditions are not met to get added to the registry depending on the approach taken.

Can you clarify what you mean? I'm not sure I follow. My thinking is:

• In the register method, we always instantiate the WP_Ability class with the given $name and $properties, without any validation.
  • This can be either the custom provided ability_class or the default WP_Ability class.
• The instantiation is wrapped in a try catch loop. The class is expected to throw an InvalidArgumentException if anything with $name or $properties is wrong.
• In the catch, we translate the exception to the current error format and don't register the ability.
• If no exception occurs, we can register the ability.

Does that align with your thinking, or do you have something else in mind?

[gziolo]

Does that align with your thinking, or do you have something else in mind?

My point was that we could do some of that validation in the registry with public methods, but it's also fine to move everything to the constructor (which calls protected validate method) as illustrated in the discussion. I think, we can move forward with the implementation based two last comments:

• Improve the properties validation when using ability_class during registration #53 (comment)
• Improve the properties validation when using ability_class during registration #53 (comment)

Together, this sounds like a realid solid solution with all the advantages of OOP when using ability_class property, or even direct usage of WP_Ability.

[felixarntz]

My point was that we could do some of that validation in the registry with public methods

Got it, thanks. While it could be done either way, I think moving all validation to the WP_Ability class (or its child classes) keeps the validation closer to the source of truth. Except for $name, the properties are only relevant in the scope of a WP_Ability.

[gziolo]

Except for $name, the properties are only relevant in the scope of a WP_Ability.

Yes, it's the only check we need to run before adding the new instance of ability to the registry to avoid accidental name clashes.