[bugfix] Handle RFC2822 parsing in non-en locales, fixes #3985 #4077

Petrukha · 2017-07-20T00:05:08Z

No description provided.

jsf-clabot · 2017-07-20T00:05:09Z

All committers have signed the CLA.

icambron · 2017-07-20T02:24:54Z

src/test/locale/ru.js

@@ -353,3 +353,35 @@ test('weeks year starting sunday formatted', function (assert) {
    assert.equal(moment([2012,  0,  9]).format('w ww wo'), '3 03 3-я', 'Jan  9 2012 should be week 3');
 });

+test('parsing RFC 2822', function (assert) {


It's confusing for this to be in the ru test file. You can test it where the rest of the RFC2822 tests are and just try/finally around moment.locale('ru') to show that it ignores the locale.

ok, moved it

icambron · 2017-07-20T02:26:36Z

src/lib/create/from-string.js

+};
+
+function parse10(inty) {
+    return parseInt(inty, 10);


This parse10 business was something dumb I did in Luxon to satisfy my linter and I already regret it. You can just replace this with parseInt(v) everywhere and get rid of this function.

icambron · 2017-07-20T02:27:40Z

src/lib/create/from-string.js

+
+function extractFromRFC2822Strings(yearStr, monthStr, dayStr, hourStr, minuteStr, secondStr) {
+    var result = {
+        year: yearStr.length === 2 ? untrucateYear(parse10(yearStr)) : parse10(yearStr),


It might be simpler to just return an array from here and then hand it directly to config._a below

yep, corrected it

icambron · 2017-07-20T02:33:34Z

This is looking good. I requested a few changes and you need to sign the CLA.

icambron · 2017-07-20T02:39:21Z

src/lib/create/from-string.js

-        if (match[1]) { // day of week given
-            var momentDate = new Date(match[2]);
-            var momentDay = ['Sun','Mon','Tue','Wed','Thu','Fri','Sat'][momentDate.getDay()];
+function untrucateYear(year) {


Moment has some specific, configurable function for this somewhere. You should use it unless the RFC spec defines this behavior.

Well actually the spec says:

Where a two or three digit year occurs in a date, the year is to be interpreted as follows: If a two digit year is encountered whose value is between 00 and 49, the year is interpreted by adding 2000, ending up with a value between 2000 and 2049. If a two digit year is encountered with a value between 50 and 99, or any three digit year is encountered, the year is interpreted by adding 1900.

In moment there's a method:

hooks.parseTwoDigitYear = function (input) { return toInt(input) + (toInt(input) > 68 ? 1900 : 2000); };

So 68 is hardcoded and according to spec we also have to consider 3-digit year, so I've corrected untrucateYear to match the spec

Petrukha · 2017-07-21T17:01:56Z

Cannot get rid of this 'CLA not signed yet' message... I've created the pull request from a different Email and adding it to my account does not help...

icambron · 2017-07-21T17:42:04Z

Oh I see. I bet you can fix it by rebasing your commits to have the same author. If that doesn't work, the fact is that you have signed it so we can probably move forward.

fastrde · 2017-07-22T20:25:59Z

Does this PR fix the issue that the +/-9999 is ignored and get set to GMT +0000 in the switch block, also?

 2140   default: // UT or +/-9999
 2141     timezone = match[5];  //my temporary fix
 2142     //timezone = timezones[' GMT'];

Petrukha · 2017-07-24T15:31:44Z

@icambron do I have to do something with this PR now or it just will be merged in new moment version?

@fastrde I'm afraid not, and the fix you propose will not do after this PR as the implementation is changed. I can fix this. Is this bug regestered? If not, can you, please register it?

icambron · 2017-07-24T17:24:41Z

It will get merged when we do a release. No further action is required of you. Thanks for contributing!

maggiepint · 2017-07-25T22:59:45Z

@fastrde you're referring to #4083 correct? There will be merge conflicts between this fix and that.

fastrde · 2017-07-26T10:45:41Z

@maggiepint yes. This is the fix I've implemented (but I didn't know that there was an issue or pr for that already). When this is already documented and worked on my question is answered. Thank you.

ichernev · 2017-08-03T11:12:50Z

Isn't this related to #3938 ?

Or because we're not merging people keep piling the same stuff on top :)

ichernev · 2017-08-03T12:15:38Z

src/lib/create/from-string.js

@@ -94,70 +97,93 @@ export function configFromISO(config) {
 }

 // RFC 2822 regex: For details see https://tools.ietf.org/html/rfc2822#section-3.3
-var basicRfcRegex = /^((?:Mon|Tue|Wed|Thu|Fri|Sat|Sun),?\s)?(\d?\d\s(?:Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec)\s(?:\d\d)?\d\d\s)(\d\d:\d\d)(\:\d\d)?(\s(?:UT|GMT|[ECMP][SD]T|[A-IK-Za-ik-z]|[+-]\d{4}))$/;
+var rfc2822 = /^(?:(Mon|Tue|Wed|Thu|Fri|Sat|Sun),?\s)?(\d{1,2})\s(Jan|Feb|Mar|Apr|May|Jun|Jul|Aug|Sep|Oct|Nov|Dec)\s(\d{2,4})\s(\d\d):(\d\d)(?::(\d\d))?\s(?:(UT|GMT|[ECMP][SD]T)|([Zz])|(?:([+-]\d\d)(\d\d)))$/;


You dropped the military timezone variants from the new regex.

ichernev · 2017-08-03T12:16:47Z

src/lib/create/from-string.js

+    if (year <= 49) {
+        return 2000 + year;
+    } else if (year <= 999) {
+        return 1900 + year;


Is this logic part of the spec? If not we need to use whatever is used in the rest of moment, and I think that part is pluggable (configurable from outside).

It is. See discussion in one of the collapsed-by-Github-comments above

ichernev · 2017-08-03T12:24:15Z

OK, so there are 2 issues with RFC2822, one is that it doesn't handle the numeric timezone, and the other is that it fails for non en locales.

We'd need to see how the merge goes, but the timezone issue is a minor one (from a code perspective), so we'll have to merge this one first.

ichernev · 2017-08-07T01:11:14Z

Merged in 0d0b291

[bugfix] Handle RFC2822 parsing in non-en locales, fixes #3985

[fixup] Fix RFC2822 rewrite, fixes #4077

Fix moment#3985 fix Invalid date for RFC 2822 with some refactoring

27604da

Petrukha mentioned this pull request Jul 20, 2017

"Invalid date" for RFC 2822 date time format, if changing locale #3985

Closed

icambron requested changes Jul 20, 2017

View reviewed changes

icambron reviewed Jul 20, 2017

View reviewed changes

Fix moment#3985 code review changes

344e1af

icambron approved these changes Jul 21, 2017

View reviewed changes

icambron added the Pending Next Release label Jul 21, 2017

ichernev reviewed Aug 3, 2017

View reviewed changes

ichernev changed the title ~~Fix #3985 fix Invalid date for RFC 2822 with some refactoring~~ [bugfix] Handle RFC2822 parsing in non-en locales, fixes #3985 Aug 3, 2017

ichernev closed this Aug 7, 2017

ichernev added a commit that referenced this pull request Aug 7, 2017

Merge pull request #4077 from Petrukha:fix3985

0d0b291

[bugfix] Handle RFC2822 parsing in non-en locales, fixes #3985

ichernev added a commit that referenced this pull request Aug 9, 2017

Merge pull request #4108 from ichernev:fixup-rfc2822

2a584dd

[fixup] Fix RFC2822 rewrite, fixes #4077

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bugfix] Handle RFC2822 parsing in non-en locales, fixes #3985 #4077

[bugfix] Handle RFC2822 parsing in non-en locales, fixes #3985 #4077

Petrukha commented Jul 20, 2017

jsf-clabot commented Jul 20, 2017 •

edited

icambron Jul 20, 2017

Petrukha Jul 20, 2017

icambron Jul 20, 2017

Petrukha Jul 20, 2017

icambron Jul 20, 2017

Petrukha Jul 20, 2017

icambron commented Jul 20, 2017

icambron Jul 20, 2017

Petrukha Jul 20, 2017

icambron Jul 20, 2017

Petrukha commented Jul 21, 2017

icambron commented Jul 21, 2017

fastrde commented Jul 22, 2017

Petrukha commented Jul 24, 2017

icambron commented Jul 24, 2017

maggiepint commented Jul 25, 2017

fastrde commented Jul 26, 2017

ichernev commented Aug 3, 2017

ichernev Aug 3, 2017

ichernev Aug 3, 2017

icambron Aug 3, 2017

ichernev commented Aug 3, 2017

ichernev commented Aug 7, 2017

[bugfix] Handle RFC2822 parsing in non-en locales, fixes #3985 #4077

[bugfix] Handle RFC2822 parsing in non-en locales, fixes #3985 #4077

Conversation

Petrukha commented Jul 20, 2017

jsf-clabot commented Jul 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icambron commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Petrukha commented Jul 21, 2017

icambron commented Jul 21, 2017

fastrde commented Jul 22, 2017

Petrukha commented Jul 24, 2017

icambron commented Jul 24, 2017

maggiepint commented Jul 25, 2017

fastrde commented Jul 26, 2017

ichernev commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ichernev commented Aug 3, 2017

ichernev commented Aug 7, 2017

jsf-clabot commented Jul 20, 2017 •

edited