Skip to content
This repository has been archived by the owner on Aug 18, 2018. It is now read-only.

Bypass the native php extention #52

Closed
shaggyz opened this issue Jan 9, 2016 · 44 comments
Closed

Bypass the native php extention #52

shaggyz opened this issue Jan 9, 2016 · 44 comments

Comments

@shaggyz
Copy link
Owner

shaggyz commented Jan 9, 2016

As @Gummibeer comments on #45 (comment)

  • Check Symfony Translation Package
  • Integrate the singletton
  • Test with different cache types
  • Update tests
@simplenotezy
Copy link

Can't wait for this!

@morfin
Copy link

morfin commented Mar 9, 2016

News?

@Gummibeer
Copy link

Can I help anything more here? should I add any features in the Class, like domains or something? I think that this will fix a lot of bug reports related to native gettext.

@shaggyz
Copy link
Owner Author

shaggyz commented Mar 10, 2016

Thanks @Gummibeer I'm on it. Between today and tomorrow, 3.1.2 will be released. It's a critical change so I have to ensure backward compatibility. I'll keep you updated in this thread.

@simplenotezy
Copy link

Nice @shaggyz ! Good job.

@Gummibeer
Copy link

@shaggyz I know - that's why I'm asking if you need help with something - or some demo projects where it can be tested in a real environment. And just saying: cause of the range of this change I would say that this is a mayor update so something like 4.0.0, this willa lso ensure that everyone that uses the native gettext, wnat's to use it in future doesn't have to change anything and everyone that want's the new Symfony translator can use the 4.* in composer.

@shaggyz
Copy link
Owner Author

shaggyz commented Mar 10, 2016

@Gummibeer You're right, this must be a major release. I'm currently working with docker and some webserver+php combinations on demo-projects, but help with testing would be great, I'll publish the pre-release (when ready) here before change the package documentation. Thank you!

@morfin
Copy link

morfin commented Mar 10, 2016

Wonderful.

@shaggyz
Copy link
Owner Author

shaggyz commented Mar 11, 2016

Well, it's working! 💃 but there are some points to resolve, help here would be great:

This branch can be tested with a current project (remember to disable the php-gettext extension), in composer:

"xinax/laravel-gettext": "4.x-dev"

Or pulling from 4.x branch.
Any sugestion or help will be very very welcome!

@morfin
Copy link

morfin commented Mar 11, 2016

I suggest using custom functions that way we don't have to disable gettext_module.

t() sounds good.

Good Work!

@morfin
Copy link

morfin commented Mar 11, 2016

Notice that on certain web environments, removing gettext is not easy, take for example Homestead,
There's no way to "comment out" the module.

So it seems that using a custom function for translation is the way to go.

These are the stand-alone php modules available in Homestead:

gd.so    json.so       mysqli.so   pdo_mysql.so  pdo_sqlite.so  readline.so  xdebug.so
curl.so  imap.so  memcached.so  opcache.so  pdo_pgsql.so  pgsql.so       sqlite3.so

As you can see, gettext seems to be included as a built-in module.

@shaggyz
Copy link
Owner Author

shaggyz commented Mar 12, 2016

@morfin Totally agreed. I was thinking that an user that doesn't have admin rights in deploy servers can't use this package. Let see what community think about this, but I guess that's the right way. Thanks!

@simplenotezy
Copy link

I think it is more correct to disable gettext if you are not using it.

I guess there must be a way in homestead to disable such stuff @ every provision.

Also, IMO people that don't have server rights should not push packages like this.

Or maybe a combination of both t() and the mapping could be an option?

Because if you change the methods, software scanning for these gettext variables need to change as well (i.e. Poedit)?

@KasperFranz
Copy link
Contributor

In my company we have a server but we have other application there are using the gettext, so disabling it shouldn't be necessary but an option.

The mapping was changed in latest version so it's possible to specify other functions to translate

@morfin
Copy link

morfin commented Mar 13, 2016

@canfiax You can define custom functions in POEDIT. Notice that some frameworks like Zend have been using custom functions to avoid conflicts with gettext. I've been using poedit with custom functions for years, I've never had a problem.

Using a custom function means getting rid of workarounds for once and for all.

@shaggyz
Copy link
Owner Author

shaggyz commented Mar 14, 2016

@canfiax I have many servers where gettext applications share webserver and php configuration.

I think custom functions is the only way, as other localization packages did, but we still having some issues to resolve with this option:

  • Custom PoEdit keywords: we need to map this custom keywords dinamically, otherwise translations with custom keywords will not work.
  • Pluralization, this is the gettext plural function signature:
