Reapply "Fix DateFormatter TimeZone Setter" #1725
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Because of the use of 'timeZone' instead of 'newValue' in the setter, setting the timeZone of a DateFormatter doesn't actually work. Instead, it winds up re-setting the old value, which happens to be whatever was originally set, usually what we get from copying the default down in CoreFoundation. Also adds a unit test to help prevent breaks like this again.
This one is nasty, since it creates problems for DateFormatter, while appearing to behave correctly. (Mostly) By copying 1 too many bytes, we wind up with the null terminator in the CFString. This breaks DateFormatter in an annoying way that the system TimeZone is always considered to be GMT by the formatter. You can ask for the zone’s abbreviation, and it will be correct. The name will appear to be correct, except the length is 1 longer than it should be. All sorts of fun. But the moment you ask DateFormatter to give you a string (or parse one), it assumes the TimeZone is GMT and ruins your day. Included is a test case which can detect this. The downside is that it doesn’t detect the problem if GMT is already the current TimeZone. It is possible a second, more targeted test should also be added here? This also breaks getting the description on the NSTimeZone/CFTimeZone, but not the TimeZone because of the wrapping at play, because of the terminator that gets inserted into the string description.
Cleans up the DateFormatter test, improves the information about how it doesn’t catch issues when the system time zone is already GMT, and adds a more focused test for the SystemTimeZone name issue that can catch it when GMT is the system time zone.
The test assumes that it is running as GMT/UTC. Because of the TimeZone.current fixes, the test is no longer guaranteed to be using GMT/UTC, and will use the system TimeZone and breaks when in PDT for example. By making the UTC assumption explicit, we remove one more variable on this test.
This is hitting an issue on GMT time zones where it shows up as UTC when formatted.
|
@swift-ci test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a fix for PR #1704, which was reverted by #1721 due to a break in the test I added in the original PR.
This includes rebased versions of the commit history of #1704, plus a new commit that disables Case 1 of the test which was failing in the case of GMT, because DateFormatter was spitting out UTC as the abbreviation. I've opened https://bugs.swift.org/browse/SR-8994 to track the issue of TimeZone's abbreviation and DateFormatter not agreeing on GMT vs UTC.