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

Make module and email translations work again when using a theme other than classic #15139

Merged
merged 3 commits into from Aug 21, 2019

Conversation

@eternoendless
Copy link
Member

commented Aug 16, 2019

Questions Answers
Branch? 1.7.6.x
Description? Fixes a bug where it would be impossible to translate modules and email subjects when the current theme was anything other than "classic"
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #15133
How to test? Install a different theme than classic. Test all the translation types (except Email body which is handled differently and not affected by this change).

More information

There was much confusion between the "selected" element (a module name when translating modules, a theme name when translating themes) and the selected theme. Up until now, it was half-managed by request parameters, half-forced to the current theme.

This change clarifies things a little:

  • Module translations are no longer attached to the currently selected theme (you'll have to edit the theme translations if you want a different translation for each theme; this requires the specific wording to be used in the theme's templates or else it won't appear in the translation interface)
  • Only theme translations are attached to a theme. All other translations are now global.
  • Email translations no longer are attached to an invalid theme called "subject"

Regarding #14809, we'll have to discuss changes in how Mail translations are managed so that email translations work like modules (where type would be 'mails' and selected would be either subject or body). But this will be problematic, because I think that in difference to email subjects, which are global, mail bodies are theme-dependent.


This change is Reviewable

@eternoendless eternoendless added this to the 1.7.6.1 milestone Aug 16, 2019

@eternoendless eternoendless requested a review from PrestaShop/prestashop-core-developers as a code owner Aug 16, 2019

@@ -35,7 +35,9 @@
var data = {
baseUrl: '{{ app.request.getBasePath() }}',
locale: '{{ app.request.query.get('locale') }}',
selected: '{{ 'modules' == app.request.query.get('type') ? theme.name : app.request.query.get('selected') }}',
type: '{{ app.request.query.get('type') }}',
currentTheme: {{ theme.name|json_encode|raw }},

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Aug 16, 2019

Contributor

watchout with raw.

Suggested change
currentTheme: {{ theme.name|json_encode|raw }},
currentTheme: '{{ theme.name|escape('js') }}',

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 21, 2019

Author Member

That won't work with null, functions are not json encodable, and html would be encapulated in a string. I'm I missing something?

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Aug 21, 2019

Contributor

If it's not working with null.

Suggested change
currentTheme: {{ theme.name|json_encode|raw }},
currentTheme: {% if theme.name === null %}null{% else %}'{{ theme.name|escape('js') }}'{% endif %},

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 21, 2019

Author Member

Looks overcomplicated when json_encode is build exactly for that purpose: it outputs the right Javascript syntax for whatever value you throw at it.

@mickaelandrieu

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Module translations are no longer attached to the currently selected theme (you'll have to edit the theme translations if you want a different translation for each theme; this requires the specific wording to be used in the theme's templates or else it won't appear in the translation interface)

If I've well understood, this introduces is a regression to fix a bug, right?

* @param null $search
*
* @return array
*/
private function getNormalTree($lang, $type, $selected, $search = null)
private function getNormalTree($lang, $type, $theme, $search = null)

This comment has been minimized.

Copy link
@matks

matks Aug 19, 2019

Contributor

If getNormalTree is supposed to only be called for Themes, I suggest we strongly validate the input by checking $theme actually exists and refers to an installed theme, or throw an exception.

This comment has been minimized.

Copy link
@matks

matks Aug 19, 2019

Contributor

(maybe not in a patch version though 😛)

This comment has been minimized.

Copy link
@jolelievre

jolelievre Aug 20, 2019

Contributor

Since we planned a refacto maybe it should be done in the refacto?

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 21, 2019

Author Member

This is called on line 159 with a null theme (BO translations and other)

*
* @return array
*/
private function getModulesTree($lang, $type, $selected, $search = null)
private function getModulesTree($lang, $selectedModuleName, $search = null)

This comment has been minimized.

Copy link
@matks

matks Aug 19, 2019

Contributor

I suggest same defensive logic: if $selectedModuleName is illogical, we crash. Translations are a mess right now, this would clear the path and avoid ambiguous successfull behaviors

This comment has been minimized.

Copy link
@matks

matks Aug 19, 2019

Contributor

(maybe not in a patch version though 😛)

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 21, 2019

Author Member

I think it will crash but deeper in the call stack. But we agree that this whole thing needs to be refactored.

@matks
Copy link
Contributor

left a comment

OK for me, same feedback than @PierreRambaud

@jolelievre jolelievre added this to In progress in PrestaShop 1.7.6 via automation Aug 20, 2019

$treeBuilder = new TreeBuilder($locale, $this->getSelectedTheme());
$treeBuilder = new TreeBuilder($locale, $theme);

This comment has been minimized.

Copy link
@jolelievre

jolelievre Aug 20, 2019

Contributor

So $theme is always null ?

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 21, 2019

Author Member

Yes when translating modules. Otherwise you'd have to translate your modules again for every theme (including if you switch themes). With this change only the wordings used in module templates overridden by a theme will be allowed to have a specific translation for that theme. This could be improved later by adding an option that brings ALL wordings, including core and modules, and that allows translating them for a specific theme.

This comment has been minimized.

Copy link
@jolelievre

jolelievre Aug 21, 2019

Contributor

then you don't need this $theme variable you can simply call $treeBuilder = new TreeBuilder($locale, null);

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 21, 2019

Author Member

I did it so that the parameter role is evident, but it could be removed as well later.

*/
private function getSelectedTheme()
private function getCurrentThemeName()

This comment has been minimized.

Copy link
@jolelievre

jolelievre Aug 20, 2019

Contributor

So it seems this method is no longer used?

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 21, 2019

Author Member

Right, I'll delete it

@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Restarting Travis again

@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

And again

@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

And again

I could almost believe the test fails for a good reason

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

And again!

Always the same random reason :/

Fix translations interface
- Module translations are no longer attached to the currently selected theme
- Email translations no longer are attached to an invalid theme called "subject"

@eternoendless eternoendless force-pushed the eternoendless:fix-translations-bo branch from 641289a to 1e070c0 Aug 21, 2019

@eternoendless

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2019

I removed the unused method and rebased.

If I've well understood, this introduces is a regression to fix a bug, right?

Yes and no. It made no sense, it was counterintuitive, and in the end, it didn't work. It's a regression in the same way as fixing a bug is a regression: "But I liked that bug, put it back!" 😛

The system has to be rethought, and in most importantly... specified.

@PierreRambaud PierreRambaud moved this from In progress to To be reviewed in PrestaShop 1.7.6 Aug 21, 2019

@PierreRambaud PierreRambaud moved this from To be reviewed to To be tested in PrestaShop 1.7.6 Aug 21, 2019

@sarahdib sarahdib added the QA ✔️ label Aug 21, 2019

@sarahdib sarahdib removed this from To be tested in PrestaShop 1.7.6 Aug 21, 2019

@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

Thank you @eternoendless

@matks matks merged commit 36f5da4 into PrestaShop:1.7.6.x Aug 21, 2019

2 checks passed

PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eternoendless eternoendless deleted the eternoendless:fix-translations-bo branch Aug 21, 2019

@sathishkumar16

This comment has been minimized.

Copy link

commented Aug 22, 2019

When we will get this as an offical update in the prestashop v1.7.6.X.

@eternoendless

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2019

@sathishkumar16

This comment has been minimized.

Copy link

commented Aug 26, 2019

@eternoendless How many days will it take to get the updates officially, because we are planning to go production within a month for us the translation module is very important before going to prod.
Else is there any alternate/temporary solution to fix it?

@eternoendless

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

@sathishkumar16 The QA process is looking good so it won't be long, probably within the week. You can follow the discussion on Gitter or wait for the announcement on Build.

@PrestaShop PrestaShop locked as off topic and limited conversation to collaborators Aug 26, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
9 participants
You can’t perform that action at this time.