string ngettext ( string $msgid1 , string $msgid2 , int $n )

and this, the symfony signature:

$translator->transChoice(
    'There is one apple|There are %count% apples',
    10,
    array('%count%' => 10)
);

I can't find an easy way to adapt this, the last symfony array parameter is the problem.

As @morfin says with this implementation a lot of issues will disappear.

@Gummibeer
Copy link

@shaggyz the most common custom functions for gettext/translation are simple prefixed by _

<?php
__("world"); # http://php.net/manual/de/ref.gettext.php
_n("%d world", "%d worlds", 1); # http://php.net/manual/de/function.ngettext.php

and the third parameter is optional and just for inserting text into the translation - and it uses the php strstr() function to do this http://php.net/manual/de/function.strtr.php

The only thing the native ngettext() does is inserting the number into the string with the digit-replacement code required %d - a simple solution would be:

<?php
public function _n($singular, $plural, $amount) {
    return $translator->transChoice(
        $singular . '|' . $plural,
        $amount,
        ['%d' => $amount]
    );
}

If you want to allow other parameters there should be a fourth parameter $replacements and the developer has to fill this by it's own, but I think that this isn't any thing that's translation related so I won't support this.

@shaggyz
Copy link
Owner Author

shaggyz commented Mar 21, 2016

@Gummibeer Ok! With this we fix the "already defined function" error from native gettext php module and plural feature. But custom keywords will not work, since the only mapped locale functions are _n and __
If we are ok without "Custom poedit keywords" feature, this can be released.

@Gummibeer
Copy link

What should be possible inside these custom functions? one solution would be to have a helper-function that allows to call anything on the Symfony translator itself, so something like the laravel Log, DB classes have - Trans::getTranslator() or getSymfonyTranslatorInstance() for example. With this it's possible for every dev to create his own translation functions. And with the other idea with the config for custom keywords #45 it's possible for the using dev to let poedit know what functions are translations.

@simplenotezy
Copy link

How to disable php gettext? I get:

[Exception] LaravelGettext: You must disable/uninstal l 'php-gettext' in order to use the Symfony handler

@Gummibeer
Copy link

@canfiax http://installion.co.uk/ubuntu/precise/main/p/php-gettext/uninstall/index.html
or just disable it in your php.ini

@simplenotezy
Copy link

@Gummibeer I have searched for a long time now. I have checked my 7.0 fpm/cli ini and gettext is not present.

@simplenotezy
Copy link

Your link does not work either @Gummibeer :/

@Gummibeer
Copy link

@canfiax don't know if something is changed in php7 but there should be an entry: http://php.net/manual/de/gettext.installation.php

But also a question for @shaggyz where is the problem if gettext is enabled? The Symfony Translation package itself doesn't have any conflicts with gettext (don't found any).
So they are not conflicting just replacing. I don't think that it's nice to force the user to disable gettext!?

@KasperFranz
Copy link
Contributor

I won't use this package, if we have to remove gettext, as some of the places our code is distributed we dont have 100% control over the php.ini

@Gummibeer
Copy link

In our app it's also not needed and everything work's fine - gettext is enabled and we use the symfony translator.
That's why I'm wondering why it's forced to disable gettext!?

@shaggyz
Copy link
Owner Author

shaggyz commented Mar 24, 2016

Hi all, 4.x is not yet relased you can use it but only for testing purposes only. We are (currently) discussing about this release. And most important, the reason for that preview release is incompatible with gettext is dicussed in this same thread. Please read it.
The goal is keep the package compatible with gettext and use symphony translator package instead of native gettext module (cache issues, remember?).
I need some time to implement @Gummibeer solutions for function name collapse (between Symphony Translator and php-gettext module)
You should still using 3.x a little more.

@simplenotezy
Copy link

Can you explain how to disable gettext on a homestead server @shaggyz ?

@morfin
Copy link

morfin commented Apr 19, 2016

Hi,

@shaggyz Any news ?

@canfiax Did you figure out how disable gettext on homestead ?

@abdullah-almesbahi
Copy link

Any update?

@shaggyz
Copy link
Owner Author

shaggyz commented May 9, 2016

Working symfony translator and/or php-gettext native extension. Tested on nginx, fpm and apache. There is a new option on config to set the translation handler (new default is symfony):

    /**
     * Translation handlers, options are:
     *
     * - symfony: (recommended) uses the symfony translations component. 
     * - gettext: requires the php-gettext module installed. This handler has well-known cache issues
     */
    'handler' => 'symfony',

Can be tested in 4.x branch:

"xinax/laravel-gettext": "4.x-dev"

Not released yet since I want to update the docs (the new package default behaviour is more simple)

@morfin
Copy link

morfin commented May 9, 2016

@shaggyz Great job.

@Gummibeer
Copy link

Gummibeer commented May 10, 2016

Just a note/hint: we discovered some problems with the symfony translator and the .po way of pluralization.
The problem is that symfony automaticly concats the strings to a pipe seperated string but just on one key. In our cases it was everytime the plural key.

Example:

msgid "Einstellung"
msgid_plural "Einstellungen"
msgstr[0] "Setting"
msgstr[1] "Settings"

# will be

[
    // ...
    'Einstellungen' => 'Setting|Settings',
    // ...
],

But it also wasn't everytime - we haven't discovered any rule or something that it follows - most times we discovered this problem was whenone of the singular r plural strings was used with the normal translation function so it occurs twice in the po-file.

I don't know if this problem will be in this package but symfony is a bit strange there.

@shaggyz
Copy link
Owner Author

shaggyz commented May 10, 2016

@Gummibeer yes, like you describe symfony translator (more accurately in the PoFileLoader) fails on pluralization reads. I wasted all the afternoon to realize that (thinking that was a bug on laravel-gettext).

I've added a getTranslator and setTranslator methods to LaravelGettext, useful to customize the pluralization function. No solution yet, if you see the wrapper code:

    /**
     * Translates a plural string
     *
     * @param $singular
     * @param $plural
     * @param $amount
     */
    public function translatePlural($singular, $plural, $amount)
    {
        return $this->symfonyTranslator->transChoice(
            // Symfony translator looks for 'singular|plural' message id in catalog,
            // and obviously doesn't exists, so always the fallback string will be returned.
            // $singular . '|' . $plural, //<-- this just doesn't works, idk wtf is wrong.
            $amount >1 ? $plural : $singular,
            $amount,
            ['%count%' => $amount],
            $this->getDomain(),
            $this->getLocale()
        );
    }

This is not a solution, and sucks, maybe the right way is write a custom loader that extends the original symfony PoFileLoader. There is an issue about this, but seems stalled:

symfony/symfony#10152

Any suggestion?

@Gummibeer
Copy link

Gummibeer commented May 10, 2016

There are multiple problems with the Symfony po pluralization.

1.) It concats all the strings to one translated pipe seperated
2.) It doesn't translate in the transChoice just choosing - the pipe seperated string have to be translated (why @symfony?)
3.) We haven't get any rule when it does what - best rule we have was the thing with using __n() and __() with the same key
4.) It isn't documented with the PoFileLoader

So yes, the best thing could be to override the default symfony PoFileLoader to a custom one that just holds key values and the translatePlural() could look like something like this:

    public function translatePlural($singular, $plural, $amount)
    {
        $transChoice = '{1}%s|]1,Inf]%s';
        $transChoice = sprintf($transChoice, __($singular), __($plural));
        return $this->symfonyTranslator->transChoice(
            $transChoice,
            $amount,
            ['%count%' => $amount],
            $this->getDomain(),
            $this->getLocale()
        );
    }

The __() is the simple translation function (replace it with the proper method) and with the custom PoFileLoader it will just return the right translation and no pipe seperated strings.

@simplenotezy
Copy link

simplenotezy commented May 10, 2016

So is it safe to implement on production? Currently we translate strings using

_('String')

@shaggyz
Copy link
Owner Author

shaggyz commented May 10, 2016

@canfiax no, it is not.

@Gummibeer
Copy link

@canfiax: the simple translation without pluralization should work - not for production but would be great if you can Test it in a fb-branch and give Feedback.

@Gummibeer
Copy link

@shaggyz How is it going here?

@simplenotezy
Copy link

I am also really excited for this update. How is it going @Gummibeer ?

@shaggyz
Copy link
Owner Author

shaggyz commented Sep 30, 2016

I been very busy those months, sorry guys. My plan is to release 4.0 next month. I want to test every single change in deep and this is translated in more time.
Don't be scare about stalled PR's, all of them will be included in this release.
Cheers.

@simplenotezy
Copy link

@shaggyz Can't wait for this! 👍

Also, I think optimization label is quite understated. I believe a lot of the open issues is due to the native gettext extension.

@shaggyz shaggyz added the bug label Oct 17, 2016
@shaggyz
Copy link
Owner Author

shaggyz commented Oct 17, 2016

@shaggyz shaggyz closed this as completed Oct 17, 2016
@kkomelin
Copy link

Thanks everyone who contributed to the 4th release! Great job

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants