Tracking issue: Improve types for certain functions

[swissspidy]

I was just experimenting with migrating a project from PHPStan level 9 to 10, and found a lot of instances of mixed types. I thought I'd open a tracking issue to figure out if and how those could be improved, either in phpstan-wordpress or (preferably) wordpress-stubs:

• [x] get_registered_settings(): document the shape of the returned settings (see register_setting())
• [ ] WP_Taxonomy::$labels: define object shape with expected keys (see get_taxonomy_labels())
• [ ] WP_Post_Type::$labels: define object shape with expected keys (see get_post_type_labels())
• [ ] WP_Taxonomy::$cap: define object shape with expected keys
• [ ] WP_Post_Type::$cap: define object shape with expected keys (see get_post_type_capabilities())
• [ ] WP_User::$caps: define as array<string, bool>
• [ ] wp_parse_args(): could an extension be built to dynamically set the return type based on the shape of the arguments?

[IanDelMar]

• WP_User::$caps: define as array<string, bool>

Couldn’t this be implemented directly in core? Core already uses the array<keyType, valueType> notation in some places. I also don’t see anything in the coding standards that leads me to believe array<keyType, valueType> is not permitted.

[swissspidy]

Yeah maybe. But also in the stubs repo it could be enhanced to use generics perhaps

[swissspidy]

@johnbillion since you are regularly updating docblocks, maybe this is one for you?

[johnbillion]

get_registered_settings() and get_post_type_capabilities() were implemented in https://core.trac.wordpress.org/changeset/60236.

The problem with declaring the shapes on class properties is it can lead to a load of duplication and we don't have a way to ensure they stay in sync with return values or arguments that populate the properties.

[szepeviktor]

implemented in https://core.trac.wordpress.org/changeset/60236.

Fresh out of the oven!

[johnbillion]

https://core.trac.wordpress.org/ticket/57299 is probably relevant too, we don't yet know if the docs parser for developer.wordpress.org can handle array key notation. Needs somebody to volunteer to get that tested.

[swissspidy]

get_blogs_of_user() would be a good candidate to improve in core as well, but requires object shape notation support.

[swissspidy]

Another one: taxonomy_exists() should assert that get_taxonomy() does not return false

Edit: and that get_terms(), wp_get_object_terms, andget_objects_in_term won't return WP_Error in that case.

Similarly, post_type_exists should assert for get_post_type_object()

Or wp_update_comment_count for get_post()

[IanDelMar]

Note concerning object shapes

For anyone working on that feature request: object shapes do not automatically inherit the class of the object whose shape they describe and their properties are read-only. They must be intersected with the class.

See this example on phpstan.org/try.

[IanDelMar]

@swissspidy I've looked into specifying types based on, e.g., taxonomy_exists() and thought about it for a while. I always came back to the more fundamental question: why call taxonomy_exists() before calling

• get_taxonomy()
• get_terms()
• wp_get_object_terms()
• get_objects_in_term()?

These are "get or error" style functions and will return pretty early if the taxonomy does not exist.

I don't know your exact use case, but I think that in general using taxonomy_exists() as a guard is rarely necessary. I would argue, that

if (taxonomy_exists('taxonomy')) {
    $taxonomy = get_taxonomy('taxonomy');
    // taxonomy logic
}

could and maybe should be written as

$taxonomy = get_taxonomy('taxonomy');
if ($taxonomy instanceof WP_Taxonomy) {
    // taxonomy logic
}

It is also unsafe to assume that a true branch of taxonomy_exists() implies a subsequent get_taxonomy() returns a WP_Taxonomy. Intervening logic can invalidate the taxonomy:

if (taxonomy_exists('taxonomy')) {
    unregister_taxonomy('taxonomy');
    $taxonomy = get_taxonomy('taxonomy'); // returns false
}

Admittedly, no one would write it exactly like this, but it illustrates that DB state can change between calls. So, accurately inferring from taxonomy_exists() that get_taxonomy() returns WP_Taxonomy would require to rule out all of the scenarios where taxonomy_exists('taxonomy') might no longer be true when get_taxonomy('taxonomy') is called.

In the end, is it worth the effort of trying to infer types in such situations - when simply following the get-then-check pattern guarantees that the result is a WP_Taxonomy and keeps both runtime behaviour and static analysis straightforward? Tbh, I'd say no, but I'm curious to hear your use case and reasoning; there may be aspects I haven't considered.

[swissspidy]

I didn't have a specific use case in mind. Your assessment sounds reasonable at first glance 👍