search

singpolyma / openpgp-php

OpenPGP.php is a pure-PHP implementation of the OpenPGP Message Format (RFC 4880).
http://singpolyma.github.io/openpgp-php/
The Unlicense
179 stars 69 forks source link

Update for latest php version #92

Closed divine closed 4 years ago

divine commented 4 years ago
divine commented 4 years ago

@singpolyma sorry to disturb you, could you please review this, thank you!

singpolyma commented 4 years ago

I am very concerned about removing support for older PHP versions, as well as older versions of phpseclib. I'm all for supporting newer versions, but can we somehow keep the old support intact?

peter279k commented 4 years ago

I am very concerned about removing support for older PHP versions, as well as older versions of phpseclib. I'm all for supporting newer versions, but can we somehow keep the old support intact?

Perhaps we can release new 1.0 version to drop old PHP versions.

And keep 0.x version to support the old PHP version supports.

@singpolyma, what do you think?

singpolyma commented 4 years ago

@peter279k If necessary, however reviewing this in more detail I think we could switch to substr instead of using either of the old or new index syntax and thus have it work on all versions still?

And I think we can just leave in the matrix of phpseclib compatibility, it's not clear from the code changes why it was removed...

peter279k commented 4 years ago

Yes. It needs the explanation why the string manipulation approach is changed on this PR :).

Using the string {} or [] indexing are no difference.

To be consistency, I think we can choose one to do indexing for array or string manipulation.

divine commented 4 years ago

@peter279k If necessary, however reviewing this in more detail I think we could switch to substr instead of using either of the old or new index syntax and thus have it work on all versions still?

@singpolyma Just one question, why we should keep old version support while they're not supported officially for many years and most importantly doesn't receive any security updates at all.

This library mostly used for some encryption and doesn't make sense to keep vulnerable versions of PHP supported.

However, I might take a look at it.

And I think we can just leave in the matrix of phpseclib compatibility, it's not clear from the code changes why it was removed...

There is no reason to keep old version of phpseclib, version 2.0 is LTS release and the minimum PHP version: 5.3.3, so my point that it should just work without breaking anything.

However, looking at PR #45 (issues #44 #42 #37) some changes might be required to improve compatibility and keep version always updated.

Yes. It needs the explanation why the string manipulation approach is changed on this PR :).

Using the string {} or [] indexing are no difference.

@peter279k It's actually deprecated within 7.4 version and throwing errors. Here is more information https://wiki.php.net/rfc/deprecate_curly_braces_array_access

Thank you!

singpolyma commented 4 years ago

why we should keep old version support while they're not supported officially for many years and most importantly doesn't receive any security updates at all.

This library mostly used for some encryption and doesn't make sense to keep vulnerable versions of PHP supported.

The reason (in my mind) is that many users do not get a choice as to what PHP version they are using, and I would like them to be able to get fixes to this library, even if they cannot get fixes to PHP.

There is no reason to keep old version of phpseclib, version 2.0 is LTS release and the minimum PHP version: 5.3.3, so my point that it should just work without breaking anything.

However, looking at PR #42 some changes might be required to improve compatibility and keep version always updated.

The reason I started testing against every known phpseclib version is that (a) a version broke by accident once (b) it's not that hard and guarentees that every version will work on every version we claim to support.

divine commented 4 years ago

@singpolyma I've added older versions of php to travis as well as to composer.

singpolyma commented 4 years ago

Looks good -- can we get the phpseclib matrix back on travis too, or was that breaking something?

divine commented 4 years ago

Looks good -- can we get the phpseclib matrix back on travis too, or was that breaking something?

Well, I don't really think we should test every possible version of phpseclib since it's a LTS release and previous issue will not happen again.

The are 23 releases in total of phpseclib and adding each release for each php version is 115 lines and I'm unsure how much time it will take for travis to build it.

itguy614 commented 4 years ago

+1 for getting this merged!

jstanden commented 4 years ago

+1 here too. In @cerb we switched to openpgp-php from ext/gnupg, and it's been great having full control over PGP packets, passphrases on private sub keys (w/o pinentry), etc.

However, we have many clients on PHP 7.4, and we've resorted to suppressing E_DEPRECATED.

Perhaps this could be split into two PRs. One could refactor the curly braces string offset syntax to brackets offset syntax, with the existing PHP <=7.2 tests. The second could update the tests for PHP 7.4, with the new phpUnit, where the current 6mo holdup here is.

singpolyma commented 4 years ago

Master now has the fix parts of this PR along with the complete test matrix. Passing for every phpseclib version on PHP up to 7.4

divine commented 4 years ago

Sounds great, thanks for taking care.