Filesystem: Update constructor parameter default value to an empty array#11593
Open
Soean wants to merge 46 commits into
Open
Filesystem: Update constructor parameter default value to an empty array#11593Soean wants to merge 46 commits into
Soean wants to merge 46 commits into
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. |
Member
Author
|
Maybe we should also rename |
Member
|
@Soean I've added a commit to address a wide array of PHPStan issues. On composer phpstan -- -c phpstan.neon.dist --level=5 src/wp-admin/includes/class-wp-filesystem*With the commit, the only error left at rule level 5 is:
Now, clearly these changes could be out of scope here and could be reverted. However, by providing these additional types it is revealing possible bugs which were not previously detected. So maybe the scope should be increased to add the WP docs as well as the PHPStan docs for static analysis. |
westonruter
reviewed
Jun 9, 2026
westonruter
reviewed
Jun 9, 2026
westonruter
reviewed
Jun 9, 2026
westonruter
reviewed
Jun 9, 2026
Co-authored-by: Weston Ruter <westonruter@gmail.com>
Co-authored-by: Weston Ruter <westonruter@gmail.com>
Co-authored-by: Weston Ruter <westonruter@gmail.com>
Co-authored-by: Weston Ruter <westonruter@gmail.com>
Soean
commented
Jun 9, 2026
Co-authored-by: Sören Wünsch <soerenwrede@gmail.com>
Member
|
All errors up to rule level 9 have been fixed. What remains are the following rule level 10 errors: |
Member
|
And there we have it! All PHPStan rule level 10 errors addressed. |
There was a problem hiding this comment.
Pull request overview
This PR updates the WordPress filesystem transport classes to use array-based constructor options (instead of an empty string default), and improves inline documentation/static-analysis types for filesystem listings and connection options.
Changes:
- Update FTPext/ftpsockets/SSH2 constructors to accept
null/arrays and document the supported option shapes (including PHPStan types). - Refine filesystem API docblocks/return types (e.g.,
get_contents_array()element types; addFileListingPHPStan shape in the base class). - Adjust some filesystem behaviors/guards (e.g., additional connection/resource checks) and update
composer.jsonextension suggestions.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/wp-admin/includes/class-wp-filesystem-ssh2.php | Constructor/doc/type updates; options handling; connection/auth logic; dirlist metadata typing. |
| src/wp-admin/includes/class-wp-filesystem-ftpsockets.php | Constructor/doc/type updates; safer return typing; dirlist() handling adjustments. |
| src/wp-admin/includes/class-wp-filesystem-ftpext.php | Constructor/doc/type updates; options handling; additional connection checks and typing refinements. |
| src/wp-admin/includes/class-wp-filesystem-direct.php | Typing refinements for file listings and recursive chgrp/chown behavior. |
| src/wp-admin/includes/class-wp-filesystem-base.php | Introduces PHPStan FileListing shape and improves various docblocks/typing. |
| composer.json | Adds ext-ftp and ext-ssh2 to suggested extensions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+67
to
+72
| /** | ||
| * @since 7.1.0 | ||
| * @var array | ||
| * @phpstan-var Options | ||
| */ | ||
| public $options; |
Member
There was a problem hiding this comment.
👋 Claude Code here. Same situation as the FTPext class — intentional, and explained in full there: #11593 (comment). TL;DR: omitting the default is what lets the narrowed @phpstan-var Options type attach (a default fails PHPStan level 10), and the null case is unreachable because WP_Filesystem() bails on errors->has_errors() before any method reads $this->options.
Comment on lines
+32
to
+37
| /** | ||
| * @since 7.1.0 | ||
| * @var array | ||
| * @phpstan-var Options | ||
| */ | ||
| public $options; |
Member
There was a problem hiding this comment.
Not sure about this (and the other duplicate such review comments from Copilot).
The purpose here is to document the type, but it's true that $options could be null if an error occurs when initializing the class.
Member
There was a problem hiding this comment.
👋 Claude Code here, replying after digging into this.
This redeclaration intentionally omits the = array() default. The whole point of redeclaring $options in the subclass is to attach the narrowed @phpstan-var Options type, and there's no way to keep both a non-null default and the strict shape:
public $options = array();with@phpstan-var Optionsfails PHPStan level 10 withproperty.defaultValue("does not accept default value of typearray{}"), because the shape's keys are required.- Widening to
@phpstan-var Options|array{}makes the default valid but then regresses every unguarded read inconnect()($this->options['hostname'], etc.) with "offset might not exist onarray{}". This one slips pastphpstan-diff --changedbecause the reads are on unchanged lines, but a full level-10 run flags them. - A non-null constant default is impossible anyway, since
hostname/usernamearenon-empty-string— even''wouldn't satisfy the type.
On the runtime "offset on null" risk: $this->options is only null on an errored, never-successfully-constructed instance. WP_Filesystem() checks $wp_filesystem->errors->has_errors() after construction and bails before connect() (or anything else that reads $this->options) is ever called, so the null-key access isn't reachable through supported usage. The strict Options type correctly describes the property as the consuming methods see it.
Leaving this as-is.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…into wp_fs_type * 'trunk' of https://github.com/WordPress/wordpress-develop: General: Add support for unicode email addresses in is_email and sanitize_email Media: Make image editor help icon scheme-aware. Media: Fix filter toolbar spinner alignment. Twenty Twenty: Add missing documentation for some global variables.
…ets. Capture the current working directory before changing into the target path so the FTP session's working directory is restored, rather than capturing it afterward (which left the session in the target path and made is_dir() no longer side-effect-free). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A username is mandatory for SSH authentication regardless of method: ssh2_auth_pubkey_file() takes a required username argument, and the SSH protocol authenticates a named user even when a key proves identity. Previously the constructor only required a username for password auth, so constructing with keys but no username produced no error, and connect() would then silently skip authentication entirely. Validate the username unconditionally so this surfaces a clear "SSH2 username is required" error, and update the option type shapes to mark username as required. With the username guaranteed, drop the now-redundant isset() guards for it in connect(), along with the password isset/array_key_exists checks, using null coalescing to satisfy the option type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Restore the public-key authentication branch to a plain else. Since $this->keys is set true only when both public_key and private_key are provided, the previous `elseif ( isset( ... ) )` was always true when reached, and the unreachable third path meant connect() could fall through to ssh2_sftp() with no authentication attempted at all. Use null coalescing on the key paths to satisfy the option type shape, which marks public_key and private_key as optional; the empty-string fallbacks cannot occur at runtime, and would fail authentication safely rather than skipping it if they ever did. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ets. Handling the false return of the underlying ftp->dirlist() is correct, but combining the path-existence check with OR changed behavior: a populated listing was discarded whenever exists() disagreed (it probes via a separate nlist() call with documented quirks). Restore the original semantics of only erroring when the listing is empty AND the path does not exist, while still treating an outright false as a failure. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ails. The new is_string() guard around the cwd() result left $base at its incoming '.'/'' value when cwd() returns false (the FTP transports do this on failure), changing the directory search root. Trunk derived $base from trailingslashit( $this->cwd() ), which yielded '/' for a false cwd(). Preserve that behavior by falling back to '/' explicitly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
westonruter
reviewed
Jun 10, 2026
westonruter
reviewed
Jun 10, 2026
| * @type string $password Optional. SSH password. May be empty when using keys. | ||
| * @type string $public_key Optional. Path to public key file for publickey authentication. | ||
| * @type string $private_key Optional. Path to private key file for publickey authentication. | ||
| * @type array $hostkey Optional. Hostkey options passed to ssh2_connect. |
Member
There was a problem hiding this comment.
The hostkey isn't actually an arg. It is an option that is computed by the public_key and private_key.
Suggested change
| * @type array $hostkey Optional. Hostkey options passed to ssh2_connect. |
The FTP/SSH2 subclasses now assign $this->options only when the constructor records no errors, so a failed construction leaves $this->options null rather than the base class's array() default. Calling connect() directly on such an object (bypassing the has_errors() check WP_Filesystem() performs) would then access an offset on null. Guard each connect() with an early return when errors are present, matching the check in WP_Filesystem(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The FileListing shape and the dirlist() @type docs declared time as string|false, but the FTP transports build it from mktime()/strtotime() in parselisting(), i.e. an integer Unix timestamp, while Direct and SSH2 format it as a gmdate() string. Widen the shared shape to int|string|false and note in the docs that FTP transports return a timestamp, so the type reflects the actual data across transports. Also harden the ftpsockets dirlist() consumer to guard the untyped PemFTP ftp::dirlist() return with is_array() rather than a strict false check, so an unexpected non-array return is handled instead of trusted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
westonruter
approved these changes
Jun 11, 2026
westonruter
left a comment
westonruter
left a comment
Member
There was a problem hiding this comment.
@Soean aside from maybe adding some tests, I think this is good to go, if you concur.
Member
|
@Soean are you happy with this? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The constructor parameters of the
WP_Filesystem_FTPext,WP_Filesystem_ftpsocketsandWP_Filesystem_SSH2classes should be anarraynot an emptystring(''). We also should improve the doc block.Trac ticket: https://core.trac.wordpress.org/ticket/65409
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.