Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chris.leversuch’s picture

Status: Active » Needs review
FileSize
22.16 KB

First attempt.

amateescu’s picture

Status: Needs review » Postponed
jhodgdon’s picture

Issue tags: +Novice, +Needs backport to D7

tagging

amateescu’s picture

Version: 8.x-dev » 7.x-dev
Status: Postponed » Needs review
Issue tags: -Needs backport to D7

All the clean-up from #1 has been moved to the patch from #1323124-19: Convert file transfer system to PSR-0. Leaving this open for D7.

chris.leversuch’s picture

Status: Needs review » Needs work

Nothing to review yet :)

jhodgdon’s picture

Status: Needs work » Active
jhodgdon’s picture

Status: Active » Patch (to be ported)

Oh wait, this is "patch to be ported". :)

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review
FileSize
22.07 KB

D7 backport.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! Needs a bit of cleanup:

a) top of filetransfer.inc

 /*
- * Base FileTransfer class.
+ * @file
+ * Defines a base file transfer class and associated classes.
+ */
+
+/*
+ * Defines a base file transer class.

- spelling (transfer not transer)
- Both doc blocks need to start with /** not /*

b) filetransfer.inc, in the doc for the constructor:

+   *
+   * 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. :)

c) filetransfer.inc

@@ -34,19 +73,25 @@ abstract class FileTransfer {
    *   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.
+   *
+   * @throws FileTransferException
    */
   static function factory($jail, $settings) {

Why did the @return get removed? Should stay there.

d) filetransfer.inc

@@ -165,14 +219,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.

e) filetransfer.inc, class FileTransfer -- don't remove @returns for isDirectory and isFile. They should be fixed but not removed.

f) ftp.inc

@@ -137,6 +190,9 @@ class FileTransferFTPExtension extends FileTransferFTP implements FileTransferCh
   }
 }
 
+/**
+ * Changes the permissions of the file / directory using an FTP SITE command.
+ */
 if (!function_exists('ftp_chmod')) {

This is not something that needs a docblock, is it?

g) local.inc, FileTransferLocal class

+  /**
+   * Overrides FileTransfer::factory().
+   *
+   * @return FileTransferLocal
+   *   Returns a FileTransferLocal object
+   */
   static function factory($jail, $settings) {

Needs . at end of @return doc.

jhodgdon’s picture

Note that items (b) and (d) also apply to the D8 patch, and I've put them over on that other issue.

crazyrohila’s picture

Assigned: chris.leversuch » Unassigned
Status: Needs work » Needs review
FileSize
21.92 KB

Here is patch with modified things.

Status: Needs review » Needs work
Issue tags: -Novice, -docs-cleanup-2011

The last submitted patch, cleanup-api-for-filetransfer-1382222-11.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +docs-cleanup-2011
crazyrohila’s picture

Thanks Testbot. :) this time you passed me :)

jhodgdon’s picture

Status: Needs review » Needs work

#10(b) was not done. Nor (c). I stopped reviewing there...

crazyrohila’s picture

@jhodgdon Sorry I didn't understand #10 b).
Now Updated patch attached. but still couldn't solve #10 d) and e). Sorry :(

jhodgdon’s picture

OK, hopefully someone else will take care of d/e.

webkenny’s picture

Ok, I am handling d/e now (or at least taking my best shot). While I am seeing it though I am also introducing this small adjustment:

   * If a path is a windows path, makes it POSIX compliant by removing the drive
   * letter.

Should be:

   * If a path is a Windows path, makes it POSIX compliant by removing the drive
   * letter.

Let's give Windows the capital W it "deserves" ;)

webkenny’s picture

Status: Needs work » Needs review
FileSize
22.38 KB

Ok, so it could be that I am not seeing where @returns was removed in the newest patch but please let me know if I missed it. I've done the following:

  • Removed all discussion of what will happen when $this->chroot is populated. It seems like the description of $strip_chroot covers it but I could be wrong.
  • Capitalized Windows as its a product/brand.
xjm’s picture

Status: Needs review » Needs work

