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.

Sign up for GitHub

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

Jump to bottom

feat: support phpseclib3 #2019

Merged
merged 4 commits into from
Dec 28, 2020
Merged

feat: support phpseclib3 #2019

merged 4 commits into from
Dec 28, 2020

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 as a code owner 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

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

@bshaffer
Copy link
Contributor

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

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
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

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

@bryanlaura736 bryanlaura736 bryanlaura736 approved these changes

@bshaffer bshaffer bshaffer approved these changes

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