Added option to keep initial translated messages #370
Conversation
| * @var bool | ||
| */ | ||
| private $keepOldTranslationsMessages; | ||
|
|
There was a problem hiding this comment.
Very minor nitpick, this variable should be named keepOldTranslationMessages (without the s on Translation)
|
Looks interesting. One small nitpick that I left in the comments about the variable name. I'd be ready to merge this feature except it is missing tests and it would also be nice to have a little documentation added about the switch. Thanks for the PR. |
|
Should I change everywhere from plural to singular? Since it's everywhere as plural, not only those 2 situations you showed me. Or you want only those 2 cases? I'll be adding docs soon, as for tests I don't know exactly where to write them and how to run them. |
|
Yeah all the function names should be adjusted accordingly once the variable name is changed. |
|
As for tests, there are some already in the Tests directory, I would suggest there should be a Tests/Translation/UpdaterTest.php file created that runs one or two tests with the switch on and off, ensuring that the output is as expected. You may have to create a fixture file someplace there to check against. |
| foreach ($domainCatalogue->all() as $message) { | ||
|
|
||
| $translated = $this->translator->trans($message->getId(), array(), $message->getDomain(), $locale); | ||
| $message->setLocaleString($translated); |
There was a problem hiding this comment.
So this seems odd to me that you are translating as you go. If all you want to do is keep any messages that were already there, why is this necessary?
|
Well if you have a project that you haven't ran the command on just yet, and you run it you will get
instead of
All your messages will be replaced with their keys. My method however, it first translates the key and sets it as the message. |
|
Hm, I don't seem to find the cookbook docs on the website? They should be added, they contain useful info. Or you need access from jms? |
|
I wanted to run the current tests before adding my own to see if it works. I'm running Could it be because I'm on Windows? I'd like to have these work before trying to add my own. Any idea on what should I do? |
|
Hey @Nyholm should we get a PR that uses DIRECTORY_SEPARATOR so that tests function on windows when dealing with the paths? |
|
I think I found that DIRECTORY_SEPARATOR might be a solution for this problem too, but thought of it to be troublesome for me to try and do it properly. I'm going to write tests for these as soon as I'll get on a linux machine. |
|
I have heard that newer versions of Windows (maybe Windows server 2013) handles this properly. @clytemnestra, what OS are you running? |
|
That was on Win10 with XAMPP. |
|
I have review this but I do not understand the problem. Or, I can reproduce it. I have a project where I do not use this bundle. The project has a lot of translations in /app/Resources/translations
My config: jms_translation:
locales: ['en', 'sv'] # List of locales supported by your application
source_language: 'en' # The language that the sources is written in
configs:
app:
dirs: ["%kernel.root_dir%", "%kernel.root_dir%/../src"]
output_dir: "%kernel.root_dir%/Resources/translations"
ignored_domains: [routes]
excluded_names: ["*TestCase.php", "*Test.php"]
excluded_dirs: [cache, data, logs, Tests]Can you give me a way to reproduce this error where all translations is gone when using the bundle on an existing project? |
|
That's what I also did on a project that had translations not in app/Resources, but in bundles. The output was sent in app/Resources as xliff, but the keys were put as the translation value. The page wasn't chaging unless I deleted cache, even in dev mode. I'll try tomorrow to test again and see if it still happens, maybe it was an issue with an old version. |
|
Yeah, same issue. jms_translation:
configs:
app:
dirs: [%kernel.root_dir%, %kernel.root_dir%/../src]
output_dir: %kernel.root_dir%/Resources/translations
ignored_domains: [routes]
excluded_names: ["*TestCase.php", "*Test.php"]
excluded_dirs: [cache, data, logs]
extractors: [alias_of_the_extractor]And after deleting the cache I still get keys instead of messages. Source: Result: |
|
What if you add output format to your config? jms_translation:
configs:
app:
dirs: [%kernel.root_dir%, %kernel.root_dir%/../src]
output_dir: %kernel.root_dir%/Resources/translations
ignored_domains: [routes]
excluded_names: ["*TestCase.php", "*Test.php"]
excluded_dirs: [cache, data, logs]
extractors: [alias_of_the_extractor]
output_format: "yaml" |
|
Same result. Just got .xlf instead of .xliff |
|
What if you use the "keep"? translation:extract en --keep --dir=./src/ --output-dir=./app/Resources/translations |
|
I thought --keep is for keeping translation keys in the already generated files that have been removed from the project, so they don't get removed from the file, too. I'll try to run it tomorrow and see what it does, but doubt it'll solve the problem. |
|
Yup, won't work. The thing is nowhere is the translator service used to translate the keys, they are just extracted and inserted in the file as is. I've applied a translation to the keys before they are added in the file, that's all. |
|
It is strange, this also works for me, whenever I run the extract command the old translations stay in place, only the new strings are added to the file, but no translations are removed. |
|
Hi, I'm really interested in this feature, will this PR be merged anytime soon ? |
Description
I've remade this PR on branch master, without whitespace changes.
The idea was to do this, so I added an option
--keeptmwhich will keep your already defined translation messages.Todos