Comments for "Net_SSH2"

» Submit Your Comment
Comments are only accepted during the "Proposal" phase. This proposal is currently in the "Finished" phase.
» Comments
  • Daniel O'Connor  [2009-01-28 23:30 UTC]

    With the unit tests; is there any way you can mock out large chunks of it so you don't have to setup a configuration file?

    For instance,

    proc_open creates streams for you to write to, and there's no way for the unit tests to replace that with something else (say, a php in memory stream) - think about ways to mock that out; and you can simulate a lot of behaviour / control your test environment.

    You might also be interested in PHPUnit's data provider features; which allow you to write 1x test and supply many assorted types of input.

    Also the tests are often a good way to demonstrate how the code is used - the way you've written them doesn't quite do that (though it does provide coverage)

    Finally Consider naming your tests in an agile fashion (

    Might be worth swapping around some of the places where you do:

    function foo() {
    if (condition) {
    ... large part of function ...

    return true;
    } else {
    throw new Exception("BLEHHHHH!");

    to be:
    function foo() {
    if (!condition) {
    throw new Exception("BLEHHHHH!");

    // ... large part of function ...
    return true;

    Other than that, LGTM
  • Kevin van Zonneveld  [2009-01-30 10:47 UTC]

    You don't have to make system calls to execute remote commands with ssh. You can check-for & use libssh instead.

    Works for scp as well.
  • Christian Weiske  [2009-06-25 08:07 UTC]

    Is this proposal dead, or are you going to finish it?
  • Chuck Burgess  [2009-09-08 22:27 UTC]

    I can't download the tarball or see the PHPS files online... is prompting for a username/password...
  • Chuck Burgess  [2009-09-09 15:19 UTC]

    This looks pretty good. I built something very similar to the OpenSSH piece at work last year, but talked myself out of proposing it since it was mainly all shell exec stuff ;-) I could probably help you maintain this package if it's accepted.
  • Matthew Fonda  [2009-09-09 18:10 UTC]

    Good looks great, nice work. I'd love to see this make its way into pear
  • Till Klampaeckel  [2009-09-09 19:45 UTC]

    Hey Luca,

    first off, I'm too looking forward to this code in PEAR.

    I also briefly looked at your code and here are some suggestions:

    * Move constants into the Net_SSH2 class.

    * & is not necessary in PHP5.

    * Always work with visibility (public, static, etc.) on methods (e.g. the factory).

    * Add optional dependency on PEAR::System and ext/proc to package.xml. (PEAR::File should be optional as well).

    * Maybe briefly test for File and System, before use (maybe in __construct()). Or introduce a small autoload, or require_once the files in between file and class comment.

    * Maybe wrap the options into a Net_SSH2::$data array, instead of $this->$key = $val, $this->data[$key] = $val (in the drivers __construct()).

    * In Net_SSH2_LibSSH2 you could support the .dll version (on Windows), you could also attempt loading the php_ssh2.dll/ before you error out.

    * Document thrown Exceptions with @throws.

    * Instead of die() in your tests, please use $this->markTestIncomplete() or $this->markTestSkipped().

    * Also, for some tests, take a look at $this->setExpectedException().

    Anyway, your code is great regardless of what I find. Especially nice that you put a lot of effort into your test suite. :)