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

Weird parsing behavior for invalid dates with years before 1970 #2645

Closed
elreimundo opened this Issue Oct 2, 2015 · 5 comments

Comments

Projects
None yet
3 participants
@elreimundo

elreimundo commented Oct 2, 2015

> moment([2015, null, null]).isValid()
< false
> moment([1968, null, null]).isValid()
< true

The second date is just as invalid as the first date. This appears to be a direct result of the pre-1970 workaround in https://github.com/moment/moment/blob/develop/src/lib/create/date-from-array.js.

@mj1856 mj1856 added the Bug label Oct 2, 2015

davidreiman-ck-zz pushed a commit to elreimundo/moment that referenced this issue Oct 2, 2015

@mj1856

This comment has been minimized.

Member

mj1856 commented Oct 2, 2015

Closed in favor of #2646

@mj1856 mj1856 closed this Oct 2, 2015

ichernev added a commit that referenced this issue Nov 9, 2015

ichernev added a commit that referenced this issue Nov 9, 2015

@ichernev ichernev reopened this Dec 23, 2015

@ichernev

This comment has been minimized.

Contributor

ichernev commented Dec 23, 2015

@elreimundo what is moment([1968, null, null]) supposed to mean? If you have a year only, you can use moment([1968]), this will default the rest to 1 (month, day, time to midnight). The current fix (or I would say hack) proposed in #2646 only works for the year. m = moment([1945, 5, null]) happily produces 1st Jan 1945 (because the null makes the whole thing invalid, and then we set the year but only to fix years in the range 0 - 100, not to fix invalid inputs.

If we ever want to make moment([1968, null, null]) and/or moment([2000, 5, null]) work we'd better cut the array after the first null and use that. And complain if there are any non-nulls after the first null. -- I do not want moment([2000, null, 5]) to be valid at all.

@elreimundo

This comment has been minimized.

elreimundo commented Dec 23, 2015

@ichernev 1) it doesn't matter whether you agree with the validity of moment([1968, null, null]); the fact that there is a difference in the validity of moment([2015, null, null]) vs moment([1968, null, null]) is evidence of a bug in the source. 2) I wouldn't have opened the PR if I hadn't run into this very issue in production using separate dropdowns for year/month/day. If year has a value but month and day do not, moment behaves inconsistently for pre- and post-1970 dates.

My fix does not make moment([2015, null, null]) valid, and nor should it; rather, it brings pre-UTC-era dates in line with the behavior of post-1970 dates.

Also, what grounds do you have for calling "hacky" a legitimate check for isFinite and corresponding tests that verify that pre-1970 years and post-1970 years behave equivalently?

@ichernev

This comment has been minimized.

Contributor

ichernev commented Dec 25, 2015

@elreimundo well, in your PR you mentioned that one date was valid and the other wasn't, and you made them both invalid (but I did't pay attention and thought they were both made valid). That is why I said "hack", because I though it "fixes" something that is not supposed to work.

I agree -- the behavior of the two should not be different. We changed the code in the last (unreleased) version to only setFullYear if the year is from 0 to 100, which will happen much less likely.

But then -- why don't we check if the Date object is valid, not if its year is finite. There can not be an infinite year in a valid date.

sereysethy added a commit to sereysethy/moment that referenced this issue Mar 14, 2016

@ichernev

This comment has been minimized.

Contributor

ichernev commented Aug 1, 2016

Fixed in #2646

@ichernev ichernev closed this Aug 1, 2016

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