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

[bugfix] Fix #3500: ISO 8601 parsing should match the full string, not the beginning of the string. #3502

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
5 participants
@markstos
Contributor

markstos commented Oct 13, 2016

Before the fix, date strings which started with a valid ISO pattern but ended with non-conforming data would pass as valid ISO dates and not trigger the deprecation warning, causing garbage-in/garbage-out bugs and silent failure.

Example of a garbage date which was considered Valid ISO before:

 2016-01-Just a bunch of junk

Example of silent failure cases:

2016-12-4 would be match the "YYYY-MM" ISO pattern, so would silently become "2016-12-1". 
2000-01-01T11:00:00 PM would match the "  2000-01-01T11:00:00" ISO pattern, silently turning 11 PM into 11 AM. 

Non of the above cases trigger the "non-ISO deprecation warning", either.

Because of the nature of how the related tests are put together, the newly added test passes even before the fix is applied.

This happens because the "non-ISO" tests are tested against the same ISO regex which is being fixed.

Because the broken regex is also used in the test, it allowed the non ISO date to be validated before the fix.


This fix exposed another problem with non-ISO dates in the test suite, so it causes 3 other test failures, which are now fixed, too. The other test failures are in:

test/moment/is_same_or_before.js
test/moment/is_same_or_after.js

All concern dates which look like this:

2013-02-01T-05:00

From what I can tell, it is not valid IS0 8601 to include a timezone offset without a time. The references I checked included:

These tests were updated to avoid this invalid date format after @icambron agreed the dates were not ISO format but should be.

Fix #3500: ISO 8601 parsing should match the full string, not the beg…
…inning of the string.

Because of the nature of the how related tests are put together, the newly added test
*passes* even before the fix is applied.

This happens because the "non-ISO" tests are tested against the same ISO which is being fixed.

Because the broken regex is also used in the test, it allowed the non ISO date to be validated
before the fix.
@icambron

This comment has been minimized.

Member

icambron commented Oct 19, 2016

See #2985. I'll reiterate my take there: we should always parse ISO strictly and the check should match the parser. So I'm +1 here.

Re: the Travis failures: I agree that 2013-02-01T-05:00 is bogus.

@markstos

This comment has been minimized.

Contributor

markstos commented Oct 19, 2016

@icambron: Thanks for the response.

I read through the related thread and saw some other people also ran into problems with junk at the end of ISO-valid strings-- Like my case where December 4th became December 1st, someone else had an unnerving case where 11 PM became 11 AM.

I see no need to add a "strict" flag as part of fixing this. I see this fix as simply making the ISO parser work as advertised-- more correctly! I don't see the current behavior has being "not strict", it's not like "not behaving advertised by silently discarding at the end of valid ISO patterns".

I pushed another commit to fix the bogus date strings in the test suite that the original ISO fix turned up.

This pull request will put a stop the unknown amount of silent failures where invalid date strings are translated into other ISO dates or junk is simply allowed through as a "ISO valid" when it is not.

@markstos

This comment has been minimized.

Contributor

markstos commented Oct 19, 2016

Fixing Travis complaint.

Fix invalid ISO format dates in the test suite.
Fixing the strictness of ISO date parsing exposed this invalid ISO format dates
which included a time zone offset without including a time.

By appending a time of "00:00:00" to the date strings they became ISO-valid again
without changing the meaning of the test.
@icambron

This comment has been minimized.

Member

icambron commented Oct 19, 2016

Non-strict really just means "wrong but in ways that are hopefully helpful". I agree that the current behavior is an especially unhelpful flavor of non-strictness. But AFAIK, it's the only bit of slack in the parser, so fixing has the effect of making it strict. My point in the other thread was that I don't think need a global non-strict parsing flag to make ISO in particular strict. I think you agree.

@markstos

This comment has been minimized.

Contributor

markstos commented Oct 19, 2016

@icambron I think we agree on all points.

Where does that leave the future of this pull request? It seems to be in alignment with changes you support.

@icambron

This comment has been minimized.

Member

icambron commented Oct 19, 2016

I have a little member badge, but I'm not in charge. It seems like an important bug fix, especially because it breaks the passthrough to the native parser. But it's a breaking change, so I'll ask the rest of the team to weigh in and make the call. (cc @ichernev, @maggiepint, @mj1856)

@ichernev

This comment has been minimized.

Contributor

ichernev commented Oct 20, 2016

About this change not being backwards compatible -- we've done those in the past with varying success. So we can try to ship in in 2.16 and if there is massive backlash cut out 2.16.1 ... not really something I can predict now, but in general, given how many mistakes have happened even in our code (tests) its hard to be against this PR.

I'll merge it in next release (2.16)

@maggiepint

This comment has been minimized.

Member

maggiepint commented Nov 1, 2016

Heads up. Because we came under JS Foundation management, we need a CLA signed for this to be merged. Should be just a couple clicks.

@markstos

This comment has been minimized.

Contributor

markstos commented Nov 1, 2016

I found the CLA here and completed it. https://js.foundation/CLA/

@ichernev

This comment has been minimized.

Contributor

ichernev commented Nov 1, 2016

@markstos well, not according to the checkmark :) We'll investigate

@mj1856

This comment has been minimized.

Member

mj1856 commented Nov 1, 2016

@markstos - Try this link please.

@markstos

This comment has been minimized.

Contributor

markstos commented Nov 1, 2016

OK, done now.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Nov 6, 2016

Merged in 68a68b8

@ichernev ichernev changed the title from Fix #3500: ISO 8601 parsing should match the full string, not the beginning of the string. to [bugfix] Fix #3500: ISO 8601 parsing should match the full string, not the beginning of the string. Nov 6, 2016

@ichernev ichernev closed this Nov 6, 2016

ichernev added a commit that referenced this pull request Nov 6, 2016

Merge pull request #3502 from markstos:fix-3500-iso-regex-problem
[bugfix] Fix #3500: ISO 8601 parsing should match the full string, not the beginning of the string.

@mj1856 mj1856 added Bug-fix and removed Pending Next Release labels Nov 10, 2016

@mj1856 mj1856 added this to the 2.16.0 milestone Nov 10, 2016

@markstos markstos deleted the markstos:fix-3500-iso-regex-problem branch Dec 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment