emoncms icon indicating copy to clipboard operation
emoncms copied to clipboard

Gettext replacement

Open reedy opened this issue 4 years ago • 15 comments

Rework of https://github.com/emoncms/emoncms/pull/890 using composer base for the library, cherry picking in the commits ontop (because I can't modify that PR)

reedy avatar May 08 '20 15:05 reedy

Thanks @reedy, Looks great, have you moved just the user module files as an example? Not sure that Im following the rationale for moving locale to languages?

How does a user use this to enable gettext?

TrystanLea avatar May 08 '20 15:05 TrystanLea

Thanks @reedy, Looks great, have you moved just the user module files as an example? Not sure that Im following the rationale for moving locale to languages?

How does a user use this to enable gettext?

The second commit on here, https://github.com/emoncms/emoncms/pull/1602/commits/6f960a4e728a6389103ee6a8abb54991bab49f04 is not my work :). It's a literal cherry pick of https://github.com/emoncms/emoncms/pull/890/commits/b15886c7e0dbdae668ebe4e22691c9c5181631f3 with no changes on my part. The reasons for renaming the files, you'd need to ask @gablau as the author

As for "enabling gettext", it'd be a case of composer update --no-dev, which would probably need to become part of the installation instructions, if gettext is used in the course of normal web requests

reedy avatar May 08 '20 15:05 reedy

https://github.com/emoncms/emoncms/pull/890/commits/cb563b8765f1c553e7eecf3457634810683d301a doesn't apply cleanly, which is hardly a surprise after 2 years

reedy avatar May 08 '20 15:05 reedy

That's everything but https://github.com/emoncms/emoncms/pull/890/commits/7b1bd4fe2df4230f4e3ee2145ffe98c028580330 cherry-picked and rebased

I just noticed in https://github.com/emoncms/emoncms/pull/890/commits/8841e100d9bca821a8fff3f7f5b53335cdce3d15 that two different libraries are brought in...

reedy avatar May 08 '20 15:05 reedy

I just noticed in 8841e10 that two different libraries are brought in...

So probably means this stack doesn't quite work with the adding/renaming of functions...

reedy avatar May 08 '20 15:05 reedy

Thanks @reedy whats the reason for renaming _( to e( and __(? and @gablau do we need to move the language files or can we just keep them in the current locale directories?

TrystanLea avatar May 08 '20 18:05 TrystanLea

Is there perhaps a way to just take the way things are working at the moment and just load gettext via composer without further modifications or are those needed because of a different version of gettext or something?

TrystanLea avatar May 08 '20 18:05 TrystanLea

Thanks @reedy whats the reason for renaming _( to e( and __(?

Again, ask @gablau :). I just rebased his patches. Only the composer one at the bottom of the stack is mine

Is there perhaps a way to just take the way things are working at the moment and just load gettext via composer without further modifications or are those needed because of a different version of gettext or something?

As a replacement for the PHP gettext extension/ext-gettext?

As long the the API is the same, the replacement should be relatively simple. The question to some extent is how the existence of both in one request/similar play nicely. I've had to deal with that with some of the pear/mail pacakges; moving from PECL and debian packages to composer included.. But being pear/pecl, it had weird "require_once" calls, so it was more a case of making those conditional if the library was loaded via composer

reedy avatar May 08 '20 18:05 reedy

As a replacement for the PHP gettext extension/ext-gettext?

yes that's what I was thinking, but maybe im missing something relating to the other changes :)

TrystanLea avatar May 08 '20 18:05 TrystanLea

Using __() , e() and having module domain on the translation file name and change of paths seems a lot of risk that will break compatibility with all modules out of scope of core. This should be rethinked to implement composer if that is the need but not to break anything else.

chaveiro avatar May 09 '20 10:05 chaveiro

This should be rethinked to implement composer if that is the need but not to break anything else.

This is also my preference if possible @gablau let us know how to proceed :)

TrystanLea avatar May 12 '20 18:05 TrystanLea

Here I am, sorry but I was a little busy.

whats the reason for renaming _( to e( and __( ?

I kept the translation functions separate so there is no problem for modules not yet passed to the new translation system.

I made sure that for the first period the two systems can coexist, without breaking anything

In addition, the new functions are simple to use and save code: "_e(" instead of "echo _ ("

do we need to move the language files or can we just keep them in the current locale directories?

You could keep everything in the same folders but in my opinion it is easier to have a single folder with all the translation files inside. Let's simplify the work of translators who don't have all the folders called "LC_MESSAGES" and all the language files with the same name "messages.po". I translated the whole part into Italian, and start from scratch and manage all those files and a nice mess

I did a PR on Reedy's brench, with the things that were missing to make it work.

gablau avatar May 15 '20 08:05 gablau

The only doubt I have is the current module installation system, which is done via git.

On someone he has an old version of emoncms without the new translation system and "update" a module that has the new system. In this case it would not find the new translation functions and it would not work.

gablau avatar May 15 '20 09:05 gablau

In addition, the new functions are simple to use and save code: "_e(" instead of "echo _ ("

PHP isn't JS/CSS where every character matters ;)

reedy avatar May 15 '20 13:05 reedy

two systems can coexist, That is what worries me, no two systems should coexist.

You seem to know what you are doing but such a disruptive change is not welcome on this project due to the nature of distributed work and lack of centralized control of addons sources. Changing files path is not welcome also for the same reason as it will break modules that are not part of core or are 3rd party plugins some not maintained much. You can update to support composer on one commit to core files to simplify review. Later can try to optimize things but not to by breaking compatibility with anything out of the commit scope.

Anyway, if you can ensure compatibility as explained above, provide evidences that the changes are good (eg. are a best practice and emoncms was wrong) and everything tests out ok, i would approve :)

chaveiro avatar May 15 '20 15:05 chaveiro