Look for meridiem indicator when matching iso format

[maggiepint]

As presented in #2983, using the date format "2016-02-23 11:31:23 PM" will match an ISO format, even though it is not. This causes a wrong date to be parsed:

moment('2016-02-23 11:31:23 PM').format() = "2016-02-23T11:31:23-06:00"

This is because 2016-02-23 11:31:23 technically is an ISO format.
We need to change the from-string file to check for a meridiem indicator and do something other than match ISO format if it is there.

[mattjohnsonpint]

Ouch! That one looks like it might bite someone for sure. No deprecation warning either.

[icambron]

Seems like maybe just making sure the ISO checker goes all the way to the end of the string suffices, e.g. by adding a $ to the end of extendedIsoRegex and basicIsoRegex.

[maggiepint]

@icambron I don't think we can do that. Currently the following test passes:

    assert.ok(moment('2016-01-01 is my date').isValid(), 'test extra chars after iso date')

It's almost a certainty that there is someone out there in the world doing that. Change the regex and that test fails (along with a couple others that I haven't hunted down yet).

[icambron]

Hmm, OK. My take would be to deprecate that and fix this AM/PM thing when we drop support for it. I don't see a good reason why we should support moment('2016-01-01 is my date')

[maggiepint]

I've been thinking about this a bit, and I kind of wonder if the least-invasive answer to some of the parser issues we're seeing - like this one - would be to actually default the parser to strict mode. It seems like more often than not, forgiving parsing issues (like this one) are solved by switching to strict mode (because the user should have been using strict mode in the first place). Maybe this is one of those 'help people fall into the pit of success' things? That would leave existing functionality possible, but push the dev down the right path.

[icambron]

Agree, we've long wanted to do that. I think it's been listed as a possible 3.0 item for a long time. But certainly having the automagic one-arg signature be strict is a small step towards that.

[maggiepint]

I actually started coding strict-mode-by-default today by adding a toggle-able global state variable called globalStrict, which I defaulted to true. The strict mode setting can then be switched back to false by calling:

moment.globalStrict(false)

This makes everything behave the way it always did, without having to change every call to moment's parser.
It takes about four lines of code to make this change - and then one has to fix hundreds of unit tests :-)
To me though, this might be a way to roll out strict-by-default without having to wait for v3.
I feel like since users can easily change it back, they might accept the change without too much complaint.

Alternately, the globalStrict setting could default to false, and we could strongly encourage developers to set it to true in the documentation. This is less invasive and maybe helpful?
I'll be the first person to say that global state variables SUCK from a testability standpoint, and for that reason alone this idea might be terrible.

I could also, as suggested, just default one-argument calls to strict mode. That will upset the probably thousands of people who are still falling back to JS date construction though.

[icambron]

I like all of that, except one thing:

That will upset the probably thousands of people who are still falling back to JS date construction though.

To be clear, my suggestion wasn't to finish off the can't-fall-back-to-JS-constructor deprecation. In fact, the opposite: in this case, we want to do just that but the ISO check is preempting us.

[maggiepint]

So, are you saying you would try the global state thing, or that you would avoid it? Maybe I'll just finish the unit tests and make the PR and we can talk about it there.

[icambron]

Sorry, I was unclear: I'm not against the global state thing at all. I just don't think it precludes us from making the moment(string)'s ISO check strict all the time, even without the state set.

[Aukhan]

@maggiepint thanks for pointing me to the right thread.
and you've correctly stated above that I was "one of those people out there" :P doing things like
moment("2016-04-06Tnull").isValid()
with full confidence that moment is going to reject an invalid string.

So whats the easiest way to turn on strict parsing by default ? (sorry, being lazy)

[maggiepint]

@Aukhan we never implemented global strict because of an issue with strict mode parsing that needed to be fixed first. (but it was, so we may yet get there).

To accomplish what you're doing, I think you just need to specify the ISO_8601 constant and strict mode inline in your code:

moment("2016-04-06Tnull", moment.ISO_8601, true).isValid()
false

[Aukhan]

@maggiepint thanks again for your reply ...

That's a great suggestion but I wouldn't want to replace hundreds of such expressions where not just the ISO_8601 but different custom formats are being used, so I guess the global strict option would be nice to have.
I started looking into the codebase, but obviously such huge and great work cant be understood overnight.

I may not have the required skills but if I can help out in any way please let me know.
Thanks !

[maggiepint]

@Aukhan I'll see if I can't pick that work item up again. The problem is that to make it work, PR #3078 needs to be merged. That was why the thing got dropped in the first place.

[icambron]

Closing in favor of #3502