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

Fix email not translated when installing a new language #16294

Open
wants to merge 2 commits into
base: 1.7.6.x
from

Conversation

@atomiix
Copy link
Contributor

atomiix commented Nov 6, 2019

Questions Answers
Branch? 1.7.6.x
Description? This fixes emails not being translated when installing a new language
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #16273
How to test? See #16273 ⚠ As it modifies quite a lot the language management, we should also test if mails are correctly generated during a fresh install (using something else than english) from web, and with CLI And check that the manual generation from the form also still works ⚠

This change is Reviewable

@atomiix atomiix requested a review from PrestaShop/prestashop-core-developers as a code owner Nov 6, 2019
@eternoendless eternoendless changed the title fix email no translated when installing a new language Fix email no translated when installing a new language Nov 6, 2019
@eternoendless eternoendless changed the title Fix email no translated when installing a new language Fix emails not being translated when installing a new language Nov 6, 2019
@atomiix atomiix changed the title Fix emails not being translated when installing a new language Fix email not translated when installing a new language Nov 6, 2019
*
* @param string $locale
*/
private function cleanTranslatorLocaleCache($locale)
private function addLanguageToTranslator(string $locale)

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Nov 6, 2019

Contributor

5.6compatible, you can't do that :)

Suggested change
private function addLanguageToTranslator(string $locale)
private function addLanguageToTranslator($locale)
@atomiix atomiix force-pushed the atomiix:fix-email-generation-translation branch from 767e239 to 7b52199 Nov 6, 2019
{
if (!method_exists($this->translator, 'addLoader')

This comment has been minimized.

Copy link
@jolelievre

jolelievre Nov 6, 2019

Contributor

Be careful, I remember this check was here for a reason because sometimes the injected translator was not PrestaShopBundle\Translation\Translator but PrestaShopBundle\Translation\DataCollectorTranslator which doesn't have the method addRessource nor addLoader
Although since there have been modifications on the translator (we had 2 different components) by @matthieu-rolland maybe this fixed the issue

This comment has been minimized.

Copy link
@eternoendless

eternoendless Nov 6, 2019

Member

I think it's fixed but it should be tested.

FYI you get DataCollectorTranslator when you're in dev mode (it tracks translation hits for the Symfony profiler)

This comment has been minimized.

Copy link
@atomiix

atomiix Nov 7, 2019

Author Contributor

Yeah, actually if I check if those methods exists it will return false because I got a DataCollectorTranslator but as DataCollectorTranslator is wrapping the Translator class, those methods are still available via the magic function __call that call the translator functions and make it works.

@atomiix atomiix force-pushed the atomiix:fix-email-generation-translation branch from 7b52199 to 44cd465 Nov 7, 2019
* @param string $locale
* @param string $notName
*/
public function addLanguageToTranslator($locale, $notName = null)

This comment has been minimized.

Copy link
@jolelievre

jolelievre Nov 7, 2019

Contributor

It seems $notName is not really useful here (you don't use it in the Language class)

@atomiix atomiix force-pushed the atomiix:fix-email-generation-translation branch 2 times, most recently from 4506c56 to 2a7e69a Nov 8, 2019
@atomiix atomiix requested a review from eternoendless Nov 8, 2019
@atomiix atomiix added this to the 1.7.6.2 milestone Nov 8, 2019
/**
* @param string $locale
*/
public function addLanguageToTranslator($locale)

This comment has been minimized.

Copy link
@eternoendless

eternoendless Nov 14, 2019

Member

You're adding responsibilities to Context that aren't theirs.

I think that you should extract a class TranslationLanguageLoader that would basically take over the responsibility for loading a language into an already instantiated translator.

It would work like this:

$isAdminContext = defined('_PS_ADMIN_DIR_');
$translator = Context::getContext()->getTranslator();
(new TranslatorLanguageLoader($isAdminContext))->loadLanguage($translator, $localeCode); // this returns void

This way, I don't think that you'd need to change the getTranslator() signature anymore.

@atomiix atomiix force-pushed the atomiix:fix-email-generation-translation branch from 2a7e69a to ea1359a Nov 18, 2019
@atomiix atomiix force-pushed the atomiix:fix-email-generation-translation branch from ea1359a to f121cdd Nov 18, 2019
_PS_CACHE_DIR_ . '/translations/' . $this->locale,
false
);
if ($translatorLocale !== $this->locale) {

This comment has been minimized.

Copy link
@jolelievre

jolelievre Nov 18, 2019

Contributor

Waow! So fixing "only" this allows to manage the new language correctly?!

This comment has been minimized.

Copy link
@atomiix

atomiix Nov 18, 2019

Author Contributor

Yes... Took time to find it though 😅
I just nedd to follow @eternoendless's recommandation by adding a new TranslatorLanguageLoader and we should be good with that PR

classes/lang/DataLang.php Outdated Show resolved Hide resolved
classes/Context.php Outdated Show resolved Hide resolved
$sqlTranslationLoader = new SqlTranslationLoader();
if (null !== $theme) {
$sqlTranslationLoader->setTheme($theme);

This comment has been minimized.

Copy link
@jolelievre

jolelievre Nov 18, 2019

Contributor

I'm wondering since the loader depends on the theme, shouldn't the loader be also depends on it Something like:

$translator->addLoader('db-'.$theme, $sqlTranslationLoader);

Or are we sure that the theme stays unmodified during the whole process (for each calls)

This comment has been minimized.

Copy link
@eternoendless

eternoendless Nov 19, 2019

Member

I don't think the theme is ever modified during runtime

This comment has been minimized.

Copy link
@jolelievre

jolelievre Nov 20, 2019

Contributor

true!

@atomiix atomiix force-pushed the atomiix:fix-email-generation-translation branch from b6f2c1b to e5fb0ee Nov 19, 2019
interface PrestaShopTranslatorInterface extends TranslatorInterface
{
/**
* @param $locale

This comment has been minimized.

Copy link
@eternoendless

eternoendless Nov 19, 2019

Member
Suggested change
* @param $locale
* @param string $locale
@@ -130,6 +130,16 @@ public function transChoice($id, $number, array $parameters = array(), $domain =
return vsprintf(parent::transChoice($id, $number, array(), $domain, $locale), $parameters);
}
/**
* @param $locale

This comment has been minimized.

Copy link
@eternoendless

eternoendless Nov 19, 2019

Member
Suggested change
* @param $locale
* @param string $locale
*/
public function isCatalogLoaded($locale)
{
return isset($this->catalogues[$locale]);

This comment has been minimized.

Copy link
@eternoendless

eternoendless Nov 19, 2019

Member

Shouldn't it be better to check for emptyness?

Suggested change
return isset($this->catalogues[$locale]);
return !empty($this->catalogues[$locale]);
Copy link
Member

eternoendless left a comment

Just a couple comments!

@atomiix atomiix force-pushed the atomiix:fix-email-generation-translation branch from 5159e45 to a8b9006 Nov 20, 2019
->notName($notName)
->in($this->getTranslationResourcesDirectories());
// only way to reset the catalogue
$translator->addLoader('Array', new ArrayLoader());

This comment has been minimized.

Copy link
@jolelievre

jolelievre Nov 20, 2019

Contributor

Maybe you should put this in the TranslatorLanguageLoader::loadLanguage method?

/**
* @param PrestaShopTranslatorInterface $translator
* @param string $locale
* @param Theme|null $theme

This comment has been minimized.

Copy link
@jolelievre

jolelievre Nov 20, 2019

Contributor

The PHPDoc is not in the right order (or the function's parameters)

}
}
$adminContext = defined('_PS_ADMIN_DIR_');
$withDB = !$this->language instanceof PrestashopBundle\Install\Language;

This comment has been minimized.

Copy link
@jolelievre

jolelievre Nov 20, 2019

Contributor

I don't understand this check? In which case don't you want the database to be loaded? Maybe it's worth a comment here

Besides what other type can $this->language be? \Language? So it means you only disable the db loading during install process? Why?

This comment has been minimized.

Copy link
@atomiix

atomiix Nov 21, 2019

Author Contributor

We don't need to load DB translations when $this->language in an instance of PrestashopBundle\Install\Language because that's only translations for the web installer and when we load the installer language we didn't configured the DB yet...

This comment has been minimized.

Copy link
@jolelievre

jolelievre Nov 21, 2019

Contributor

oooookay, this makes sense but maybe you can add a comment explaining that, because it's not that intuitive at first ^^

->in($this->getTranslationResourcesDirectories());
// only way to reset the catalogue
$translator->addLoader('Array', new ArrayLoader());
$translator->addResource('Array', [], $locale);

This comment has been minimized.

Copy link
@eternoendless

eternoendless Nov 20, 2019

Member

This could be better managed inside the translator itself:

public function resetCatalogue($locale)
{
    unset($this->catalogues[$locale]);
}

Just add this in the Translator trait and in the PrestaShopTranslatorInterface

* @param PrestaShopTranslatorInterface $translator
* @param string $locale
* @param Theme|null $theme
* @param bool $withDB

This comment has been minimized.

Copy link
@eternoendless

eternoendless Nov 20, 2019

Member

Arguments are not in the right order

This comment has been minimized.

Copy link
@eternoendless

eternoendless Nov 20, 2019

Member

Consider writing descriptions for public functions and arguments.

For example

/**
 * Loads a language into a translator
 *
 * @param PrestaShopTranslatorInterface $translator Translator to modifiy
 * @param string $locale Locale code for the language to load
 * @param bool $withDB [default=true] Whether to load translations from the database or not
 * @param Theme|null $theme [default=false] Currently active theme (Front office only)
 */
@atomiix atomiix force-pushed the atomiix:fix-email-generation-translation branch from 7e19da9 to a95aef4 Nov 21, 2019
public function isCatalogLoaded($locale);
/**
* @param $locale

This comment has been minimized.

Copy link
@eternoendless

eternoendless Nov 21, 2019

Member
Suggested change
* @param $locale
* @param string $locale
}
/**
* @param $locale

This comment has been minimized.

Copy link
@eternoendless

eternoendless Nov 21, 2019

Member
Suggested change
* @param $locale
* @param string $locale
@atomiix atomiix force-pushed the atomiix:fix-email-generation-translation branch from a95aef4 to 5a401a0 Nov 21, 2019
@atomiix atomiix force-pushed the atomiix:fix-email-generation-translation branch from 5a401a0 to ff34693 Nov 21, 2019
@matks
matks approved these changes Nov 21, 2019
@matks
matks approved these changes Nov 21, 2019
@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Nov 21, 2019

Hi,

I've a new bug :

When I import a localization pack of a country with several official languages (for example, Belgium), only one language has the generated emails in the correct language. The other languages are in English.

After manual regeneration (in Design > Email), the emails are still in English.

Some details for my Belgium example :

If only English is installed, French will be OK, German and Dutch will be NOK.
If English and French are already installed (and OK), German will also be OK, only Dutch will be NOK.

In 1752, it's OK.

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 21, 2019

Hi,

I've a new bug :

When I import a localization pack of a country with several official languages (for example, Belgium), only one language has the generated emails in the correct language. The other languages are in English.

After manual regeneration (in Design > Email), the emails are still in English.

Some details for my Belgium example :

If only English is installed, French will be OK, German and Dutch will be NOK.
If English and French are already installed (and OK), German will also be OK, only Dutch will be NOK.

In 1752, it's OK.

So, if I understand correctly, we have 2 reproductable failing scenarios (at least):

Scenario 1

  1. I install a shop in English. I download a localization pack for Belgium. This pack contains French, Dutch, and German.
  2. I generate the mails for languages French, Dutch, German.
  3. French, Dutch, German mails mails are generated but are not translated. They are in English.

Scenario 2

  1. I install a shop in French. This means 2 languages are available on my shop: French and English. I make sure both are activated.
  2. I download a localization pack for Belgium. This pack contains French, Dutch, and German.
  3. I generate the mails for languages French, Dutch, German.
  4. French, and German mails are generated and translated. But Dutch mails are in English.

Is that correct @Robin-Fischer-PS ?

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Nov 22, 2019

Not exactly @matks

Scenario 2 is OK. For Scenario 1, step 3 is :
French mails are generated and translated. Dutch, German mails mails are generated too but are not translated. They are in English.

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Nov 22, 2019

Hi,

New bug. When I do a fresh install with a country with 3 or more official languages (Belgium, Switzerland), it is KO.

pr16294 fail install

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.