PHP Fatal error on background update: $wp_filesystem is not an object

[markjaquith]

A bunch of my sites experienced the following PHP fatal error when attempting to update to 3.7.1 in the background:

PHP Fatal error: Call to a member function abspath() on a non-object in /srv/www/example.com/releases/20131030050216/wp/wp-admin/includes/class-wp-upgrader.php on line 1339

These sites will update manually (on the server) without any problem. Indeed, another site on the same server, but with the filesystem method forced via filter to 'direct' auto updated without a problem.

I think this is related to an issue whereby during manual updates, the FTP info will pop up momentarily before it proceeds with direct file writes.

One thing to take into account: this uses Capistrano deploys, so the docroot is/srv/www/example.com/current, which is a symlink to the docroot path seen above. I don't know if that is somehow resulting in problems.

[SergeyBiryukov]

In case of an error, WP_Upgrader::fs_connect() returns either false (before $wp_filesystem global is set) or a WP_Error object: tags/3.7.1/src/wp-admin/includes/class-wp-upgrader.php#L62​.

In WP_Upgrader::run(), we check for both: tags/3.7.1/src/wp-admin/includes/class-wp-upgrader.php#L313​.

In Plugin_Upgrader::bulk_upgrade(), Theme_Upgrader::bulk_upgrade(), and Language_Pack_Upgrader::bulk_upgrade(), we check for false before calling $this->run():
tags/3.7.1/src/wp-admin/includes/class-wp-upgrader.php#L536​
tags/3.7.1/src/wp-admin/includes/class-wp-upgrader.php#L921​
tags/3.7.1/src/wp-admin/includes/class-wp-upgrader.php#L1180​

In Core_Upgrader::upgrade(), however, we only check for is_wp_error():
tags/3.7.1/src/wp-admin/includes/class-wp-upgrader.php#L1335​

I guess we should check for false as well.

[dd32]

Correct fix is to check for false, like 25817.patch​ to avoid the Fatal.

However, WP_Automatic_Updater doesn't expect a false return either and may fail further down the stack somewhere in the notify emails/etc. as they expect true or WP_Error.

I've had a few days to think about how this is possible to trigger, and I can't think of a easy way to trigger it.
There are several ways:

1. Have a filter on filesystem_method that is removed between Background updates triggering, and Core attempting an upgrade
2. Have disk IO issues where get_filesystem_method() returns Direct on the first call, but FTP on the Core update check
3. Have an incorrect password defined in FTP_PASS (probably the easiest way)
4. Have WP_Filesystem() not be able to include the Class, or the FTP Server timeout.

So I'm not sure how Mark triggered this, but would love to know if it's reproducible .

[SergeyBiryukov]

Replying to dd32:

However, WP_Automatic_Updater doesn't expect a false return either and may fail further down the stack somewhere in the notify emails/etc. as they expect true or WP_Error.

25817.2.patch​ makes sure Core_Upgrader::upgrade() returns WP_Error if fs_connect() failed.

[dd32]

Replying to SergeyBiryukov:

Replying to dd32:

However, WP_Automatic_Updater doesn't expect a false return either and may fail further down the stack somewhere in the notify emails/etc. as they expect true or WP_Error.

25817.2.patch​ makes sure Core_Upgrader::upgrade() returns WP_Error if fs_connect() failed.

That just makes it differ from Plugin_Upgrader & Theme_Upgrader though, both of which return false in these events too, and is arguably correct for them to do so.

We should just make sure that Core_Upgrader returns the false without fataling, and that Automatic_Updater can handle a false return value from Core_Upgrader, Plugin_Upgrader, or Theme_Upgrader.

[dd32]

25817.diff​ is probably the minimum we could do here

[dd32]

In 26148:

Background Updates: Fix a PHP Fatal error which could be encountered on some systems when using FTP. Fixes #25817 for trunk.

[nacin]

In 27881:

Background Updates: Fix a PHP fatal error which could be encountered on some systems when using FTP.

Merges [26148] from 3.8 to the 3.7 branch.

props dd32.
fixes #25817.