-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix XSLT format-date and format-time to output only date or time with empty format string #121028
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
base: main
Are you sure you want to change the base?
Conversation
…meter Fixed MSFormatDateTime to use the isDate parameter when format string is empty. When format is empty, now uses "d" for date-only and "T" for time-only formatting. Added comprehensive tests for both functions. Co-authored-by: stephentoub <[email protected]>
|
@copilot, there are test failures, e.g. |
…String Changed assertion to use regex pattern that matches both 12-hour and 24-hour time formats. The test was failing in en-US culture where "T" format produces "1:02:03 AM" instead of "01:02:03". Co-authored-by: stephentoub <[email protected]>
... Fixed in commit cfb0afc. The test assertion was culture-dependent - it expected "01:02:03" but en-US culture produces "1:02:03 AM" with the "T" format. Changed to use a regex pattern |
src/libraries/System.Private.Xml/tests/Xslt/XslCompiledTransformApi/XslCompilerTests.cs
Outdated
Show resolved
Hide resolved
Removed FormatDateWithExplicitFormatString and FormatTimeWithExplicitFormatString tests. These tests were failing on .NET Framework. The fix is specifically for empty format strings, so focusing tests on that scenario. Co-authored-by: stephentoub <[email protected]>
src/libraries/System.Private.Xml/src/System/Xml/Xsl/Runtime/XsltFunctions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.Xml/tests/Xslt/XslCompiledTransformApi/XslCompilerTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.Xml/tests/Xslt/XslCompiledTransformApi/XslCompilerTests.cs
Outdated
Show resolved
Hide resolved
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.
Pull Request Overview
This PR fixes a regression in XSLT extension methods format-date() and format-time() where both functions incorrectly output both date and time when using an empty format string, instead of outputting only the date or only the time respectively.
Key changes:
- Fixed
MSFormatDateTimeinXsltFunctions.csto use theisDateparameter when format string is empty - Added two focused unit tests to verify the fix for both
format-dateandformat-timefunctions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| XsltFunctions.cs | Modified MSFormatDateTime to apply "d" format for dates and "T" format for times when format string is empty |
| XslCompilerTests.cs | Added two test methods to verify that empty format strings produce date-only or time-only output |
- Replaced manual try-finally culture management with ThreadCultureChange helper - Changed xml and xsl strings to const with PascalCase naming (Xml, Xsl) - Added using System.Tests for ThreadCultureChange Co-authored-by: stephentoub <[email protected]>
src/libraries/System.Private.Xml/src/System/Xml/Xsl/Runtime/XsltFunctions.cs
Show resolved
Hide resolved
Reuse the CultureInfo object returned by GetCultureInfo instead of creating a new one from the locale name. Also removed unused locale variable. Co-authored-by: tarekgh <[email protected]>
src/libraries/System.Private.Xml/tests/Xslt/XslCompiledTransformApi/XslCompilerTests.cs
Show resolved
Hide resolved
src/libraries/System.Private.Xml/tests/Xslt/XslCompiledTransformApi/XslCompilerTests.cs
Show resolved
Hide resolved
Changed FormatDateWithEmptyFormatString and FormatTimeWithEmptyFormatString tests to use Assert.Equal with the expected formatted date/time values instead of checking for presence/absence of substrings. This provides more precise and robust test validation. Co-authored-by: tarekgh <[email protected]>
Fixing XSLT extension methods format-date & format-time to respect the isDate parameter when using empty format string.
Plan:
Issue Summary:
The MSFormatDateTime function in XsltFunctions.cs ignored the
isDateparameter when the format string was empty. This caused both format-date() and format-time() XSLT functions to output both date and time instead of just the date or just the time respectively when using the default (empty) format string.Changes Made:
Added 2 focused tests in XslCompilerTests.cs to verify the fix for empty format strings:
Fixed MSFormatDateTime in XsltFunctions.cs:
Removed tests with explicit format strings that were failing on .NET Framework
Test Results:
Original prompt
This section details on the original issue you should resolve
<issue_title>XSLT extension methods format-date & format-time both output date & time when using the empty string (default) format</issue_title>
<issue_description>### Description
I'm porting a .NET FW application to .NET (6 and above).
The application performs an XML to HTML conversion using an XSLT transform via System.Xml.Xsl.XslCompiledTransform.
Under .NET FW, this XSL line:
<xsl:value-of select="ms:format-date('2001-02-03T01:02:03', '')"/>Produces:
03/02/2001UK date format since that's my locale.
However, under .NET 6 (I've also tried 8), it produces:
03/02/2001 01:02:03Similarly, the format-time function also outputs both the date & time.
Reproduction Steps
It's easy to see there's a problem from the identified source code.
Expected behavior
The default (empty) format string should cause format-date() to output just the date, and correspondingly, format-time() to output just the time - as was the case under .NET FW and is how the functions are documented.
Actual behavior
Both functions output both the date and time.
Regression?
Yes, the functions works as documented for .NET FW.
Known Workarounds
Don't use the default (empty) format string - pass either "d" or "T" to the functions.
Configuration
.NET 6 and above (I've not tried anything < 6)
Other information
Looking at the .NET source code, the issue seems to be in the MSFormatDateTime function. Note that the function doesn't use the "isDate" parameter.
Compare that code with the .NET FW version which clearly uses the isDate parameter to do just the Date or Time appropriately.</issue_description>
Comments on the Issue (you are @copilot in this section)
@ Tagging subscribers to this area: @dotnet/area-system-xml See info in [area-owners.md](https://github.com/dotnet/runtime/blob/main/docs/area-owners.md) if you want to be subscribed.Issue Details
Description
I'm porting a .NET FW application to .NET (6 and above).
The application performs an XML to HTML conversion using an XSLT transform via System.Xml.Xsl.XslCompiledTransform.
Under .NET FW, this XSL line:
<xsl:value-of select="ms:format-date('2001-02-03T01:02:03', '')"/>Produces:
03/02/2001UK date format since that's my locale.
However, under .NET 6 (I've also tried 8), it produces:
03/02/2001 01:02:03Similarly, the format-time function also outputs both the date & time.
Reproduction Steps
It's easy to see there's a problem from the identified source code.
Expected behavior
The default (empty) format string should cause format-date() to output just the date, and correspondingly, format-time() to output just the time - as was the case under .NET FW and is how the functions are documented.
Actual behavior
Both functions output both the date and time.
Regression?
Yes, the functions works as documented for .NET FW.
Known Workarounds
Don't use the default (empty) format string - pass either "d" or "T" to the functions.
Configuration
.NET 6 and above (I've not tried anything < 6)
Other information
Looking at the .NET source code, the issue seems to be in the MSFormatDateTime function. Note that the function doesn't use the "isDate" parameter.
Compare that code with the .NET FW version which clearly uses the isDate parameter to do just the Date or Time appropriately.
area-System.Xml,untriagedWe should figure out why that change was made and make sure we don't regress issue being fixed there while fixing this.
@Dave-Lowndes are you perhaps interested in making a pull request?</comment_new>
<comment_new>@krwq
I agree that would be ideal but they're no longer on our team (and even if I doubt anyone...
Fixes #93189
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.