Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keep BO from using two different translators in parallel #14966

Merged
merged 6 commits into from Aug 13, 2019

Conversation

@matthieu-rolland
Copy link
Contributor

commented Aug 2, 2019

Questions Answers
Branch? 1.7.6.x
Description? Fix translation issues, read below
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #14932
How to test? Check that translation is still working, especially translations coming from database in modules.

First, you should read this PR from @eternoendless
#14845

His PR renames translation test modules, allows underscores in translation domain name, and adds useful comments to understand the two translators.

From there, this PR adds a few things, the main goal being that wether we are in FO or BO, we always use only one translator, either the translator component or the one from frameworkbundle. Also make it so that we still have the translation part in symfony's debug toolbar, and that the database translations are still working in modules.

  • make frameworkbundle's translator load SQL translations with yaml service configuration
  • fix timing issue in controllerCore (service container was called after the first call to the translator) . (edit: removed due to issue with php 5.6)
  • Fetch the right translator depending on the availability or not of the symfony service container
  • Make it possible to specify which translator you need

I carefully checked that the component translator is never called in back office, and that the framework bundle translator is never called in front office.


This change is Reviewable

@matthieu-rolland matthieu-rolland requested a review from PrestaShop/prestashop-core-developers as a code owner Aug 2, 2019
@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:fix-double-translation branch from 6f7089e to 51ac915 Aug 2, 2019
Copy link
Member

left a comment

some minor comments

@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:fix-double-translation branch 2 times, most recently from 65cdd2c to 8bbf1cf Aug 5, 2019
@prestonBot prestonBot added the Bug label Aug 6, 2019
@marionf marionf removed the fix label Aug 6, 2019
@matthieu-rolland

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

I removed the part where I changed the place where the container was called for the first time, in
the classes/controller/Controller.php

The reason is that it creates a bug, only on PHP 5.6 when opcache is disabled.

Capture d’écran 2019-08-06 à 15 15 20

I know, it's weird, it looks a lot like this bug:

https://bugs.php.net/bug.php?id=66773

opcache allows importing a class from another namespace with a name that is already in use in the current namespace

Why only in php 5.6? I dont know...

@eternoendless

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

[2017-09-05 18:19 UTC] nikic@php.net
This issue has been fixed in PHP 7 only.

It seems that they fixed it only in PHP 7 😅

@eternoendless eternoendless added this to the 1.7.6.1 milestone Aug 7, 2019
Copy link
Member

left a comment

LGTM, QA label can be added once tests are green

@khouloudbelguith

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

Hi @matthieu-rolland,

I tried with different modules for example productcomments & ps_mainmenu.
The shop installed with an English language/France country & The employee language => French.
I would like to translate some expression's module from the back office in French => not translated.
In the FO => ok.
https://drive.google.com/file/d/16haO3ZAwdtaG0iP1PhzyRyiHG41s21Hr/view
When the employee's language is English => Translations => OK.
Thanks!

@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:fix-double-translation branch 2 times, most recently from 13a85bd to f7a8655 Aug 8, 2019
@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:fix-double-translation branch from f7a8655 to 2747e6d Aug 10, 2019
@@ -364,16 +365,23 @@ public function updateCustomer(Customer $customer)
/**
* @return Translator
*/
public function getTranslator()
public function getTranslator($needComponentTranslator = false)

This comment has been minimized.

Copy link
@matthieu-rolland

matthieu-rolland Aug 12, 2019

Author Contributor

I made it possible to tell via a parameter which translator you need, because during installation it would get the framework bundle translator, which is configured to init the db sql loader, but the database is not ready yet, so it fails...

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 12, 2019

Member

I don't think this is a good idea, because

  1. TranslatorComponent is meant to go away eventually
  2. It exposes internal functionality
  3. It's impossible tu understand if you don't know the underlying complexities 🗡

If this is a special case just for the installer, let's just own it up:

Suggested change
public function getTranslator($needComponentTranslator = false)
public function getTranslator($isInstaller = false)

or alternatively:

Suggested change
public function getTranslator($needComponentTranslator = false)
public function getTranslator($withDatabase = true)

In any case, this parameter must be declared and explained in Phpdoc

This comment has been minimized.

Copy link
@matthieu-rolland

matthieu-rolland Aug 12, 2019

Author Contributor

done !

@@ -364,16 +365,23 @@ public function updateCustomer(Customer $customer)
/**
* @return Translator
*/
public function getTranslator()
public function getTranslator($needComponentTranslator = false)

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 12, 2019

Member

I don't think this is a good idea, because

  1. TranslatorComponent is meant to go away eventually
  2. It exposes internal functionality
  3. It's impossible tu understand if you don't know the underlying complexities 🗡

If this is a special case just for the installer, let's just own it up:

Suggested change
public function getTranslator($needComponentTranslator = false)
public function getTranslator($isInstaller = false)

or alternatively:

Suggested change
public function getTranslator($needComponentTranslator = false)
public function getTranslator($withDatabase = true)

In any case, this parameter must be declared and explained in Phpdoc

classes/Context.php Show resolved Hide resolved
- change getTranslator's parameter name
- add comments
- improve phpdoc
@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:fix-double-translation branch from 2d032b3 to 2a6bfb3 Aug 12, 2019
Copy link
Member

left a comment

Looks good now, thanks!

@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

@matks matks merged commit 38e171b into PrestaShop:1.7.6.x Aug 13, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.