The origin of this is kinda crazy, and not worth mentioning, but basically, for the end user, they have no need to see both FTP options.

They should see FTP and SSH. And FTP should be the ftp extension is available, and the stream wrapper if not.

Updating your site | My Site

CommentFileSizeAuthor
#8 602520-8.ftp_one_option.patch8.8 KBdww
#8 602520-8.ftp_one_option.interdiff.txt3.47 KBdww
#4 ftp_one_option-602520-4.patch7.81 KBJacobSingh

Comments

dww’s picture

dww
we/he/they
commented

Yup, that's stupid alright. ;)

dww’s picture

dww
we/he/they
commented
Category: task » bug
Priority: Normal » Critical

Based on an IRC chat, this is a release-blocking UX bug as far as webchick and Dries are concerned...

dww’s picture

dww
we/he/they
commented
Issue tags: +Usability

Tagging for UX folks...

JacobSingh’s picture

JacobSingh commented
StatusFileSize
new ftp_one_option-602520-4.patch7.81 KB

This patch puts it back to using a factory to figure out which FTP class is appropriate. I didn't cut a screen because the only front-end difference is that the dropdown says "FTP" and "SSH" now on authorize.php.

If/When we add a fsock based FTP client, we would also inherit the base class.

I also snuck in an improvement to the FileTransfer classes, an interface for canChangeFilePermissions. It's not really implemented too much in our client code, but it is a good abstraction, so now if someone calls ->chmod() on a FileTransfer which can't do it, it will throw an exception. Also, client code can be intelligent about it and not try if it is not available by using class_implements().

JacobSingh’s picture

JacobSingh commented
Status: Active » Needs review
dries’s picture

dries commented
Status: Needs review » Needs work 
+++ includes/filetransfer/filetransfer.inc
@@ -314,4 +315,23 @@ class FileTransferException extends Exception {
+ * A FileTransfer Class implementing this interface cna be used to chmod files.

Typos in this line.

+++ includes/filetransfer/filetransfer.inc
@@ -314,4 +315,23 @@ class FileTransferException extends Exception {
+interface canChangeFilePermissions {

This is a really odd interface name. At a minimum, interfaces should start with capital letter.

I don't understand why this is a separate interface.

+++ includes/filetransfer/ftp.inc
@@ -13,6 +13,33 @@ class FileTransferFTPWrapper extends FileTransfer {
+    switch (true) {

Eh?

JacobSingh’s picture

JacobSingh commented

re: the interface name, I don't know why I gave it a lower case... just blanked I guess, for some reason it looked good :)

The name is standard format AFAIK (adjectival form) and the use case is pretty much the pure use case for interfaces. FileTransferChmodInterface is @dww's suggestion. There are varying opinnions out there, I don't care much either way.

http://java.sun.com/docs/books/tutorial/java/IandI/createinterface.html
"public class OperateBMW760i implements OperateCar"
"public class RectanglePlus implements Relatable"

http://msdn.microsoft.com/en-us/library/8bc1fexb%28VS.71%29.aspx
"Name interfaces with nouns or noun phrases, or adjectives that describe behavior. For example, the interface name IComponent uses a descriptive noun. The interface name ICustomAttributeProvider uses a noun phrase. The name IPersistable uses an adjective."

Why it makes sense
This is more important.
We have an abstract class which requires its implementers to do x,y and z. In this case ->copy, ->remove, etc..

We have some (most) child classes which can also chmod, but not all of them. So adding it to the base class is a bad idea. Creating an interface allows a call site to only show a user a list of transfer mechanisms which will allow for a permission change. Otherwise, they show all, the operation runs, and then we throw an exception that we don't support chmod for this type and the user says "wtf... You told me it would work"

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
StatusFileSize
new 602520-8.ftp_one_option.interdiff.txt3.47 KB
new 602520-8.ftp_one_option.patch8.8 KB

- Renamed the interface to FileTransferChmodInterface. This seems to more closely adhere to our existing Interface standards, such as they are. Here are the existing Interfaces in core that I found with a bit of grep:

ArchiverInterface
DrupalCacheInterface
DrupalEntityControllerInterface
MailSystemInterface
StreamWrapperInterface
DrupalStreamWrapperInterface
StreamWrapperInterface
DrupalUpdaterInterface
DatabaseStatementInterface
QueryConditionInterface
QueryAlterableInterface
QueryPlaceholderInterface
QueryExtendableInterface
SelectQueryInterfaceextends
DrupalStreamWrapperInterface
DrupalQueueInterface

- Fixed the code comments and typos (there was a FileTrasnferException in the previous patch, too).

- Changed switch (true) into an if() {} elseif() {} else {}. Also, we now throw a FileTransferException if the FTP factory can't find any possible FTP backends to use.

- Cleaned up a bit of PHPDoc comments.

- I hope comment #7 clarifies why we need a separate interface. Not all of our FileTransfer implementations support chmod(). Defining this as a separate interface that various FileTransfer subclasses can either implement or not gives us a clean way of indicating which classes support chmod() and which don't. Callers can now discover this ahead of time, instead of relying on wonky Exception handling.

New patch and interdiff from #4 attached...

dww’s picture

dww
we/he/they
commented
Assigned: Unassigned » dww
Status: Needs review » Needs work

Tee hee, upon testing, fatal errors when you try to update your site. ;) I'm re-rolling...

JacobSingh’s picture

JacobSingh commented
Assigned: dww » Unassigned
Status: Needs work » Reviewed & tested by the community

Note that because of the class autoloader, you need to clear your cache before attempting to test this.

dww’s picture

dww
we/he/they
commented

Right, clearing the cache works, but further testing revealed the following bugs:

#612546: Remove the broken FileTransferFTPWrapper (file stream) class
#612548: FileTransferFTPWrapper can't handle passwords with special characters

Upon closer consideration, both of those are really unrelated to this issue (the factory itself is working great) so I submitted them separately. Leaving this one RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Awesome!

My only concern with this patch (apart from some PHPDoc style issues, but those are just copy/pasted code) is this:

+    if (!in_array('FileTransferChmodInterface', class_implements(get_class($this)))) {
+      throw new FileTransferException('Unable to change file permissions');
+    }

...feels an awful lot like babysitting code to me. However, I talked this over with Jacob and he pointed out that it's either this or a WSOD with no clue how to resolve. The exception is more graceful since it can be handled by the caller. I'm going to defer to him on this, since my OO skills are more than a bit emaciated after working with Drupal so long. ;)

Committed to HEAD with a couple minor doc fixes. Thanks a lot for fixing this! +100000 for usability, and looks like cleaner code, too.

Status: Fixed » Closed (fixed)
Issue tags: -Usability, -Update manager

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +Usability, +Update manager

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.