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

Change underlying library for discord because it's not compliant with oauth2-client v2.x #90

Merged
merged 3 commits into from
Feb 26, 2018

Conversation

tristanbes
Copy link
Contributor

Because the previous library is only compatible with the ultra old thephpleague/oauth2-client in version 1.x.

The documentation no longer reference the library https://github.com/teamreflex/oauth2-discord. Instead, they promote https://github.com/wohali/oauth2-discord-new.

See: http://oauth2-client.thephpleague.com/providers/thirdparty/

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.

In src/Client/Provider/DiscordClient.php the namespace of Discord\OAuth\Parts\User probably also should be updated.

README.md Outdated
@@ -66,7 +66,7 @@ via Composer:
| [Clever](https://github.com/schoolrunner/oauth2-clever) | composer require schoolrunner/oauth2-clever |
| [DevianArt](https://github.com/SeinopSys/oauth2-deviantart) | composer require seinopsys/oauth2-deviantart |
| [DigitalOcean](https://github.com/chrishemmings/oauth2-digitalocean) | composer require chrishemmings/oauth2-digitalocean |
| [Discord](https://github.com/teamreflex/oauth2-discord) | composer require team-reflex/oauth2-discord |
| [Discord](https://github.com/wohali/oauth2-discord-new) | composer require team-reflex/oauth2-discord |
Copy link
Member

Choose a reason for hiding this comment

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

Should be "... require wohali/oauth2-discord-new" in the end, you changed the link URL only

@bocharsky-bw
Copy link
Member

bocharsky-bw commented Feb 22, 2018

On one hand, it makes sense to move Discord to the OAuthClient v2, but on the other - we may provide a BC break with this PR. Probably to avoid BC breaks we need to keep both? Or, maybe a bit complicate the logic with some extra checks like if the new class from wohali/oauth2-discord-new is exist - use it, otherwise, fallback to the class from teamreflex/oauth2-discord? Anyway, I think we need to avoid bumping major version after this PR

@tristanbes
Copy link
Contributor Author

I fixed your comments. good catch. Also, the edits from maintainers are activated if you want to provide any kind of fallbacks (which is overrated imo)

@weaverryan weaverryan merged commit ee11f72 into knpuniversity:master Feb 26, 2018
@weaverryan
Copy link
Member

Thanks @tristanbes!

And yea, I agree - this is a small BC break, but I also really don't want to deal with fallback logic. Hopefully this type of thing will be rare.

@tristanbes
Copy link
Contributor Author

Maybe just add it in UPGRADE or Readme to tell user that they need to require the new lib instead of the previous one. And everything will work the same ;-)

My pleasure.

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