Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.4] Add new JSON loader for translations #16424

Merged
merged 2 commits into from
Nov 16, 2016
Merged

[5.4] Add new JSON loader for translations #16424

merged 2 commits into from
Nov 16, 2016

Conversation

themsaid
Copy link
Member

This PR introduces a new form of handling localization that's based on .json files instead of current PHP array files, the new format is similar to the popular gettext format except that it loads from json instead of po files.

__('Product ":product" was saved successfully.', [$product->name]);

This line will load the resources/lang/en.json and look for a Product ":product" was saved successfully. key inside it, if key wasn't found it just returns the given string.

The replacement of parameters happens even if the key wasn't found so that you don't have to have translation files for your main language, using the keys only would work.

If no translation was found in the JSON files, Laravel will start looking for translation keys in the old PHP array files located in resources/lang/en, in case a match was found it's returned to the user. That way the method is fully backward compatible.

The old translation methods will work as they used to trans() & trans_choice(), these PR doesn't look into deprecating these methods, it just introduces an alternative approach.

@themsaid themsaid changed the title [Proposal] Add new JSON loader for translations [5.4][Proposal] Add new JSON loader for translations Nov 15, 2016
protected function loadJson($path, $locale)
{
if ($this->files->exists($full = "{$path}/{$locale}.json")) {
return (array) json_decode($this->files->get($full));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use json_decode($this->files->get($full), true) instead? It'll give us an associative array [1] :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, changed :)

@taylorotwell taylorotwell merged commit 2ad4805 into laravel:master Nov 16, 2016
@tillkruss
Copy link
Contributor

@themsaid: Maybe the __() helper should also use the array type hint, just like getFromJson() does?

@dionysiosarvanitis
Copy link
Contributor

Why not using something more fluent over one-letter functions? __() seems kind of noisy or at least use t().
Issue is related to laravel/ideas#252.

@kduma
Copy link
Contributor

kduma commented Nov 18, 2016

I think this PR provides unnecessary mess and redundancy with old trans function.

@themsaid
Copy link
Member Author

@kduma how?

@taylorotwell
Copy link
Member

Actually “__” was the name of the function in Laravel 1.0 - 3.0. #trivia :)

@kduma
Copy link
Contributor

kduma commented Nov 18, 2016

@themsaid now there are two ways of storing translations, the current way in PHP files (trans function) and new way, in json files ('__' function).

@kduma
Copy link
Contributor

kduma commented Nov 18, 2016

@taylorotwell in my first PHP projects, I've also used __ functions for translations. (It was in times of free Translation API from google)

@kkomelin
Copy link

Glad to see the Laravel translation system improving. Thanks.

@sisve
Copy link
Contributor

sisve commented Nov 19, 2016

  1. The title implies that there is a new json loader, but there isn't. Why wasn't this implemented as a new JsonFileLoader (implementing LoaderInterface)?
  2. Why is __ introduced as a new translation function that's hardcoded to work against a specific storage format?

@taylorotwell
Copy link
Member

  1. What's the difference?
  2. It doesn't really matter to me what the function name is. __ is what the translation function was called previously in Laravel. I haven't though through the implementations of having trans check the JSON translations first and then fall back to the grouped array files but it may be a possibility.

@sisve
Copy link
Contributor

sisve commented Nov 19, 2016

  1. Structure, mostly. There's already a ArrayLoader and a FileLoader. I doubt the file-loader will be extended to support all future file formats; I consider it more appropriate with a JsonLoader in this case.
  2. I do not have a problem with a __ function for translation, the problem, as I see it, is that it is hardcoded to a specific type of translation thingie (it calls $translator->getJson()) which ends up with a json_decode($this->files->get($full), true) call that basically circumvents the existing LoaderInterface structure.

This, combined with the makeJsonReplacements method, looks odd. Doesn't trans() already support this parameter substitution? Why wasn't that code reused?

@kkomelin
Copy link

@sisve Although your questions/doubts might be reasonable, it would be more constructive if you'd propose your improvements instead of questioning solutions of others.
Opensource means collaboration. If we think something is not perfect or we'd implement it in a better way, we propose our improvements through issues or PRs. I encourage you to do the same. Thanks.

@taylorotwell
Copy link
Member

makeJsonReplacements was removed.

@taylorotwell
Copy link
Member

@sisve we welcome your improvements to the loader setup via PR.

@cjmaxik
Copy link

cjmaxik commented Jan 25, 2017

So, what about piped plural strings?
{0}Nobody playing|:count player|:count players

@GrahamCampbell GrahamCampbell changed the title [5.4][Proposal] Add new JSON loader for translations [5.4] Add new JSON loader for translations Jan 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants