Convert file transfer system to PSR-0

Trying to get my feet wet in this new PSR-0/namespaces stuff :)

Note for reviewers: I also fixed a @todo in this patch, namely:

+++ /dev/null
@@ -1,418 +0,0 @@
-/**
- * Provides an interface for iterating recursively over filesystem directories.
- *
- * Manually skips '.' and '..' directories, since no existing method is
- * available in PHP 5.2.
- *
- * @todo Depreciate in favor of RecursiveDirectoryIterator::SKIP_DOTS once PHP
- *   5.3 or later is required.
- */
-class SkipDotsRecursiveDirectoryIterator extends RecursiveDirectoryIterator {
-  /**
-   * Constructs a SkipDotsRecursiveDirectoryIterator
-   *
-   * @param $path
-   *   The path of the directory to be iterated over.
-   */
-  function __construct($path) {
-    parent::__construct($path);
-  }
-
-  function next() {
-    parent::next();
-    while ($this->isDot()) {
-      parent::next();
-    }
-  }
-}

This wasn't converted to PSR-0 because we depend on PHP 5.3 now, so I replaced it with:

... new RecursiveDirectoryIterator($source, RecursiveDirectoryIterator::SKIP_DOTS) ...

Now I'm wondering if we should also bring #1382222: Clean up API docs for includes/filetransfer in here, so we don't waste time re-rolling patches that touch exactly the same code.

-5 days to next Drupal core point release.

+++ b/core/includes/Drupal/FileTransfer/FileTransfer.php
@@ -0,0 +1,366 @@
+/**
+ * Classes extending this class perform file operations on directories not
+ * writable by the webserver. To achieve this, the class should connect back
+ * to the server using some backend (for example FTP or SSH). To keep security,
+ * the password should always be asked from the user and never stored. For
+ * safety, all methods operate only inside a "jail", by default the Drupal root.
+ */

Not a valid docblock. It's missing a proper one-liner.

+++ b/core/includes/Drupal/FileTransfer/FileTransfer.php
@@ -0,0 +1,366 @@
+  /**
+   * Classes that extend this class must override the factory() static method.
+   *
+   * @param string $jail
+   *   The full path where all file operations performed by this object will
+   *   be restricted to. This prevents the FileTransfer classes from being
+   *   able to touch other parts of the filesystem.
+   * @param array $settings
+   *   An array of connection settings for the FileTransfer subclass. If the
+   *   getSettingsForm() method uses any nested settings, the same structure
+   *   will be assumed here.
+   * @return object
+   *   New instance of the appropriate FileTransfer subclass.
+   */
+  static function factory($jail, $settings) {
+    throw new FileTransferException('FileTransfer::factory() static method not overridden by FileTransfer subclass.');
+  }

Someone was watching my Code Smells talk where I call out this docblock as being too vague. :-) Thanks for fleshing it out!

That said, I'd suggest not doing any major doc refactoring at the same time. I even marked the DB doc cleanup as postponed until after the PSR-0-shuffling is done. Both are important, but should be done as separate tasks.

