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

Revert unintentional nullability changes and fix inconsistencies in StringContent ctrs #83423

Closed
antonfirsov opened this issue Mar 14, 2023 · 2 comments · Fixed by #103442
Closed
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Http in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@antonfirsov
Copy link
Member

antonfirsov commented Mar 14, 2023

This is a follow-up on #81506 (comment). We introduced some inconsistencies and unintentional nullability changes in StringContent's constructors. Chronologically:

#81722 provides a minimal fix, however, since this was an unintentional change, I suggest to revisit it for .NET 8.0, relaxing nullability for all mediaType declarations, explicitly setting text/plain when null is being passed. This should be fairly trivial to implement.

public class StringContent : ByteArrayContent
{
-   public StringContent(string content, MediaTypeHeaderValue  mediaType);
+   public StringContent(string content, MediaTypeHeaderValue? mediaType);
-   public StringContent(string content, Encoding? encoding, string  mediaType);
+   public StringContent(string content, Encoding? encoding, string? mediaType);
-   public StringContent(string content, Encoding? encoding, MediaTypeHeaderValue  mediaType);
+   public StringContent(string content, Encoding? encoding, MediaTypeHeaderValue? mediaType);
}
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Mar 14, 2023
@ghost
Copy link

ghost commented Mar 14, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

This is a follow-up on #81506 (comment). We introduced some inconsistencies and unintentional nullability changes in StringContent's constructors. Chronologically:

Since this was an unintentional change, I suggest to revisit it for .NET 8.0, relaxing nullability for all mediaType arguments, explicitly setting text/plain when null is being passed. This should be fairly trivial to implement.

public class StringContent : ByteArrayContent
{
-   public StringContent(string content, MediaTypeHeaderValue  mediaType);
+   public StringContent(string content, MediaTypeHeaderValue? mediaType);
-   public StringContent(string content, Encoding? encoding, string  mediaType);
+   public StringContent(string content, Encoding? encoding, string? mediaType);
-   public StringContent(string content, Encoding? encoding, MediaTypeHeaderValue  mediaType);
+   public StringContent(string content, Encoding? encoding, MediaTypeHeaderValue? mediaType);
}
Author: antonfirsov
Assignees: -
Labels:

area-System.Net.Http, untriaged

Milestone: -

@ManickaP ManickaP added this to the 8.0.0 milestone Mar 21, 2023
@ManickaP ManickaP removed the untriaged New issue has not been triaged by the area owner label Mar 21, 2023
@MihaZupan MihaZupan added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 21, 2023
@karelz karelz modified the milestones: 8.0.0, Future Jun 13, 2023
@karelz karelz modified the milestones: Future, 9.0.0 Jul 18, 2023
@MihaZupan MihaZupan self-assigned this Jun 11, 2024
@MihaZupan MihaZupan added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 12, 2024
@bartonjs
Copy link
Member

bartonjs commented Jun 13, 2024

Video

The changes look consistent with the code's treatment of those parameters.

public class StringContent : ByteArrayContent
{
-   public StringContent(string content, MediaTypeHeaderValue  mediaType);
+   public StringContent(string content, MediaTypeHeaderValue? mediaType);
-   public StringContent(string content, Encoding? encoding, string  mediaType);
+   public StringContent(string content, Encoding? encoding, string? mediaType);
-   public StringContent(string content, Encoding? encoding, MediaTypeHeaderValue  mediaType);
+   public StringContent(string content, Encoding? encoding, MediaTypeHeaderValue? mediaType);
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 13, 2024
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Jun 13, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Http in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants