Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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
radekbenkel commentedJul 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 thishttps://github.com/GerHobbelt/nicejson-php/blob/master/nicejson.php ? Another solutions is to:
|
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.
GromNaN commentedJul 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 .'}'; |
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.
hacfi commentedJul 27, 2013
@Singles Great addition..thanks for the PR! |
radekbenkel commentedJul 28, 2013
Should I squash these commits into one and/or open new PR? |
wouterj commentedJul 28, 2013
Just squash them |
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?
stof commentedAug 5, 2013
you also need to create the service in FrameworkBundle to register 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 should be a private method
fabpot commentedAug 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. |
hacfi commentedAug 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. |
fabpot commentedAug 11, 2013
@hacfi Which online tool are you talking about? Any examples? |
radekbenkel commentedAug 11, 2013
@fabpot Crowdin for examplehttp://crowdin.net/page/tour/supported-formats About this JSON PITA thing - I would say that is a subjective opinion. |
hacfi commentedAug 12, 2013
We're usinghttps://phraseapp.com/ and the provider we used before (can't On Sunday, August 11, 2013, Fabien Potencier wrote:
|
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.
hacfi commentedSep 28, 2013
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.