Thanks @webkenny! Reviewed and found only a few small things:

  1. +++ b/includes/filetransfer/ftp.incundefined
    @@ -14,13 +22,13 @@ abstract class FileTransferFTP extends FileTransfer {
    -   * @param string $jail
    -   * @param array $settings
        * @return FileTransferFTP
        *   The appropriate FileTransferFTP subclass based on the available
        *   options. If the FTP PHP extension is available, use it.
    

    We can probably remove the @return as well, unless it differs from the one in the base class. (I did not check.)

  2. +++ b/includes/filetransfer/ftp.incundefined
    @@ -137,6 +190,9 @@ class FileTransferFTPExtension extends FileTransferFTP implements FileTransferCh
    +  // Changes the permissions of the file / directory using an FTP SITE command.
    

    This comment addition seems kind of out of scope? In any case, I think it should also be directly above the following line. Also, I don't think we need the spaces around the slash.

    Edit: Oh, I see where this came in. Let's just remove this line.

  3. +++ b/includes/filetransfer/ftp.incundefined
    @@ -137,6 +190,9 @@ class FileTransferFTPExtension extends FileTransferFTP implements FileTransferCh
    + ¶
    

    Smidge of trailing whitespace being added here.

  4. +++ b/includes/filetransfer/ssh.incundefined
    @@ -23,6 +36,12 @@ class FileTransferSSH extends FileTransfer implements FileTransferChmodInterface
    +   *   Returns a FileTransferSSH object
    

    Missing period here.

Note that I did not check whether all the methods documented as overriding such-and-such were present in the base class. It would be good if someone could double-check that for each.

Zgear’s picture

this a little better?

Albert Volkman’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, cleanup-api-for-filetransfer.1382222-21.patch, failed testing.

jhodgdon’s picture

When I tried to apply that patch I got this error:

patch: **** malformed patch at line 349: @@ -39,7 +47,7 @@ abstract class FileTransferFTP extends FileTransfer {
Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
1.46 KB
22.02 KB

Hrm, I can't see what's wrong with the patch (it was giving me that error too), so I went ahead and applied the requested changes to #19 and interdiff'd.

xjm’s picture

Thanks @Zgear and @Albert Volkman; that looks better.

+++ b/includes/filetransfer/filetransfer.incundefined
@@ -81,11 +125,18 @@ abstract class FileTransfer {
+   * Changes the permissions of the file / directory specified.
...
+   *   The file / directory to change the permissions of.

Can we replace the slash with the word "or" here?

Other than that, the patch looks good to me. I think we just need to have someone go through and confirm that the overridden methods are documented properly, and that their @return (etc.) are added only when they differ from the parent method's.

filijonka’s picture

done some checking and sorry for not presenting it as easy as @xjm.
filetransfer.inc

in class FileTransfer

function ...final chmod
* there is an @see after an @param; i can't find any references to that this is ok but maybe it is.

function sanitizePath($path)
* the return is missing it's variable

abstract public function isDirectory($path);
abstract public function isFile($path);
* the docblock missing an empty line between @param and @return

public function getSettingsForm()
* @return states Array -> array

class SkipDotsRecursiveDirectoryIterator
* block for the class: after @todo it says Depreciate, is that correct?

ftp.inc

class FileTransferFTP

function __construct
* @params are missing

ssh.inc

function __construct
* missing @params

function isDirectory
* perhaps a @todo would be good here..to test it's functionality?

ok just some thoughts from me..probably the __construct doesn't need params since it's extends a class..

jhodgdon’s picture

Status: Needs review » Needs work

Status update due to #26/27

Zgear’s picture

Status: Needs work » Needs review
FileSize
22.01 KB

@filijonka I didn't touch anything you seemed unsure about. Other than that I applied all the changes asked for :)

Zgear’s picture

FileSize
1.85 KB

Sorry guys forgot the interdiff

NROTC_Webmaster’s picture

Status: Needs review » Needs work
  1. +++ b/includes/filetransfer/filetransfer.incundefined
    @@ -10,21 +15,53 @@
    +   * @param $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.
    
  2. I'm not sure about the wording of this one. The object isn't actually performing any operations.

  3. +++ b/includes/filetransfer/filetransfer.incundefined
    @@ -325,6 +384,9 @@ abstract class FileTransfer {
    +   * @return
    +   *   array containing a form definition.
    
  4. This should have a capital letter for the first word in the description.

  5. +++ b/includes/filetransfer/ftp.incundefined
    @@ -14,13 +22,12 @@ abstract class FileTransferFTP extends FileTransfer {
        *   The appropriate FileTransferFTP subclass based on the available
        *   options. If the FTP PHP extension is available, use it.
    
  6. These lines should not be indented.

jhodgdon’s picture

RE #31 comment 1 - An object of this class actually *is* doing work, so I think that can stay as-is.

RE #31 comment 3 - Actually it should be indented, but the line above it that removed the @return is wrong -- it should be left in:

-   * @return FileTransferFTP
    *   The appropriate FileTransferFTP subclass based on the available
    *   options. If the FTP PHP extension is available, use it.
Zgear’s picture

Status: Needs work » Needs review
FileSize
22.01 KB

Hopefully this is good enough to go through

NROTC_Webmaster’s picture

Status: Needs review » Needs work

Hopefully I'm not mistaken on any of these items but if so please let me know.

  1. +++ b/includes/common.incundefined
    @@ -2821,7 +2821,7 @@ function drupal_add_html_head_link($attributes, $header = FALSE) {
    - *     - CSS_DEFAULT: (default) Any module-layer CSS.
    + *     - CSS_DEFAULT: Any module-layer CSS.
    
  2. I'm just wondering why (default) is being removed here.

  3. +++ b/includes/filetransfer/filetransfer.incundefined
    @@ -36,17 +73,24 @@ abstract class FileTransfer {
        * @return object
        *   New instance of the appropriate FileTransfer subclass.
    +   * @throws FileTransferException
    
  4. There should be a blank line between these.

  5. +++ b/includes/filetransfer/filetransfer.incundefined
    @@ -36,17 +73,24 @@ abstract class FileTransfer {
    +   * Implements the magic __get() method.
    
  6. I honestly don't know the right answer here but should this be "Implements the Magic Method __get()."

  7. +++ b/includes/filetransfer/filetransfer.incundefined
    @@ -260,11 +320,10 @@ abstract class FileTransfer {
        * @return boolean
    
  8. This and others should be bool

    http://drupal.org/node/1354#functions

  9. +++ b/includes/filetransfer/filetransfer.incundefined
    @@ -273,8 +332,7 @@ abstract class FileTransfer {
        * @return boolean
    
  10. Same as above

  11. +++ b/includes/filetransfer/ftp.incundefined
    @@ -14,13 +22,13 @@ abstract class FileTransferFTP extends FileTransfer {
        * @return FileTransferFTP
        *   The appropriate FileTransferFTP subclass based on the available
        *   options. If the FTP PHP extension is available, use it.
    
  12. xjm mentioned removing this in #20 unless it differs from the base class. I don't know if it does or not but I didn't see an answer to the question either.

  13. +++ b/includes/filetransfer/ftp.incundefined
    @@ -137,6 +190,9 @@ class FileTransferFTPExtension extends FileTransferFTP implements FileTransferCh
    + ¶
    
  14. Whitespace

Additionally I don't think the comment in #26 made it into the last patch.

filijonka’s picture

Status: Needs work » Needs review

hi

the only thing I'm still reacting on is that to the function chmod in filetransfer.inc the @param long is described with an @see.

I tried to get answers from people who might know but none gotten, in my humble opinion we shouldn't have an @see for that but a proper description.

otherwise it's looking good

xjm’s picture

This is still NW for everything in #34 and #35. Thanks for the reviews!

Albert Volkman’s picture

  • Re-added (default)
  • Added extra space before @throws
  • Fixed magic comment per other code documentation
  • Replaced booleans with bools
  • Removed remaining @return documentation from FileTransferFTP:factory method
  • Exchanged / with or
  • Copied down chmod comment from D8

Interdiff'd against #33.

filijonka’s picture

about my #35 I had an irc conversation and sun gave this suggestion to write as description for the $mode @param "The new file permission mode to be passed to chmod()."

Albert Volkman’s picture

So, if we want to change it to that, we'd need to change it in D8 first. @xjm, thoughts?

xjm’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work
Issue tags: +Needs backport to D7

Agreed, now that #1323124: Convert file transfer system to PSR-0 is in, let's move this issue back there and correct whatever we want to correct in D8 (as above). Once those points are fixed in D8, we can continue with the backport.

kriskd’s picture

Status: Needs work » Needs review
FileSize
8.41 KB

Here is the inner diff from the back ported D7 patch in comment 8 and the current D7 patch in comment 37.

jhodgdon’s picture

Status: Needs review » Needs work

We still need a D8 patch. Please leave the status at "needs work" until there's a patch. Thanks!

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
1.26 KB

Docs updated in FileTransfer.php and ChmodInterface.php.

jhodgdon’s picture

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Patch (to be ported)

Thanks! sorry for the delay; patch above is committed to 8.x.

Now let's see... I'm still confused about whether there is anything more that people wanted to get into the 8.x docs before moving back to D7, but I'm going to assume no and move this back to D7. Can someone upload a (rerolled if necessary) patch for D7 so we can move forward on that -- or else state what needs to be done still for 8.x so we can do that? Thanks!

Albert Volkman’s picture

Here's a D7 patch.

Albert Volkman’s picture

Status: Patch (to be ported) » Needs review

Setting status.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Looks good, I'll get it committed shortly. Thanks!

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Active

I committed the one-line patch to 7.x.

Now... Let's see what still needs to be done on this issue.

In 7.x: The 4 files in includes/filetransfer don't appear to have been cleaned up at all. I think the patches above that were committed to 8.x have not been ported to 7.x (and it may just be easier to start over?).

In 8.x: There are 7 files in core/lib/Drupal/Core/FileTransfer. These mostly look fine, except:
- SSH.php & FileTransferException.php - class doc first line should start with a verb.

So, we need a quick 8.x patch to take care of those two lines, and then we can move back to 7.x and do a general cleanup.

Albert Volkman’s picture

Status: Active » Needs review
FileSize
950 bytes

Like this?

Lars Toomre’s picture

Those fixes look good for D8.

jhodgdon’s picture

Status: Needs review » Needs work

Usually we use "Defines" for an interface, and "Represents" or some action verb for a class:
http://drupal.org/node/1354#classes
(The idea is for the verb to say what the class/interface does. An interface defines behavior for a class to implement, and a class represents some concept or data, or performs an action, or something like that.)

For these FileTransfer classes, a better verb might be "Handles" or "Performs" or "Transfers" -- like "Transfers files via SSH" or "Handles file transfers via SSH" (or performs) maybe?

jhodgdon’s picture

(It might also make sense to see what the first lines are of the other FileTransfer classes -- making them all consistent would not be horrible, and it would only add about 3 more lines to this two-line patch.)

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
2.92 KB
2.9 KB

Hopefully I didn't miss the mark on this one.

The last submitted patch, includes_transfer_docs-1382222-55.patch, failed testing.

Albert Volkman’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Closed (won't fix)

These issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.

Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!