-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Proposed fix for issue XSLT extension methods format-date & format-time #97402
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
Conversation
Rework MSFormatDateTime to use the isDate parameter to try and reproduce the prior .NET FW version behaviour that used the Win32 GetDateFormat & GetTimeFormat APIs to only output date or time parts.
|
Tagging subscribers to this area: @dotnet/area-system-xml Issue DetailsRework MSFormatDateTime to use the isDate parameter to try and reproduce the prior .NET FW version behaviour that used the Win32 GetDateFormat & GetTimeFormat APIs to only output date or time parts.
|
|
@dotnet-policy-service agree |
|
Thanks, can you add a test? |
src/libraries/System.Private.Xml/src/System/Xml/Xsl/Runtime/XsltFunctions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.Xml/src/System/Xml/Xsl/Runtime/XsltFunctions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.Xml/src/System/Xml/Xsl/Runtime/XsltFunctions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.Xml/src/System/Xml/Xsl/Runtime/XsltFunctions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.Xml/src/System/Xml/Xsl/Runtime/XsltFunctions.cs
Outdated
Show resolved
Hide resolved
I can't even build the project, so adding a test is beyond my (lack of) pay grade. |
|
@krwq, could you kindly review and assess the proposed change? |
src/libraries/System.Private.Xml/src/System/Xml/Xsl/Runtime/XsltFunctions.cs
Show resolved
Hide resolved
src/libraries/System.Private.Xml/src/System/Xml/Xsl/Runtime/XsltFunctions.cs
Show resolved
Hide resolved
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
eiriktsarpalis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per reviewer feedback, some basic regression testing is required before we merge this.
|
Having already tested this change to my own satisfaction (via a stand-alone test project before I submitted the PR), if there's no one available and able to add unit tests you may as well abandon this pull request now. |
|
@Dave-Lowndes if you share code you've used for testing I can turn that into unit-test and add to this PR on your behalf |
|
@krwq There's not much to speak of. All I did was copy the proposed function change into a C# console project and fake out the internal XsdDateTime lines and make some test calls to it. You presumably won't need to replace XsdDateTime code. internal class Program
{
// Copied from the internal code, you won't need this:
private static CultureInfo GetCultureInfo(string lang)
{
Debug.Assert(lang != null);
if (lang.Length == 0)
{
return CultureInfo.CurrentCulture;
}
else
{
try
{
return new CultureInfo(lang);
}
catch (System.ArgumentException)
{
// throw new XslTransformException(SR.Xslt_InvalidLanguage, lang);
throw;
}
}
}
public static string MSFormatDateTime(string dateTime, string format, string lang, bool isDate)
{
try
{
// XsdDateTime is private, so fake this out and use DateTime
#if true
DateTime dt = DateTime.Parse(dateTime);
#else
if (!XsdDateTime.TryParse(dateTime, XsdDateTimeFlags.AllXsd | XsdDateTimeFlags.XdrDateTime | XsdDateTimeFlags.XdrTimeNoTz, out XsdDateTime xdt))
{
return string.Empty;
}
DateTime dt = xdt.ToZulu();
#endif
CultureInfo ci = string.IsNullOrEmpty(lang) ?
CultureInfo.CurrentCulture :
GetCultureInfo(lang);
if (isDate)
{
DateOnly dateOnly = DateOnly.FromDateTime(dt);
return dateOnly.ToString(!string.IsNullOrEmpty(format) ? format : null, ci);
}
else
{
TimeOnly timeOnly = TimeOnly.FromDateTime(dt);
if (!string.IsNullOrEmpty(format))
{
return timeOnly.ToString(format, ci);
}
else
{
// Duplicate what the Win32 GetTimeFormat API returns
// with an empty format - the long time string.
return timeOnly.ToLongTimeString();
}
}
}
catch (Exception ex) when (ex is ArgumentException or FormatException)
{
return string.Empty;
}
}
static void Main(string[] args)
{
string result;
var dtStr = DateTime.Now.ToString();
Console.WriteLine($"Result: {dtStr}");
result = MSFormatDateTime( dtStr, string.Empty, CultureInfo.CurrentCulture.Name, true);
Console.WriteLine($"Result: {result}");
result = MSFormatDateTime( dtStr, string.Empty, CultureInfo.CurrentCulture.Name, false);
Console.WriteLine($"Result: {result}");
result = MSFormatDateTime(dtStr, "D", "", true);
Console.WriteLine($"Result: {result}");
result = MSFormatDateTime(dtStr, "T", "", false);
Console.WriteLine($"Result: {result}");
result = MSFormatDateTime(dtStr, "d", "", true);
Console.WriteLine($"Result: {result}");
result = MSFormatDateTime(dtStr, "t", "", false);
Console.WriteLine($"Result: {result}");
}
} |
|
I The template XSLT is: <?xml version="1.0" encoding="UTF-8" ?>
<xsl:stylesheet version="1.0"
xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
xmlns:date="http://exslt.org/dates-and-times"
xmlns:ms="urn:schemas-microsoft-com:xslt"
exclude-result-prefixes="date">
<xsl:output indent="yes" omit-xml-declaration="yes"/>
<xsl:template match="data">
<out>
<dates>
<test1>
<xsl:value-of select="ms:format-date(date, 'D')"/>
</test1>
<test2>
<xsl:value-of select="ms:format-date(date, 'd')"/>
</test2>
<test3>
<xsl:value-of select="ms:format-date(date, '')"/>
</test3>
<test4>
<xsl:value-of select="ms:format-date(date, 'd', 'en-US')"/>
</test4>
<test5>
<xsl:value-of select="ms:format-date(date, 'D', 'fr-FR')"/>
</test5>
<test6>
<xsl:value-of select="ms:format-date(date, 'd', 'fr-FR')"/>
</test6>
</dates>
<times>
<test1>
<xsl:value-of select="ms:format-time(date, 'T')"/>
</test1>
<test2>
<xsl:value-of select="ms:format-time(date, 't')"/>
</test2>
<test3>
<xsl:value-of select="ms:format-time(date, '')"/>
</test3>
<test4>
<xsl:value-of select="ms:format-date(date, 't', 'en-US')"/>
</test4>
<test5>
<xsl:value-of select="ms:format-date(date, 'T', 'fr-FR')"/>
</test5>
<test6>
<xsl:value-of select="ms:format-date(date, 't', 'fr-FR')"/>
</test6>
</times>
</out>
</xsl:template>
</xsl:stylesheet>
|
|
I added unit-tests that currently PASS in PR #113507 |
|
Now that the unit test for this have landed in PR #113507 (and are coded for the old and wrong return values), we need to update the test data in this PR before merging or the new unit tests will fail We need to include an update to the expected values in bug93189.xml test data changing: out/dates/test3 to be |
Fixes #93189
Rework MSFormatDateTime to use the isDate parameter to try and reproduce the prior .NET FW version behaviour that used the Win32 GetDateFormat & GetTimeFormat APIs to only output date or time parts.
EDIT (@krwq): added
Fixes ...syntax so that PR is linked with the issue