Fix documented return type for add_link(), edit_link(), wp_update_link()#11094
Fix documented return type for add_link(), edit_link(), wp_update_link()#11094IanDelMar wants to merge 5 commits intoWordPress:trunkfrom
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
src/wp-admin/includes/bookmark.php
Outdated
| * | ||
| * @param int $link_id Optional. ID of the link to edit. Default 0. | ||
| * @return int|WP_Error Value 0 or WP_Error on failure. The link ID on success. | ||
| * @return int 0 on failure. The link ID on success. |
There was a problem hiding this comment.
Right, it cannot return WP_Error because the $wp_error arg is not supplied to wp_insert_link(). It can, however, return never if there is a permission issue.
| * @return int 0 on failure. The link ID on success. | |
| * @return int|never 0 on failure. The link ID on success. Exits when unauthorized. |
There was a problem hiding this comment.
never can not be part of a union type. - https://3v4l.org/IWMlC#v8.1.34
There was a problem hiding this comment.
In PHP type hints, yes, but in phpdoc, PHPStan is fine with it, right?
There was a problem hiding this comment.
There was a problem hiding this comment.
@IanDelMar while you are not wrong. And while core only has a single union type with never as of now. In recent PRs (#11008, #11012, #11009) they were introduced in the review cycle, in order to improve static analysis with PHPStan. @westonruter suggested to add them as I refactored union types with void.
As of last week (#10419 / 8a82e67) PHPStan level 0 is part of the Test and Build Scripts, that being said, @westonruter is currently eying PRs that can improve the PHPDoc in order to reduce the noise in PHPStan, and thats the reason to introduce |never in case a function might exit inside where PHPStan does not identify that correctly.
See as well:
- https://make.wordpress.org/core/2025/07/11/proposal-phpstan-in-the-wordpress-core-development-workflow/
- https://core.trac.wordpress.org/ticket/64238
I hope that adds some light to the why.
There was a problem hiding this comment.
To me it seems there is a misconception of "subtype" or "never". When you add never to a union, you are trying to widen the type. But you simply cannot widen a type by adding a subtype to a union. Again the more straight forward example: You cannot widen mixed by adding string to it. mixed|string is still mixed just like fruits|apples is still fruits (This is how PHPStan handles unions, see Type Normalization). What you actually want is to narrow the type to a subtype. If some condition is met, we know that mixed is string. When the store runs out of any other fruits, I buy apples. This is a conditional return type.
In particular, the issue you were trying to solve in ticket 64703 can be addressed without introducing any union that includes never. Instead, you can use a conditional return type to refine wp_die() from void to its more specific subtype never, conditional on $args['exit']. See https://phpstan.org/r/b1604252-60e1-4981-bee6-36506466d9ed. In that snippet, I also added a native void return type to wp_die() to demonstrate that PHPStan correctly treats never as a subtype of void. If it did not, PHPStan would flag the @return tag as incompatible with the native return type.
So doing something that is weird, like adding never to a union "to improve static analysis" might just mask an underlying issue with the code or the PHPDocs used to document it.
There was a problem hiding this comment.
The conditional return type actually makes sense.
@westonruter your thoughts?
There was a problem hiding this comment.
In regards to string|mixed, in Performance Lab we have level 8 PHPStan and we write filter callback code like this:
/**
* Filters foo.
*
* @param string|mixed $foo Foo.
* @return string Foo.
*/
function filter_foo( $foo, int $post_id ): string {
if ( ! is_string( $foo ) ) {
$foo = '';
}
/**
* Because plugins do bad things.
*
* @var string $foo
*/
// Filtering logic goes here.
return $foo;
}
add_filter( 'foo', 'filter_foo', 10, 2 );The reason is that it is documenting the normal type as strong but acknowledging that other plugins often do bad things and return incorrect values for filters, like null or array. So this is why a native PHP type hint is not used here, to avoid a fatal error.
It's true that the type could just be mixed, but the value lies in the documentation for developers to be able to see at a glance what the expected value will be.
This is what I had in mind with using never in @return union types.
But if you feel strongly that there could be issues with adding never, I'm fine with just documenting the never case in the description (e.g. this function may exit if X). Otherwise, implementing never as part of a conditional return should be implemented correctly in #11009.
There was a problem hiding this comment.
but the value lies in the documentation for developers to be able to see at a glance what the expected value will be
That only works if everyone involved is aware of (and follows) the same convention. Others might be confused. And yes, filters are a bit of a type hell 😄
This is what I had in mind with using never in @return union types.
In the context of your statement that PHPStan is fine with it and the ticket one of the PRs is linked to, it sounded like that it is correct because PHPStan does not flag it and that this was meant to address a concrete problem. That's why I pushed back and tried to make the point that PHPStan doesn't even check for this and demonstrated that it is not required to resolve PHPStan errors.
But if you feel strongly that there could be issues with adding never
I am not sure what issues you are referring to exactly. From PHPStan's perspective it still normalises to int anyway. If there is consensus among core developers that this is the preferred documentation pattern for such cases, then fair enough. Personally, I wouldn't add it. Its documentation value is limited. That's different from your filters example, where the annotation can serve as a reminder that developers must do something (i.e., ensure a string is actually consumed).
All the arguments from my earlier comments still hold from my perspective, but ultimately, the decision to add or not add never in this specific case is pointless, if there's no broader, consistently applied convention. You’re likely more familiar with WordPress' documentation conventions, so I'll leave it up to you.
Co-authored-by: Christoph Daum <c.daum@me.com>
Co-authored-by: Christoph Daum <c.daum@me.com>
This reverts commit b5ed63f.
Trac ticket: https://core.trac.wordpress.org/ticket/64764
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.