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

New clients added #67

Merged
merged 9 commits into from
Nov 3, 2017
Merged

New clients added #67

merged 9 commits into from
Nov 3, 2017

Conversation

luchianenco
Copy link
Contributor

Another portion of clients: Salesforce, Zendesk, Strava, Uber, Unsplash, PSN, Mollie are added

Copy link
Member

@bocharsky-bw bocharsky-bw left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@weaverryan weaverryan merged commit 0bca7b4 into knpuniversity:master Nov 3, 2017
@weaverryan
Copy link
Member

Ahh... so cool! You rock @luchianenco! Check out our clients list now - https://github.com/knpuniversity/oauth2-client-bundle#step-1-download-the-client-library - 35! Woo!

@luchianenco
Copy link
Contributor Author

thanks ;)
many providers are a bit outdated...do not support ResourceOwnerInterfacer

@weaverryan
Copy link
Member

Many in our list are outdated? Or many other client libraries are outdated (so we can't add them)?

@luchianenco
Copy link
Contributor Author

luchianenco commented Nov 4, 2017

Many client libraries doesn't support ResourceOwnerInterface and therefore we cannot use them as I understand.

@weaverryan
Copy link
Member

Hmm, yea that makes sense -we require them to return that. Well, probably in those cases, those libraries aren't very used anyways :)

@luchianenco
Copy link
Contributor Author

Some of them, what I would like to add in the first row are Envato, MeetUp, Reddit, Twitch and some others that have really big usage...there are many also that we can add but as I understand they are not so popular.

@weaverryan
Copy link
Member

Hmm. Yea, I looked at a few of those. They're indeed implemented incorrectly - e.g. https://github.com/tpavlek/oauth2-twitch/blob/ead8e17d393610b35d06f6e42723a75590833a3c/src/Provider/Twitch.php#L130 should return a ResourceOwnerInterface (https://github.com/thephpleague/oauth2-client/blob/a887c33ff02ec7f92029903e9e7e0fc5a1c52583/src/Provider/AbstractProvider.php#L742), but it does not.

That's a bug in their code. So, to support these, the libraries themselves would need to be improved, or replaced with new libs :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants