patrickbussmann / oauth2-apple

Sign in with Apple Provider for the OAuth 2.0 Client
MIT License
95 stars 64 forks source link

Replace fproject/php-jwt with firebase/php-jwt #7

Closed adrianbardan closed 4 years ago

adrianbardan commented 4 years ago

Both fproject/php-jwt and firebase/php-jwt use the same namespace: Firebase\JWT.

When using another library that depends on firebase/php-jwt (for example: league/oauth2-google) there will be a conflict and the call JWK::parseKeySet(...) in src/Token/AppleAccessToken.php will fail because the method definition from firebase/php-jwt does not accept a string as parameter.

This issue has been reported here: #5

To fix this I've replaced the fproject/php-jwt dependency with firebase/php-jwt and I've update the parameter for the JWK::parseKeySet(...) call to be a string.

firebase/php-jwt and fproject/php-jwt seem to be almost identical and firebase/php-jwt seems to still be maintained.

Unit testing passed successfuly, but I don't know for sure if there are any other ripple effects.

patrickbussmann commented 4 years ago

Thanks for the pull request @adrianbardan. I'll check within the next week.

Z3R6 commented 4 years ago

Guys, any updates?

samhibberd commented 4 years ago

Would be great to see this merged.

deguif commented 4 years ago

@patrickbussmann could you release a new version with this fix, please?

deguif commented 4 years ago

I just submitted a PR to fix a version constraint mismatch that can install a version of the package firebase/php-jwt which hasn't the class JWK available See https://github.com/patrickbussmann/oauth2-apple/pull/11

patrickbussmann commented 4 years ago

@deguif I checked it. But it was okay because Travis already testing this. The version was ^5.0 so when you use composer install it will automatically use 5.2. But its ok. 👍

deguif commented 4 years ago

Yes @patrickbussmann on a fresh install it works. For my case, I already had the package (firebase/php-jwt) installed at a previous version and so it wasn't reinstalled nor upgraded until I updated it manually which leads to an error with the code relying on JWK class ;)

deguif commented 4 years ago

Thanks for the release, this avoids to use the master version any longer ;)