Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Sep 27, 2013

Conversation

Projects
None yet
8 participants
Contributor

singles commented Jul 20, 2013

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
Contributor

singles commented Jul 20, 2013

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:

  1. Remove JsonDumper
  2. Throw exception in PHP < 5.4

@GromNaN GromNaN commented on the diff Jul 20, 2013

...mfony/Component/Translation/Loader/JsonFileLoader.php
+class JsonFileLoader extends ArrayLoader implements LoaderInterface
+{
+ /**
+ * {@inheritdoc}
+ */
+ public function load($resource, $locale, $domain = 'messages')
+ {
+ if (!stream_is_local($resource)) {
+ throw new InvalidResourceException(sprintf('This is not a local file "%s".', $resource));
+ }
+
+ if (!file_exists($resource)) {
+ throw new NotFoundResourceException(sprintf('File "%s" not found.', $resource));
+ }
+
+ $messages = json_decode(file_get_contents($resource), true);
@GromNaN

GromNaN Jul 20, 2013

Contributor

You should use json_last_error to handle errors.

@GromNaN GromNaN commented on an outdated diff Jul 20, 2013

...mfony/Component/Translation/Dumper/JsonFileDumper.php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Translation\Dumper;
+
+use Symfony\Component\Translation\MessageCatalogue;
+
+/**
+ * IniFileDumper generates an ini formatted string representation of a message catalogue.
@GromNaN

GromNaN Jul 20, 2013

Contributor

The comment is incorrect. ini => json

Contributor

GromNaN commented Jul 20, 2013

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 jakzal commented on an outdated diff Jul 27, 2013

...mfony/Component/Translation/Dumper/JsonFileDumper.php
+
+ /**
+ * Formats JSON into human readable form.
+ * Fallback for PHP < 5.4, where JSON_PRETTY_PRINT option is missing
+ *
+ * @param array $messages
+ * @return string Human readable JSON representation
+ */
+ protected function formatPretty(array $messages)
+ {
+ $lines = array();
+ foreach ($messages as $key => $value) {
+ $lines[] = json_encode($key) . ': ' . json_encode($value);
+ }
+
+ return '{' . PHP_EOL . ' ' . implode(',' . PHP_EOL . ' ', $lines) . PHP_EOL . '}';
@jakzal

jakzal Jul 27, 2013

Member

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.

@jakzal jakzal commented on an outdated diff Jul 27, 2013

...mfony/Component/Translation/Dumper/JsonFileDumper.php
+use Symfony\Component\Translation\MessageCatalogue;
+
+/**
+ * JsonFileDumper generates an json formatted string representation of a message catalogue.
+ *
+ * @author singles
+ */
+class JsonFileDumper extends FileDumper
+{
+ /**
+ * {@inheritDoc}
+ */
+ public function format(MessageCatalogue $messages, $domain = 'messages')
+ {
+ $data = $messages->all($domain);
+ if (PHP_MINOR_VERSION < 4) {
@jakzal

jakzal Jul 27, 2013

Member

We usually use the version_compare() function to verify the PHP version:

if (version_compare(PHP_VERSION, '5.4.0', '<')) {
}
Contributor

hacfi commented Jul 27, 2013

@singles Great addition..thanks for the PR!

Contributor

singles commented Jul 28, 2013

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

Member

wouterj commented Jul 28, 2013

Just squash them

@wouterj wouterj referenced this pull request in symfony/symfony-docs Jul 28, 2013

Closed

[WIP] Bootstrapped the Translation documentation #2545

1 of 1 task complete

@staabm staabm commented on the diff Aug 4, 2013

...mfony/Component/Translation/Loader/JsonFileLoader.php
+ $errorMsg = 'Maximum stack depth exceeded';
+ break;
+ case JSON_ERROR_STATE_MISMATCH:
+ $errorMsg = 'Underflow or the modes mismatch';
+ break;
+ case JSON_ERROR_CTRL_CHAR:
+ $errorMsg = 'Unexpected control character found';
+ break;
+ case JSON_ERROR_SYNTAX:
+ $errorMsg = 'Syntax error, malformed JSON';
+ break;
+ case JSON_ERROR_UTF8:
+ $errorMsg = 'Malformed UTF-8 characters, possibly incorrectly encoded';
+ break;
+ default:
+ $errorMsg = 'Unknown error';
@staabm

staabm Aug 4, 2013

Contributor

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.

@singles

singles Aug 4, 2013

Contributor

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

@stof stof commented on an outdated diff Aug 5, 2013

...mfony/Component/Translation/Dumper/JsonFileDumper.php
+/**
+ * JsonFileDumper generates an json formatted string representation of a message catalogue.
+ *
+ * @author singles
+ */
+class JsonFileDumper extends FileDumper
+{
+ /**
+ * {@inheritDoc}
+ */
+ public function format(MessageCatalogue $messages, $domain = 'messages')
+ {
+ $data = $messages->all($domain);
+ if (version_compare(PHP_VERSION, '5.4.0', '<')) {
+ return $this->formatPretty($data);
+ } else {
@stof

stof Aug 5, 2013

Member

no need for else as the if returns

@stof stof and 1 other commented on an outdated diff Aug 5, 2013

...mfony/Component/Translation/Loader/JsonFileLoader.php
+ $messages = array();
+ }
+
+ $catalogue = parent::load($messages, $locale, $domain);
+ $catalogue->addResource(new FileResource($resource));
+
+ return $catalogue;
+ }
+
+ /**
+ * Translates JSON_ERROR_* constant into meaningful message
+ *
+ * @param integer $code Error code returned by json_last_error() call
+ * @return string Message string
+ */
+ protected function getJSONErrorMessage($code)
@stof

stof Aug 5, 2013

Member

on PHP 5.5, you could use json_last_error_msg to get the message for the last error.

and the method should be private

@singles

singles Aug 5, 2013

Contributor

Yes, but I left it this way to keep messages the same for PHP < 5.5.0. And code of this method was taken directly from PHP manual.

Does Symfony have some coding standard about private vs protected? This, or this?

@stof

stof Aug 9, 2013

Member

We use private everywhere, except in places where we want to provide an extension point.

Member

stof commented Aug 5, 2013

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

@stof stof commented on an outdated diff Aug 5, 2013

...mfony/Component/Translation/Dumper/JsonFileDumper.php
+ /**
+ * {@inheritDoc}
+ */
+ protected function getExtension()
+ {
+ return 'json';
+ }
+
+ /**
+ * Formats JSON into human readable form.
+ * Fallback for PHP < 5.4, where JSON_PRETTY_PRINT option is missing
+ *
+ * @param array $messages
+ * @return string Human readable JSON representation
+ */
+ protected function formatPretty(array $messages)
@stof

stof Aug 5, 2013

Member

this should be a private method

Owner

fabpot commented Aug 10, 2013

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.

Contributor

hacfi commented Aug 11, 2013

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.

Owner

fabpot commented Aug 11, 2013

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

Contributor

singles commented Aug 11, 2013

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

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

Contributor

hacfi commented Aug 12, 2013

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/symfony/symfony/pull/8534#issuecomment-22453447
.

@fabpot fabpot and 2 others commented on an outdated diff Aug 13, 2013

...mfony/Component/Translation/Dumper/JsonFileDumper.php
+use Symfony\Component\Translation\MessageCatalogue;
+
+/**
+ * JsonFileDumper generates an json formatted string representation of a message catalogue.
+ *
+ * @author singles
+ */
+class JsonFileDumper extends FileDumper
+{
+ /**
+ * {@inheritDoc}
+ */
+ public function format(MessageCatalogue $messages, $domain = 'messages')
+ {
+ $data = $messages->all($domain);
+ if (version_compare(PHP_VERSION, '5.4.0', '<')) {
@fabpot

fabpot Aug 13, 2013

Owner

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

@singles

singles Aug 13, 2013

Contributor

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

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

@singles

singles Sep 3, 2013

Contributor

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.

@fabpot

fabpot Sep 3, 2013

Owner

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.

@stof

stof Sep 3, 2013

Member

@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.

@singles

singles Sep 3, 2013

Contributor

@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:

  1. If php < 5.4 then mark test as skipped
  2. 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.
  3. Instead of checking file format, parse file and check parsed data structure, but for me it not good test case for dumper (look above)
  4. ?
@fabpot

fabpot Sep 3, 2013

Owner

Option 1 seems the best.

@singles

singles Sep 3, 2013

Contributor

@fabpot Updated PR.

@fabpot fabpot and 1 other commented on an outdated diff Sep 3, 2013

...mfony/Component/Translation/Dumper/JsonFileDumper.php
+ define('JSON_PRETTY_PRINT', 128);
+}
+
+/**
+ * JsonFileDumper generates an json formatted string representation of a message catalogue.
+ *
+ * @author singles
+ */
+class JsonFileDumper extends FileDumper
+{
+ /**
+ * {@inheritDoc}
+ */
+ public function format(MessageCatalogue $messages, $domain = 'messages')
+ {
+ $data = $messages->all($domain);
@fabpot

fabpot Sep 3, 2013

Owner

no need to create a temp var here.

@singles

singles Sep 3, 2013

Contributor

@fabpot True, fixed and updated.

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

1 check passed

default The Travis CI build passed
Details

@singles singles deleted the singles:translation-json branch Sep 27, 2013

Contributor

hacfi commented Sep 28, 2013

Thanks @fabpot !

@singles singles referenced this pull request in symfony/symfony-docs Jan 5, 2014

Merged

Translation - Added info about JsonFileLoader added in 2.4 #3428

weaverryan added a commit to symfony/symfony-docs that referenced this pull request Jan 9, 2014

feature #3428 Translation - Added info about JsonFileLoader added in …
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment