Upgrade/Install: Do not reuse an unconnected filesystem in WP_Upgrader::maintenance_mode(). - #13257
Conversation
…er::maintenance_mode()`. A failed `WP_Filesystem()` call leaves an unconnected object in the global, which the `! $wp_filesystem` guard accepted. Check for recorded errors as `fs_connect()` does. Props bejignesh, hatterops. See #65942.
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @hatterops. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
|
Thanks for picking it up and submitting a patch. Wanted to add some diagnostic detail from the investigation. the crashing $wp_filesystem object in the system had: is_object( $wp_filesystem ) → true Ruled out bad/missing FTP credentials specifically (valid FTP_HOST/FTP_USER/FTP_PASS, clean manual FTP login, OK LOGIN in vsftpd's log immediately before the crash) before landing on this. So in the case, connect() genuinely appears to have never been called at all on the object sitting in the global — not called-and-failed (which would leave errors populated, or link as false), just never invoked. Given that, the proposed guard: |
Trac ticket: https://core.trac.wordpress.org/ticket/65942
maintenance_mode()guards initialization with! $wp_filesystem, which treats any object in the global as usable.WP_Filesystem()assigns that global atfile.php:2213before connecting and returns false at:2226on a constructor error, so a failed call earlier in the request leaves an unconnected object behind.maintenance_mode()then callsabspath()on it, and under PHP 8 the transport fatals on a null link. It is called once perWP_Automatic_Updater::update(), so the whole update run aborts rather than the one item.This is the branch [60107] did not cover on the same function, and the check mirrors
fs_connect()in this class.Design decision worth flagging: the guard treats any recorded error as unusable.
WP_Filesystem_Base::$errorsis set in the constructor and never cleared, andWP_Filesystem_FTPext::connect()deliberately toleratesconnectandauthcodes so a failed attempt can be retried on the same instance. An instance that failed once and then connected successfully still carries those codes, so this guard will discard it and reinitialize. That costs a reconnect in a rare case, and I preferred it to inspecting transport-specific link properties from the caller.fs_connect()is stricter still and hard-fails on the same condition.Testing
Set
FS_METHODtoftpext, callWP_Filesystem()once with incomplete credentials, then runmaintenance_mode( true ). Before the change that fatals in the transport; after it, the existing "Could not access filesystem." notice path from [60107] handles it.