Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update for latest php version #92

Open
wants to merge 10 commits into
base: master
from
Open

Update for latest php version #92

wants to merge 10 commits into from

Conversation

@divine
Copy link

divine commented Jan 10, 2020

  • Update phpseclib #83
  • Update deprecated curly braces #88
  • Update phpunit
  • Update travis
  • Remove unsupported php versions
divine added 4 commits Jan 10, 2020
Remove deprecated curly braces
Remove unsupported php versions
Update minimum php version, phpunit and phpseclib
@divine

This comment has been minimized.

Copy link
Author

divine commented Jan 12, 2020

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

composer.json Outdated Show resolved Hide resolved
@singpolyma

This comment has been minimized.

Copy link
Owner

singpolyma commented Jan 14, 2020

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

This comment has been minimized.

Copy link

peter279k commented Jan 14, 2020

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

This comment has been minimized.

Copy link
Owner

singpolyma commented Jan 15, 2020

@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

This comment has been minimized.

Copy link

peter279k commented Jan 15, 2020

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

This comment has been minimized.

Copy link
Author

divine commented Jan 16, 2020

@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

This comment has been minimized.

Copy link
Owner

singpolyma commented Jan 17, 2020

divine added 4 commits Jan 18, 2020
Update travis add older php versions
Update composer php and php unit requirements
Update versions
@divine

This comment has been minimized.

Copy link
Author

divine commented Jan 18, 2020

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

composer.json Outdated Show resolved Hide resolved
@singpolyma

This comment has been minimized.

Copy link
Owner

singpolyma commented Jan 20, 2020

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

This reverts commit 916f31c.
@divine

This comment has been minimized.

Copy link
Author

divine commented Jan 20, 2020

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.

@divine divine requested a review from singpolyma Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.