search

translation / laravel

Laravel translation made __('simple').
https://translation.io/laravel
MIT License
168 stars 22 forks source link

Support for multiple JSON files #24

Closed inxilpro closed 3 years ago

inxilpro commented 3 years ago

We have a very large application that has multiple JSON translation files, registered with Lang::addJsonPath(). These files are split based on multiple modules within the app.

It looks like it'd be relatively simple to update GettextPOGenerator::jsonFiles() to use all the registered JSON paths. Something like:

$paths = [$this->application['path.lang']];

$loader = Lang::getLoader();
if ($loader instanceof FileLoader) {
  foreach ($loader->jsonPaths() as $jsonPath) {
    $paths[] = $jsonPath;
  }
}

foreach ($paths as $path) {
  foreach (glob($path . DIRECTORY_SEPARATOR . '*.json') as $filename) {
    $files[] = $filename;
  }
}

The trickier issue would be resolving which JSON file to write back to on sync. Is this something you're interested in supporting? I've been digging around in GettextTranslationSaver and it seems like it'd take a bit of a refactor to support this use-case.

MichaelHoste commented 3 years ago

I didn't know Lang::addJsonPath() was a thing, but it's a good idea to support it.

Your code seems the right approach to it.

The trickier issue would be resolving which JSON file to write back to on sync.

I'm wondering if this is really important. Since translations will be managed on Translation.io (only source JSON files will be touched by developers), the structure of target JSON files is not really relevant. Everything could be stored in resources/lang/TARGET.json and it would work, right?

The other more complex solution would be to store the path to the JSON file in the context of translations, like we do to differentiate JSON translations and GetText translations internally:

We could make the context more specific and transform it from:

Extracted from JSON file

to:

Extracted from JSON file [./resources/lang/module/en.json]

to be able to reuse this information when writing the target JSON.

What do you think about this?

inxilpro commented 3 years ago

My preference would definitely be to keep the translation files separate. Even though everything is managed via translation.io, it's really helpful to know which strings are associated with which major feature of the application. If we ever do a significant refactor or decide to sunset a feature, we need to be able to understand which translations will be affected.

MichaelHoste commented 3 years ago

If we ever do a significant refactor or decide to sunset a feature, we need to be able to understand which translations will be affected.

I understand that, and adding the path in the context will also allow you to filter the segments by feature on Translation.io interface.

But we need to keep Extracted from JSON file without any path for the default path, or else it will create duplicate segments on Translation.io for existing users.

Feel free to create a PR for this feature, or else we will work on it in the following weeks.

Could you please give me some of the Lang::addJsonPath() that you use? To be sure to test it on a real scenario.

inxilpro commented 3 years ago

I'm tinkering with a PR, but honestly—it almost feels like it'd be easier to write our own sync command that just calls the API. But it looks like the sync API isn't documented. Is this something that's stable and can be used by customers?

inxilpro commented 3 years ago

I've opened #25 for the sake of discussion, while I explore the options.

inxilpro commented 3 years ago

Also, if they sync API isn't supported, are there rate limit considerations around calling https://translation.io/api/v1/segments to implement a sync?

MichaelHoste commented 3 years ago

I've opened #25 for the sake of discussion, while I explore the options.

Thank you for this! I may help continue the PR.

Does your project already sync correctly with these changes? It I understand correctly, all your source JSON files will be synced correctly but the translations will be saved in the standard xx.json path, right? Does it bother Laravel or does your project already work correctly like this?

But it looks like the sync API isn't documented. Is this something that's stable and can be used by customers?

The API that is consumed by this package is not documented and uses GetText PO files to embed all translations (PHP, JSON and GetText) that your project may use.

We created a more modern documented API that is optimized for init and sync: https://translation.io/docs/create-library

And a classic API to simple manipulation of segments here: https://translation.io/docs/api

Also, if they sync API isn't supported, are there rate limit considerations around calling https://translation.io/api/v1/segments to implement a sync?

The only rate limite consideration is that only one sync can be executed simultaneously per project (actually, you can start many at the same time but they will be executed one after another using a mutex, with a timeout if it takes too long).

But my guess is that it will be way more simple to finish this PR. I may help finish it.

Could you please answer to this?

Could you please give me some of the Lang::addJsonPath() that you use? To be sure to test it on a real scenario.

I'm not sure what directory structure you are using for your JSON files.

inxilpro commented 3 years ago

As for our directory structure, we use this library:

https://github.com/InterNACHI/modular

This means our app can look like

And we may use source strings from both JSON files and key strings that look like group.foo for the primary app translations and module1::group.foo. (There are many modules.)

I haven't tried using our branch yet, but I was able to get one side of the sync working with similar changes direct in our vendor folder. It's one 1/4 of the work, though. The JSON translations are discovered and send up, but are written back to the wrong file, and the namespaced PHP translations aren't picked up at all.

MichaelHoste commented 3 years ago

Thank you for your directory structure.

As much as I can see the point of supporting JSON files from different paths (the single xx.json file limitation of Laravel seems problematic), I think that dealing with different complete resources/lang paths for PHP localization files, introducing a new module:: key prefix goes too far for the purpose of this package.

Dealing with different modules in the hierarchy of PHP localization files already exists in Laravel with the module/feature.key syntax that we already support.

Would you consider using this naming convention in the main resources/lang folder?

If so, your JSON files could be structured like this by completing your PR:

Does this make sense to you? If so, I could get the JSON files to be written back to the correct locations by extending your PR.

inxilpro commented 3 years ago

Yeah, I get where you're coming from. I think I'll take a stab at a custom sync command that works with our project, since it's somewhat outside of the scope of a "typical" Laravel project.

MichaelHoste commented 3 years ago

Yeah, I get where you're coming from. I think I'll take a stab at a custom sync command that works with our project, since it's somewhat outside of the scope of a "typical" Laravel project.

That works too, but be careful that PHP has key/value segments and JSON has source/translation segments.

We allow the source edition of key/values segments in the Translation interface (see here), but then you have to use this endpoint to avoid creating inconsistencies between your branches: https://translation.io/docs/create-library#source-edits

I'll still try to finish this Pull Request about JSON files, it still makes sense for typical Laravel projects I guess.

inxilpro commented 3 years ago

The module:: syntax comes from Laravel's localization implementation. Basically, vendor packages can use the package:: prefix, and you can override them in lang/vendor/{package}/{locale}/{group}.php.

See FileLoader::namespaces() for details.

In an ideal world, this package would be compatible with both JSON and PHP files registered anywhere, as long as they're registered properly with the FileLoader.

MichaelHoste commented 3 years ago

The good news

I almost finished the JSON implementation and it works (read&write).

I still need to:

I'll try to finish it tomorrow.

The bad news

I still feel that the module:: syntax is still too complex to implement in this package for a need that is very marginal.

But maybe this could be implemented the same way as the "subdirectories" feature, I'm not sure. This commit should give you an idea of what part it will need to touch: https://github.com/translation/laravel/commit/c14042eeadc27e177c03781a4d311d34c5d1c14a

Quick question: Is FileLoader::namespaces() completely parallel to FileLoader::jsonPaths() or both of them are linked in a way? Does adding a namespace also add a json path in the same directory? Like resources/lang?

inxilpro commented 3 years ago

FileLoader::namespaces() are registered PHP translations. FileLoader::jsonPaths() are registered JSON paths. They're totally separate.

They're mostly used in packages. See: https://laravel.com/docs/8.x/packages#translations

MichaelHoste commented 3 years ago

Thank you for the link and the precision.

I think that's a bit weird to have 2 completely separate processes for PHP and JSON paths, but it conforts me that supporting only multiple JSON paths (for now?) would already be an improvement.

If this works well with JSON, I might try to implement the same logic for PHP files without bringing too much complexity.

I'll keep you in touch.

Thank you for your help on this issue!

inxilpro commented 3 years ago

Here's a somewhat messy but working implementation of the two-way-sync that we're going to use. It might be useful for your implementation, too:

https://gist.github.com/inxilpro/a989488c34975d57321bdea6c6ffb36f

inxilpro commented 3 years ago

I just added a revision to my gist above. At this point, it's working very nicely for us. It handles:

It has partial support for source edits, but not 100%.

The main thing this doesn't do is any gettext-related functionality, since that's not the native Laravel way to handle translations.

MichaelHoste commented 3 years ago

Thanks @inxilpro,

I'm glad you have something that works for your use case.

We now also have a way to sync JSON files from different paths in the new 1.16 release thanks to you.

Unfortunately, we don't intend to go further and implement the loading of PHP files from different modules. Maybe in the future but right now it will just make the code more complex, and I'm not sure that feature is needed by anyone else.

Have a great week-end!