Bug 37303

Created attachment 168712 [details] [review]
Bug 37303: Replace po2json with a JS version

When using __() (ie. Gettext.js) we are seeing the translations that are marked as fuzzy.
This is definitely not the expected behaviour.

It happens because (our version of) po2json are old and no longer maintained,
and just embed them.

It seems that the bin we have has been upgraded to a JS version
(different authors).

Test plan:
(replace LANG with your language code)
0. Do not apply this patch
Edit misc/translator/po/LANG-messages-js.po
Mark a string as fuzzy
Edit ./intranet-main.tt and add the following lines inside $(document).ready
  console.log(_("Your string"));
  console.log(__("Your string"));
Replace "Your string" with the string you are actually testing.

Update the templates: `koha-translate --update LANG --dev kohadev && restart_all`

Go to the Koha home page, open the console.

=> Notice that the second log in the console is displaying the fuzzy string.

1. Apply this patch
Install the new version of po2json using `yarn install`
Repeat the previous steps.

=> With this patch applied both logs show the English version of the
string.

Remove fuzzy, update the templates and try again.

This seems bad, how didn't we catch that before?

I guess it only affects languages where not everything is translated - either due to lack of translators or because it's English to English (so no translation needed, in most cases). Speaking for the en-GB translation crew, we simply assumed it was our fault; that we'd messed up our file at some point in the past!

Patch does not work for me. po2json expects 2 arguments (<input> <output>) and we only give one.
So the resulting locale_data.json looks like this:

var json_locale_data = {"Koha":yarn run v1.22.20
>>$ /home/koha/koha/node_modules/.bin/po2json /home/koha/koha/misc/translator/po/fr-FR-messages-js.po

>>output argument is required

>>Usage: po2json <input> <output> [options]
[... full of usage ...]
};

Can't we add a "next if $po->fuzzy" in misc/translator/po2json ? It looks easier

(In reply to Julian Maurice from comment #5)
> Can't we add a "next if $po->fuzzy" in misc/translator/po2json ? It looks
> easier

Maybe, but I thought it was better to rely on a maintained library and remove the binary we embedded.

Created attachment 168713 [details] [review]
Bug 37303: Replace po2json with a JS version

When using __() (ie. Gettext.js) we are seeing the translations that are marked as fuzzy.
This is definitely not the expected behaviour.

It happens because (our version of) po2json are old and no longer maintained,
and just embed them.

It seems that the bin we have has been upgraded to a JS version
(different authors).

Test plan:
(replace LANG with your language code)
0. Do not apply this patch
Edit misc/translator/po/LANG-messages-js.po
Mark a string as fuzzy
Edit ./intranet-main.tt and add the following lines inside $(document).ready
  console.log(_("Your string"));
  console.log(__("Your string"));
Replace "Your string" with the string you are actually testing.

Update the templates: `koha-translate --update LANG --dev kohadev && restart_all`

Go to the Koha home page, open the console.

=> Notice that the second log in the console is displaying the fuzzy string.

1. Apply this patch
Install the new version of po2json using `yarn install`
Repeat the previous steps.

=> With this patch applied both logs show the English version of the
string.

Remove fuzzy, update the templates and try again.

There is a deprecation warning that should be removed.

I compared locale_data.js before and after the patch:
with the patch:
* keys are not sorted (does not matter)
* header keys are lowercase (should not matter but needs to be tested)
* header values are trimmed (good)
Other than that they are identical

(In reply to Julian Maurice from comment #9)
> Other than that they are identical
And other than the fact that fuzzy messages are not included, obviously :)

Created attachment 169536 [details] [review]
Bug 37303: Replace po2json with a JS version

When using __() (ie. Gettext.js) we are seeing the translations that are marked as fuzzy.
This is definitely not the expected behaviour.

It happens because (our version of) po2json are old and no longer maintained,
and just embed them.

It seems that the bin we have has been upgraded to a JS version
(different authors).

Test plan:
(replace LANG with your language code)
0. Do not apply this patch
Edit misc/translator/po/LANG-messages-js.po
Mark a string as fuzzy
Edit ./intranet-main.tt and add the following lines inside $(document).ready
  console.log(_("Your string"));
  console.log(__("Your string"));
Replace "Your string" with the string you are actually testing.

Update the templates: `koha-translate --update LANG --dev kohadev && restart_all`

Go to the Koha home page, open the console.

=> Notice that the second log in the console is displaying the fuzzy string.

1. Apply this patch
Install the new version of po2json using `yarn install`
Repeat the previous steps.

=> With this patch applied both logs show the English version of the
string.

Remove fuzzy, update the templates and try again.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

It works! :)

