Page MenuHomePhabricator

MultimediaViewer: Usage of non-standard date format parsing with Moment.js is deprecated
Closed, ResolvedPublic

Description

Opening https://commons.wikimedia.org/w/index.php?title=File:Daemonicus_LOGO.jpg#mediaviewer/File:Daemonicus_LOGO.jpg logs to the console:

Deprecation warning: moment construction falls back to js Date. This is discouraged and will be removed in upcoming major release. Please refer to https://github.com/moment/moment/issues/1407 for more info.


Version: unspecified
Severity: normal

Details

Reference
bz66635

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 3:12 AM
bzimport added a project: MediaViewer.
bzimport set Reference to bz66635.
bzimport added a subscriber: Unknown Object (MLST).
Krinkle created this task.Jun 15 2014, 12:00 PM
Tgr added a comment.Jun 15 2014, 7:20 PM

We use moment to try to format the date string consistently but fall back to showing the original string if it that fails, so changes in how moment's date parsing is implemented do not matter much. As for the warning itself, there doesn't seem to be a way to suppress it when you we handing an unknown string to moment. So as far as I can see this is a WONTFIX.

(In reply to Tisza Gergő from comment #1)

As for the warning itself,
there doesn't seem to be a way to suppress it when you we handing an unknown
string to moment. So as far as I can see this is a WONTFIX.

The documented [1] way to parse a date string with a user-defined fallback when the string is not properly ISO-formatted, is to override moment.createFromInputFallback.

The current functionality allowing to do moment('4 3 2000') and expecting a “useful” result is deprecated and possibly will be removed in a future version, so I don’t see how WONTFIXing this could work.

[1] https://github.com/moment/moment/issues/1407#issuecomment-41620885

Tgr added a comment.Jun 19 2014, 5:46 PM

(In reply to Mormegil from comment #2)

The current functionality allowing to do moment('4 3 2000') and expecting a
“useful” result is deprecated and possibly will be removed in a future
version, so I don’t see how WONTFIXing this could work.

As I said, when moment cannot parse the string, we just show it as-is, so changes in what formats moment exactly accepts don't really affect MediaViewer. Users will see dates formatted in non-standard ways more often, on the other hand false positives (e.g. dd/mm/yyyy parsed as mm/dd/yyyy) and issues like bug 56794 will go away. On the whole I'm not sure this will be a bad thing.

Tgr added a comment.Jun 26 2014, 10:40 PM

Closing per above. If the presence of the warning itself is a problem, please reopen and we'll figure out some workaround.

Gilles raised the priority of this task from High to Unbreak Now!.Dec 4 2014, 10:11 AM
Gilles moved this task from Untriaged to Done on the Multimedia board.
Gilles lowered the priority of this task from Unbreak Now! to High.Dec 4 2014, 11:22 AM
Krinkle reopened this task as Open.Dec 20 2014, 4:00 AM

The API should provide data in a consistent format. The client may then parse as a value in that format.

Right now this warning still shows up on e.g. https://en.wikipedia.org/wiki/Angelina_Jolie#mediaviewer/File:Angelina_Jolie_2_June_2014_(cropped).jpg

For that file, the API data is:

timestamp: "2014-06-12T11:00:26Z"
extmetadata: {
  DateTime: {value: "2014:06:12 09:00:00", source: "file-metadata", hidden: ""}
}

There is nothing exceptional or non-standard about that date. It comes straight from the wikitext of the File page at Commons. And is given to {{Information as plain text. The format used in this case is the preferred and documented format for dates in the Information template on Wikimedia Commons. When viewing the file page on Commons itself it is actually parsed and localised properly by their templates.

I'm surprised this isn't handled or normalised in some way already. This should be recognised and parsed by the CommonsMetadata extension (it it the prime case for dates, like 80% of all files) and provided by the API in a format of choice (ISO maybe). If CommonsMetadata also wants to support other date formats, it's probably best to do that server-side so that all consumers can use it equally. Right now it relies on the non-standard and undocumented implementation by different browsers' Date parser.

When Moment removes support for this we'll likely see Multimedia viewer's nicely formatted dates like "Created: June 12, 2014" disappear for the vast majority of files.

Tgr added a comment.Dec 21 2014, 7:14 AM

DateTime always comes from the file and is usually (as in this case) in EXIF format which is non-standard as far as common date parsing libraries go (moment.js does not recognize it either); it is usually not needed by MediaViewer though. The wikitext date is in DateTimeOriginal and contains HTML (at least for the standard Commons templates).

This should indeed be fixed in CommonsMetadata; the relevant bugs are T66014 for EXIF dates and T63701 for template dates. I have been saving those for GCI but they will be fixed one way or another by the time moment.js is updated.

CommonsMetadata might still encounter formats it does not understand in which case the sensible reaction is to output them as they are, and moment.js might still be able to parse those, there is no harm in at least trying, so I don't see anything needing fixing as far as MediaViewer goes.

Tgr added a comment.Dec 30 2014, 5:43 AM

@Krinkle: the two CommonsMetadata bugs have now been fixed, IMO it now transforms everything it can reasonably be expected to understand into an ISO timestamp. That should cover the huge majority of templates (everything where the date parameter can be parsed by {{#time}}) and all dates coming from the file itself.

Can this task be closed or do you think other changes are needed?

Gilles closed this task as Resolved.Jan 8 2015, 5:32 AM
Gilles claimed this task.