Class constants #1160

rrran · 2017-08-04T14:13:09Z

Proof of concept.
Use static classes constants instead of dynamically create global constants.
If it will work, I can do the same for other constants.

terrafrost · 2017-08-05T19:33:42Z

phpseclib/Net/SSH2_CHANNEL_EXTENDED_DATA_TYPE_CODES.php

+
+class SSH2_CHANNEL_EXTENDED_DATA_TYPE_CODES extends Enum
+{
+    const TTY_OP_END=0;


Here's how net\SSH2.php currently defines this:

$this->channel_extended_data_type_codes = [ 1 => 'NET_SSH2_EXTENDED_DATA_STDERR' ];

terrafrost · 2017-08-05T19:44:40Z

phpseclib/Net/SSH2_DISCONNECT.php

+        // RFC 5656 - Elliptic Curves (for curve25519-sha256@libssh.org)
+    const KEX_ECDH_INIT = 30;
+    const KEX_ECDH_REPLY = 31;
+}


These should probably be defined in phpseclib/Net/SSH2_MSG.php instead of SSH2_DISCONNECT.php.

Also, I would say /these/ constants should be prefixed with SSH_ since they are in the SSH specs:

https://tools.ietf.org/html/rfc4254#section-9

Not all constants should be prefixed with SSH_. For example, the Terminal Codes shouldn't be since they're not in the SSH2 specs:

https://tools.ietf.org/html/rfc4254#section-8

But these I think should be.

Related to this.... I'm tentatively thinking that probably the disconnect reasons ought to be prefixed with SSH_DISCONNECT_ since that's how the RFC does it:

https://tools.ietf.org/html/rfc4253#page-26

terrafrost · 2017-08-05T19:51:33Z

phpseclib/Net/SSH2_CHANNEL_OPEN_FAILURE_REASONS.php

+class SSH2_TERMINAL_MODES extends Enum
+{
+    const TTY_OP_END=0;
+}


The class name (and constants defined therein) should probably be SSH2_CHANNEL_OPEN_FAILURE_REASONS lol.

It also looks like phpseclib only defines one of four possible reasons defined in https://tools.ietf.org/html/rfc4254#page-6

terrafrost · 2017-08-05T19:59:18Z

phpseclib/Common/Enum.php

+    protected static function init()
+    {
+        if (self::$constants==null) {
+            $reflect = new \ReflectionClass(get_called_class());


I like static::class more than get_called_class() personally :)

Me too, but I try to keep php 5.3 compatibility. static::class - it's from 5.5.

The master branch (which this PR targets) only works on PHP 5.6+. Most significantly, every file in File/ASN1/Maps sets a class constant to an array, which wasn't possible until 5.6:

http://php.net/manual/en/migration56.new-features.php

A lot of the files also use constant expressions. The end result being that all of this greatly facilitates re-use.

terrafrost · 2017-08-05T20:15:37Z

phpseclib/Net/SSH2.php

+
+//            $message_number = isset($this->message_numbers[ord($payload[0])]) ? $this->message_numbers[ord($payload[0])] : 'UNKNOWN (' . ord($payload[0]) . ')';
+
+            $messageText=(SSH2_MSG::valueExists(ord($payload[0])))?SSH2_MSG::name(ord($payload[0])):'UNKNOWN (' . ord($payload[0]) . ')';


I think this would be a cleaner way to write that:

$messageText = SSH2_MSG::valueExists(ord($payload[0])) ? SSH2_MSG::name(ord($payload[0])) : 'UNKNOWN (' . ord($payload[0]) . ')';

Yes, thanks.

terrafrost · 2017-08-05T20:18:20Z

I like it!

I made a few in-line comments. Here are a few more general thoughts:

Instead of doing phpseclib/Net/SSH2_CHANNEL_EXTENDED_DATA_TYPE_CODES.php what about doing phpseclib/Net/SSH2/CHANNEL_EXTENDED_DATA_TYPE_CODES.php.
It makes since for the const's in the classes to be all caps but I don't think the classes need to be all caps. eg.
- CHANNEL_EXTENDED_DATA_TYPE_CODES becomes ChannelExtendedDataTypeCodes
- TERMINAL_MODES becomes TerminalModes
- MSG becomes Messages
- DISCONNECT becomes DisconnectReasons
- CHANNEL_OPEN_FAILURE_REASONS becomes OpenFailureReasonCodes

Naming fix

rrran · 2017-08-07T13:05:30Z

Instead of doing phpseclib/Net/SSH2_CHANNEL_EXTENDED_DATA_TYPE_CODES.php what about
doing phpseclib/Net/SSH2/CHANNEL_EXTENDED_DATA_TYPE_CODES.php.
May be. How ever, I think, it could be better when an RFC constant use
if ( SSH_OPEN_ADMINISTRATIVELY_PROHIBITED )
becomes

use blah\blah\SSH_OPEN;

if ( SSH_OPEN::ADMINISTRATIVELY_PROHIBITED )

and not

use blah\blah\SSH\OPEN;

if ( OPEN::ADMINISTRATIVELY_PROHIBITED )

In first case it looks almost like "real" constant.
But it's up to you.

CHANNEL_EXTENDED_DATA_TYPE_CODES becomes ChannelExtendedDataTypeCodes
.........
I'd prefer to keep it in upper case with underscores.
For usual classes and objects - I agree, CamelCase rules. But here, I'd like to point, that we're using (simulating) Enums, not classes or objects.

terrafrost · 2017-08-07T13:20:13Z

use blah\blah\SSH_OPEN;

if ( SSH_OPEN::ADMINISTRATIVELY_PROHIBITED )

and not

use blah\blah\SSH\OPEN;

if ( OPEN::ADMINISTRATIVELY_PROHIBITED )

In first case it looks almost like "real" constant.
But it's up to you.

Yah - it is cosmetic but none-the-less I prefer the second lol.

That said, I just want to re-iterate that I do really like the idea :) define's are so PHP4. I've moved away from them with RSA.php, BigInteger.php, etc, and will eventually work on removing them from SSH2.php, but right now my main focus is ECDSA (in particular, I'm working on ECDSA over binary fields atm). The SSH2 changes will be part of a major BC breaking refactor on SSH2 but there are a lot of things that will come before that, including a major refactor of ASN1 / X509.

rrran · 2017-08-07T13:38:48Z

BTW, if you're going to break BC, may be it's a good idea to kick out ssh1 support?

terrafrost · 2017-08-07T15:28:00Z

BTW, if you're going to break BC, may be it's a good idea to kick out ssh1 support?

Yah - that's the plan.

rrran · 2017-08-09T10:26:08Z

As resume, you're refactoring a lot right now, so at this moment this patch is not relevant, but the idea is good.
I'll wait till you push the new code :)
Right?

# Conflicts: # phpseclib/Net/SSH2.php

terrafrost · 2017-08-09T12:39:46Z

As resume, you're refactoring a lot right now, so at this moment this patch is not relevant, but the idea is good.
I'll wait till you push the new code :)
Right?

I am refactoring the master branch lots but right now my main focus is on getting ECDSA done. Honestly, I figure that could take a few months. ECDSA over prime fields seems pretty straight forward and there are lots of easy to understand reference implementations. ECDSA over binary fields otoh is more complicated. I guess I could forgo support for ECDSA over binary fields but I'm not ready to "give up" yet lol.

After that I figure I'll completely revamp ASN1 / X509. That's liable to be another multi month long project.

Honestly, I might not get around to revamping SSH2 this year I don't think it's realistic to expect you to be able to get back into this after taking that kind of break lol.

I'd say: go ahead and pursue this and I'll merge it into master if you can get my suggestions implemented. If you can't (or don't want to lol) that's fine - it is a good idea and I can do it myself when the time comes for the refactor lol

