Remove the broken FileTransferFTPWrapper (file stream) class

An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: /authorize.php?batch=1&id=6&op=do StatusText: OK ResponseText: Warning: ftp_mkdir(): /sites/default/modules: No such file or directory in /includes/filetransfer/ftp.inc on line 129 Warning: Cannot modify header information - headers already sent by (output started at /includes/filetransfer/ftp.inc:129) in /bootstrap.inc on line 1003 {"status":true,"percentage":"100","message":"Completed 1 of 1.\u003cbr \/\u003e"}

I would imagine this beign a critical bug indeed, as otherwise we require another php exstention to exist.

It looks like prepareInstallDirectory doesn't create $directory (sites/all/modules) unless both the directory is missing and the parent directory is unwritable. But prepareInstallDirectory should also create the directory if the directory is missing and the parent directory is writable. I fixed this logic and I can no longer reproduce the error.

(I also replaced $install_location with $directory as mentioned by the OP).

The title for this issue should probably change to "prepareInstallDirectory doesn't create directory unless it's parent is unwritable", but as I'm not 100% sure I'm fixing the same problem brought up by the OP, I'm leaving it as-is.

I can no longer replicate this issue at HEAD, with or without the above patch.

In ftp.inc, I changed:

    if (function_exists('ftp_connect')) {
      $class = 'FileTransferFTPExtension';
    }
    elseif (ini_get('allow_url_fopen')) {
      $class = 'FileTransferFTPWrapper';
    }

to:

    if (ini_get('allow_url_fopen')) {
      $class = 'FileTransferFTPWrapper';
    }

Is that enough?

So what's the status of this? Already fixed due to other changes?

I can no longer make that landing page show up like that. There must be some other condition to reproduce the error.

In preparation for the new "major" issue priority, I'm tagging this (not actually critical) issue as priority-major.
See: http://drupal.org/node/669048#comment-3019824 (#669048: We should have four priorities not three (introduce a new 'major' priority)) for more information about the coming major issue priority.

PHP's FTP stream wrapper does not support chmod so you simply and absolutely _cannot_ use it for installing modules. This is why FileTransferFTPWrapper doesn't implement chmod. It can't. Why its even in Drupal is a mystery to me. FTP without chmod? Totally broken and useless.
Now sure it can create directories and files but the user will still be required to use an FTP client or their web hosts 'file manager' to set the appropriate permissions which defeats the whole purpose.

I propose to:
Not use the FTP stream wrapper for this and implement a pure PHP FTP client to use as an alternative for when PHP's FTP extension can't be used (Wordpress does this).

We have not added the raw socket based FTP implementation because it seemed a lot of work and it seemed since PHP5 you could do overwrites with the FTP stream wrapper and it seemed enough. Guess what? We were wrong. Happens. We should write / borrow from Joomla a socket based raw FTP implementation. Sucks!

To elaborate on my "FTP without chmod? Totally broken and useless." remark:

In order for PHP's FTP stream wrapper to work for web browser based module installation you must be able to chmod the files it transfers in order for them to be read by Drupal. Most servers don't set the umask for new files to be world readable and world executable for directories. This is done for security reasons. For example say you have a private directory in your home directory called 'private'. You don't want files in this directory to be readable by everyone when you upload them via FTP. And an FTP client that doesn't support chmod is in my opinion broken and not very useful -- because I feel it is a critical feature.

I'll assign this issue to myself since I already have a pure PHP FTP client API implementation. My FTP class API mirrors PHP's FTP extension functions for the most part (function names, parameters and return values). There is also an option to return messages sent by the FTP server on failure instead of returning FALSE (so you can find out exactly why something failed). I don't know if I can get it worthy for Drupal but I'll try. Lots of work to do.

Ok, finally, here is the patch I said I'd make. I held off on it for awhile.

The replaced FTP wrapper class uses the same code as used in the FTP extension class except that it calls methods in a new FTP class provided in the patch. There is an equivalent method for most of FTP extension's functions (the most important ones anyway). I'll provide an exact list of FTP functions supported in another post.

Nice code comments!
Found a lot of trailing white spaces, you better find a tool or script to remove them, if you gonna do it manual here is a list with the problems I found!