----

commit:
> It happens because (our version of) po2json are old and no longer maintained,
> and just embed them.

(In reply to Jonathan Druart from comment #6)
> (In reply to Julian Maurice from comment #5)
> > Can't we add a "next if $po->fuzzy" in misc/translator/po2json ? It looks
> > easier
> 
> Maybe, but I thought it was better to rely on a maintained library and
> remove the binary we embedded.


About that... 
The latest version is from 2016 ^^"
https://github.com/mikeedwards/po2json/tags
And the dates of the alpha version suggest the efforts for the next version have stalled :(

That causes worries about the dependencies of po2json https://github.com/mikeedwards/po2json/issues/101
That nomnom dependency was last published 10 years ago and is officially abandoned: https://www.npmjs.com/package/nomnom
(still 424 180 Weekly Downloads ﷐[U+1F480]﷑)
And it depends on underscore 1.6.0 which has the security vulnerability mentioned in the po2json ticket.
But nomnom seems to be just to parse command line arguments for po2json so it should be okay. Though it shows the need to look at the rest of the dependency tree.
That would be bad if malicious translations could be used to remotely execute code at the packaging step. Or on devs machines.
Though that's still a worry with the perl po2json. It uses a 2014 version of Locale::PO to read the .po files and a 2011 version of JSON to generate JSON.

Created attachment 169718 [details] [review]
Bug 37303: Replace po2json with a JS version

When using __() (ie. Gettext.js) we are seeing the translations that are marked as fuzzy.
This is definitely not the expected behaviour.

It happens because (our version of) po2json are old and no longer maintained,
and just embed them.

It seems that the bin we have has been upgraded to a JS version
(different authors).

Test plan:
(replace LANG with your language code)
0. Do not apply this patch
Edit misc/translator/po/LANG-messages-js.po
Mark a string as fuzzy
Edit ./intranet-main.tt and add the following lines inside $(document).ready
  console.log(_("Your string"));
  console.log(__("Your string"));
Replace "Your string" with the string you are actually testing.

Update the templates: `koha-translate --update LANG --dev kohadev && restart_all`

Go to the Koha home page, open the console.

=> Notice that the second log in the console is displaying the fuzzy string.

1. Apply this patch
Install the new version of po2json using `yarn install`
Repeat the previous steps.

=> With this patch applied both logs show the English version of the
string.

Remove fuzzy, update the templates and try again.

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Thanks for the work here guys.. all looking good from my side.  Passing QA

Thanks for all the hard work!

Pushed to main for the next 24.11.00 release as RM Assistant

I'm not sure if this error message is related to this bug when trying to install a language in KTD:

koha-translate --install de-DE --dev kohadev
error Command "po2json" not found.                                             
read_file '/tmp/de-DE-messages.po' - open: No such file or directory at /kohadevbox/koha/misc/translator/LangInstaller.pm line 461.

(Assuming that this is still the correct way to just install a language for testing, not update or test any other language changes.)

This is on the current main, with KTD all up to date (git pull and ktd pull), and po on main and up to date as well.

Created attachment 169975 [details] [review]
Bug 37303: Update yarn.lock after adding new dependency to packages.json

use ./xt/verify-yarnlock.t test in BZ 37490 to confirm

I have pushed Mason's follow-up to main. 

I think there was some discussion about using a different module/dependency maybe, but felt that would fix things for now while we dig deeper - maybe on a separate bug?

Backported to 24.05.x for upcoming 24.05.04

Conflicts on backport to 23.11.x
Please provide rebased patches if possible.

A quick note for here, nomnom has been removed in the 1.0.0-alpha

https://github.com/mikeedwards/po2json/releases/tag/1.0.0-alpha

Looks like gettext is using it: https://github.com/guillaumepotier/gettext.js/pull/61

https://www.npmjs.com/package/@myrotvorets/po2json is that any good as a more maintained option?  It's not well documented which worries me, but it does appear to have more recent commits.

(In reply to Martin Renvoize (ashimema) from comment #22)
> https://www.npmjs.com/package/@myrotvorets/po2json is that any good as a
> more maintained option?  It's not well documented which worries me, but it
> does appear to have more recent commits.

It's not doing anything useful.

https://github.com/myrotvorets/po2json/blob/master/po2json.js

vs the one we use:

https://github.com/mikeedwards/po2json/blob/master/lib/parse.js

And commits are only bots (but at least there are releases).

yarn is now mandatory it seems, should we make it a dependency?

https://chat.koha-community.org/koha-community/pl/19zg9oqccj8u3pkxjqrqctayow

Mason, we need yarn in production to generate the translation.
For ktd we are installing it using their repo https://gitlab.com/koha-community/koha-testing-docker/-/blob/main/dists/bookworm/Dockerfile?ref_type=heads#L114
I don't remember why but I guess it is to get the same version across the OS, and have an up-to-date version.

Should we do the same in prod env? Or simply install yarnpkg from apt?

(In reply to Jonathan Druart from comment #26)
> Mason, we need yarn in production to generate the translation.
> For ktd we are installing it using their repo
> https://gitlab.com/koha-community/koha-testing-docker/-/blob/main/dists/
> bookworm/Dockerfile?ref_type=heads#L114
> I don't remember why but I guess it is to get the same version across the
> OS, and have an up-to-date version.
> 
> Should we do the same in prod env? Or simply install yarnpkg from apt?

The problem is yarn versions vary and it was not working. Do we need Yarn in production now?

Having looked at the change... It feels like adding `yarnpkg` to the dependencies should be enough. That said:

* `yarn run po2json` is going to fetch the library from the internet in runtime, right?
* The problem with node libraries is not the *Yarn* version but *Node.js* itself. Some libraries will not work on some versions of Node. That's why we make sure every KTD image has the exact version we are expecting from *Node.js*. In some cases we are pinning the version because the OS provides a newer than desired one. For Yarn we are pulling the latest available all the time and should not be problematic.

We really need to try on the supported OSes.

(In reply to Tomás Cohen Arazi (tcohen) from comment #27)
> > Should we do the same in prod env? Or simply install yarnpkg from apt?
> 
> The problem is yarn versions vary and it was not working. Do we need Yarn in
> production now?

It looks like we need it in order to be able to install and update languages (also see email to the koha mailing list)

(In reply to Julian Maurice from comment #5)
> Can't we add a "next if $po->fuzzy" in misc/translator/po2json ? It looks
> easier

^^ we need to rollback and do what Julian proposes.

Adding nodejs and yarn as dependencies for production is not straight-forward.

I know MTJ has ben looking at Debian's `node-po2json` package. But shouldn't we revert in stable until we have a proper/tested solution?

(In reply to Tomás Cohen Arazi (tcohen) from comment #30)
> (In reply to Julian Maurice from comment #5)
> > Can't we add a "next if $po->fuzzy" in misc/translator/po2json ? It looks
> > easier
> 
> ^^ we need to rollback and do what Julian proposes.
> 
> Adding nodejs and yarn as dependencies for production is not
> straight-forward.
> 
> I know MTJ has ben looking at Debian's `node-po2json` package. But shouldn't
> we revert in stable until we have a proper/tested solution?

That makes sense to me. Could you provide a patch for Julian's suggestion?

I am not sure yet if these patches will revert cleanly, if not we might also need a patch to do it (or someone to give me a hint)

Continuation is in Bug 38164

(In reply to Victor Grousset/tuxayo from comment #32)
> Continuation is in Bug 38164

Only for stable

Hi guys will this be backported to 23.11 or not advisable?

(In reply to Pedro Amorim from comment #34)
> Hi guys will this be backported to 23.11 or not advisable?

No, please don't backport these patches. 

I think the second patch from bug 38164 could be good for backporting, but I'd like Joubu or Julian to confirm.

None should be backportesbackported here, you need the second parch from bug  38164.

Stupid phone