Breaking change for v1.8.0

[tobytwigger]

Hi all! I'm not sure what's considered the public API of this package, but the most recent update has broken the socialiteproviders/providers package which allows social logins in Laravel. It'll also have broken any custom OAuth providers which extend the League\OAuth1\Client\Server\Server class, due to the typehinting of the methods.

In this particular case, it's made more problematic by the fact that Laravel socialite defines its own user to return, so the typehintinh for getUserDetails cannot be met without changes in several repositories.

Was this release meant as a minor release, or should it be upgraded to a major to stop any existing extensions being broken?

[bencorlett]

Interesting. The public api always indicated Via docblocks that an instance of OAuth1\Client\User should be returned so to not follow that would be mutually exclusive to expecting no breakages. Having said that, I think if we are type declaring on other methods and that’s breaking inheritance, I will revert this change for now and it’ll just come with the new version (see PRs). Thanks for the report!

…

Sent from my iPhone

On 4 Sep 2020, at 6:15 pm, tobytwigger ***@***.***> wrote:  Hi all! I'm not sure what's considered the public API of this package, but the most recent update has broken the socialiteproviders/providers package which allows social logins in Laravel. It'll also have broken any custom OAuth providers which extend the League\OAuth1\Client\Server\Server class, due to the typehinting of the methods. In this particular case, it's made more problematic by the fact that Laravel socialite defines its own user to return, so the typehintinh for getUserDetails cannot be met without changes in several repositories. Was this release meant as a minor release, or should it be upgraded to a major to stop any existing extensions being broken? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[tobytwigger]

Thank you! I think the reasoning was that the user model is used for OAuth2 too. Either way, a major version change will give time to fix this before upgrading - thanks! :)

[bencorlett]

@tobytwigger would you be able to help test this for me? If so, could you add:

"league/oauth1-client": "dev-fix/revert-public-api"

To your composer.json and run composer update?

[bencorlett]

Here's a comparison between 1.7 and the fix branch. I think it all looks good, it's passing unit tests, linting and static analysis. A real world test would be great though and I will push out 1.8.1 with this.

[tobytwigger]

@bencorlett Just tested with our Laravel app using socialite, all working perfectly!

[bencorlett]

@bencorlett Just tested with our Laravel app using socialite, all working perfectly!

Great! Thank you. This was a big oversight on my part and I really wanted to resolve this quickly. I've done some more testing on my end and I'm pretty comfortable it's looking good.

I'll be pushing v1.8.1 shortly with this in it :)

[tobytwigger]

Thanks a lot!

[bencorlett]

You're welcome. Thanks for pointing the issue out and helping resolve it :)

I'm just waiting on builds to pass then I'll tag up v1.8.1.

[shehi]

That's why it shouldn't have been 1.8 release but 2.0 release, to avoid BC implications 🤦 Now all that work done towards PHP 7.1-7.2 was undone, and twice at that.

[bencorlett]

That's why it shouldn't have been 1.8 release but 2.0 release, to avoid BC implications 🤦 Now all that work done towards PHP 7.1-7.2 was undone, and twice at that.

🤦‍♂️🤣👍👍

v2.0 (develop) is a rewrite. It took about 25 minutes to add php 7.1+ Language features to v1.8 and less to revert it, so I’m not too worried.

I am not interested in releasing 2.0, then 3.0 a few days later, I think this would not be worth the overhead.

[bencorlett]

@shehi i should add - do let me know your thoughts on v2 - I’d appreciate that.

I will be documenting it shortly and polishing it up!