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

restore missing translation keys #9183

Merged
merged 5 commits into from Jun 13, 2018

Conversation

Projects
None yet
6 participants
@tomlev
Member

tomlev commented Jun 12, 2018

Questions Answers
Branch? 1.7.4.x
Description? restore missing translation keys
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test?

Important guidelines


This change is Reviewable

@@ -0,0 +1,2 @@
This directory aims to get all translation keys that need to be referenced, but not cearly visible in code.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 12, 2018

Contributor

cearly? what does it means?

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 12, 2018

Contributor

That means translation tool need to fetch trans() method to retrieve and store translations (what a mess 😄 ): https://github.com/PrestaShop/TranslationToolsBundle/blob/master/Translation/Extractor/Visitor/TranslationNodeVisitor.php

use PrestaShopBundle\Translation\TranslatorComponent as Translator;
class AttributeGroupLang

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 12, 2018

Contributor

Would you mind to add PHPDoc for classes?

This comment has been minimized.

@tomlev

tomlev Jun 12, 2018

Member

there was none before being deleted, I don't really know each class meaning, here we just need translation to be kept back

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 12, 2018

I have multiples concerns with this pull request:

  • why PHP files and not xliff files directly? This looks so dirty :/
  • I don't understand how translator is found in every class, I have the feeling that theses classes shouldnt be instanciated at all, am I right?
  • finally, I don't think these files should be placed in the bundle. Maybe in Core or better: in a new package called "missing-translations" or why not directly on TranslationsToolsBundle?
@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 12, 2018

Other questions:

  • do these files/translation keys are going to change over time or it is something we need to keep and never change again?
  • is it possible to get these translations in a more common format? I know we can parse Xliff files really easy, with a minor update of translation tools.
  • why not replace the translations where they were in the past? #8696

@mickaelandrieu mickaelandrieu added the wip label Jun 12, 2018

@tomlev

This comment has been minimized.

Member

tomlev commented Jun 12, 2018

1/ translation extraction should not delete some old keys : they can be used by modules.
this change will be addressed later
2/ these translation keys cannot be replaced in their original location, because it was not clean to mix business behavior and translation-specific storage (that's the reason I deleted these keyx without doubt ^^)
3/ about the format, the goal of this PR was simply to fix the missing keys, but we can add it the change of the translation tool if needed

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 12, 2018

Thanks for the answers, I'm in favor of puting these files in TranslationTools app then, in a specific folder and then to add this folder to the paths the command used to extract translations. In a second time, to define how we can introduce again these keys in the core: wdyt?

@tomlev

This comment has been minimized.

Member

tomlev commented Jun 13, 2018

I'm not sure putting these classes out of the core is a good idea. This will make things hard to maintain...
I prefer attaching them in a specific folder in the core

@@ -0,0 +1,2 @@
This directory aims to get all translation keys that need to be referenced, but not clearly visible in code.
The translation parser will be grateful to find such readable keys, and will not delete them from the translation system

This comment has been minimized.

@eternoendless

eternoendless Jun 13, 2018

Member

The translation parser will be grateful to find such readable keys

😂

$this->translator->trans('Weight', array(), 'Shop.Demo.Catalog', $this->locale);
$this->translator->trans('Compositions', array(), 'Shop.Demo.Catalog', $this->locale);
$this->translator->trans('Styles', array(), 'Shop.Demo.Catalog', $this->locale);
$this->translator->trans('Properties', array(), 'Shop.Demo.Catalog', $this->locale);

This comment has been minimized.

@eternoendless

eternoendless Jun 13, 2018

Member

I don't think this class is needed anymore, demo data is supposedly translated on the fixture files, and there's no more untranslation/retranslation, isn't it?

This comment has been minimized.

@tomlev

tomlev Jun 13, 2018

Member

I don't know how demo data is handled, and there is still untranslation/retranslation ;)

$this->translator->trans('120g', array(), 'Shop.Demo.Catalog', $this->locale);
$this->translator->trans('0.31 in', array(), 'Shop.Demo.Catalog', $this->locale);

This comment has been minimized.

@eternoendless

eternoendless Jun 13, 2018

Member

Same goes for this class

protected function init()
{
$this->translator->trans('Mr.', array(), 'Admin.Shopparameters.Feature', $this->locale);
$this->translator->trans('Mrs.', array(), 'Admin.Shopparameters.Feature', $this->locale);

This comment has been minimized.

@eternoendless

eternoendless Jun 13, 2018

Member

And this class

@eternoendless

This comment has been minimized.

Member

eternoendless commented Jun 13, 2018

I'm not sure how TranslationTool discovers the wordings, i.e I don't know if it is enough for them just to be there or what.

@PierreRambaud, @Quetzacoalt91 what do you guys think?

In any case, I'd prefer for them to be in a configuration folder somewhere, we do have a lot to choose from.

protected function init()
{
$this->translator->trans('Home', array(), 'Admin.Catalog.Feature', $this->locale);

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 13, 2018

Contributor

Useless class duplicate from CategoryLang

@eternoendless

This comment has been minimized.

Member

eternoendless commented Jun 13, 2018

According to some tests run by @PierreRambaud, we can put the files pretty much anywhere as long as it's within one of the following directories:

  • classes
  • controllers
  • admin-dev
  • modules
  • src
  • override
  • install-dev
  • themes
  • pdf

Additionally, the file doesn't even need to be a class, this exact code snippet would work as well:

<?php

trans('test', 'Test.test');
@PierreRambaud

This comment has been minimized.

Contributor

PierreRambaud commented Jun 13, 2018

Directories can be configurable if we update the translationtool project :)

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 13, 2018

@PierreRambaud good to know :)

@mickaelandrieu mickaelandrieu removed the wip label Jun 13, 2018

@mickaelandrieu mickaelandrieu added this to the 1.7.4.0 milestone Jun 13, 2018

your comment have been adressed

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 13, 2018

Thanks @tomlev !

@mickaelandrieu mickaelandrieu merged commit 6307eeb into PrestaShop:1.7.4.x Jun 13, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
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