Page MenuHomePhabricator

Add date format preference and respect it in date selection controls
ClosedPublic

Authored by lpriestley on Jun 12 2015, 8:00 PM.

Details

Summary

Ref T8362, Add date format preference and respect it in date selection controls

Test Plan

Set date format preference in the user settings panels, create new event, select new start date in the correct format.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

lpriestley updated this revision to Diff 32054.Jun 12 2015, 8:00 PM
lpriestley retitled this revision from to Add date format preference and respect it in date selection controls.
lpriestley updated this object.
lpriestley edited the test plan for this revision. (Show Details)
lpriestley added a reviewer: epriestley.
lpriestley updated this revision to Diff 32055.Jun 12 2015, 8:02 PM
lpriestley edited edge metadata.

Adding Big-Endian

unneeded viewer variable

eadler added a subscriber: eadler.Jun 13 2015, 2:19 AM
eadler added inline comments.
src/applications/settings/panel/PhabricatorDateTimeSettingsPanel.php
81

Can you add ISO 8601 as well (big endian with - as a separator)

jasonrumney added inline comments.
src/applications/settings/panel/PhabricatorDateTimeSettingsPanel.php
81

Probably just changing the "Big-Endian" format is enough, as the countries that use that order in practice do not use / as a separator (they use Chinese or Korean characters but ISO format will be familiar to them too).

eadler added inline comments.Jun 13 2015, 3:53 AM
src/applications/settings/panel/PhabricatorDateTimeSettingsPanel.php
81

That's fine with me too.

lpriestley updated this revision to Diff 32069.Jun 13 2015, 4:38 AM

Big-Endian format should be separated by dashes instead of slashes.

src/applications/settings/panel/PhabricatorDateTimeSettingsPanel.php
81

Yep, this makes sense.

epriestley requested changes to this revision.Jun 13 2015, 3:05 PM
epriestley edited edge metadata.

I don't think this will actually work, because there are no meaningful parser changes on the server side, so I don't see how it can distinguish between US and non-US formats after the form is submitted. Specifically, try this:

  • Set date format to European (d/m/y).
  • Create an event on an ambiguous date (2/3/2015 -- should mean March 2).

I expect the date will be parsed as Feb 3 after you actually save the form, because the PHP code doesn't know that it's supposed to parse things the other way. Does that actually happen, or do things work correctly?

If this does happen, just mangling it before parsing it is a possible approach:

Dates in the m/d/y or d-m-y formats are disambiguated by looking at the separator between the various components: if the separator is a slash (/), then the American m/d/y is assumed; whereas if the separator is a dash (-) or a dot (.), then the European d-m-y format is assumed.
http://php.net/strtotime

strotime() should be the same logic used by new Date().

Otherwise you could try parsing first, then fall back to new Date() to catch ISO 8601. You have to fall back or otherwise end up in new Date() to handle "3 days ago".

src/applications/settings/panel/PhabricatorDateTimeSettingsPanel.php
83–84

Maybe consider calling these "European" (or "Non-US", or "Civilized", or similar, to taste) and "ISO 8601".

In particular, "Big-Endian" and "Little-Endian" have a strong significance in programming contexts already (byte order), and these terms feel out of place to me. I theorize other users with C experience may also find the terms unclear.

I also think we should make ISO 8601 the default format -- it is unambiguous, while the other two formats are potentially confusing to a lot of users (either all US users or all non-US users).

src/view/form/control/AphrontFormDateControl.php
133

Quotes unnecessary.

144

Specifically, consider Y-m-d as a default here instead.

webroot/rsrc/js/core/behavior-fancy-datepicker.js
36

I think we should split on any reasonable separator (any of /, -, ., , at least).

We have to choose a separator when rendering or writing a date, but if a user types in 2015.05.23, it's completely unambiguous and there's no reason we can't parse it.

167–168

How can we hit this?

170–172

I think this is unusual in US dates, at least -- I'd expect 2/28/2015, not 02/28/2015.

(But I think it is required by ISO-8601, so it shouldn't just be removed.)

This revision now requires changes to proceed.Jun 13 2015, 3:05 PM
lpriestley edited edge metadata.
lpriestley marked 4 inline comments as done.

server-side date formatting

lpriestley marked 2 inline comments as done.Jun 13 2015, 10:51 PM
lpriestley added inline comments.
webroot/rsrc/js/core/behavior-fancy-datepicker.js
167–168

No, but seems like a reasonable failsafe?

epriestley accepted this revision.Jun 15 2015, 1:26 PM
epriestley edited edge metadata.
epriestley added inline comments.
src/view/form/control/AphrontFormDateControlValue.php
203

This is probably stumbling into T4103 territory, but it would be nice to have only one copy of the default value anywhere in the codebase.

I think the likely fix is T4103 + T7707 handling this, though.

210–214

You can maybe do this a little more cleanly with preg_split():

$parts = preg_split('@[,./:]@', $date);
220

I'd expect '-' to be present on this list.

223–224

I think this may need to fall back to parsing the unmangled input if this parse of a mangled input fails.

For example, suppose I enter a string like:

3.5 months ago

I'm not sure if that really works (I don't think it does), but you can imagine there likely exist some range of inputs that are valid and contain ,, ., /, or :, but are no longer valid after those characters are replaced with -.

In the case above, we'd mangle into something like this:

3-5 months ago

...which would fail to parse, then could try the original:

3.5 months ago

...which could succeed.

I can't actually come up with any examples of these date inputs (except date inputs which include times, I suppose), so it's possible that none exist. If you think none exist or that waiting for users to report them is fine, that's OK too.

This revision is now accepted and ready to land.Jun 15 2015, 1:26 PM
lpriestley updated this revision to Diff 32170.Jun 15 2015, 4:25 PM
lpriestley marked 4 inline comments as done.
lpriestley edited edge metadata.

returning original date input if failed to parse.

This revision was automatically updated to reflect the committed changes.