Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

Do not allow non-dates being parsed as dates #3294

Closed
wants to merge 1 commit into from
Closed

Do not allow non-dates being parsed as dates #3294

wants to merge 1 commit into from

Conversation

jmayday
Copy link

@jmayday jmayday commented Feb 13, 2015

I removed code allowing values which do not match format being parsed as dates.

@jmayday jmayday mentioned this pull request Feb 13, 2015
@jmayday
Copy link
Author

jmayday commented Feb 13, 2015

This is just squashed #3288

@wesleycho
Copy link
Contributor

This does not look like it would be a valid fix.

I am hesitant to merge this, especially since I have a fix that will likely be merged later this week here.

@wesleycho
Copy link
Contributor

After thinking about this, instead of the new Date constructor, it would be fine to have the value set to null. A test would need to be added for this that it turns invalid date strings/objects into null - this should be accomplishable by essentially doing element.controller('ngModel').$formatters[0] to grab the formatter unshifted into the array (if there isn't test code already doing so).

It also should not return undefined, but null IMO. The if-else conditional there can be refactored to then do only one return after $setValidity is called. This would also require testing.

@wesleycho wesleycho added this to the 0.13.0 milestone Mar 24, 2015
@chrisirhc
Copy link
Contributor

I'm moving this out of 0.13.0 milestone because:

  1. this is a breaking change. Users might be relying on this behavior.
  2. this needs tests for validity of the date, so there's still some work needed here.

@rvanbaalen
Copy link
Contributor

@jmayday Please update your PR so it can be merged when the time comes.

@jmayday
Copy link
Author

jmayday commented Apr 30, 2015

So, to wrap things up (because I'm little confused right now) - should we abandon this PR because of #3417 like @wesleycho suggested or should we keep it? (null approach also suggested by @wesleycho)?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants