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

[apermo]

Co-Authored-By: xateman

Trac ticket: https://core.trac.wordpress.org/ticket/64702

Use of AI Tools

Used AI for research, documentation and for the replacements. Everything was reviewed by myself and @xateman before opening this PR.

---

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.

[github-actions]

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props apermo, xate, mukesh27, westonruter.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[xateman]

Looks good!

[github-actions]

Test using WordPress Playground

The 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

• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[westonruter]

LGTM

[apermo]

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

[westonruter]

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?

[apermo]

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?

[westonruter]

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?

[apermo]

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.

[westonruter]

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.

[apermo]

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

[Copilot]

Pull request overview

Updates WordPress Administration-related PHPDoc return types by replacing void in union types with null/never, and aligns a few implementations with the documented behavior to improve static analysis and clarity.

Changes:

• Replace object|void / array|void style return unions with object|null / array|null and add explicit return null in a few places.
• Document “non-returning” paths using @return never and remove redundant return statements before exit.
• Improve/clarify return documentation (including an array-shape return) for a few admin-facing helpers.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/wp-includes/class-wp-admin-bar.php	Documents node getters as returning null instead of void and adds explicit null returns.
src/wp-admin/includes/post.php	Updates write_post() return docs and simplifies control flow.
src/wp-admin/includes/plugin.php	Updates uninstall_plugin() docblock to use null instead of void.
src/wp-admin/includes/media.php	Documents media_send_to_editor() as never and removes redundant return calls where it exits.
src/wp-admin/includes/file.php	Updates validate_file_to_edit() return docs and adds an explicit return null.
src/wp-admin/includes/dashboard.php	Updates wp_dashboard_quota() return docs to use null instead of void.
src/wp-admin/includes/class-wp-site-health.php	Replaces `mixed
src/wp-admin/includes/class-wp-privacy-requests-table.php	Updates column_status() return docs to use null instead of void.
src/wp-admin/includes/class-wp-importer.php	Expands documentation and updates return types to use never where the method exits.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The updated docblock for write_post() says it "dies on failure", but wp_write_post() can return 0 (see the empty( $post_id ) branch in the same file) and write_post() will pass that 0 back to callers without calling wp_die(). Please adjust the description/return type to document the possible 0 return (or change the implementation to treat 0 as a failure if that’s the intent).

Suggested change

	* @return int|never Post ID on success. Dies on failure.
	* @return int|0|never Post ID on success, 0 on failure that is not represented as WP_Error. Dies on WP_Error failures.

[westonruter]

Humm. 0 is an int

[apermo]

But a special int.

Joke aside, I made Claude review all core

That pattern (int|0) doesn't make sense as a PHPDoc type anyway — 0 isn't a type, it's a value

But none of them use 0 as a literal type in the union (like int|0|WP_Error). They all use int|WP_Error as the type and explain the 0 value in the description text.

The int|0|never pattern from your question doesn't exist anywhere in core. Literal value types like 0 in @return unions aren't a convention WordPress follows — the only instance is @return -1|0|1 in the vendored SimplePie library, which is a
spaceship-operator-style comparison return.

So I'd say this falls under AI hallucination.

[westonruter]

Well, using a literal for a type is okay, and PHPStan supports that. But it doesn't make sense when int is a superset of 0.

[apermo]

And according to the research through core it was never done besides the vendored library. And -1|0|1 is cherry picking 3 ints, vs int|0 is saying all integers and a single one from all integers. So thats a huge difference.