[release/7.0] Fix incorrect "illegal XML comment chars" check & add regression tests #74823
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
ghost
added
the
|
Tagging subscribers to this area: @dotnet/area-system-xml Issue DetailsBackport of #74787 to release/7.0 Customer ImpactTestingRiskIMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.
|
GrabYourPitchforks
added
the
Servicing-consider
danmoseley
removed
the
Servicing-consider
|
(No servicing label required while in M2 mode as we are in 7.0 still) .. Approved, basic functionality regressed. |
|
@eiriktsarpalis or @krwq can we get a code review sign off, please? |
|
CI failure is tracked by #73688. |
|
I'm not authorized to check in to this branch. The great privilege of hitting the merge button falls to somebody else. :) |
I gladly volunteer as tribute. Just waiting for CI to finish. |
|
Approved and signed-off. |
Backport of #74787 to release/7.0. Resolves #74752.
Customer Impact
As of 7.0,
XmlTextWritercan no longer write XML comments which begin with a hyphen ('-') character. This was due to an incorrect optimization in System.Xml introduced in the 7.0 timeframe. We should forbid hyphens at the end of the string, not at the beginning.This is a regression from 6.0 and Full Framework. Partner teams like F# have had to work around this in their own code bases since it's affecting some of their build scenarios.
Workaround for customers is that if they call
XmlWriter.WriteComment, they ensure that the argument does not begin with a hyphen. I don't imagine a hyphened prefix is all that common, but as above, the F# team did run into it. Fortunately they controlled the input and had the ability to strip the hyphen. Other customers might not be able to exercise such control.Testing
Tested through ad hoc sample applications and increasing unit test coverage. I also compared the outputs against core3.1, 6.0, and netfx481 to validate that we're not morphing the data unexpectedly.
I also went through and double-checked the original PR which introduced the regression, looking for other places where we may have introduced a similar error. That investigation turned up only a single additional suspicious call site, but that call site was already addressed by a previous PR before RC1 snap. The call site addressed by this PR is the only remaining call site.
Risk
Low risk. This changes a single line of code, and I've improved unit test coverage both as a regression test and to ensure that we didn't inadvertently block other legitimate scenarios.
No packaging impact.