make SSH2::exec behave as expected

[chrisnew]

SSH2::exec used to behave differently when false was given as callback, this has been changed in the method signature to be nullable but not reflected in the code.

We got a huge chunk of PHP code using phpseclib v2 and while we ported our code to phpseclib v3 we came across a few issues which one of them needs to be fixed upstream :-)

I hope that makes sense for you, I’d appreciate to have this merged and released quite soon.

[terrafrost]

I'm curious... did you port your v2 code to v3 before or after this change? If the latter, then remember that as a major release, v3 isn't aiming to be BC with v2 at all. For that there's phpseclib/phpseclib_compat. If the former then https://xkcd.com/1172/ seems relevant.

[chrisnew]

Regarding the code. If you leave the typehint callable to enforce either null or a valid callable object, the code on line 2626 makes partially no sense ($callback === false), but in my particular case the code works well when you allow false as a callback (as it was possible in v2 and still documented/stated in the phpdoc comment of this very file :-)).

[terrafrost]

the code on line 2626 makes partially no sense ($callback === false)
...
still documented/stated in the phpdoc comment of this very file :-)

Easy solution to that is to change $callback === false to !$callback or is_null($callback) or some such. And to update the comments as well.

That said, it's unclear what your use case is where it's beneficial for it to be false. There are some definite benefits to it being type hinted as a callable. eg.

function doSomething() {}

$ssh->exec('ping 127.0.0.1', 'doSmething');

That's a typo and the callable type hit can alert you to that fact. Well, it may not tell you that it's a typo but it'll tell you that there's an issue on that line.

So there are definitely advantages to having something type hinted as a callable. The advantages of not doing so are as yet unclear to me.

[chrisnew]

Easy solution to that is to change $callback === false to !$callback or is_null($callback) or some such. And to update the comments as well.

My first attempt was to do exactly that changing to null, null checks and the type hint. That broke the logic that exec is supposed to return all received data if no callback is set. Some other parts of our codebase suddenly was stuck since it was expecting the output of the command executed.

That said, it's unclear what your use case is where it's beneficial for it to be false.

Passing false gives me basically the chance to take care of any data received on the exec channel there. We got a piece of code which basically does the following ssh a command-1 | ssh b command-2. So quite a nasty implementation case here… but in v2 land it worked quite beautifully 😅

Either way, I love using the library and null/bool/callable was giving a lot of flexibility we actually made use of.

[terrafrost]

In reviewing the code.... I think the most appropriate code change to address the if condition that can't ever be true ($callback === false) would be to simply delete it.

If you want to achieve the same effect without using $callback = false I would do $ssh->enablePTY().

The feature in 1.0 / 2.0 mainly exists to facilitate SCP, which the 3.0 branch dropped.

[terrafrost]

In reviewing the code.... I think the most appropriate code change to address the if condition that can't ever be true ($callback === false) would be to simply delete it.

That change has been made:

4f53331

[chrisnew]

Sorry for being so quiet, I’ll check out your change! :-)