Skip to content

Fix documented return type for add_link(), edit_link(), wp_update_link() #11094

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.

Sign up for GitHub

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

Closed
IanDelMar wants to merge 5 commits into from
Closed

Fix documented return type for add_link(), edit_link(), wp_update_link() #11094

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8bfdfb5
Fix documented return type for add_link(), edit_link(), wp_update_link()
IanDelMar Feb 28, 2026
21c0850
Update src/wp-admin/includes/bookmark.php
IanDelMar Mar 1, 2026
b5ed63f
Update src/wp-admin/includes/bookmark.php
IanDelMar Mar 1, 2026
4af2b61
Revert "Update src/wp-admin/includes/bookmark.php"
IanDelMar Mar 1, 2026
86ea883
Update bookmark.php
IanDelMar Mar 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/wp-admin/includes/bookmark.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
*
* @since 2.0.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.
*/
function add_link() {
return edit_link();
Expand All @@ -23,7 +23,7 @@ function add_link() {
* @since 2.0.0
*
* @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.
Copy link
Copy Markdown
Member

@westonruter westonruter Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
* @return int 0 on failure. The link ID on success.
* @return int|never 0 on failure. The link ID on success. Exits when unauthorized.

Copy link
Copy Markdown
Author

@IanDelMar IanDelMar Feb 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never can not be part of a union type. - https://3v4l.org/IWMlC#v8.1.34

Copy link
Copy Markdown
Member

@westonruter westonruter Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In PHP type hints, yes, but in phpdoc, PHPStan is fine with it, right?

Copy link
Copy Markdown
Member

@westonruter westonruter Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://phpstan.org/r/d067b4b5-761a-4888-a8f8-c92069b24145

Copy link
Copy Markdown
Member

@westonruter westonruter Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cc @apermo

Copy link
Copy Markdown

@apermo apermo Mar 1, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

I hope that adds some light to the why.

Copy link
Copy Markdown
Author

@IanDelMar IanDelMar Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@apermo apermo Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional return type actually makes sense.
@westonruter your thoughts?

Copy link
Copy Markdown
Member

@westonruter westonruter Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@IanDelMar IanDelMar Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*/
function edit_link( $link_id = 0 ) {
if ( ! current_user_can( 'manage_links' ) ) {
Expand Down Expand Up @@ -295,7 +295,7 @@ function wp_set_link_cats( $link_id = 0, $link_categories = array() ) {
* @since 2.0.0
*
* @param array $linkdata Link data to update. See wp_insert_link() for accepted arguments.
* @return int|WP_Error Value 0 or WP_Error on failure. The updated link ID on success.
* @return int 0 on failure. The updated link ID on success.
Comment thread
IanDelMar marked this conversation as resolved.
Outdated
*/
function wp_update_link( $linkdata ) {
$link_id = (int) $linkdata['link_id'];
Expand Down
Loading