WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#10889 closed defect (bug) (fixed)

filesystem's put_content() methods have inconsistent arguments -- causes .maintenance file to not be created

Reported by: Denis-de-Bernardy Owned by: dd32
Milestone: 3.0 Priority: normal
Severity: normal Version: 2.8.4
Component: Filesystem API Keywords: has-patch bug-hunt
Focuses: Cc:

Description

I ran into this odd bug in my theme's custom.css editor -- the file wasn't being created. Un-silencing the FTP transport's call to ftp_fput() revealed:

Warning: ftp_fput() [function.ftp-fput]: Mode must be FTP_ASCII or FTP_BINARY in /path/to/public_html/wp-admin/includes/class-wp-filesystem-ftpext.php on line 125

Basically, I had mindlessly used the same signature as other places where it's used in WP, i.e.:

$wp_filesystem->put_contents($maintenance_file, $maintenance_string, FS_CHMOD_FILE);
$wp_filesystem->put_contents($file, $maintenance_string, FS_CHMOD_FILE);

but with the FTP transport and others, those two should actually read as:

$wp_filesystem->put_contents($file, $maintenance_string);

else, the .maintenance files are not created at all.

Attachments (3)

10889.diff (2.7 KB) - added by Denis-de-Bernardy 5 years ago.
10889.2.diff (4.2 KB) - added by dd32 4 years ago.
10889.3.diff (4.5 KB) - added by dd32 4 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Denis-de-Bernardy5 years ago

It's also returning the following issue:

Warning: ftp_fput() [function.ftp-fput]: Rename/move failure: No such file or directory in /home/yourau5c/public_html/wp-admin/includes/class-wp-filesystem-ftpext.php on line 125

Seems related to this:

http://www.php.net/manual/en/function.ftp-fput.php#86872

comment:2 Denis-de-Bernardy5 years ago

  • Keywords has-patch added

to reproduce the bug, use something like:

$wp_filesystem->put_contents(WP_CONTENT_DIR . '/foo.txt', 'bar');

Patch is against WP 2.8.4, and is tested with both of the FTP-based file extensions. There might be a similar change needed for the SSH-based file extension.

comment:3 Denis-de-Bernardy5 years ago

mmm, there actually is another issue:

Warning: ftp_chdir() expects exactly 2 parameters, 1 given in /home/yourau5c/public_html/wp-admin/includes/class-wp-filesystem-ftpext.php on line 137

Denis-de-Bernardy5 years ago

comment:4 Denis-de-Bernardy5 years ago

  • Keywords needs-testing added

new patch also fixes the ftp_chdir() issue

comment:5 dd325 years ago

  • Keywords needs-patch added; has-patch needs-testing removed

IMO, The items should be changed to the same signature as the Direct class.. Ie. ($file, $contents, $chmod = null, $filetype = null)


As for that additional code you've added, I think its because you're using the class in a way that isnt expected.

you do NOT pass a local path to the filesystem classes, You pass a remote path.

ie. ABSPATH = /home/blah/wordpress/ Root in FTP = /wordpress/ All content should be referred to as /wordpress/blahblahblah

Use $wp_filesystem->abspath() and $wp_filesystem->wp_content_dir() instead, As the code in WordPress should already be using.

The additional folder lookups are not worth it, Its incredibly IO/FTP hungry using that function, It should be avoided as much as possible, FTP isnt the most efficient protocol for lots of lookups.

comment:6 Denis-de-Bernardy5 years ago

this one needs a patch no matter what:

Warning: ftp_chdir() expects exactly 2 parameters, 1 given in /home/yourau5c/public_html/wp-admin/includes/class-wp-filesystem-ftpext.php on line 137

comment:7 follow-up: ryan4 years ago

  • Milestone changed from 2.9 to 3.0

[12369] for the ftp_chdir() part. I'm not sure where the discussion is on the remainder of the patch. Postponing to 3.0 for the remainder.

comment:8 in reply to: ↑ 7 Denis-de-Bernardy4 years ago

Replying to ryan:
I'm not sure where the discussion is on the remainder of the patch. Postponing to 3.0 for the remainder.

Basically, the put_contents() method has inconsistent arguments. Depending on the FS used, it succeeds or fails. When it fails, it leaves a site that is getting upgraded without a .maintenance file.

These two:

$wp_filesystem->put_contents($file, $maintenance_string, FS_CHMOD_FILE);
$wp_filesystem->put_contents($maintenance_file, $maintenance_string, FS_CHMOD_FILE); 

Should be changed accordingly. Or the put_contents() method should be fixed.

comment:9 dd324 years ago

Darn, forgot about this ticket.

I'm not sure where the discussion is on the remainder of the patch.

Denis is correct, the put_contents() function signature varies between classes, causing some upgrades to proceed without a .maintainence file. I'll take a look at it now and post a patch, You can decide if it should still wait for 3.0.

Attached patch also fixes the "fix" that was used elsewhere, since put_contents() didnt do the chmod correctly, another call to chmod was made. At present, on the Direct transport, this means Chmod() will be called twice.

dd324 years ago

comment:10 dd324 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

Ah.. Just noticed $type is used for a fopen type name in direct, and Ascii/Binary in FTP, and not used at all for ssh, and not needed in any of the cases.

Personally, I'm all for just removing the $type param all together. Its not used by core, and cant be used by plugins due to it doing who-knows-what depending on the setup.

3rd patch on this ticket does just that, removes $type all together. which i'd prefer over the 2nd patch.

So:

  • for 2.9, If you're happy removing a unuseable buggy param, 3rd patch.
  • If you'd rather just make them all act "the same" to core, the 2nd patch.
  • Or just wait until 3.0 and use the 3rd patch.

dd324 years ago

comment:11 dd324 years ago

  • Keywords dev-feedback removed

Can we get the latest patch here into trunk?

comment:12 hakre4 years ago

+1 for getting the function parameters consistent between all classes in the family.

comment:13 hakre4 years ago

(if the testserver runs with php5, the reflection classes could be used to verify that)

comment:14 ryan4 years ago

Looks fine here. I'll leave commit to dd32.

comment:15 dd324 years ago

  • Status changed from new to accepted

I'll leave commit to dd32.

I'll recheck tonight to see if anythings changed since this patch.

comment:16 dd324 years ago

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

(In [12723]) Standardise WP_Filesystem_*::put_contents() arguments to support chmod reliably across all transports. Fixes #10889

comment:17 Denis-de-Bernardy4 years ago

  • Keywords bug-hunt added
Note: See TracTickets for help on using tickets.