+++ includes/filetransfer/drupalftp.inc
@@ -0,0 +1,1606 @@
+  /** ¶
+   * A status flag to indicate a connection was established.
+   *

trailing white space

(repeat review issue 12x)

@aspilicious: 1) Dreditor allows to annotate multiple code line selections. 2) In such cases, it's far more helpful to just clarify that you're facing the same review issue all over again in the patch, e.g., by using a "(and elsewhere)" prefix, or whatever else makes sense for you. But anyway, thanks for reviewing! :)

Sun should I delete most of the lines to keep things readable?

lol you could've just said 'there's a ton of white spaces' , but thanks for taking the time to go thru all that wow.

feel free to nitpick, and I kinda suck at writing documentation so if anything doesn't make sense please let me know.

so this is both needs work and needs review I guess.

Listed are the FTP extension functions and equivalent methods implemented by the class. Method parameters and return values are also equivalent for the most part, and any differences are noted.

• ftp_alloc()

Not implemented. Allocates space for a file upload. Most servers don't support this.

• ftp_cdup()

DrupalFTP::cdup()

• ftp_chdir()

DrupalFTP::chdir()

• ftp_chmod()

DrupalFTP::chmod()

• ftp_close()

DrupalFTP::logout()

• ftp_connect()

DrupalFTP::connect() does not take a $timeout parameter as ftp_connect() does, instead the timeout is set as an option passed into the class constructor or DrupalFTP::setOptions().

• ftp_delete()

DrupalFTP::delete()

• ftp_exec()

Not implemented. I don't think most FTP servers support this.

• ftp_fget() and ftp_fput()

Not implemented. Same as ftp_get() and ftp_put() except it takes an open file to save to when downloading, or read from when uploading.

• ftp_get()

DrupalFTP::get()

• ftp_login()

DrupalFTP::login()

• ftp_mdtm()

Not implemented (yet). Gets a file's last modification time.

• ftp_mkdir()

DrupalFTP::mkdir()

• ftp_nb_*()

Not implemented. These are the extension's non blocking functions. Since stream_select() is broken for early PHP 5.2 versions the class can't reliability support non blocking IO.

• ftp_nlist()

DrupalFTP::nlist()

• ftp_pasv()

This tells the extension to use passive mode instead of active mode. The class always uses passive mode so there is no equivalent method.

• ftp_put()

DrupalFTP::put()

• ftp_pwd()

DrupalFTP::pwd()

• ftp_quit()

DrupalFTP::logout()

• ftp_raw()

Not implemented. From the PHP doc, this function: sends an arbitrary command to an FTP server.

• ftp_rawlist()

DrupalFTP::rawList() -- an unparsed directory listing.
Class also provides DrupalFTP::getList() and DrupalFTP::stat() (better to use if the server supports it) which return parsed directory listings.

• ftp_rename()

DrupalFTP::rename()

• ftp_rmdir()

DrupalFTP::rmdir()

• ftp_site()

DrupalFTP::site()

• ftp_size()

DrupalFTP::size()

• ftp_ssl_connect()

DrupalFTP::connect has a $secure parameter which turns on SSL for the control connection only.
DrupalFTP::sslEnableData() turns on SSL encryption for data connections. These are separate because data transfers may not work when encrypted due to FTP server configuration.

• ftp_systype()

Not implemented (yet). Gets the FTP server's system type (Unix, Windows, etc). Don't think it'll have much use. One use for it would be in parsing a directory listing, since Windows has a different format than Unix, however the format used is really FTP server specific and not system specific. So going by the system type command likely won't be reliable.

Btw I haven't looked into writing tests for this and I'm not sure if it's even possible. It's possible if

a) The test servers all have an FTP server with the proper setup.
b) Every FTP server has the same username & password associated with the Drupal installation, or some way to get that information.
c) To test SSL, each FTP server has to be configured for SSL.

With that said, I've tested the class extensively myself, each and every method, with several FTP servers on Win and Unix.

In order to test this patch you need to enable the Update Manager module and patch your Drupal with the patch over at http://drupal.org/node/747252#comment-3188930

You'll also need to disable the FTP extension. Or hack includes/filetransfer/ftp.inc by changing FileTransferFTPExtension with FileTransferDrupalFTP (the easiest way, assuming stream_socket_client isn't being disallowed) on line 31.

 if (function_exists('ftp_connect')) {
      $class = 'FileTransferFTPExtension';
    }
    elseif (function_exists('stream_socket_client')) {
      $class = 'FileTransferDrupalFTP';
    }

After doing this just head on over to the 'Install a module' link or whatever it says at the top of the modules page.

And I'm not going to risk any further time on this patch until someone steps in and says it will be considered as a fix for this issue as I've already spent a lot of time documenting this just for Drupal.

Changing to major as per tag.

ooo I forgot this was tagged as major.

Since Drupal is close to a beta release and fixing this requires an API change/addition (either by removing the FTP stream wrapper, or by adding a pure-PHP FTP API), I don't see a fix for this getting into 7.

This seems to be an important issue. Let's discuss possibilities for D7 a bit more.

Can't we test whether the FTP stream wrapper is available, and if not, dynamically load the proposed drop-in replacement?

Tag update

@sun

Did you mean provide a drop-in replacement for when the FTP extension isn't available? My patch does this.

Let me provide a brief overview:

The FTP stream wrapper doesn't support chmod (PHP limitation) which is really important for FTP (especially for web files) and the reason this issue exists.

The patch I provided gives Drupal a pure PHP FTP drop-in replacement for when the FTP extension isn't available. Both Joomla and Wordpress do this too. chx told me that they were going to write a pure PHP FTP implementation but they thought the FTP stream wrapper would suffice, which turned out to be wrong as he noted in #16.

Here's the current flow (does not take SSH into account, most users won't be able to use that):

If SuPHP is enabled PHP will be executed as the user's ID so Drupal will transfer the files locally using the FileTransferLocal class. SuPHP is enabled on many web hosts (particularly on shared hosting), but there's also many web hosts that do not use it (especially people with VPSs, dedicated servers).

If there's no SuPHP, it checks for the FTP extension, if that isn't enabled, it tries to use the FTP stream wrapper. Again there's many web hosts which have the FTP extension and many that do not. For those without the extension, it's broken.

So far dww and bohjan have been bitten by this, and surely there will be many many more people affected. My guess is that it's going to affect around 20-30% of users, and that's not a very small number to me.

Sadly, many hosting providers still don't provide any means of SSH and just FTP. Oddly enough, at the same time, PHP's FTP extension is rarely seen. Based on my gut feeling, those percentage values are much higher. I suspect that the on-site module installer/updater won't be usable for many users without this patch. At the same time, I don't know of any contributed module that would be affected by this API addition (minimal change).

Thanks for your input sun. It would be great if webchick or dries chimed in to let us know if they support this API addition. I found that I could write tests for the class, but it's going to eat lots of time so I'd rather write them after I know the class is going to get used by Drupal in some way, or maybe as a contrib module. In the meantime I'll fixup the whitespaces.

It's really hard to say how many users are going to be affected, but you're probably right in that the numbers are going to be higher than 20-30%. If that's the case then this is really critical (even if only 20% I think) since the major priority implies this only affects a small number of users.

Hopefully fixed all the white spaces. Added equivalent methods for the PHP extension's ftp_systype and ftp_mdtm functions and tweaked some areas of the documentation.

fixed a comment and white spaces.

Marking critical per #31 / #32

Back to major priority as it was for a while (using the tag)

Hosting providers that only provide ftp are forcing their clients to send their passwords in plain text. This is a host you want to move off of anyway. I'll bet that most of them default to php4 which won't run D7 anyway. Basically, we could even ship without FTP support. We don't want to, but thats a different matter.

moshe, let's not forget that most FTP servers nowadays support SSL, and there are web hosts that do provide this as enabling it is just a matter of an SSL certificate and a slight configuration tweak. Most web hosts provide alternative means to manage files such as via a control panel too. In fact it is far more likely that a web host supports FTP-SSL plus the PHP openSSL extension than support for SCP _and_ the SSH PHP extension.

In any case, if the web host doesn't use suPHP then Drupal will need SCP access plus the PHP SSH extension in order to login securely. Frankly I don't know why FTP-SSL support wasn't added when the updator class was written if security was in mind whatsoever. Adding the code for this is trivial and minimal and something I considered doing, however, this issue has been waiting for committer feedback for over a month and it's getting later everyday so I'm not really interested in messing with this further.

So we need a patch that simply removes the FTP stream wrapper class since it won't work for reasons cited in above comments. Simple patch, perfect for someone to get their feet wet.

Thank you very much dww for replying :)

I do think it would be a good idea to implement FTP-SSL. Turning it on with the PHP FTP extension is very simple, although before choosing to use it you'd want to check to ensure the server really does support SSL so it can fallback to plain-old FTP, that would probably be the bulk of the code, but it doesn't need much. On the other hand if we're relying on the FTP extension and it is true that most web hosts don't have it installed, then it is probably going to be largely a wasted effort.

With a pure PHP FTP implemention you don't need any FTP extension and you only need OpenSSL to use FTP-SSL, so it would be usable by a much much greater number of users, especially for newcomers that are probably gonna want to use it.

Sad but true

Committed to CVS HEAD. Thanks.