WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 3 weeks ago

#25817 closed defect (bug) (fixed)

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

Reported by: markjaquith Owned by: dd32
Milestone: 3.7.2 Priority: high
Severity: major Version: 3.7
Component: Upgrade/Install Keywords: has-patch commit fixed-major
Focuses: Cc:

Description

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.

Attachments (3)

25817.patch (690 bytes) - added by SergeyBiryukov 6 months ago.
25817.2.patch (783 bytes) - added by SergeyBiryukov 6 months ago.
25817.diff (1009 bytes) - added by dd32 5 months ago.

Download all attachments as: .zip

Change History (11)

SergeyBiryukov6 months ago

comment:1 SergeyBiryukov6 months ago

  • Keywords has-patch added

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.

comment:2 follow-up: dd326 months ago

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 .

SergeyBiryukov6 months ago

comment:3 in reply to: ↑ 2 ; follow-up: SergeyBiryukov6 months ago

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.

comment:4 in reply to: ↑ 3 dd326 months ago

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.

dd325 months ago

comment:5 dd325 months ago

  • Keywords needs-testing added

25817.diff is probably the minimum we could do here

comment:6 dd325 months ago

  • Owner set to dd32
  • Resolution set to fixed
  • Status changed from new to closed

In 26148:

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

comment:7 dd325 months ago

  • Keywords commit fixed-major added; needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:8 nacin3 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.