thoughts

[terrafrost]

So the idea behind this repo is discussed in the README.md file. This repo shouldn't contain any features that the baseline phpseclib doesn't. This repo is only a PHP5-version of phpseclib.

I figure we can throw exceptions, namespace it, maybe do autoloading, etc. Devs can be added to this repo without being added to the main phpseclib repo as well.

A few things I wonder though. Should Crypt_Base be an abstract class? I'm pretty hostile to the idea of private / protected / public methods (for the reasons described at http://aperiplus.sourceforge.net/visibility.php and for a few others) and abstract seems to be somewhat in that same vein but I don't feel as strongly about it.

Anyway, food for thought!

[mpscholten]

I think the first thing which should be done is to add namespacing and autoloading.
But what should the vendor-namespace be? PhpSecLib or Phpseclib?
And then we should move the phpseclibdirectory into a src directory.

Should Crypt_Base be an abstract class? Yes I think this would be a good decision 👍

[bantu]

Whether the directory is called phpseclib or src does not matter at all, so I'd just leave it as is.

[bantu]

Using an extra PHP5 repository is going to be a huge pain when it comes to github tickets. Stuff reported against phpseclib can not be easily moved to phpseclib-php5 etc.

Also, development with two independent repositories is just not going to work (at all).

I would like to stress my opinion of the only thing that is going to work well:

• Single repository
• Branches (master branch with php4, develop branch with php5 and new features)
• After merging a patch into the master branch, the master branch MUST be merged into develop immediately, usually by the same person (who reviewed and understood the original changes and can resolve conflicts if necessary.
• Bugfixes for bugs present in the master branch SHOULD go into the master branch first, if possible.
• New features SHOULD be added to the develop branch, so that new language features can be used.
• Once the develop branch is deemed stable, the master branch is merged into an oldstable branch and develop is merged into master.

[bantu]

Developer access to different repositories is nice to have but not necessary. If people commit crap, you just slap them and if they don't behave you kick them and if necessary revert. Also, it should be required that everything MUST go through a review process anyway.

[JonTheNiceGuy]

Why not enforce all changes to be done in a personal fork, and then every request requires a pull request?

This lets you separate the change from the approval, and you can see who has approved everything.

[bantu]

@JonTheNiceGuy Yes, I would like to see that (or something similar). Nevertheless, developers need write access to the repository as someone has to do the merging.

[mpscholten]

👍

[terrafrost]

After giving this some thought... I'm willing to try it out with a few changes.

Instead of the php5 branch being called develop I think it ought to be called php5.

Also, I think new features should be added to the master branch unless the feature can only be implemented in PHP5 and is absolutely essential.

Like with the SFTP stream wrapper I define a class variable as static to re-use SSH connections. Whether or not that's essential is debatable, I suppose, but given how easily I can come up with use-cases where that'd be the expected behavior I'm leaning towards saying it is.

And even that one, PHP5-only though it is, is in master.

Really, in my mind, the php5 branch should chiefly be for system-wide changes. phpseclib does not need to use namespacing. It might be nice but SSH and X.509 and elliptical curves and whatever else can be implemented without namespaces. That syntactic sugar is what the php5 branch is for.

And, imho, php5-only code can go in master, with the caveat that it should only go in master if it's absolutely necessary. And even then namespaces shouldn't be used in master. The API's should be consistent. Everything namespace'd in the php5 branch, nothing namespace'd in the master branch.

[bantu]

One idea of adding new features to the development branch only is forcing people to upgrade their PHP installations. Another is that you can use proper object-orientation as in modern PHP projects and do not have to worry about all the weirdnesses that PHP4 might do to objects.

[terrafrost]

The problem I have with forcing people to upgrade their PHP installations is that phpseclib's first major bullet point on it's homepage is "Compatibility". phpseclib (in particular it's SSH implementation) was originally written to be usable everywhere libssh2 wasn't. That it's also superior in most other ways is a nice little bonus but that wasn't the intent going into it.

Like if you want to be doing BigInteger stuff right you shouldn't be using phpseclib's BigInteger class at all - you should use gmp. But phpseclib isn't about doing stuff right - it's about working everywhere it can. And imho that's a pretty big part of phpseclib's branding - the fact that it works everywhere.

[bantu]

Todays world looks like the following to me: PHP5 everywhere, GMP usually not installed. So not relying on GMP is a good idea if you just want it to work, but not making use of PHP5's features is just unnecessarily painful.

Also, nobody tests new features on PHP4 because nobody is using it anyway. Not even for development and the test suite wouldn't run on PHP 4 either. It's been dead for too long.

[terrafrost]

I don't actively test on PHP4 myself either lol. I develop code that I expect will work on PHP4 but do all my testing with PHP 5.5. I mean, so long as you follow a few simple rules of thumb, it's not that hard to write code that /should/ work on PHP4. ie. don't do public / private (which I don't like anyway), don't use __construct, etc.

Like with phpseclib/phpseclib@22502c2 / phpseclib/phpseclib#154 (comment). I didn't test the commit prior to that on any version of PHP lower than 5.5. And when it broke on PHP 5.3 someone filed a bug report and it was fixed. Really, I think that's the best we can do. Well, short of unit tests, I guess, but even then, it looks like travis-ci.org is running their 5.3 unit tests on 5.3.3 - not on 5.3.6 or 5.3.7 or whatever.