Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of meta-issue #1310084: [meta] API documentation cleanup sprint
Comment | File | Size | Author |
---|---|---|---|
#55 | includes_transfer_docs-1382222-55.patch | 2.9 KB | Albert Volkman |
#55 | interdiff.txt | 2.92 KB | Albert Volkman |
#50 | includes_transfer_docs-1382222-50.patch | 950 bytes | Albert Volkman |
#46 | includes_transfer_docs-1382222-46.patch | 576 bytes | Albert Volkman |
#43 | includes_transfer_docs-1382222-43.patch | 1.26 KB | Albert Volkman |
Comments
Comment #1
chris.leversuch CreditAttribution: chris.leversuch commentedFirst attempt.
Comment #2
amateescu CreditAttribution: amateescu commentedPostponing on #1323124: Convert file transfer system to PSR-0.
Comment #3
jhodgdontagging
Comment #4
amateescu CreditAttribution: amateescu commentedAll 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.
Comment #5
chris.leversuch CreditAttribution: chris.leversuch commentedNothing to review yet :)
Comment #6
jhodgdonComment #7
jhodgdonOh wait, this is "patch to be ported". :)
Comment #8
Albert Volkman CreditAttribution: Albert Volkman commentedD7 backport.
Comment #9
jhodgdonThanks! Needs a bit of cleanup:
a) top of filetransfer.inc
- spelling (transfer not transer)
- Both doc blocks need to start with /** not /*
b) filetransfer.inc, in the doc for the constructor:
I think we can just leave this out. Kind of a "duh" moment for me. :)
c) filetransfer.inc
Why did the @return get removed? Should stay there.
d) filetransfer.inc
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
This is not something that needs a docblock, is it?
g) local.inc, FileTransferLocal class
Needs . at end of @return doc.
Comment #10
jhodgdonNote that items (b) and (d) also apply to the D8 patch, and I've put them over on that other issue.
Comment #11
crazyrohila CreditAttribution: crazyrohila commentedHere is patch with modified things.
Comment #13
jhodgdon#11: cleanup-api-for-filetransfer-1382222-11.patch queued for re-testing.
Comment #14
crazyrohila CreditAttribution: crazyrohila commentedThanks Testbot. :) this time you passed me :)
Comment #15
jhodgdon#10(b) was not done. Nor (c). I stopped reviewing there...
Comment #16
crazyrohila CreditAttribution: crazyrohila commented@jhodgdon Sorry I didn't understand #10 b).
Now Updated patch attached. but still couldn't solve #10 d) and e). Sorry :(
Comment #17
jhodgdonOK, hopefully someone else will take care of d/e.
Comment #18
webkenny CreditAttribution: webkenny commentedOk, 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:
Should be:
Let's give Windows the capital W it "deserves" ;)
Comment #19
webkenny CreditAttribution: webkenny commentedOk, 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:
Comment #20
xjmThanks @webkenny! Reviewed and found only a few small things:
We can probably remove the @return as well, unless it differs from the one in the base class. (I did not check.)
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.
Smidge of trailing whitespace being added here.
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.
Comment #21
Zgear CreditAttribution: Zgear commentedthis a little better?
Comment #22
Albert Volkman CreditAttribution: Albert Volkman commentedComment #24
jhodgdonWhen I tried to apply that patch I got this error:
Comment #25
Albert Volkman CreditAttribution: Albert Volkman commentedHrm, 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.
Comment #26
xjmThanks @Zgear and @Albert Volkman; that looks better.
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.Comment #27
filijonka CreditAttribution: filijonka commenteddone 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..
Comment #28
jhodgdonStatus update due to #26/27
Comment #29
Zgear CreditAttribution: Zgear commented@filijonka I didn't touch anything you seemed unsure about. Other than that I applied all the changes asked for :)
Comment #30
Zgear CreditAttribution: Zgear commentedSorry guys forgot the interdiff
Comment #31
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedI'm not sure about the wording of this one. The object isn't actually performing any operations.
This should have a capital letter for the first word in the description.
These lines should not be indented.
Comment #32
jhodgdonRE #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:
Comment #33
Zgear CreditAttribution: Zgear commentedHopefully this is good enough to go through
Comment #34
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedHopefully I'm not mistaken on any of these items but if so please let me know.
I'm just wondering why (default) is being removed here.
There should be a blank line between these.
I honestly don't know the right answer here but should this be "Implements the Magic Method __get()."
This and others should be bool
http://drupal.org/node/1354#functions
Same as above
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.
Whitespace
Additionally I don't think the comment in #26 made it into the last patch.
Comment #35
filijonka CreditAttribution: filijonka commentedhi
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
Comment #36
xjmThis is still NW for everything in #34 and #35. Thanks for the reviews!
Comment #37
Albert Volkman CreditAttribution: Albert Volkman commentedInterdiff'd against #33.
Comment #38
filijonka CreditAttribution: filijonka commentedabout 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()."
Comment #39
Albert Volkman CreditAttribution: Albert Volkman commentedSo, if we want to change it to that, we'd need to change it in D8 first. @xjm, thoughts?
Comment #40
xjmAgreed, 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.
Comment #41
kriskd CreditAttribution: kriskd commentedHere is the inner diff from the back ported D7 patch in comment 8 and the current D7 patch in comment 37.
Comment #42
jhodgdonWe still need a D8 patch. Please leave the status at "needs work" until there's a patch. Thanks!
Comment #43
Albert Volkman CreditAttribution: Albert Volkman commentedDocs updated in FileTransfer.php and ChmodInterface.php.
Comment #44
jhodgdon#43: includes_transfer_docs-1382222-43.patch queued for re-testing.
Comment #45
jhodgdonThanks! 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!
Comment #46
Albert Volkman CreditAttribution: Albert Volkman commentedHere's a D7 patch.
Comment #47
Albert Volkman CreditAttribution: Albert Volkman commentedSetting status.
Comment #48
jhodgdonLooks good, I'll get it committed shortly. Thanks!
Comment #49
jhodgdonI 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.
Comment #50
Albert Volkman CreditAttribution: Albert Volkman commentedLike this?
Comment #51
Lars Toomre CreditAttribution: Lars Toomre commentedThose fixes look good for D8.
Comment #52
jhodgdonUsually 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?
Comment #53
jhodgdon(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.)
Comment #54
jhodgdonComment #55
Albert Volkman CreditAttribution: Albert Volkman commentedHopefully I didn't miss the mark on this one.
Comment #57
Albert Volkman CreditAttribution: Albert Volkman commented#55: includes_transfer_docs-1382222-55.patch queued for re-testing.
Comment #58
jhodgdonThese 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!