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

Twitch helix support #361

Merged
merged 12 commits into from
Feb 22, 2022
Merged

Conversation

ErdemUyanik
Copy link
Contributor

Added support for the vertisan/oauth2-twitch-helix bundle

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.

Cool, thanks for digging on this deeper!

Also, please, take a look at failed builds


public function getProviderDisplayName()
{
return 'Twitch';
Copy link
Member

Choose a reason for hiding this comment

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

Hm, probably Twitch Helix to be different from TwitchProviderConfigurator's name?

README.md Outdated
@@ -1407,6 +1408,8 @@ knpu_oauth2_client:
# will create service: "knpu.oauth2.client.twitch"
# an instance of: KnpU\OAuth2ClientBundle\Client\Provider\TwitchClient
# composer require depotwarehouse/oauth2-twitch
# This is deprecated and won't be supported by Twitch in the near future (Feb. 28 2022)
# use the twitch_helix one (directly below) if you create a new app
Copy link
Member

Choose a reason for hiding this comment

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

This is auto-generated section. Please, run bin/update_readme instead to update this correctly.

Also, if the old Twitch way is deprecated, please, update its name in TwitchProviderConfigurator::getProviderDisplayName() to Twitch (deprecated)


public function getProviderClass(array $config)
{
return 'Vertisan\OAuth2\Client\Provider\TwitchHelix';
Copy link
Member

Choose a reason for hiding this comment

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

Can you add use statement for this and below in getClientClass()? Then use TwitchHelix::class.

This is not done this way in most providers, simply because, iirc, they predate PHP 5.5 when this was added :)

@bocharsky-bw
Copy link
Member

This one is ready for review, I applied changes and fixed tests here

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.

@ErdemUyanik Thank you for starting working on it! This looks good to me now.

If you could give this new Twitch Helix with the latest changes a try - it would be great :)

Copy link
Collaborator

@jrushlow jrushlow left a comment

Choose a reason for hiding this comment

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

Awesome work @ErdemUyanik && @bocharsky-bw

@jrushlow jrushlow merged commit ed13752 into knpuniversity:master Feb 22, 2022
@jrushlow jrushlow mentioned this pull request Feb 23, 2022
@jrushlow jrushlow added the Feature New Feature label Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants