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

Allow null as redirect_route #381

Merged
merged 1 commit into from
Oct 31, 2022

Conversation

toooni
Copy link
Contributor

@toooni toooni commented Oct 31, 2022

The functionality “Fetching access keys via OAuth2 to be used with an API” mentioned in the README does not necessarily need a redirect_url, thus providing it should not be mandatory. We have an Endpoint (pingen.com) which does not allow redirect_url to be sent in these cases. Also, it wouldn't make much sense if it did because the route must be a valid symfony route, which in some cases must be created without a use.
I also think it makes sense to allow null here because this parameter isn't mandatory in the League\OAuth2\Client\ProviderAbstractProvider (https://github.com/thephpleague/oauth2-client/blob/master/src/Provider/AbstractProvider.php#L454).

@weaverryan
Copy link
Member

Thanks Toni - this makes sense :)

@weaverryan weaverryan merged commit 355fb37 into knpuniversity:master Oct 31, 2022
@toooni toooni deleted the empty_redirect_route branch October 31, 2022 14:03
@toooni toooni restored the empty_redirect_route branch October 31, 2022 14:04
@toooni
Copy link
Contributor Author

toooni commented Oct 31, 2022

@weaverryan Thank you for the fast merge 👍 Just fyi: I did not change anything about the config. redirect_route is still required there, but using redirect_route: null will work. I am not sure if redirect_url should have a default value null since this bundle is mostly used for purposes which require a redirect_route.

@weaverryan
Copy link
Member

I am not sure if redirect_url should have a default value null since this bundle is mostly used for purposes which require a redirect_route.

I agree. For the moment, I think we should keep the config required so that it's not "missed".

@toooni toooni deleted the empty_redirect_route branch November 1, 2022 14:38
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.

2 participants