Skip to content

Docs: Replace void with null in union return types in Administration #11008

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
apermo wants to merge 12 commits into from
Closed

Docs: Replace void with null in union return types in Administration #11008

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
544414a
Docs: Replace void with null in union return types in Administration
apermo Feb 23, 2026
b123868
fix: resolve PHPStan warnings in file.php and post.php
apermo Feb 23, 2026
2f1d8d5
Docs: Address review feedback for return types
apermo Feb 25, 2026
7d677fa
Remove return for media_send_to_editor() calls
apermo Feb 25, 2026
12a2bc5
Docs: Use array shape for perform_test() return type
apermo Feb 25, 2026
0a3e8f1
fix: Add default case to validate_file_to_edit() switch
apermo Feb 25, 2026
4013816
docs: Add @return never to media_send_to_editor()
apermo Feb 25, 2026
8f8ec70
fix: Use return null as default in validate_file_to_edit()
apermo Feb 26, 2026
ca8be53
Merge branch 'trunk' into trac-64702
desrosj Mar 19, 2026
919f39b
Merge branch 'trunk' into trac-64702
westonruter Mar 30, 2026
bfa4909
Remove never from unions
westonruter Mar 30, 2026
815886f
Add explicit return null to column_status(), wp_dashboard_quota(), an…
westonruter Mar 30, 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
12 changes: 9 additions & 3 deletions src/wp-admin/includes/class-wp-importer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,14 @@ public function get_imported_comments( $blog_id ) {
}

/**
* @param int $blog_id
* @return int|void
* Sets the blog to import to.
*
* Accepts a numeric blog ID or a URL string. When given a URL,
* the blog is looked up by domain and path. On multisite, switches
* to the resolved blog. Exits with an error if the blog cannot be found.
*
* @param int|string $blog_id Blog ID or URL.
* @return int|never Blog ID on success. Exits on failure.
*/
public function set_blog( $blog_id ) {
if ( is_numeric( $blog_id ) ) {
Expand Down Expand Up @@ -177,7 +183,7 @@ public function set_blog( $blog_id ) {

/**
* @param int $user_id
* @return int|void
* @return int|never
*/
public function set_user( $user_id ) {
if ( is_numeric( $user_id ) ) {
Expand Down
2 changes: 1 addition & 1 deletion src/wp-admin/includes/class-wp-privacy-requests-table.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,7 @@ public function column_cb( $item ) {
* @since 4.9.6
*
* @param WP_User_Request $item Item being shown.
* @return string|void Status column markup. Returns a string if no status is found,
* @return string|null Status column markup. Returns a string if no status is found,
Comment thread
apermo marked this conversation as resolved.
* otherwise it displays the markup.
*/
public function column_status( $item ) {
Expand Down
12 changes: 11 additions & 1 deletion src/wp-admin/includes/class-wp-site-health.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,17 @@ public function enqueue_scripts() {
* @since 5.4.0
*
* @param callable $callback
* @return mixed|void
* @return array{
* label: string,
* status: 'good'|'recommended'|'critical',
* badge: array{
* label: string,
* color: string,
* },
* description: string,
* actions: string,
* test: string,
* }
*/
private function perform_test( $callback ) {
/**
Expand Down
2 changes: 1 addition & 1 deletion src/wp-admin/includes/dashboard.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1648,7 +1648,7 @@ function wp_dashboard_primary_output( $widget_id, $feeds ) {
*
* @since 3.0.0
*
* @return true|void True if not multisite, user can't upload files, or the space check option is disabled.
* @return true|null True if not multisite, user can't upload files, or the space check option is disabled.
*/
function wp_dashboard_quota() {
if ( ! is_multisite() || ! current_user_can( 'upload_files' )
Expand Down
3 changes: 2 additions & 1 deletion src/wp-admin/includes/file.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,7 +734,7 @@
* @param string $file File the user is attempting to edit.
* @param string[] $allowed_files Optional. Array of allowed files to edit.
* `$file` must match an entry exactly.
* @return string|void Returns the file name on success, dies on failure.
* @return string|never Returns the file name on success, dies on failure.
Comment thread
apermo marked this conversation as resolved.
Outdated
*/
function validate_file_to_edit( $file, $allowed_files = array() ) {
$code = validate_file( $file, $allowed_files );
Expand All @@ -743,7 +743,7 @@
return $file;
}

switch ( $code ) {

Check warning on line 746 in src/wp-admin/includes/file.php

View workflow job for this annotation

GitHub Actions / PHP static analysis / Run PHP static analysis 

Function validate_file_to_edit() should return string but return statement is missing.
case 1:
wp_die( __( 'Sorry, that file cannot be edited.' ) );

Expand All @@ -751,6 +751,7 @@
// wp_die( __('Sorry, cannot call files with their real path.' ));

case 3:
default:
Copy link
Copy Markdown
Author

@apermo apermo Feb 25, 2026

Choose a reason for hiding this comment

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

@westonruter

The PHPStan warning on validate_file_to_edit() was caused by the switch statement only handling codes 1 and 3 from validate_file(), while code 2 (absolute Windows drive paths) falls through without returning or dying — breaking the string|never contract.

I added a default case that calls wp_die() with the same message, which closes the gap. But there's an argument for giving code 2 its own message (there's even a commented-out case for it). What do you think — keep the generic message via default, restore the dedicated case 2 with a distinct message or a new default message?

Copy link
Copy Markdown
Member

@westonruter westonruter Feb 25, 2026

Choose a reason for hiding this comment

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

Hummm. But I still see the check warning even with this change.

And in the case of case 2 isn't the current behavior (as the case was commented out), that the function in fact returns an implicit null? In that case, shouldn't default case be removed and a return null added to the end of this function?

Copy link
Copy Markdown
Author

@apermo apermo Feb 25, 2026

Choose a reason for hiding this comment

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

That would mimic the current behavior and at least optimize the phpdoc.

Another option could be to pull this function from the changeset and create a new issue just for it in order to define the correct behavior in all cases.

Your final thoughts?

Copy link
Copy Markdown
Member

@westonruter westonruter Feb 25, 2026

Choose a reason for hiding this comment

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

I think just adding return null and that type to @return would be fine. That leaves the behavior unchanged while also being fully typed. Not sure what would go in a new ticket?

Copy link
Copy Markdown
Author

@apermo apermo Feb 26, 2026

Choose a reason for hiding this comment

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

I thought that the behavior is wrong. And that it could be beneficial to discuss that in a bigger group.

Claudes deep analysis on the issue:

The changes date back to December 1, 2009 — committed by Ryan Boren in SVN r12310 (git 0042fa7), for WordPress 2.9.

DD32 (Dion Hulse) wrote the patch. Ryan Boren committed it with minor modifications.

Trac #11032: "Theme editor is not accessible":

The theme editor was broken on Windows systems (XAMPP). In WP 2.9, theme file paths became absolute (e.g. C:\xampp\htdocs\wordpress/wp-content/themes/default/style.css). The C: at position 1 triggered validate_file() returning code 2, which made validate_file_to_edit() die with "Sorry, can't call files with their real path."

DD32's fix had two key parts:

  1. Commented out case 2 in validate_file_to_edit() because absolute Windows paths were now the normal/expected input from the theme editor — dying on them was wrong.
  2. Reordered the checks in validate_file() — moved the allowed_files check (code 3) before the Windows drive path check (code 2), so that if a file is in the allowed list, it passes validation before the : check can reject it. DD32 explicitly noted:
    "Order in validate_file changed to increase security of theme edits while branch 2 is commented out (Else if it hit that condition, it'd pass right through without checking the allowed files)"

Ryan then also removed the stripslashes() call from validate_file_to_edit() and moved it to the callers.

The implication for your current discussion: Case 2 was commented out as a deliberate workaround for Windows absolute paths becoming normal input — not because "code 2 should silently pass." The validate_file() reorder ensures allowed_files is checked first, so code 2 is only reachable when there's no allowed_files list. In that scenario, a Windows drive path with no allowlist is suspicious input, and the default case calling wp_die() is the right fix — it restores the intended defense that DD32's workaround weakened. Adding return null would just formalize the gap DD32 was reluctantly creating.

Adding the return null; would basically cement the patched/possibly broken behavior.

In my opinion that commented out case 2 feels like quick and dirty bug fix from over 16 years ago, and it was done with a different mindset, while it never broke anything, it was never correct.

The intention was that it should return the file name on success and act as a gatekeeper and die if the path is not allowed.

validate_file_to_edit() is used twice throughout the code.

Number 1

wordpress-develop/src/wp-admin/plugin-editor.php

Lines 85 to 123 in ad87612

$file = validate_file_to_edit( $file, $plugin_files );
$real_file = WP_PLUGIN_DIR . '/' . $file;
$plugin_data = get_plugin_data( WP_PLUGIN_DIR . '/' . $plugin_files[0] );
$plugin_name = $plugin_data['Name'];
// Handle fallback editing of file when JavaScript is not available.
$edit_error = null;
$posted_content = null;
if ( 'POST' === $_SERVER['REQUEST_METHOD'] ) {
$edit_result = wp_edit_theme_plugin_file( wp_unslash( $_POST ) );
if ( is_wp_error( $edit_result ) ) {
$edit_error = $edit_result;
if ( check_ajax_referer( 'edit-plugin_' . $file, 'nonce', false ) && isset( $_POST['newcontent'] ) ) {
$posted_content = wp_unslash( $_POST['newcontent'] );
}
} else {
wp_redirect(
add_query_arg(
array(
'a' => 1, // This means "success" for some reason.
'plugin' => $plugin,
'file' => $file,
),
admin_url( 'plugin-editor.php' )
)
);
exit;
}
}
// List of allowable extensions.
$editable_extensions = wp_get_plugin_file_editable_extensions( $plugin );
if ( ! is_file( $real_file ) ) {
wp_die( sprintf( '<p>%s</p>', __( 'File does not exist! Please double check the name and try again.' ) ) );

So after not dying immediately in line 85, it will die in line 123 wp_die( sprintf( '<p>%s</p>', __( 'File does not exist! Please double check the name and try again.' ) ) ); since validate_file_to_edit() returned null.

Claude adds to this

Same story though — $plugin_files is always populated, so code 2 is unreachable there too. But if it were reached, $file would become null, and WP_PLUGIN_DIR . '/' . null would produce a bogus path.

Number 2

wordpress-develop/src/wp-admin/theme-editor.php

Line 117 in ad87612

validate_file_to_edit( $file, $allowed_files );

This one is a little more complex to see, so I have less personal and more Claudes view here:

In theme-editor.php, case 2 is actually harmless — and here's why:

The call at line 117 discards the return value:

validate_file_to_edit( $file, $allowed_files );

It's used purely as a gate: either the function dies, or execution continues with the original $file variable unchanged. The implicit null return is ignored.

But more importantly, case 2 can never be reached here in practice. Look at the call:

  1. $allowed_files is always populated (lines 80-99) — it's the theme's file list
  2. In validate_file(), the allowed_files check (code 3) runs before the Windows drive path check (code 2) — that was DD32's deliberate reorder in the same commit
  3. So a Windows absolute path like C:\xampp...\style.css that is in $allowed_files returns 0 (pass). One that isn't in $allowed_files returns 3 (dies on case 3) before the check is ever reached.

Code 2 is only reachable when $allowed_files is empty, which never happens in theme-editor.php.

Bottom line: The implicit null return from the commented-out case 2 has no real-world effect in either caller today because DD32's validate_file() reorder ensures code 2 is unreachable when an allowlist is provided. But the default case (using wp_die()) in your commit is still the right fix — it closes a theoretical gap for any future caller that passes an empty allowlist.

Long story short:

We can decide between preserving the exact runtime behavior, since it is a docs-focused changeset and possibly open a new issue for a deeper discussion as follow up, or changing the runtime behavior for a not existing edge case.

I'll commit your suggestion, if you follow my argument, you can just revert the commit and take the wp_die() approach.

Copy link
Copy Markdown
Member

@westonruter westonruter Feb 26, 2026

Choose a reason for hiding this comment

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

There's a lot of history here so I think any behavior change should be captured in a new ticket. Keeping the changes here strictly to documenting the existing behavior I think is the right move.

Copy link
Copy Markdown
Author

@apermo apermo Feb 26, 2026

Choose a reason for hiding this comment

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

I'll create a new ticket once you merged and take the research from this thread .

Comment thread
apermo marked this conversation as resolved.
Outdated
wp_die( __( 'Sorry, that file cannot be edited.' ) );
}
Comment thread
apermo marked this conversation as resolved.
}
Expand Down
7 changes: 4 additions & 3 deletions src/wp-admin/includes/media.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ function _cleanup_image_add_caption( $matches ) {
* @since 2.5.0
*
* @param string $html
* @return never
*/
function media_send_to_editor( $html ) {
?>
Expand Down Expand Up @@ -736,7 +737,7 @@ function get_upload_iframe_src( $type = null, $post_id = null, $tab = null ) {
*
* @since 2.5.0
*
* @return null|array|void Array of error messages keyed by attachment ID, null or void on success.
* @return null|array|never Array of error messages keyed by attachment ID, null on success, or exit.
*/
function media_upload_form_handler() {
check_admin_referer( 'media-form' );
Expand Down Expand Up @@ -857,7 +858,7 @@ function media_upload_form_handler() {
*/
$html = apply_filters( 'media_send_to_editor', $html, $send_id, $attachment );

return media_send_to_editor( $html );
media_send_to_editor( $html );
Comment thread
apermo marked this conversation as resolved.
}

return $errors;
Expand Down Expand Up @@ -959,7 +960,7 @@ function wp_media_upload_handler() {
$html = apply_filters( 'image_send_to_editor_url', $html, sanitize_url( $src ), $alt, $align );
}

return media_send_to_editor( $html );
media_send_to_editor( $html );
}

if ( isset( $_POST['save'] ) ) {
Expand Down
4 changes: 2 additions & 2 deletions src/wp-admin/includes/plugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1296,8 +1296,8 @@ function is_uninstallable_plugin( $plugin ) {
* @since 2.7.0
*
* @param string $plugin Path to the plugin file relative to the plugins directory.
* @return true|void True if a plugin's uninstall.php file has been found and included.
* Void otherwise.
* @return true|null True if a plugin's uninstall.php file has been found and included.
* Null otherwise.
*/
function uninstall_plugin( $plugin ) {
$file = plugin_basename( $plugin );
Expand Down
6 changes: 3 additions & 3 deletions src/wp-admin/includes/post.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -982,15 +982,15 @@ function wp_write_post() {
*
* @since 2.0.0
*
* @return int|void Post ID on success, void on failure.
* @return int|never Post ID on success. Dies on failure.
Comment thread
apermo marked this conversation as resolved.
Outdated
*/
function write_post() {
$result = wp_write_post();
if ( is_wp_error( $result ) ) {
wp_die( $result->get_error_message() );
} else {
return $result;
}

return $result;
}

//
Expand Down
19 changes: 10 additions & 9 deletions src/wp-includes/class-wp-admin-bar.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,24 +193,25 @@ final protected function _set_node( $args ) {
* @since 3.3.0
*
* @param string $id
* @return object|void Node.
* @return object|null Node.
*/
final public function get_node( $id ) {
$node = $this->_get_node( $id );
if ( $node ) {
return clone $node;
}
Comment thread
apermo marked this conversation as resolved.
return null;
}

/**
* @since 3.3.0
*
* @param string $id
* @return object|void
* @return object|null
*/
final protected function _get_node( $id ) {
if ( $this->bound ) {
return;
return null;
}

if ( empty( $id ) ) {
Expand All @@ -225,12 +226,12 @@ final protected function _get_node( $id ) {
/**
* @since 3.3.0
*
* @return array|void
* @return array|null
*/
final public function get_nodes() {
$nodes = $this->_get_nodes();
if ( ! $nodes ) {
return;
return null;
}

foreach ( $nodes as &$node ) {
Expand All @@ -242,11 +243,11 @@ final public function get_nodes() {
/**
* @since 3.3.0
*
* @return array|void
* @return array|null
*/
final protected function _get_nodes() {
if ( $this->bound ) {
return;
return null;
}

return $this->nodes;
Expand Down Expand Up @@ -307,11 +308,11 @@ public function render() {
/**
* @since 3.3.0
*
* @return object|void
* @return object|null
*/
final protected function _bind() {
if ( $this->bound ) {
return;
return null;
}

/*
Expand Down
Loading