Skip to content
This repository has been archived by the owner on Mar 17, 2022. It is now read-only.

Fix order email title & subject translations #414

Merged
merged 3 commits into from
May 6, 2019

Conversation

Annih
Copy link
Contributor

@Annih Annih commented May 1, 2019

Fixes introduced by 89ca15d added a comparison of the current locale with the order one to determine whether the content should be translated or not.

Two variables have been introduced $locale based on get_locale & $baseLocale based on WPLANG.
Sadly get_locale helper is hookable and contextual and might not return the right locale.

During my debugging session I ended up with an email title in one language and its subject in the other.

When I saw that the $baseLocale variable was present I decided to use the global WP locale of the site instead of the helper, and it fixed my issues.

Since this is an edit of your code @Jon007 what do you think?

Fix #415
Fix #416

@Jon007
Copy link
Contributor

Jon007 commented May 1, 2019

if you use the global WP locale of the site instead of the helper then the code will repeatedly unload and reload the text domain for every email string, even when the locale is already switched - this is a relatively slow and expensive operation.
Using get_locale() detects that the locale is already switched and short circuits that.
(and works fine in several languages).

Do you have any other code which is impacting locale?
Can you open this as an issue with the details of your environment and the problem you see?

Then possible pull requests should ideally be submitted as fixing issue # issue number

@Annih Annih force-pushed the fix_order_email_title_and_subject branch from 1ccd14f to 2950e65 Compare May 1, 2019 23:12
Annih added 3 commits May 2, 2019 01:13
Fixes introduced by 89ca15d added a
clever comparison of the current locale with the order one to determine
whether the content should be translated or not.

get_locale helper is contextual and might not return the right locale.
This commit uses the global WP locale of the site instead.
Now that emails fields are translated with the right locale, even the
new_order_recipient setting must have a default value otherwise below
code replace the address by null:

```
$string = __( $this->default_settings[ $email_type . '_' . $string_type, 'woocommerce' );
```
Indentation was inconsistent following recent patches.
@Annih Annih force-pushed the fix_order_email_title_and_subject branch from 2950e65 to ff178db Compare May 1, 2019 23:13
@Annih
Copy link
Contributor Author

Annih commented May 1, 2019

if you use the global WP locale of the site instead of the helper then the code will repeatedly unload and reload the text domain for every email string, even when the locale is already switched - this is a relatively slow and expensive operation.
Ahh yes you are right the call to switchLocale is conditioned by the locale, I thought it was used to determine whether the text should be translated or not. Thank you for your feedback.

I created #415 & #416 to report properly my issues and the investigation/debug I did.
I decided to keep only one PR since #416 is mainly triggered once #415 is fixed. I hope this is OK with you.

I updated my code to take into account your feedback, now that I have a better understanding of the code.

@Jon007
Copy link
Contributor

Jon007 commented May 6, 2019

I'll merge this and add the default cancelled order and failed order recipient as per the new_order_recipient

@Jon007 Jon007 merged commit d384b06 into hyyan:master May 6, 2019
@Jon007
Copy link
Contributor

Jon007 commented May 6, 2019

the default recipient is the shop email address so this should not be surrounded with the translation function __( , 'woocommerce') as it will not exist in the woocommerce translation files

Jon007 added a commit that referenced this pull request May 6, 2019
@frisonl
Copy link

frisonl commented May 7, 2019

Hi @Jon007, @Annih and of course @hyyan,

Thanks for all the amazing work. I kept having a translation issue with the emails in the heading and some parts of the mails like the topic. Even after implementing this commit.

Finnaly I found one interesting fix that solved it for me. I had the setting in my user profile on 'Site Default', which is Dutch in my case. When I changed it to English (one of the 4 languages) the emails were translated correctly.

Can you reproduce this?

@Jon007
Copy link
Contributor

Jon007 commented May 12, 2019

@frisonl I did some extra testing around the case you described and cannot reproduce, is working fine in test environment, are you sure it was not a caching or other issue.
If you have a reproducible case, please open a new issue with full details

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants