Add back IHtmlString to System.Web.HttpUtility #85673
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #83477
|
src/libraries/System.Web.HttpUtility/tests/HttpUtility/HttpUtilityTest.cs
Outdated
Show resolved
Hide resolved
| value switch | ||
| { | ||
| null => null, | ||
| IHtmlString ihs => ihs.ToHtmlString() ?? string.Empty, |
There was a problem hiding this comment.
Nit: the Framework implementation doesn't alter null to string.Empty.
There was a problem hiding this comment.
I went back and forth on it... part of the problem is since .NET Framework we've annotated the method as returning non-null.
Also, even for the non-IHtmlString case, Framework allows null to be returned and core doesn't, right? I'd need to double check but I believe that's what I saw. In which case this is just staying consistent with what the API is doing for other inputs.
There was a problem hiding this comment.
Also, even for the non-IHtmlString case, Framework allows null to be returned and core doesn't, right?
Yes - a class that returns a null value from .ToString() returns a null value on framework, but a string.Empty on core.
There was a problem hiding this comment.
Also - should IHtmlstring.ToHtmlString() be nullable since we're checking it here?
There was a problem hiding this comment.
You tell me :-) My assumption was we don't expect null to be valid; the check is just in case. But if null is valid, then yes, this should be nullable. In all the implementations you've seen, is null expected?
There was a problem hiding this comment.
Sorry - I'm out sick and slow responding :) I found a number of implementations in the wild and there are no checks to validate if it is null or not, and the equivalent type on ASP.NET Core accepts a nullable string. Seems like it should be nullable.
There was a problem hiding this comment.
I don't have a strong opinion here, though, as the type itself is the important thing.
There was a problem hiding this comment.
I've changed it in #85742.
…lityTest.cs Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
| value switch | ||
| { | ||
| null => null, | ||
| IHtmlString ihs => ihs.ToHtmlString() ?? string.Empty, |
There was a problem hiding this comment.
Also, even for the non-IHtmlString case, Framework allows null to be returned and core doesn't, right?
Yes - a class that returns a null value from .ToString() returns a null value on framework, but a string.Empty on core.
There was a problem hiding this comment.
Given that we already do ?? string.Empty for ToString(), doing the same for ToHtmlString() makes sense to me even if it's not annotated to be nullable. I tried searching for ": IHtmlString" in grep.app, but quickly realized that I can't rightly determine if any of those implementations can return null without nullable annotations. I assume it's rare though.
Fixes #83477