Hi,

CivicActions is reviewing and upgrading multiple modules for use on client sites. Part of this work is a coding standards review of internationalization handling of strings. Attached you will find a patch with a fix for the one issue found.

Cheers,
Stella

CommentFileSizeAuthor
#6 google_analytics_i18n.patch1.03 KBstella
#4 google_analytics_i18n_0.patch1.21 KBstella
google_analytics_i18n_0.patch1.13 KBstella

Comments

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

This is a good change. I'd thought about trying to take out the spans too, but stella rightly pointed out that they're needed for context. Applies cleanly and should be rtbc.

hass’s picture

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

Looks like you also found a context sensitive core bug :-). See system.admin.inc line 677+. I've taken over this stupid bug from there... :-(. I commit this later.

EDIT: correct the filename

hass’s picture

hass commented
Status: Reviewed & tested by the community » Needs work

Thought some more about this patch and the core bug. I think we should first fix the core bug and then reuse the core translation string.

The below is not 100% context sensitive, but seems to be the only way for core:

t('@module_name (<span class="admin-enabled">enabled</span>)', array('@module_name' => t('Search')))
t('@module_name (<span class="admin-disabled">disabled</span>)', array('@module_name' => t('Search')))
stella’s picture

stella commented
Status: Needs work » Needs review
StatusFileSize
new google_analytics_i18n_0.patch1.21 KB
hass’s picture

hass commented

Do we have a reference to the core case? :-)

stella’s picture

stella commented
StatusFileSize
new google_analytics_i18n.patch1.03 KB

No, after a discussion on IRC, it seems module names are not translated on purpose. So here's a new patch that just moves the leading space and the span tags outside the call to t().

Cheers,
Stella

hass’s picture

hass commented

Nope, this is incorrect and something I hear very often. People telling you this are not aware about context sensitive translation issues. It doesn't matter if module names should be translated or not. We must go with patch in #4. See my comment in http://drupal.org/node/311883#comment-1024509. Otherwise we keep it as is. Also review the code I pointed out in #2 of the systems module.

stella’s picture

stella commented

If module names are not meant to be translated, but are still needed in the string to provide context, then the patch in #4 is still wrong as it calls t() on the "Search" string, whereas it should just be passed in as "Search", no?

hass’s picture

hass commented

Yes, but the longer string with placeholders is correct. On the other side I don't know why a module name shouldn't be translatable and I know we are doing translation of module names in German if it comes to context sensitive sentences. We should first fix the context sensitive string bug on the core modules page and then we also change it in GA. I'd like to reuse this one and this is also what we are already doing today!

I could apply the patch and remove the t() on 'search' if required. Aside - where is it written that we shouldn't translate module names?

stella’s picture

stella commented

Ok, you're already aware of the post at #152375: Implement translatable module names (with context) for addressing whether or not module names should be translated. Therefore I'm not going to post a patch for Drupal core to translate the module names in admin/build/modules as that would be duplicating the work and discussion already ongoing at #152375.

From looking at that post, there doesn't seem to be a final conclusion or decision on how it should be handled - looks like the format may be changing so it includes filename or original name or similar (http://lists.drupal.org/pipermail/translations/2007-June/000390.html).

Therefore I propose we just address the leading space in the t() call on the line in question in this issue, and then when there is an agreed solution to #152375, you implement the same thing in google analytics.

Cheers,
Stella

hass’s picture

hass commented
Status: Postponed » Needs review

Seems I forgotten my own case... shame on me :-).

Have you taken a look to systems module line 677+ (~20 following lines)? This code use the string we are using in GA in core with the leading space. I have simply taken this over... If this lines change in core I will follow core to reuse the core string. :-)

t('@module_name (<span class="admin-enabled">enabled</span>)', array('@module_name' => 'Search')) looks good to me and would be good for core, too.

stella’s picture

stella commented
Status: Needs review » Postponed

Ok, so you're postponing this then - updating status accordingly.

hass’s picture

hass commented
Status: Reviewed & tested by the community » Postponed

Core patches are at #320793: Context sensitive translation issue in system module.

hass’s picture

hass commented
Status: Postponed » Fixed
Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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