The attached module uses i18nstrings.module's tt() function instead of t() which should not be used for (possibly) user supplied strings because changing the source breaks all translations.

The "downside" is that the string won't be translated when i180nstrings isn't enabled.

CommentFileSizeAuthor
#17 pm_email_notify.2011020802.patch1.07 KBjoostvdl
#14 pm_email_notify.2011020801.patch1.26 KBjoostvdl
#8 pm_email_notify_i18n3.patch6.05 KBberdir
#3 pm_email_notify_i18n2.patch5.29 KBberdir
pm_email_notify_i18n.patch4.15 KBberdir

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Patch is based on #292634-20: confirmation emails not in the right language and http://drupal.org/node/609364

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

That's great.
In admin case we should only rely on variable_set and variable_get for presentation. Calling tt() is great to create the string in text group, but the return value should never be considered for presentation / output. Else we might overwrite the variable (default language representation) with a translation just because of a language switch. If switched the system into default language -- calling tt will (usually) refuse translating default language and provides now the wrongly translated (overwritten) value.

One option would be to add $translate which can disable translation in case of FALSE:
function _pm_email_notify_text($key, $translate=TRUE, $language = NULL) {

So: In admin case we should get plain original text via variable_get inside notify_text -- no tt for return value in any case. However we should still call tt() in this case to make sure the item is added to interface translation.
So admin settings can only update the variable, not the translations.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new pm_email_notify_i18n2.patch5.29 KB

Rerolling the patch

- Added the $translate flag
- Since the main issue is imho not a technical one but the problem that admins insert translated text (works only when you're site is in a single language) when they shouldn't, I've also extended the help text a bit.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Looks much better now. I just can't believe the amount of potential issues this snippet has...
If you check calling _pm_email_notify_text you will notice that finally all calls of tt() will have $update=TRUE.
So the locale always enforces refresh in case of call via wrapper. This is bad and we might need another variable... or we can decide based on parm $translate.

Regarding hook_locale: Did you decide to implement it even if it seems to be removed in newer versions and doc is almost missing?

Many thanks for bringing translation wrapper issues forward!

YK85’s picture

YK85 commented

I was wondering if this patch allows the privatesmg notifiation email to be sent out based on the user's preferred language? If user has preferred language English=> receives english, if user has preferred language French=>receves in French. Where can this French message be typed?

Thank you!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yeah, that patch fixes that. It was already possible before, but changing the original source string (usually english) would cause to loose all translations.

You can create translations of the mail texts through the Translate interface, that information/link is also displayed when applying the patch.

YK85’s picture

YK85 commented
Status: Needs review » Reviewed & tested by the community

the patch at #3 works great! thanks

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new pm_email_notify_i18n3.patch6.05 KB

- $update is now only set to TRUE if $translate is FALSE, because the source can only be updated through the settings page
- Improved apidocs
- Compatibility with i18nstrings.module v 1.3+ (Define in hook_locale that we don't use a format)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Version: » 7.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Fixed in 6.x-2.x-dev, this needs to be ported to 7.x.

miaoulafrite’s picture

miaoulafrite commented

Hi there, thanks for this wonderful module

could somebody please tell me how to translate sent messages? so that the mail sent is sent considering the current language of the receiver?

thanks

BenK’s picture

BenK commented

Subscribing...

joostvdl’s picture

joostvdl commented
Status: Patch (to be ported) » Active

How can this work? There is no where a function that first creates the i18nstring (with i18nstrings_update). Therefore are the strings not found in the database. Translating is not possible.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

joostvdl - the api in i18n changed after this patch.
It worked before.

So we need to rewrite the code to use i18nstrings_update...

joostvdl’s picture

joostvdl commented
StatusFileSize
new pm_email_notify.2011020801.patch1.26 KB

To overcome the problem I created a patch to support the locale_refresh function. This is just a temporary solution, but works ;-)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Version: 7.x-1.x-dev »

Updating version.

The patch seems to be again a hacked version, I'm pretty sure that there are no dsm() calls in the original.

I guess it would make sense to create a separate function for the default string so that it doesn't need to be repeated.

Also, I assume it is necessary to call the update functions when submitting the settings form?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs work
joostvdl’s picture

joostvdl commented
StatusFileSize
new pm_email_notify.2011020802.patch1.07 KB

Oops... hereby the patch against the 6.x-2.x-dev version

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
--- pm_email_notify.module.orig	2011-02-08 11:42:21.000000000 +0100
+++ pm_email_notify.module	2011-02-08 10:27:07.000000000 +0100

You need to create the patch from the privatemsg top-level directory, so that the path is pm_email_notify/pm_email_notify.module

+++ pm_email_notify.module	2011-02-08 10:27:07.000000000 +0100
@@ -197,12 +197,22 @@ function pm_email_notify_locale($op = 'g
+function pm_email_notify_locale_refresh() {

A short docblock would be nice.

+++ pm_email_notify.module	2011-02-08 10:27:07.000000000 +0100
@@ -197,12 +197,22 @@ function pm_email_notify_locale($op = 'g
+  $default_subject = 'New private message at !site.';
+  $default_body = "Hi !username,\n\nThis is an automatic reminder from the site !site. You have received a new private message from !author.\n\nTo read your message, follow this link:\n!message\n\nIf you don't want to receive these emails again, change your preferences here:\n!settings";

See above, we should move the default-handling from _pm_email_notify_text() into a separate function and then just call it here. Note that we imho need to insert the actual configured value, not the original.

And additionally, update it when the settings are saved.

Powered by Dreditor.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Version: » 7.x-2.x-dev

Commited an improved version of the patch from #17. Back to 7.x-2.x

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Fixed

Ported and commited to 7.x-2.x

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.