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

[API Proposal]: Make AddBinaryFormattedResource obsolete #88217

Closed
gewarren opened this issue Jun 29, 2023 · 4 comments · Fixed by #90346
Closed

[API Proposal]: Make AddBinaryFormattedResource obsolete #88217

gewarren opened this issue Jun 29, 2023 · 4 comments · Fixed by #90346
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Resources blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@gewarren
Copy link
Contributor

Background and motivation

Most of the remaining legacy serialization APIs were obsoleted in #84383. I'm proposing that we obsolete the System.Resources.Extensions.PreserializedResourceWriter.AddBinaryFormattedResource method as well. I ran this by Levi and some others in an email:

From: Rainer Sigwald
Sent: Monday, June 26, 2023 7:09 AM
Subject: RE: AddBinaryFormattedResource method

Yeah, I’d be totally fine with obsoleting it but we will need it to keep working indefinitely. It’ll be an important scenario for a long time to support targeting .NET Framework apps from dotnet build.

From: Eric St. John
Sent: Friday, June 23, 2023 5:17 PM
Subject: RE: AddBinaryFormattedResource method

As you mention, it doesn’t itself use BinaryFormatter. It’s just writing resource data to a .resources format that will be treated as binary formatted data when that .resources file is eventually loaded.

We could obsolete it, the only person using it right now is MSBuild and they’ll just suppress that obsoletion because they need to continue to support writing those resources – warnings need to go upstream where users choose this format.

Adding the obsoletion here would discourage others from using it, imagining it as some kind of “safe” backdoor to getting binary-formatted resources loaded. I’m OK with us making it obsolete – but we can’t remove it.

API Proposal

[Obsolete ...]
public void AddBinaryFormattedResource(string name, byte[] value, string? typeName = null)

API Usage

N/A

Alternative Designs

No response

Risks

No response

@gewarren gewarren added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 29, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 29, 2023
@ghost
Copy link

ghost commented Jun 29, 2023

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

Issue Details

Background and motivation

Most of the remaining legacy serialization APIs were obsoleted in #84383. I'm proposing that we obsolete the System.Resources.Extensions.PreserializedResourceWriter.AddBinaryFormattedResource method as well. I ran this by Levi and some others in an email:

From: Rainer Sigwald
Sent: Monday, June 26, 2023 7:09 AM
Subject: RE: AddBinaryFormattedResource method

Yeah, I’d be totally fine with obsoleting it but we will need it to keep working indefinitely. It’ll be an important scenario for a long time to support targeting .NET Framework apps from dotnet build.

From: Eric St. John
Sent: Friday, June 23, 2023 5:17 PM
Subject: RE: AddBinaryFormattedResource method

As you mention, it doesn’t itself use BinaryFormatter. It’s just writing resource data to a .resources format that will be treated as binary formatted data when that .resources file is eventually loaded.

We could obsolete it, the only person using it right now is MSBuild and they’ll just suppress that obsoletion because they need to continue to support writing those resources – warnings need to go upstream where users choose this format.

Adding the obsoletion here would discourage others from using it, imagining it as some kind of “safe” backdoor to getting binary-formatted resources loaded. I’m OK with us making it obsolete – but we can’t remove it.

API Proposal

[Obsolete ...]
public void AddBinaryFormattedResource(string name, byte[] value, string? typeName = null)

API Usage

N/A

Alternative Designs

No response

Risks

No response

Author: gewarren
Assignees: -
Labels:

api-suggestion, area-System.Resources

Milestone: -

@steveharter
Copy link
Member

@rainersigwald is this OK to do in v8 - does dotnet build need to be updated to adjust to any compiler warning\error?

@rainersigwald
Copy link
Member

Go ahead; when we get the new warning we'll add an exception in our codebase.

@steveharter steveharter removed the untriaged New issue has not been triaged by the area owner label Jul 10, 2023
@steveharter steveharter added this to the 8.0.0 milestone Jul 10, 2023
@steveharter steveharter added 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 and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 10, 2023
@steveharter steveharter self-assigned this Jul 13, 2023
@terrajobst
Copy link
Member

terrajobst commented Jul 18, 2023

Video

  • Makes sense as proposed
  • We should probably reuse an existing diagnostic ID for binary formatter related obsoletions
namespace System.Resources.Extensions;

public partial class PreserializedResourceWriter
{
     [Obsolete(...)]
     public void AddBinaryFormattedResource(string name, byte[] value, string? typeName = null);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 18, 2023
@ericstj ericstj assigned ericstj and unassigned steveharter Aug 9, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 10, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 11, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators Sep 10, 2023
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.Resources blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants