zerospam / laravel-gettext

Adds localization support to laravel applications using PoEdit and GNU-Gettext.
100 stars 56 forks source link

Make laravel-gettext work with laravel 7 #46

Closed WimDeMeester closed 3 years ago

WimDeMeester commented 4 years ago

Fixes issue #45 . Tested it locally and everything seems to work.

WimDeMeester commented 4 years ago

There was still a problem with the setLocale method in LaravelGettext/Symfony.php. In the newer version of Symfony/translation, it is no longer possible to set the locale to null, which happened sometimes. I fixed this by checking for null and setting the default locale to "en". I don't know if this is the correct way to handle this. I'm not familiar enough with the laravel-gettext code for this.

MarkusJLechner commented 4 years ago

Nice it's already this far! 🌟 If the default lang comes somewhere from the config it's good enough I think. Thanks for contribution!

WimDeMeester commented 4 years ago

As far as I could see, the problem only occurred when I run

phpunit

The tests fail if there is no default locale set. In normal operation, I did not find a problem.

MarkusJLechner commented 4 years ago

I see! Of course tests should work on every given time, however it's good to hear it fails on something not common

philippe-bourdeau commented 4 years ago

There was still a problem with the setLocale method in LaravelGettext/Symfony.php. In the newer version of Symfony/translation, it is no longer possible to set the locale to null, which happened sometimes. I fixed this by checking for null and setting the default locale to "en". I don't know if this is the correct way to handle this. I'm not familiar enough with the laravel-gettext code for this.

@WimDeMeester Hello Wim, thanks for your contribution. I did run the tests in the project on the branch master without any problems. I did upgrade symfony/translation to v4.5.5 and I did not run into any issue. Can you detail a bit more how you ran into this issue ? Setting the locale normally falls back to config.php value when not found.

WimDeMeester commented 4 years ago

I'm talking about the test I run in my own project... Without the changes (checking if the locale is null), I get a lot of errors like these:

Failed asserting that exception of type "TypeError" matches expected exception "Illuminate\Validation\ValidationException". Message was: "Argument 1 passed to Symfony\Component\Translation\Translator::setLocale() must be of the type string, null given, called in /home/vagrant/code2/src/Xinax/LaravelGettext/Translators/Symfony.php on line 77"

This error never occurs when I run my web application, but only when I run

phpunit

Do I need an extra setting for my tests?

philippe-bourdeau commented 4 years ago

@WimDeMeester Did you publish the default configuration to your application ?

@see https://github.com/zerospam/laravel-gettext#2-install

WimDeMeester commented 4 years ago

Hi @philippe-bourdeau ,

I did publish the default configuration for my application... Do I have to set something specific for running my tests?

WimDeMeester commented 4 years ago

Hi @philippe-bourdeau

I did some extra checks, and there was a bug in my tests... I really did set null to the LaravelGettext::setLocale() method... Sorry about that. The second part of the pull request is not needed then.

WimDeMeester commented 4 years ago

I reverted the changes for the setLocale problem I had. The pull request can now be accepted to have laravel 7 support. The only change is now in the composer.json file.

wtfmejt commented 4 years ago

How can I install this package from the DeepskyLog repository?

WimDeMeester commented 4 years ago

To be honest, I don't know... I checked the code out, and added the following to my composer.json file:

    "repositories": [{
        "type": "path",
        "url": "/home/vagrant/laravel-gettext/"
    }],

I also made sure that I had the following in composer.json: "zerospam/laravel-gettext": "@dev" After that, I did a composer update and the correct version of laravel-gettext was installed.

hanhdo205 commented 4 years ago

How can I install this package from the DeepskyLog repository? I've put on packagist, simply using below command to install composer require hanhdo205/laravel-gettext

davidgv88 commented 4 years ago

I need compatibility with Laravel 7!

philippe-bourdeau commented 3 years ago

Thanks @WimDeMeester , I know it's been a while. We will upgrade this package as we are upgrading our server code to laravel 8. Should be available soon. Thanks for your contribution.