Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Translation] Added support for JSON format (both loader and dumper).#8534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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 thishttps://github.com/GerHobbelt/nicejson-php/blob/master/nicejson.php ? Another solutions is to:
|
| throw new NotFoundResourceException(sprintf('File "%s" not found.', $resource)); | ||
| } | ||
| $messages = json_decode(file_get_contents($resource), true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You should usejson_last_error to handle errors.
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 .'}'; |
| $lines[] = json_encode($key) . ': ' . json_encode($value); | ||
| } | ||
| return '{' . PHP_EOL . ' ' . implode(',' . PHP_EOL . ' ', $lines) . PHP_EOL . '}'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
@Singles Great addition..thanks for the PR! |
Should I squash these commits into one and/or open new PR? |
Just squash them |
| $errorMsg = 'Malformed UTF-8 characters, possibly incorrectly encoded'; | ||
| break; | ||
| default: | ||
| $errorMsg = 'Unknown error'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This part is taken directly from PHP's manual example (not user comments). What about passing errorCode as exception code?
you also need to create the service in FrameworkBundle to register it |
| * @param array $messages | ||
| * @return string Human readable JSON representation | ||
| */ | ||
| protected function formatPretty(array $messages) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this should be a private method
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. |
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. |
@hacfi Which online tool are you talking about? Any examples? |
@fabpot Crowdin for examplehttp://crowdin.net/page/tour/supported-formats About this JSON PITA thing - I would say that is a subjective opinion. |
We're usinghttps://phraseapp.com/ and the provider we used before (can't On Sunday, August 11, 2013, Fabien Potencier wrote:
|
| public function format(MessageCatalogue $messages, $domain = 'messages') | ||
| { | ||
| $data = $messages->all($domain); | ||
| if (version_compare(PHP_VERSION, '5.4.0', '<')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would remove this special case and just don't pretty format for PHP < 5.4 (because 5.3 is not supported anymore anyway).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@fabpot It is not? Didn't know that :)
I added this after first release, when Travis built (5.3 and 5.3.3) failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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)
- ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Option 1 seems the best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@fabpot Updated PR.
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/aCommits-------adf678b Added info about JsonFileLoader added in 2.4
Based on
IniFileLoader\Dumper.