+++ b/core/includes/Drupal/FileTransfer/FileTransferChmodInterface.php
@@ -0,0 +1,26 @@
+interface FileTransferChmodInterface {

The "FileTransfer" prefix on the interface is no longer necessary. Just ChmodInterface is sufficient.

+++ b/core/includes/Drupal/FileTransfer/FileTransferException.php
@@ -0,0 +1,22 @@
+class FileTransferException extends Exception {

This isn't really PSR-0 specific, but doing the database updates I've been switching from Exception to RuntimeException as my base exception. It's slightly more applicable, I think. (Both are part of PHP itself.)

+++ b/core/includes/Drupal/FileTransfer/FileTransferFTP.php
@@ -0,0 +1,56 @@
+abstract class FileTransferFTP extends FileTransfer {

Again, this can be called simply FTP. (Or maybe Ftp? No strong preference.)

+++ b/core/includes/Drupal/FileTransfer/FileTransferFTPExtension.php
@@ -0,0 +1,97 @@
+class FileTransferFTPExtension extends FileTransferFTP implements FileTransferChmodInterface {

Same comment as above. :-)

+++ b/core/includes/Drupal/FileTransfer/FileTransferLocal.php
@@ -0,0 +1,83 @@
+class FileTransferLocal extends FileTransfer implements FileTransferChmodInterface {

Yeah, same thing here. I'll stop mentioning it now.

-5 days to next Drupal core point release.

Updated patch with the following changes:

• fixed the dockblock for FileTransfer
• switched to RuntimeException
• modified the @file dockblocks according to http://drupal.org/node/1354#files

I don't agree with removing the "FileTransfer" prefix on interfaces or classes. Imagine doing a new FTP(); or Definition of FTP. in the @file dockblock (even with a use statement at the top of the file).. we might as well call it new WTF(); :)

Forgot to add the new files :/

Re #5: That's actually by design. The FileTransfer prefix is totally redundant with the namespace itself. If you're in a given namespace, you don't need to repeat the class name every time. If you're not, then you have the class listed at the top of the file anyway so you know exactly what it is. The context makes it clear what you mean. Just as in the DB layer, Insert as a class name is self-evident when you are in the Query namespace.

I could see the @file declaration having a full namespace listed? We didn't actually discuss that. Oops. :-)

I don't really enjoy discussing shed colors, so here's a patch without the FileTransfer prefix :)

Rerolled with the latest namespace standards.

Minor nit:

+++ b/core/lib/Drupal/Core/FileTransfer/FileTransferException.php
@@ -0,0 +1,22 @@
+/**
+ * @file
+ * Definition of FileTransferException.
+ */

This should be a FQCN. (Fully qualified class name.)

Other than that, this looks good to me on visual inspection. We should probably get confirmation from dww or JacobSingh though before marking it RTBC.

Oops, don't know how that slipped :) I'll try to track down dww or JacobSingh in IRC then..

Let's clean these docs while we are on it. I'm also speaking as an avatar for xjm.
I hope I listed everything...

If you need more information: http://drupal.org/node/1354

+++ b/core/lib/Drupal/Core/FileTransfer/ChmodInterface.phpundefined
@@ -0,0 +1,26 @@
+   * @param long $mode
+   *   @see http://php.net/chmod
+   * @param boolean $recursive

You can't use @see in @param, just use see. Or use a better short explanation and move the @see to the bottom of the docblock.

+++ b/core/lib/Drupal/Core/FileTransfer/FTP.phpundefined
@@ -0,0 +1,57 @@
+  public function __construct($jail, $username, $password, $hostname, $port) {

Constructors need a docblock too

+++ b/core/lib/Drupal/Core/FileTransfer/FTP.phpundefined
@@ -0,0 +1,57 @@
+   * Return an object which can implement the FTP protocol.

Returns

+++ b/core/lib/Drupal/Core/FileTransfer/FTP.phpundefined
@@ -0,0 +1,57 @@
+   * @param string $jail
+   * @param array $settings
+   *

There is no description of the param

+++ b/core/lib/Drupal/Core/FileTransfer/FTP.phpundefined
@@ -0,0 +1,57 @@
+   *   The appropriate FTP subclass based on the available
+   *   options. If the FTP PHP extension is available, use it.
+   */

Why is "options" on the second line, the first line didn't reach 80 characters yet.

+++ b/core/lib/Drupal/Core/FileTransfer/FTP.phpundefined
@@ -0,0 +1,57 @@
+   * Returns the form to configure the FileTransfer class for FTP.

Sounds very cryptical to me. (but that can be just me as I never used this subsystem)

+++ b/core/lib/Drupal/Core/FileTransfer/FTPExtension.phpundefined
@@ -1,54 +1,13 @@
+class FTPExtension extends FTP implements ChmodInterface {

This class doesn't have any docblock?

+++ b/core/lib/Drupal/Core/FileTransfer/FileTransferException.phpundefined
@@ -0,0 +1,22 @@
+  function __construct($message, $code = 0, $arguments = array()) {

Constructor needs a docblock

Erm.. no :) I wanted to do this from the start (in #2) but Crell is right, we already have #1382222: Clean up API docs for includes/filetransfer for cleaning up the docs.

Editing my comment to be a bit less knee-jerk. :)

When there are things like missing and unclear function and parameter documentation, even if this documentation predates the patch, I think we should make an effort to make the documentation in lines added or changed is correct and to standard. There's a couple reasons for this:

• Our Core Gates policy clearly states that all code/documentation added or updated in a patch should follow our standards.
• Those making functional changes to a patch will typically have a much better grasp on (say) the role of the function or parameter than a novice trying to roll a cleanup patch.
• Having good docs in the issue here also helps reviewers understand what they need to know to review the issue.
• Like any large patch, large cleanup patches are frustrating to review, reroll, etc., so the more docs we can improve in context, the better.

Working on the docs now!

Probably not perfect yet, but it's a very good start.
A few TODO's left and I'll ping xjm for a review.

+++ b/core/modules/update/update.manager.incundefined
@@ -35,6 +35,8 @@
+use Drupal\Core\FileTransfer\Local;
+
 /**
  * @defgroup update_manager_update Update manager: update

Can we put a use statement in the middle of the file.

(couldn't find the docs for psr-0 code)

I moved all the clean-up from #1382222: Clean up API docs for includes/filetransfer over here and moved that issue to D7. Interdiff is from #12.

+++ b/core/lib/Drupal/Core/FileTransfer/FTP.phpundefined
@@ -0,0 +1,53 @@
+  /**
+   * Overrides Drupal\Core\FileTransfer\FileTransfer::__construct().
+   */
+  public function __construct($jail, $username, $password, $hostname, $port) {
+    $this->username = $username;

I'm not sure this is correct, Overrides can only be used with functions having the same parameter list and name. These constructors are different. In fact constructors are always a special case. And we have to explain the different parameters.

See: http://drupal.org/node/1354#classes

+++ b/core/lib/Drupal/Core/FileTransfer/SSH.phpundefined
@@ -1,10 +1,20 @@
+   * Overrides Drupal\Core\FileTransfer\FileTransfer::__construct().

Same

Thanks @amateescu and @aspilicious. I'll try to take a closer look at this later today (I hope).

Ok apparantly amateescu made some changes not based on my latest patch.
For example, I had documentation for @throws on many functions.

ps: you referred to the database psr-0 patch somewhere, that one didn't contain any style fixes. I didn't noticed it before it got committed, so it's a bad example to refer to.

aspilicious: No. PHP requires that the "namespace" line be the first executable line in the file, and any "use" statements be the next executable lines, then any other code. That's not PSR-0; it's just the PHP compiler. :-)

Database was a special case because it was 500k long and large enough to merit a merge. :) This patch is not.

Since @Crell wants a sign-off from dww or JacobSingh, let's see if they watch their assigned queues. Trying dww first :)

Thanks to #1212634: Auto-flag when an issue is assigned to you. as soon as you assign an issue to me it shows up in my tracker, which yes, I watch very closely.

That said, it's 3:15am here, I've got a bunch of deadlines this week, and I don't have the time/energy to review a 40K patch.

Furthermore, I'm -1 on completely re-writing the docs as part of a PSR-0 conversion, but whatever, what's done is done, and #15 makes a good case as to why it should happen. However, it a) makes it harder to backport just the doc fixes to D7 and b) makes it harder to review the PSR-0-ness of this issue. Separate issues for separate changes++.

Anyway, I'm now following this, but having it assigned to me is misleading, since I'm not going to be able to do anything with this until next week at the earliest. If/when my situation changes and I can actually review this for real, I'll assign it back to myself.

Cheers
-Derek

Thanks for the prompt reply Derek.

A note for anyone who wants to do a functional review of this patch, they should look at #12 (which is only 20K), because #19 has only additional doc fixes.

Edit: My comment from #2 should also be taken into consideration when reviewing.

Re #5: That's actually by design. The FileTransfer prefix is totally redundant with the namespace itself. If you're in a given namespace, you don't need to repeat the class name every time.

@Crell I'm sorry but I must disagree. You don't type the namespace every now and then (only in use statement in the top of the file), so it may be redundant with the namespace, but it's not an issue. On the contrary, namespaces allows us to put longer class names since the use statements are a useful shortcut. On the case of the FTP class, I might not be unhappy to see it that short, because FTP is FTP, it's straightforward, but your statement is not generally true (on the opposite, it's only true here).

#28 we already broke compatibility with Ubuntu LTS (much to my disagreement) just to avoid the need to repeat the namespace in the class name so you are fighting a (very) uphill battle here.

Having non explicit class names is bad, it forces us to abuse of aliasing or as sun proposed in another issue using relative names. This is insane, it will make the code harder to read and harder to grep, harder to search in it etc etc etc... Why do people always prefer short names to explicit names? This insane, shorter names doesn't make the code better, or more readable, or cool, or nice, or pretty...

pounard: Dude, chill. :-) I don't see any confusion with the current class names, and I'd rather avoid redundancy to be consistent with most of the other systems we have. As noted in the Lock issue, though, we should (in a separate thread) revisit our namespace rules with a bit more experience. That said, let's stick with what the rules are for now since there's no screaming confusion the way there was with Lock. We want to rewrite this whole subsystem anyway, remember? :-)

I wanted to make my point: explicit is always better. It's not too late to change, it will be once all subsystems will have been converted because no one will want to rewrite them all.

RE - latest patch - I just reviewed the D7 port of the doc fixups that were applied in #19 above. Probably the same problems apply to this patch:
http://drupal.org/node/1382222#comment-5741162
hmmm. It looks like from that list, it's just items (b) and (d):

b) Constructor for the main FileTransfer class (core/lib/Drupal/Core/FileTransfer/FileTransfer.php)

@@ -11,21 +20,55 @@
...
+   *
+   * This method is also called from the classes that extend this class and
+   * override this method.

I think we can just leave this out. Kind of a "duh" moment for me. :)

d)(core/lib/Drupal/Core/FileTransfer/FileTransfer.php)

