X Tutup
Skip to content

feat: support phpseclib3#2019

Merged
bshaffer merged 4 commits intogoogleapis:masterfrom
kylekatarnls:feature/phpseclib3-support
Dec 28, 2020
Merged

feat: support phpseclib3#2019
bshaffer merged 4 commits intogoogleapis:masterfrom
kylekatarnls:feature/phpseclib3-support

Conversation

@kylekatarnls
Copy link
Contributor

@kylekatarnls kylekatarnls commented Dec 21, 2020 •
edited
Loading

  • Allow to install phpseclib/phpseclib with no composer conflicts.
  • Adapt Verify class to new phpseclib classes and methods.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 21, 2020
@kylekatarnls kylekatarnls force-pushed the feature/phpseclib3-support branch 2 times, most recently from d988f8f to 5c2447f Compare December 21, 2020 11:28
@kylekatarnls kylekatarnls force-pushed the feature/phpseclib3-support branch 4 times, most recently from 1883da4 to b5c5706 Compare December 22, 2020 11:33
@kylekatarnls kylekatarnls marked this pull request as ready for review December 22, 2020 11:44
@kylekatarnls kylekatarnls requested a review from a team December 22, 2020 11:44
bshaffer
bshaffer reviewed Dec 22, 2020
View reviewed changes
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Thank you for this!

I don't believe the current test suite covers phpseclib:~2.0. I'll try to get a PR in today to fix that, and once I can conform that's being tested this will be good to merge.

Thanks again.

@kylekatarnls
Copy link
Contributor Author

kylekatarnls commented Dec 22, 2020 •
edited
Loading

phpseclib3 namespace did not exist before, so enforcing the installation of phpseclib 2 in an environment on GitHub Actions will work just fine (100% equivalent to current state)

@kylekatarnls
Copy link
Contributor Author

kylekatarnls commented Dec 22, 2020

I see you just dropped PHP 5.4 and 5.5, 🚀 I'll will check the PHP 8 failure

@bshaffer
Copy link
Contributor

bshaffer commented Dec 22, 2020

The error I'm seeing in PHP 8.0:

1) Google_AccessToken_VerifyTest::testPhpsecConstants
TypeError: strpos(): Argument #1 ($haystack) must be of type string, array given

/home/runner/work/google-api-php-client/google-api-php-client/vendor/phpseclib/phpseclib/phpseclib/Crypt/EC/Formats/Keys/PKCS1.php:64
/home/runner/work/google-api-php-client/google-api-php-client/vendor/phpseclib/phpseclib/phpseclib/Crypt/Common/AsymmetricKey.php:162
/home/runner/work/google-api-php-client/google-api-php-client/vendor/phpseclib/phpseclib/phpseclib/Crypt/PublicKeyLoader.php:42
/home/runner/work/google-api-php-client/google-api-php-client/src/AccessToken/Verify.php:235
/home/runner/work/google-api-php-client/google-api-php-client/src/AccessToken/Verify.php:105
/home/runner/work/google-api-php-client/google-api-php-client/tests/Google/AccessToken/VerifyTest.php:41

@kylekatarnls
Copy link
Contributor Author

kylekatarnls commented Dec 23, 2020

Yes @bshaffer I checked the GitHub log yesterday already and was able to reproduce it locally.

I provided a minimal code chunk that reproduces it using the code strictly taken from the documentation that proves it needs to be fixed in phpseclib itself:
phpseclib/phpseclib#1559

@kylekatarnls kylekatarnls force-pushed the feature/phpseclib3-support branch from 18eddb0 to 857b24f Compare December 23, 2020 14:48
@kylekatarnls
Copy link
Contributor Author

kylekatarnls commented Dec 23, 2020 •
edited
Loading

PHP 8 support fixed in phpseclib/phpseclib@02fa3b1

So we should now wait for the tag to be released and bump the minimum version 3 to this tag.

@kylekatarnls
Copy link
Contributor Author

kylekatarnls commented Dec 24, 2020

PHP 8 fixed with 3.0.2 version.

@bshaffer bshaffer changed the title Support phpseclib3 feat: support phpseclib3 Dec 28, 2020
@bshaffer bshaffer merged commit e0753f9 into googleapis:master Dec 28, 2020
@kylekatarnls kylekatarnls deleted the feature/phpseclib3-support branch December 28, 2020 18:58
@jdpedrie jdpedrie mentioned this pull request Jan 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bshaffer bshaffer bshaffer approved these changes

+1 more reviewer

@bryanlaura736 bryanlaura736 bryanlaura736 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kylekatarnls @bshaffer @bryanlaura736
X Tutup