Page MenuHomePhabricator

JavascriptFFS shouldn't be building JSON manually
Open, LowPublic

Description

Roan comments:
Line 508-513 (concatenate separated strings): this doesn't account for single quotes
Why don't you just use real JSON here instead of fragile hacks?

I assume one reason would be formatting, but there should be code snippets which do that too.


Version: unspecified
Severity: normal

Details

Reference
bz31331

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 11:53 PM
bzimport set Reference to bz31331.
bzimport added a subscriber: Unknown Object (MLST).

In the backend FormatJson can be used, including an option to prettify whitespace.

On the client side use the 'jquery.json' module to do this (aliases to the built-in browser method JSON.stringify when available, implements a JSON encoder otherwise).

(In reply to comment #1)

In the backend FormatJson can be used, including an option to prettify
whitespace.

What option? I haven't seen any readily available pretty-printer for JSON yet.

(In reply to comment #2)

(In reply to comment #1)

In the backend FormatJson can be used, including an option to prettify
whitespace.

What option? I haven't seen any readily available pretty-printer for JSON yet.

Check out how API outputs format=jsonfm. There is an boolean parameter to FormatJson::encode that triggers pretty-print (the function documentation is vague about this but it has been this way for a while now).

I tried that. It generates diffs like:

-var I18n = {

  • on_leave_page: "Вашыя зьмены могуць быць страчаныя",

+var I18n =
+{
+ "on_leave_page": "\u0412\u0430\u0448\u044b\u044f \u0437\u044c\u043c\u0435\u043d\u044b \u043c\u043e\u0433\u0443\u0446\u044c \u0431\u044b\u0446\u044c \u0441\u0442\u0440\u0430\u0447\u0430\u043d\u044b\
};

Would be nice to avoid the asciization, any ideas?

It depends. There are options for that in the latest PHP's native json_encode, namely JSON_UNESCAPED_UNICODE.

But I'm not sure if the Services_JSON fallback has such option.

Either way, is there a problem with the escaped unicode characters? It is perfectly valid JSON. As far as I know it shouldn't cause any trouble.

It is also valid JavaScript, and outputs fine when put into a DOM element or other output. The JSON decoders also support it (they have to, since this encoding is part of the JSON specification).

Looking at JavascriptFFS, I take it the primary use is not related to transferring data from the client to the server. i.e. it is not run on a normal request, is that correct?

If so, and when it is only run from the command-line, you could bump the phpversion requirement for this script to 5.4 and use json_encode directly like this:

echo json_encode( $data, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE );

that'll make it look really pretty.

See also: http://php.net/json_encode

The primary purpose is storage of translations in version control system. Overt escaping is not needed and causes increase in file size, and makes it hard for occasional readers (if any).

json_encode looks nice, however translatewiki.net server is still running on PHP 5.3.2 for time being.

Krinkle, Niklas: See gerrit 50140.

(In reply to comment #8)

Krinkle, Niklas: See Gerrit change #50140.

...which is "Combine JavaScript and JSON encoding logic"

PleaseStand, I've added this bug to https://www.mediawiki.org/wiki/Mentorship_programs/Possible_projects#Extensive_and_robust_localisation_file_format_coverage. Would you be able to help a student by reviewing contributions such as a patch for this bug? If yes please add yourself there as co-mentor, then we'll be able to mark it featured.

(In reply to Niklas Laxström from comment #0)

Roan comments:
Line 508-513 (concatenate separated strings)

That's around line 100 of ffs/JavaScriptFFS.php now.

this doesn't account for
single quotes
Why don't you just use real JSON here instead of fragile hacks?

From MediaWiki-General

2014-10-12 19.57 < Nemo_bis> The bug is about using standard functions to reduce mistakes in JSON production. PleaseStand linked a function in core which you'll probably need to use (with the utf8 option). Comment 0 also mention a specific bug the current implementation might have (single quotes).

(In reply to Nemo from comment #12)

From MediaWiki-General
2014-10-12 19.57 < Nemo_bis> The bug is about using standard functions to
reduce mistakes in JSON production. PleaseStand linked a function in core
which you'll probably need to use (with the utf8 option). Comment 0 also
mention a specific bug the current implementation might have (single quotes).

I want to know what type of string is stored in $data and what will be the output of $messages[keys].
It will be very helpful if you can give an example of the data being entered and the corresponding output.

(In reply to Alisha Jain from comment #13)

It will be very helpful if you can give an example of the data being entered
and the corresponding output.

I'm not sure that's worth the effort for this bug: you should probably just look at how JSON is produced in the last gerrit link above.
However, if you want real data you can set up a Translate wiki with some JSON-based projects, copying configuration from https://git.wikimedia.org/tree/translatewiki.git

I want to know how can I use this file, where can I see output?

(In reply to Alisha Jain from comment #15)

I want to know how can I use this file, where can I see output?

I guess "set up a Translate wiki" wasn't clear enough, I added more explicit pointers in https://www.mediawiki.org/?diff=1224226 . Getting support for setting up a test environment, however, is best done at [[mw:Extension talk:Translate]] than an individual bugzilla report.

Change 166990 had a related patch set uploaded by Prianka:
ffs/JavaScriptFFS.php Replaced unnecessary use of str_replace

https://gerrit.wikimedia.org/r/166990

Change 167018 had a related patch set uploaded by Alishajain:
JSON prouction using standard function

https://gerrit.wikimedia.org/r/167018

Change 199590 had a related patch set uploaded (by Ayushimrigen):
Concatenate strings in JavaScriptFFS

https://gerrit.wikimedia.org/r/199590

Change 199590 abandoned by Nikerabbit:
Concatenate strings in JavaScriptFFS

https://gerrit.wikimedia.org/r/199590

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 22 2015, 11:02 PM