@@ -166,14 +227,18 @@ abstract class FileTransfer {

   /**
    * Returns a modified path suitable for passing to the server.
-   * If a path is a windows path, makes it POSIX compliant by removing the drive letter.
-   * If $this->chroot has a value, it is stripped from the path to allow for
-   * chroot'd filetransfer systems.
+   *
+   * If a path is a windows path, makes it POSIX compliant by removing the drive
+   * letter. If $this->chroot has a value, it is stripped from the path to allow
+   * for chroot'd filetransfer systems.
    *
    * @param $path
+   *   The path to modify.
    * @param $strip_chroot
+   *   Whether to remove the path in $this->chroot.

The description above implies that stripping chroot always happens, but the $strip_chroot param says otherwise.

Should be a quick repatch to figure these out and fix the docs...

Here is a re-roll that also fixes the issues mentioned in #33.

Doc looks better, thanks.

pushing back to dww see #26

+++ b/core/lib/Drupal/Core/FileTransfer/Local.phpundefined
@@ -1,36 +1,61 @@
+  /**
+   * Overrides Drupal\Core\FileTransfer\FileTransfer::factory().
+   */
   static function factory($jail, $settings) {
-    return new FileTransferLocal($jail);
+    return new Local($jail);

I think we should have a better name than 'Local' here and in other places. Too much contextual information is lost here, in my opinion. It might be better to duplicate the words FileTransfer and to go for "FileTransfer/LocalFileTransfer"? Suggestions?

+1 on the idea of making class names more contextual. For instance, someone searching on api.drupal.org would not find "Local" if they search for "FileTransfer", which would be a shame. (At the moment, api.drupal.org does not understand namespaces, which is a separate issue.)

Couldn't agree more.

In #1323082-53: Establish standards for namespace usage [no patch], I suggested "using" up until the subsystem name, so #37 would become return new FileTransfer\Local($jail);, but Crell and pounard disliked that idea, and they're both more fluent with namespaces than I am. We also have a similar issue of short final class names in #1460476: Figure out sub-namespaces for DBTNG, establishing precedent for other subsytems., where the #1321540: Convert DBTNG to namespaces; separate Drupal bits patch that went in removed the Query prefix from a bunch of final class names by moving them into a Query namespace, so that in D7 we had QueryAlterableInterface, but in D8 we now have simply AlterableInterface (in a Drupal\Core\Database\Query namespace).

I'm not too keen on the idea of repeating information in a class name that already exists in its namespace name, so Drupal\Core\FileTransfer\LocalFileTransfer seems wrong to me. Before we go ahead with that, can people give some more feedback on why #1323082-53: Establish standards for namespace usage [no patch] is bad?

[Edit: never mind. see #42]

The problem with not having longer class names is that the short class names are no longer meaningful by themselves. I'll comment on the other issue though.

Ignore #40. Anyone who wants to provide feedback in the other issue is welcome to, but I now see that Symfony uses redundancy in namespace and class names. For example, Symfony\Component\HttpFoundation\SessionStorage\ArraySessionStorage. So I withdraw my objection to Drupal\Core\FileTransfer\LocalFileTransfer.

[Edit: This was an x-post with #41, but still applies.]

I'm not too keen on the idea of repeating information in a class name that already exists in its namespace name

As jhodgdon says, it's not about not repeating things, it's about readability. If we always or almost always use the use statement people will never have to read the repeating information, they will just read plain class names without namespaces in code, class names needs to be explicit.

I recommend we proceed with the patch as is if that's the only issue, and revise later after #1507828: [policy, no patch] Revised standards for class naming within namespaces since that will likely affect a number of subsystems.

Glad y'all sorted it out, and sorry I never had a chance to review/test this. I just hope update manager still works. ;)