Skip to content
This repository has been archived by the owner on Jun 24, 2021. It is now read-only.

Compability for .NET Core 3.0 (preview 6) #12

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Herdo
Copy link

@Herdo Herdo commented Jul 19, 2019

In accordance with dotnet/aspnetcore#3756 and dotnet/aspnetcore#3612 this pull requests creates compability for .NET Core 3.0.

This PR contains the breaking change that the library has to target .NET Core 3.0.

Primary changes in Discord.OAuth2.csproj and DiscordHandler.cs.
Changes in Discord.OAuth2.targets are optional, eventually to be replaced later.

@Herdo
Copy link
Author

Herdo commented Jul 19, 2019

This PR should be the solution for #11.

@@ -28,9 +28,9 @@ protected override async Task<AuthenticationTicket> CreateTicketAsync(ClaimsIden
if (!response.IsSuccessStatusCode)
throw new HttpRequestException($"Failed to retrieve Discord user information ({response.StatusCode}).");

var payload = JObject.Parse(await response.Content.ReadAsStringAsync());
var payload = JsonDocument.Parse(await response.Content.ReadAsStringAsync());

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

The new JsonDocument type implements IDisposable, so you should add a using here:

https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/blob/5a7bdf0d5708fbfe1e9f86e3aa8d4881475f1cca/src/AspNet.Security.OAuth.Discord/DiscordAuthenticationHandler.cs#L51

I hope it's OK to use the root element (JsonElement) outside the disposed scope?
See a3177e1

Choose a reason for hiding this comment

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

I don’t think so. It should be disposed when you’ve finished reading anything from it. If you look at the Microsoft OAuth providers for 3.0 they do the same as the code in the link I posted.

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

Successfully merging this pull request may close these issues.

2 participants