Internationalization code review [#312546]

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 based on a review with the coder module and a careful examination of the code. Thanks!

The changes are all related to the handling of links in strings passed to t(). Currently you are using the format:

t('some text !link ', array('!link' => l('my link text', 'http://www.drupal.org'))),

However this doesn't give any context to the translator and the following format should be used instead:

t('some text <a href="@link">my link text</a> ', array('@link' => url('http://www.drupal.org'))),

Cheers,
Stella

Comment	File	Size	Author
#8	cck_strong.patch	3.33 KB	stella
#6	cck_url.patch	4.37 KB	stella
#2	cck_i18n.patch	4.88 KB	stella
	cck_i18n.patch	4.87 KB	stella

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

catch

he/him

English

CreditAttribution: catch commented 24 September 2008 at 14:50

Very minor, but I'm not sure about:

+        '#description' => t("Advanced usage only: PHP code that returns a default value. Should not include &lt;?php ?&gt; delimiters. If this field is filled out, the value returned by this code will override any value specified above. Expected format: <pre>!sample</pre>Using <a href=\"@link_devel\">devel.module's</a> 'devel load' tab on a %type content page might help you figure out the expected format.", array(

Core tries to avoid backslashes within t() for clarity where possible. I think we can do this without damaging the wording too much, or even slightly improving it i.e.:

+        '#description' => t('Advanced usage only: PHP code that returns a default value. Should not include &lt;?php ?&gt; delimiters. If this field is filled out, the value returned by this code will override any value specified above. Expected format: <pre>!sample</pre>To figure out the expected format, you can use the <em>devel load</em> tab provided by <a href="@link_devel">devel module</a> on a %type content page.', array(

Comment #2

stella CreditAttribution: stella commented 24 September 2008 at 18:15

File	Size
cck_i18n.patch	4.88 KB

I've attached another version of the patch - the only difference is the slight adjustment to the wording as proposed by catch.

Cheers,
Stella

Comment #3

catch

he/him

English

CreditAttribution: catch commented 24 September 2008 at 18:24

Status:

Needs review

» Reviewed & tested by the community

Thanks Stella. Either of the two patches should be RTBC depending on the wording you want to use.

Comment #4

KarenS CreditAttribution: KarenS commented 26 September 2008 at 10:59

Status:

Reviewed & tested by the community

» Fixed

Committed. Thanks!

Comment #5

hass CreditAttribution: hass commented 4 October 2008 at 13:35

Status:

Fixed

» Needs work

Does this url() really makes sense? url('http://www.drupal.org/projects/schema') I would remove it... aside  should be changed into STRONG. And i wouldn't write . -> . might be better.

Comment #6

stella CreditAttribution: stella commented 4 October 2008 at 20:05

Status:

Needs work

» Needs review

File	Size
cck_url.patch	4.37 KB

Here's a patch to fix the url() bit. I agree  tags should be changed to strong but it's not really an internationalization issue, so I've omitted that from this patch.

Cheers,
Stella

Comment #7

hass CreditAttribution: hass commented 4 October 2008 at 20:08

I know STRONG is not an i18n issue, but getting the strings STABLE is a real issue for translators. So better we get them permanently stable than changing every few weeks :-(.

Comment #8

stella CreditAttribution: stella commented 4 October 2008 at 20:18

File	Size
cck_strong.patch	3.33 KB

Ok well here's a patch that just changes  to .

Comment #9

hass CreditAttribution: hass commented 4 October 2008 at 20:44

The url('http://www.drupal.org/project/devel') bugfix is missing :-)

Comment #10

stella CreditAttribution: stella commented 4 October 2008 at 23:46

hass: they're two separate patches, by design. Both will need to be applied.

Comment #11

KarenS CreditAttribution: KarenS commented 6 October 2008 at 14:55

Status:

Needs review

» Fixed

Committed.

Comment #12

stella CreditAttribution: stella commented 8 October 2008 at 21:23

One more thing which you already know, but just for completeness, running the "interface text translatability" coder review provided by the potx module highlights additional internationalization issues.

The problem for all of these are the same. The t() function is not intended for use on $variables, constants, strings that have already been passed through t() or strings which have variables concatenated onto them. Running t() on $variables also prevents these strings from being extracted by the potx module for translation. This is why the i18nstrings module provides the tt() module, which you should consider using instead. However, it's not an ideal solution, see http://groups.drupal.org/node/15177 for a discussion.

Cheers,
Stella

Comment #13

hass CreditAttribution: hass commented 9 October 2008 at 06:35

@Stella: Not every t'ified string using a variable is wrong. I know potx complains, but "all" this strings are somewhere else defined correctly and I'm not aware about a string that is not extracted with potx... have you found one that is untranslatable?