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 error preventing to translate backoffice wordings when using a theme other than classic #14962

Merged

Conversation

@eternoendless
Copy link
Member

commented Aug 1, 2019

Questions Answers
Branch? 1.7.6.x
Description? This PR fixes an issue making it impossible to translate BO wordings if the current theme is anything other than "classic". Read below for more.
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #14733
How to test? See issue + test translating at least a wording in BO, FO, classic theme and custom theme.

More information about this bug

Before this fix, back office translations would be pulled from the current FO theme as well, and any saved translation would also be attached to that theme, even if the wording didn't belong to the FO. This issue would render the translation of backoffice wordings impossible when using a theme other than "classic" (which has a custom behavior compared to any other theme since its wordings are included in the core).

The problem is related to how the theme translation section works when dealing with themes: it analyzes the theme's code looking for wordings, extracts them into a xliff catalogue in cache, then reads it to build the form. This extraction ONLY happens when working with theme translations. So what happened here was that when requested to load the backoffice translations, it would try to load the theme translations for some reason, and since its xliff catalogue hadn't been extracted, it raised an exception.

This PR reverts changes in #13088 that introduced the forced theme.


This change is Reviewable

@eternoendless eternoendless added this to the 1.7.6.1 milestone Aug 1, 2019
@eternoendless eternoendless requested a review from PrestaShop/prestashop-core-developers as a code owner Aug 1, 2019
@kpodemski

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Hello @eternoendless

i've run some tests, just to clarify current state of translation system in 1.7

  1. first scenario, your version from branch from this PR, edited controllers/admin/AdminAddressesController.php, changed "First Name" to "Customer first name"

both, with changed domain to Admin.Custom and Admin.Global, new wording never found in translation system.

i've moved this file to override folder, again, two domains. Result? new wording never found in translation system. This worked well in 1.6.

  1. second scenario, wording from front-office, we want to change some strings related to products sorting wich are present in core files, for example controllers/front/listing/SearchController.php, this line:
    $this->getTranslator()->trans('Search results', array(), 'Shop.Theme.Catalog')

i'm going to Translations -> Theme -> My theme -> my language -> search "Search results", and what? no results, let's try with Classic theme!

i'm going to Translations -> Theme -> Classic -> my language -> search for "Search results" and... voila! it's working
image

In my opinion this is very important issue, we can't fully translate our custom theme.

But hey, let's try to insert translation directly to ps_translation table, this worked well in 1.7.5!
Result? Yes, it's working fine, still not a best solution for non-tech person using PrestaShop.

Maybe this is similar issue to what you've worked in this PR? I think that Translator should be aware of every new strings in core files and template files.

Another important issue which i have with new Translation system, is that we can't translate modules in theme context, so we can't have two themes, one module, and two translations for it, because module translations are stored in /modules/modulename/translation/iso.php instead of what we have in 1.6: /themes/modules/modulename/translation/iso.php, and i'm not sure if we have any workaround for this issue?

@eternoendless

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

Hi @kpodemski

I think the best place to discuss general issues with translations would be in this dedicated epic, or better, within the issues tracked there. Most of the issues you are talking about are there.

Regarding the first scenario, that's because backoffice translations are extracted from the core at build time (before we release a minor version), then put in XLIFF files which are bundled with the release or downloaded as language packs. The BO translation interface is based solely on these files when it comes to back office or front office translations, currently there's no way to make it pull new strings from the code. This is tracked by this issue: #13870

The second scenario happens in part because the theme translation system is different for classic and for other themes. The classic theme pulls its wordings directly form the XLIFF catalogue, while other themes must be analyzed to find new wordings. We need to make some tricky decisions regarding the expected behavior there (eg. should all wordings be displayed in the BO interface or just those that don't exist in the front office wordings? If we display them all, then what's the role of front office wordings?).

There are many issues with translations, and I'm meaning to untangle the bigger part of that mess in 1.7.7. For the time being, this PR only fixes the issue mentioned in the PR description, and only that.

This change reverts #13088

Before this change, ALL edited wordings would be attached to the current theme, which only make sense when we're editing a theme's translations. Also, in the later case, it should be the *selected* theme and no the *current* theme.
@eternoendless eternoendless force-pushed the eternoendless:fix-cannot-translate-bo branch from 8c8e684 to 718d088 Aug 8, 2019
$catalogue = new MessageCatalogue($locale);
// do not try and load translations for a locale that cannot be saved to DB anyway
if ($locale === 'default') {

This comment has been minimized.

Copy link
@jolelievre

jolelievre Aug 9, 2019

Contributor

👍

@khouloudbelguith

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

Hi @eternoendless,

Seems that the translation is not updated in the Symfony page
Steps to reproduce the issue:

  1. Go to BO => International => Translations => Select
    1.1. Type of translation => Back office
    1.2 Select your language => English
    1.3 Click on Modify
  2. Search for the expression "View my shop"
  3. Try to translate it
  4. click Save => OK
  5. Go to the Dashboard page => the new expression well displayed
  6. Go to any page migrated to Symfony => old expression is displayed

https://drive.google.com/file/d/1n50LmEhI01xRq55Y1X16WVFR6znmet_O/view
Issue created here: #15052 (reproduced with PS1.7.5.1 & PS1.7.6.0 so not related to this PR)

Except for this issue => OK => PR validated
Thanks!

@eternoendless

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

Thanks everyone!

@eternoendless eternoendless merged commit 253f967 into PrestaShop:1.7.6.x Aug 9, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eternoendless eternoendless deleted the eternoendless:fix-cannot-translate-bo branch Aug 9, 2019
@fafa0659

This comment has been minimized.

Copy link

commented Aug 9, 2019

hello

Someone can he help me and let me wich files must modifoed in this issue ?

thanks you

@ElgorTR

This comment has been minimized.

Copy link

commented Aug 10, 2019

Hello,

I changed the code in the specified files, but the same problem persists. do I need to make another change?

@eternoendless

@direwald

This comment has been minimized.

Copy link

commented Aug 15, 2019

Even after applying the changes from this PR, the issue still presists when trying to translate installed modules. Any chance of a follow up to this?

@eternoendless

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

@direwald This PR fixes that: #15139

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.