wp-cli / i18n-command

Provides internationalization tools for WordPress projects.
MIT License
96 stars 52 forks source link

`i18n make-pot` with `--merge` doesn't copy translated plural terms #324

Closed senadir closed 2 years ago

senadir commented 2 years ago

Bug Report

Describe the current, buggy behavior

When running wp i18n make-pot to generate a new .po file based on an existing .po, translations for plural terms aren't being copied.

I know i18n is meant to generate pot files, not po files, but I'm using it to an existing translation .po after some paths changed, so in a nutshell:

An example is the health-check plugin:

Describe what you would expect as the correct outcome

Plural strings, just like regular strings, should be preserved.

Let us know what environment you are running this on This was on mac and also docker.

Provide a possible solution

I know that msgmerge works fine, I'm not sure if this is an issue with php-gettext or wp i18n itself. It should be noted that when you don't pass merge and provide attempt to rewrite on the old file, all translation is gone, contrary to the docs., I was wrong, to use the same file, you need to pass --merge with no value.

swissspidy commented 2 years ago

Thanks for reporting. I'll try to take some time to investigate this.

Since you mentioned msgmerge as an alternative, perhaps the proposed wp i18n update-po command in #316 might be better suitable for this task. Perhaps you could give that a try? I hope it does not have the same problems.

It should be noted that when you don't pass merge and provide attempt to rewrite on the old file, all translation is gone, contrary to the docs.

Can you elaborate on that?


Aside: I never understood why WooCommerce Blocks does all this complex logic with translation files. It can quickly lead to issues, for example when using the Preferred Languages plugin. What's wrong with using the translation files provided by WordPress.org? And if there's a bug, why not create a Trac ticket for it?

senadir commented 2 years ago

Since you mentioned msgmerge as an alternative, perhaps the proposed wp i18n update-po command in https://github.com/wp-cli/i18n-command/pull/316 might be better suitable for this task. Perhaps you could give that a try? I hope it does not have the same problems.

It seems it does exactly what I want, I will try build the command and give it a try.

Aside: I never understood why WooCommerce Blocks does all this complex logic with translation files. It can quickly lead to issues, for example when using the Preferred Languages plugin. What's wrong with using the translation files provided by WordPress.org? And if there's a bug, why not create a Trac ticket for it?

The Preferred Languages issue was fixed, or at least, is pending merge and release :D

Main issues come from the fact that we have dynamically generated files that are loaded on demand with webpack (lazy loaded mainly), and we need to ensure the json chunk file is loading, the issue with Preferred Languages is that some files changes path/output location, but were not reflected in the code that loads the json.

Future releases would remove this explicit coupling and makes loading translation more automated.

We also use the translation in wordpress.org, the goal of updating a .po manually is for our E2E tests. We have tests that will switch the language and make sure that one is being loaded, but if the issue above happens (files switched), the language no longer match until we do a release and wait for translate.wordpress.org to release new json files, so we build those json files ourselves in CI to simulate a ready release version.

senadir commented 2 years ago

It should be noted that when you don't pass merge and provide attempt to rewrite on the old file, all translation is gone, contrary to the docs.

Can you elaborate on that?

After testing again, it seems I didn't use the command correctly. Originally, I assumed if you want to write to an existing .po, it will get updated, at least, this is what I assumed with "if left empty"

  [--merge[=<paths>]]
    Comma-separated list of POT files whose contents should be merged with the extracted strings.
    If left empty, defaults to the destination POT file. POT file headers will be ignored.

What I did was, I had a ready file test.po and I just run make-pot over it, so wp i18n make-pot ./ test.po ..... After reading the docs again, I just passed --merge and it worked, so wp i18n make-pot ./ test.po --merge.

swissspidy commented 2 years ago

I just added a test case to #316 to verify that plural translations are correctly kept when updating a PO file from a POT file.

And since the make-pot command's --merge argument is otherwise meant for merging two POT files (without translations), I don't think it warrants any changes there.

Hopefully soon we'll be able to release the new version of this package with the new update-po command.

Closing this ticket for now.

The Preferred Languages issue was fixed, or at least, is pending merge and release

Awesome! 👍

We also use the translation in wordpress.org, the goal of updating a .po manually is for our E2E tests. We have tests that will switch the language and make sure that one is being loaded, but if the issue above happens (files switched), the language no longer match until we do a release and wait for translate.wordpress.org to release new json files, so we build those json files ourselves in CI to simulate a ready release version.

Ah, that makes sense. Nice to see plugins testing localization in e2e tests :)