Semantic versioning?

[Echron]

Not a bug, just a question.

With the latest release (3.0.36) some of our internal code broke because we extend \phpseclib3\Net\SSH2 and the isConnected method didn't have the $level parameter. It's easy to fix that; there's no issue there.

However, would it be interesting to follow Semver? Then 3.0.36 would have been just a bugfix release and should not have broken things, while if someone sees that the second digit has changed, you know there are potential breaking changes. So this would have been version 3.1.0? But I understand that the API, in theory, didn't change as the $level is an optional parameter...

Great work by the way!

[terrafrost]

How did that change break your code?

Ultimately, when you get right down to it, even bug fixes can be potentially breaking changes. 3.0.36 also added some guardrails to isPrime and what not but how do I know that someone isn't legit trying to use phpseclib to verify the primality of a number with 16K bits? Previously that'd "work" but, depending on what extensions you have installed, it might take a tooon of time. Like if you set the timeout to 0 and were doing it from the CLI PHP would happily run for 30m if you let it. And who's to say that someone isn't doing that?

https://xkcd.com/1172/ nicely illustrates this.

https://phpseclib.com/docs/versioning talks about my versioning philosophy.

Great work by the way!

Thanks :)

[Echron]

I extend the SSH2 class, we do some stuff in our own isConnected methods (retry, dispose connection if not used for a while, etc) so suddenly our class wasn't compatible anymore as the signature was different. We only implemented that Thursday last week, before then, nothing would have happened. So if you would be so kind as to stop looking at what mess we develop, to then break things, go find your fun elsewhere 😄.

Jokes aside, it was really a non-issue, just wondering if there was any logic in the numbering, I should have looked further on your site to find your versioning info, makes perfect sense.

[terrafrost]

Well extending phpseclib is certainly a valid use case. Carbon does that to \DateTime, for example. If I didn't consider that to be a valid use case then I ought to make the class a final class.

Altho I guess that could mean that my changing of openChannel to open_channel in 792314e could be a BC breaking change, too, as that method is protected and not private (SFTP.php makes use of that method).

🤷‍♂️ It's a use case I prob ought to try to consider more often. I don't want to be paralyzed to the point where every seemingly small change is a BC break but I'd just assume not break things for people either.

[malohtie]

this change broke our code too we keep getting this exception No data received from server

we rolled back to 3.0.35 as temporary solution, and everything start to work as expected.

[terrafrost]

@malohtie - it's unclear what change 3.0.36 introduced that would be causing that behavior. If you want the issue actually fixed you'll need to provide more information. For a start, SSH logs. eg. do define('NET_SSH2_LOGGING', 2); at the top and echo $ssh->getLog();.

Thank you.

[terrafrost]

Actually, I wonder if c948a9a maybe is causing this issue.

As discussed in #1977 (comment) the change caused no issues in my own tests nor did it cause any issues in the unit tests but it's ultimately fairly difficult to determine the ultimate impact of that change. Quoting my comment in that post:

incorporating this change could be a boon to people (such as yourself) currently having issues but it could also break things for other people (like I said, the consequences are hard to predict).

To test all you need do is this:

#
#-----[ OPEN ]------------------------------------------
#
Net/SSH2.php
#
#-----[ FIND ]------------------------------------------
#
        stream_set_timeout($this->fsock, $sec, $usec);
        $raw = stream_get_contents($this->fsock, $this->decrypt_block_size);
#
#-----[ REPLACE WITH ]----------------------------------
#
        //stream_set_timeout($this->fsock, $sec, $usec);
        $raw = stream_get_contents($this->fsock, $this->decrypt_block_size);

[malohtie]

@malohtie - it's unclear what change 3.0.36 introduced that would be causing that behavior. If you want the issue actually fixed you'll need to provide more information. For a start, SSH logs. eg. do define('NET_SSH2_LOGGING', 2); at the top and echo $ssh->getLog();.

Thank you.

i m unable to get any logs the exception No data received from server keep raising

Actually, I wonder if c948a9a maybe is causing this issue.

As discussed in #1977 (comment) the change caused no issues in my own tests nor did it cause any issues in the unit tests but it's ultimately fairly difficult to determine the ultimate impact of that change. Quoting my comment in that post:

incorporating this change could be a boon to people (such as yourself) currently having issues but it could also break things for other people (like I said, the consequences are hard to predict).

To test all you need do is this:

#
#-----[ OPEN ]------------------------------------------
#
Net/SSH2.php
#
#-----[ FIND ]------------------------------------------
#
        stream_set_timeout($this->fsock, $sec, $usec);
        $raw = stream_get_contents($this->fsock, $this->decrypt_block_size);
#
#-----[ REPLACE WITH ]----------------------------------
#
        //stream_set_timeout($this->fsock, $sec, $usec);
        $raw = stream_get_contents($this->fsock, $this->decrypt_block_size);

Yes commenting this line solve this issue.
After some digging we see that we define sftp class with 0 timeout to keep things running

$sftp = new SFTP($server->ip, $server->port, 0);

and with this current change

$sec = (int) floor($this->curTimeout);
$usec = (int) (1000000 * ($this->curTimeout - $sec));
stream_set_timeout($this->fsock, $sec, $usec);

it will always timeout

[terrafrost]

@malohtie - good catch.

I just got to the office for my day job so I won't be able to fix this for ~8h or so but I'll implement a fix when I get home.

Thanks!

[malohtie]

@terrafrost

No worries! Take your time, and we appreciate your commitment!

Thanks again.

[terrafrost]

@malohtie - c20dd78 should fix this!

LMK if that works for you and if so I can do a new release this weekend!

[Echron]

@malohtie when I posted this, I got the same issue and saw we passed 0 as timeout as well, but I assumed it was an error on our side.
I see you got this all figured out, I'll give it a try.

[malohtie]

@terrafrost

it will absolutely work with the if statement.

thanks

[terrafrost]

3.0.37 has been released.

Thanks!