682754 - Date.parse() with ISO date should tolerate a missing colon in the timezone

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Phil Ringnalda (:philor)]

Attachment: fix v.1

15.9.4.2 says that if a putative date string doesn't exactly match the 15.9.1.15 format, then it's implementor's choice, pick your heuristics and your other formats to support or NaN it.

As bug 394119, bug 394113, and the very existence of feedvalidator.org show, paying attention to a datetime format long enough to get clear out to the timezone part and remember whether this format calls for a colon (ISO8601/W3CDTF/RFC3339) or does not (RFC822/1123/2822) is quite often beyond people who've made the mistake of writing their own date-emitting code.

Thunderbird's feed reader and the toolkit feed parser would both like to see "2007-08-28T21:38:22-0700" as a date, rather than NaN, and if my vague understanding of what date_parseISOString is doing is right, this patch should let them, without breaking anything else.

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 556443 [details] [diff] [review]
fix v.1

Review of attachment 556443 [details] [diff] [review]:
-----------------------------------------------------------------

I am kind of meh about adding more non-standard date syntax, and I think it would be best if everyone just used ISO.  I also want a pony and world peace.  You can't always get what you want.  <guitarsolo/>

I tested other browsers, and it looks like only Chrome has your proposed behavior, so there's not an argument from following what others do.  Since I don't have other feelings on this I poked it at dmandelin, and he's fine with it, so let's do it.

arr+ with the tweaks noted below, and I don't need to see the changes you make in response -- just fix it up and fire away.

::: js/src/jsdate.cpp
@@ +842,5 @@
>              tzMul = -1;
>          ++i;
>          NEED_NDIGITS(2, tzHour);
> +        if (PEEK(':'))
> +          ++i;

Since this method purports to parse ISO strings, not non-ISO strings, let's add a comment of some sort here to highlight this deviation from the format, something like this:

        /*
         * Non-standard extension to the ISO date format (permitted by ES5):
         * allow "-0700" as a time zone offset, not just "-07:00".
         */

::: js/src/tests/ecma_5/Date/15.9.4.2.js
@@ +161,5 @@
>    check("2009-07-23T00:00:00.000-07:00", dd(2009,7,23,7,0,0,0));
>    check("2009-07-23T24:00:00.000-07:00", dd(2009,7,24,7,0,0,0));
>  
> +  // implementation-specific fallback for invalid formats
> +  check("2009-07-23T00:53:21.001-0700", dd(2009,7,23,7,53,21,1));

This test doesn't test a requirement of the standard, so it should go in an extensions/ directory.  I don't particularly care how the test gets there -- just copying this file into there, with this addition (and perhaps minus all the other parts that are already tested here), is good enough.

Comment 2

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfc8bbc61274

Comment 3

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/dfc8bbc61274

Comment 4

[Asa Dotzler [:asa]]

Comment on attachment 556443 [details] [diff] [review]
fix v.1

It's not clear why this can't just ride the train like most other bugs. If you want to make an explicit case for why we should take additional risk into the aurora and beta, please do so. (A nomination for making an exception to our normal bug fixing process without an explanation of why will normally get this treatment.)

Comment 5

[Mike Kaply [:mkaply]]

The reason this can't ride the train is because we've removed ISODateUtils from FF8, but the APIs you are supposed to use instead are broken without this fix until FF 10.

Comment 6

[Phil Ringnalda (:philor)]

Comment on attachment 556443 [details] [diff] [review]
fix v.1

You have to reset the flags if you want anyone to ever hear to you.

And "broken" is putting it rather strongly and wrongly.

The story from the start: bug 543535 removed ISODateUtils.jsm. Bug 693077 comment 0 said (without knowing quite what it was saying) that someone was depending on the undocumented and perhaps accidental feature of ISO8601DateUtils.parse() that it would accept any of -0800 (without the required colon) or -08 or -08monkeypants as a timezone, by in all cases except the correct -08:00 treating it as though there was no timezone and as though that should mean that it is a time in each individual user's timezone.

Then bug 543535 comment 19, bug 695345 comment 0, and I presume some non-bug talk, decided that ISO8601DateUtils had to come back (no, I don't know whether that means for Fx8, for Fx8 and Fx9, or forever). There are two ways that could be done: bring it back wholesale, the old untested code with its undocumented and rather odd features including -08monkeypants, or bring it back as just a very longwinded way to say Date.parse().

The nomination for landing this on aurora and beta is saying that it's vitally important not to remove APIs, or the feature of the removed API that tolerated out-of-spec timezones, but it's fine to change them under the covers so that 2011-10-23T18:32:00-0800 becomes the single datetime 2011-10-24T02:32:00Z rather than a different datetime for every user.

As a driver, you have three choices:

* take this patch, plus the patch that's now on bug 695345 to just make ISO8601DateUtils.parse call Date.parse(), retaining the API in name but not quite in behavior

* don't take this patch, do take the patch on bug 695345, say that having ISODateUtils work on to-spec dates is that important but having it work, differently, on a single case of out-of-spec dates is not

* don't take this patch, tell bug 695345 to put back the original ISODateUtils code so that the unremovable API retains the same odd behavior it always had

(Plus of course the fourth choice, don't take anything.)

Comment 8

[Ben Bucksch (:BenB)]

> out-of-spec timezones

As said in bug 693077 comment 8, +0800 and +08:00 and +08 are all valid timezones per ISO standard spec.

It is quite common in practice to leave the colon out in the timezone, so this fixes important use cases.

Comment 9

[Phil Ringnalda (:philor)]

As said in bug 693077 comment 9, they are all valid in different flavors of ISO 8601 datetimes, but only one of those flavors, Extended, was actually supported by ISO8601DateUtils, and in Extended, only +08:00 is valid.

Comment 10

[Mike Kaply [:mkaply]]

> but only one of those flavors, Extended, was actually supported by ISO8601DateUtils

You say this, but there is no evidence to this.

The original bug doesn't mention extended:

https://bugzilla.mozilla.org/show_bug.cgi?id=398177

No where in the code does it say extended:

http://mxr.mozilla.org/mozilla2.0/source/js/src/xpconnect/loader/ISO8601DateUtils.jsm

And you were not involved in writing the original code.

I see no evidence that this was the original intention.

Comment 11

[Phil Ringnalda (:philor)]

ISO8601DateUtils.parse("19850412T101530+0400").

Comment 12

[Phil Ringnalda (:philor)]

Comment on attachment 556443 [details] [diff] [review]
fix v.1

But you're absolutely right that "+08" is a valid Extended format timezone, so there's no way I can make you happy here. Please put ISODateUtils.jsm back, in its original form, in bug 695345, with a test which exercises its important features.

Then we'll have switched sides, and since I've got a copy of the spec in my hand, I'll be able to write 8601 tests, and file bugs on your noncompliance, for years to come :)

Comment 13

[LegNeato]

----------------------------------[ Triage Comment ]----------------------------------

We'll want to fix bug 695345 (which is basically backing out bug 543535) for Firefox 8 instead. That means there is no action for this bug for Firefox 8/Beta (though there will likely be action for Firefox 9/Aurora).

Comment 14

[Alex Keybl [:akeybl]]

Comment on attachment 556443 [details] [diff] [review]
fix v.1

[Triage Comment]
Plan for moving forward with ISO date in https://bugzilla.mozilla.org/show_bug.cgi?id=695345#c25 and https://bugzilla.mozilla.org/show_bug.cgi?id=543535#c24.