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

[Denis-de-Bernardy]

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.

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

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.

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

new patch also fixes the ftp_chdir() issue

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.

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

[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.

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.

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.

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.

Can we get the latest patch here into trunk?

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

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

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

I'll leave commit to dd32.

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

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