[Translation] Added support for JSON format (both loader and dumper). #8534

radekbenkel · 2013-07-20T21:10:41Z

Based on IniFileLoader\Dumper.

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc	this component don't have docs

radekbenkel · 2013-07-20T21:42:52Z

Heh, PHP5.3 don't have JSON_PRETTY_PRINT constant. So, in this case is there possibility to use somebody's code (with annotation of course about an author) - like this https://github.com/GerHobbelt/nicejson-php/blob/master/nicejson.php ?

Another solutions is to:

Remove JsonDumper
Throw exception in PHP < 5.4

GromNaN · 2013-07-20T22:16:10Z

src/Symfony/Component/Translation/Loader/JsonFileLoader.php

+            throw new NotFoundResourceException(sprintf('File "%s" not found.', $resource));
+        }
+
+        $messages = json_decode(file_get_contents($resource), true);


You should use json_last_error to handle errors.

GromNaN · 2013-07-20T22:27:50Z

Instead of relying on a generic JSON prettifier, you can generate it line by line:

$lines = array();
foreach ($messages->all($domain) as $key => $value) {
    $lines[] = json_encode($key) . ': ' . json_encode($value)
}
return '{' . PHP_EOL . '    ' . implode(',' . PHP_EOL . '    ', $lines) . PHP_EOL . '}';

jakzal · 2013-07-27T21:14:29Z

src/Symfony/Component/Translation/Dumper/JsonFileDumper.php

+            $lines[] = json_encode($key) . ': ' . json_encode($value);
+        }
+
+        return '{' . PHP_EOL . '    ' . implode(',' . PHP_EOL . '    ', $lines) . PHP_EOL . '}';


Strings should be concatenated without the spaces between dots. For example:

json_encode($key).': '.json_encode($value);

That's how it's done in other parts of code.

hacfi · 2013-07-27T22:04:23Z

@Singles Great addition..thanks for the PR!

radekbenkel · 2013-07-28T12:35:45Z

Should I squash these commits into one and/or open new PR?

wouterj · 2013-07-28T12:59:52Z

Just squash them

staabm · 2013-08-04T14:55:50Z

src/Symfony/Component/Translation/Loader/JsonFileLoader.php

+                $errorMsg = 'Malformed UTF-8 characters, possibly incorrectly encoded';
+                break;
+            default:
+                $errorMsg = 'Unknown error';


This should contain the code which you were not able to map, so a user of the api has information to google something without the need to adjust your code to get this information out of it.

This part is taken directly from PHP's manual example (not user comments). What about passing errorCode as exception code?

stof · 2013-08-05T09:11:18Z

you also need to create the service in FrameworkBundle to register it

stof · 2013-08-05T09:11:59Z

src/Symfony/Component/Translation/Dumper/JsonFileDumper.php

+     * @param  array  $messages
+     * @return string Human readable JSON representation
+     */
+    protected function formatPretty(array $messages)


this should be a private method

fabpot · 2013-08-10T20:29:29Z

I don't see the need for a JSON loader/dumper. First, we already have plenty options, then, and AFAIK, there is no standard for translations in a JSON file, and finally, writing JSON is a PITA.

hacfi · 2013-08-11T02:43:07Z

I think JSON is useful because web translation services usually support it and it gives you more flexibility for writing a custom tool for editing yourself.

fabpot · 2013-08-11T06:51:00Z

@hacfi Which online tool are you talking about? Any examples?

radekbenkel · 2013-08-11T07:53:00Z

@fabpot Crowdin for example http://crowdin.net/page/tour/supported-formats

About this JSON PITA thing - I would say that is a subjective opinion.

hacfi · 2013-08-12T07:05:49Z

We're using https://phraseapp.com/ and the provider we used before (can't
remember the name right now) supported it as well.

On Sunday, August 11, 2013, Fabien Potencier wrote:

@hacfi https://github.com/hacfi Which online tool are you talking
about? Any examples?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/8534#issuecomment-22453447
.

fabpot · 2013-08-13T14:33:20Z

src/Symfony/Component/Translation/Dumper/JsonFileDumper.php

+    public function format(MessageCatalogue $messages, $domain = 'messages')
+    {
+        $data = $messages->all($domain);
+        if (version_compare(PHP_VERSION, '5.4.0', '<')) {


I would remove this special case and just don't pretty format for PHP < 5.4 (because 5.3 is not supported anymore anyway).

@fabpot It is not? Didn't know that :)

I added this after first release, when Travis built (5.3 and 5.3.3) failed.

So, should I remove this line and make PHP 5.4 only version? Didn't find any information about Symfony 2.4 will require PHP 5.4.

At the top of the file, if the JSON_PRETTY_PRINT constant is not defined, define it, and remove everything else. That way, on 5.3, it won't be pretty formatted, but as of 5.4, it will.

@Singles Symfony does not require 5.4+. But as 5.3 reached its end of maintenance, it is OK to have a non-pretty file for users still on it.

@stof @fabpot So how you want tests for dumper to be handled? Currently it checks format of file, so it take things like indentation into account - which in my opinion is good, because we test dumper, and it should be human readable.

Proposed solutions from my side:

If php < 5.4 then mark test as skipped

Make two tests - for <= 5.3 and second for newer versions, and mark them skipped depending on PHP version. Then when running on 5.4 , test for 5.3 (which compares non human readable format) will be skipped, and for running on 5.3 test for 5.4 will be skipped.

Instead of checking file format, parse file and check parsed data structure, but for me it not good test case for dumper (look above)

?

Option 1 seems the best.

@fabpot Updated PR.

hacfi · 2013-09-28T00:32:38Z

Thanks @fabpot !

…2.4 (singles) This PR was merged into the 2.4 branch. Discussion ---------- Translation - Added info about JsonFileLoader added in 2.4 | Q | A | ------------- | --- | Doc fix? | no | New docs? | yes (symfony/symfony#8534) | Applies to | 2.4 | Fixed tickets | n/a Commits ------- adf678b Added info about JsonFileLoader added in 2.4

GromNaN reviewed Jul 20, 2013
View reviewed changes

jakzal reviewed Jul 27, 2013
View reviewed changes

wouterj mentioned this pull request Jul 28, 2013

[WIP] Bootstrapped the Translation documentation symfony/symfony-docs#2545

Closed

1 task

staabm reviewed Aug 4, 2013
View reviewed changes

stof reviewed Aug 5, 2013
View reviewed changes

fabpot reviewed Aug 13, 2013
View reviewed changes

[Translation] Added support for JSON format (both loader and dumper).

fcef021

fabpot merged commit fcef021 into symfony:master Sep 27, 2013

radekbenkel deleted the translation-json branch September 27, 2013 13:33

radekbenkel mentioned this pull request Jan 5, 2014

Translation - Added info about JsonFileLoader added in 2.4 symfony/symfony-docs#3428

Merged

Uh oh!

[Translation] Added support for JSON format (both loader and dumper). #8534

[Translation] Added support for JSON format (both loader and dumper). #8534

Uh oh!

Conversation

radekbenkel commented Jul 20, 2013

Uh oh!

radekbenkel commented Jul 20, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GromNaN commented Jul 20, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hacfi commented Jul 27, 2013

Uh oh!

radekbenkel commented Jul 28, 2013

Uh oh!

wouterj commented Jul 28, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Aug 5, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Aug 10, 2013

Uh oh!

hacfi commented Aug 11, 2013

Uh oh!

fabpot commented Aug 11, 2013

Uh oh!

radekbenkel commented Aug 11, 2013

Uh oh!

hacfi commented Aug 12, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hacfi commented Sep 28, 2013

Uh oh!

Uh oh!