# Conflicts: # .gitignore # phpseclib/Net/SSH2.php

Sokolovskyy Roman added 3 commits August 4, 2017 14:36

Refactor constants use. Proof of concept.

d65f814

Refactor constants use. Proof of concept.

d4f1ca2

Fix usage of old style constants

15d8b4a

terrafrost reviewed Aug 5, 2017

View reviewed changes

terrafrost mentioned this pull request Aug 7, 2017

Use the actual PHP test suite for validating mcrypt_compat behavior phpseclib/mcrypt_compat#9

Open

Better code

190b057

Naming fix

Better aproach to RFC constants

763f83b

get_called_class() -> static::class

4d0c436

Merge remote-tracking branch 'origin/master' into class_constants

97d7d37

# Conflicts: # phpseclib/Net/SSH2.php

Merge remote-tracking branch 'origin/master' into class_constants

6bb752f

# Conflicts: # .gitignore # phpseclib/Net/SSH2.php

terrafrost mentioned this pull request Nov 5, 2017

Unveil received value in unexpected value exceptions (SFTP) #1204

Merged

rrran closed this Mar 12, 2018

rrran deleted the class_constants branch March 12, 2018 15:50

terrafrost mentioned this pull request Feb 1, 2022

Set up php cs fixer and psalm ci #1748

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Class constants #1160

Class constants #1160

rrran commented Aug 4, 2017

terrafrost Aug 5, 2017

terrafrost Aug 5, 2017

terrafrost Aug 5, 2017

terrafrost Aug 5, 2017 •

edited

Loading

terrafrost Aug 5, 2017

rrran Aug 7, 2017

terrafrost Aug 7, 2017

terrafrost Aug 5, 2017

rrran Aug 7, 2017

terrafrost commented Aug 5, 2017

rrran commented Aug 7, 2017

terrafrost commented Aug 7, 2017

rrran commented Aug 7, 2017

terrafrost commented Aug 7, 2017

rrran commented Aug 9, 2017

terrafrost commented Aug 9, 2017


		// $message_number = isset($this->message_numbers[ord($payload[0])]) ? $this->message_numbers[ord($payload[0])] : 'UNKNOWN (' . ord($payload[0]) . ')';

		$messageText=(SSH2_MSG::valueExists(ord($payload[0])))?SSH2_MSG::name(ord($payload[0])):'UNKNOWN (' . ord($payload[0]) . ')';

Class constants #1160

Class constants #1160

Conversation

rrran commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrafrost Aug 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrafrost commented Aug 5, 2017

rrran commented Aug 7, 2017

terrafrost commented Aug 7, 2017

rrran commented Aug 7, 2017

terrafrost commented Aug 7, 2017

rrran commented Aug 9, 2017

terrafrost commented Aug 9, 2017

terrafrost Aug 5, 2017 •

edited

Loading