Files: 
CommentFileSizeAuthor
#55 includes_transfer_docs-1382222-55.patch2.9 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 53,321 pass(es).
[ View ]
#55 interdiff.txt2.92 KBAlbert Volkman
#50 includes_transfer_docs-1382222-50.patch950 bytesAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 42,104 pass(es).
[ View ]
#46 includes_transfer_docs-1382222-46.patch576 bytesAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,457 pass(es).
[ View ]
#43 includes_transfer_docs-1382222-43.patch1.26 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 42,114 pass(es).
[ View ]
#41 interdiff.txt8.41 KBkriskd
#37 includes_transfer_docs-1382222-d7-37.patch21.62 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 39,070 pass(es).
[ View ]
#37 interdiff.txt3.03 KBAlbert Volkman
#33 includes_transfer_docs-1382222-d7-33.patch22.01 KBZgear
PASSED: [[SimpleTest]]: [MySQL] 39,028 pass(es).
[ View ]
#30 interdiff.txt1.85 KBZgear
#29 includes_transfer_docs-1382222-d7-29.patch22.01 KBZgear
PASSED: [[SimpleTest]]: [MySQL] 38,712 pass(es).
[ View ]
#25 includes_transfer_docs-1382222-d7-25.patch22.02 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 38,635 pass(es).
[ View ]
#25 interdiff.txt1.46 KBAlbert Volkman
#21 cleanup-api-for-filetransfer.1382222-21.patch22.29 KBZgear
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cleanup-api-for-filetransfer.1382222-21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#19 cleanup-api-for-filetransfer.1382222-19.patch22.38 KBwebkenny
PASSED: [[SimpleTest]]: [MySQL] 38,432 pass(es).
[ View ]
#16 cleanup-api-for-filetransfer.1382222-16.patch22.48 KBcrazyrohila
PASSED: [[SimpleTest]]: [MySQL] 38,445 pass(es).
[ View ]
#11 cleanup-api-for-filetransfer-1382222-11.patch21.92 KBcrazyrohila
PASSED: [[SimpleTest]]: [MySQL] 38,486 pass(es).
[ View ]
#8 includes_transfer_docs-1382222-d7-8.patch22.07 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 38,481 pass(es).
[ View ]
#1 1382222-includes_transfer_docs.patch22.16 KBchris.leversuch
PASSED: [[SimpleTest]]: [MySQL] 34,365 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new22.16 KB
PASSED: [[SimpleTest]]: [MySQL] 34,365 pass(es).
[ View ]

First attempt.

Status:Needs review» Postponed

tagging

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.

Status:Needs review» Needs work

Nothing to review yet :)

Status:Needs work» Active

Status:Active» Patch (to be ported)

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

Status:Patch (to be ported)» Needs review
StatusFileSize
new22.07 KB
PASSED: [[SimpleTest]]: [MySQL] 38,481 pass(es).
[ View ]

D7 backport.

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.

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

Assigned:chris.leversuch» Unassigned
Status:Needs work» Needs review
StatusFileSize
new21.92 KB
PASSED: [[SimpleTest]]: [MySQL] 38,486 pass(es).
[ View ]

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.

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

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

Status:Needs review» Needs work

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

StatusFileSize
new22.48 KB
PASSED: [[SimpleTest]]: [MySQL] 38,445 pass(es).
[ View ]

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

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

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" ;)

Status:Needs work» Needs review
StatusFileSize
new22.38 KB
PASSED: [[SimpleTest]]: [MySQL] 38,432 pass(es).
[ View ]

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.

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.

StatusFileSize
new22.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cleanup-api-for-filetransfer.1382222-21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

this a little better?

Status:Needs work» Needs review

Status:Needs review» Needs work

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

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 {

Status:Needs work» Needs review
StatusFileSize
new1.46 KB
new22.02 KB
PASSED: [[SimpleTest]]: [MySQL] 38,635 pass(es).
[ View ]

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.

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.

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

Status:Needs review» Needs work

Status update due to #26/27

Status:Needs work» Needs review
StatusFileSize
new22.01 KB
PASSED: [[SimpleTest]]: [MySQL] 38,712 pass(es).
[ View ]

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

StatusFileSize
new1.85 KB

Sorry guys forgot the interdiff

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.

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.

Status:Needs work» Needs review
StatusFileSize
new22.01 KB
PASSED: [[SimpleTest]]: [MySQL] 39,028 pass(es).
[ View ]

Hopefully this is good enough to go through

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.

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

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

StatusFileSize
new3.03 KB
new21.62 KB
PASSED: [[SimpleTest]]: [MySQL] 39,070 pass(es).
[ View ]
  • 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.

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()."

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

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.

Status:Needs work» Needs review
StatusFileSize
new8.41 KB

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.26 KB
PASSED: [[SimpleTest]]: [MySQL] 42,114 pass(es).
[ View ]

Docs updated in FileTransfer.php and ChmodInterface.php.

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!

StatusFileSize
new576 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,457 pass(es).
[ View ]

Here's a D7 patch.

Status:Patch (to be ported)» Needs review

Setting status.

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

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

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.

Status:Active» Needs review
StatusFileSize
new950 bytes
PASSED: [[SimpleTest]]: [MySQL] 42,104 pass(es).
[ View ]

Like this?

Those fixes look good for D8.

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?

(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.)

Assigned:jhodgdon» Unassigned

Status:Needs work» Needs review
StatusFileSize
new2.92 KB
new2.9 KB
PASSED: [[SimpleTest]]: [MySQL] 53,321 pass(es).
[ View ]

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

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

Status:Needs work» Needs review

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!