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.

[Denis-de-Bernardy]

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

[Denis-de-Bernardy]

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.

[Denis-de-Bernardy]

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-Bernardy]

new patch also fixes the ftp_chdir() issue

[dd32]

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.

[Denis-de-Bernardy]

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

[ryan]

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

[Denis-de-Bernardy]

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.

[dd32]

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.

[dd32]

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.

[dd32]

Can we get the latest patch here into trunk?

[hakre]

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

[hakre]

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

[ryan]

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

[dd32]

I'll leave commit to dd32.

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

[dd32]

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