Internationalization code review [#317023]

Hi,

CivicActions is reviewing and upgrading multiple modules for use on client sites. Part of that is a review for internationalization issues. Attached is a patch for the issues found. The changes fall into 3 areas - messages passed to drupal_set_message() need to be enclosed in a call to t(); strings passed to t() should not contain leading or trailing spaces which can cause problems with translations not applying because it's missing the extra spaces; and strings passed to t() shouldn't contain unnecessary html tags that don't provide any extra context for the translator.

Hope it helps.

Cheers,
Stella

Comment	File	Size	Author
#5	views_i18n_0.patch	2.95 KB	stella
#5
	views_i18n_0.patch	6.18 KB	stella

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

stella CreditAttribution: stella commented 4 October 2008 at 12:15

There were 3 other internationalization issues found which aren't fixed in the above patch.

sites/sony/modules/contrib-head/views/plugins/views_plugin_access_perm.inc:
+17: The first parameter to t() should be a literal string. There should be no variables, concatenation, constants or other non-literal strings there.

sites/sony/modules/contrib-head/views/includes/base.inc:
+67: The first parameter to t() should be a literal string. There should be no variables, concatenation, constants or other non-literal strings there.
+97: The first parameter to t() should be a literal string. There should be no variables, concatenation, constants or other non-literal strings there.

The t() function is not intended for translating dynamic variables, regardless of where the data comes from or whether it's trusted or not. It also means that the potx module is unable to extract these strings for translation. There is an alternative which is to use the tt() function provided by the i18nstrings module. See http://groups.drupal.org/node/15177 for a discussion on this.

Comment #2

merlinofchaos CreditAttribution: merlinofchaos commented 4 October 2008 at 16:54

-  $vars['rearrange'] = l('<span>' . t('Rearrange') . '</span>', "admin/build/views/nojs/rearrange/$view->name/$display->id/$type", array('attributes' => array('class' => 'views-button-rearrange views-ajax-link', 'title' => t('Rearrange')), 'html' => true));
+  $vars['rearrange'] = '<span>' . l(t('Rearrange'), "admin/build/views/nojs/rearrange/$view->name/$display->id/$type", array('attributes' => array('class' => 'views-button-rearrange views-ajax-link', 'title' => t('Rearrange')), 'html' => true)) . '</span>';
 
-  $vars['add'] = l('<span>' . t('Add') . '</span>', "admin/build/views/nojs/add-item/$view->name/$display->id/$type", array('attributes' => array('class' => 'views-button-add views-ajax-link', 'title' => t('Add')), 'html' => true));
+  $vars['add'] = '<span>' . l(t('Add'), "admin/build/views/nojs/add-item/$view->name/$display->id/$type", array('attributes' => array('class' => 'views-button-add views-ajax-link', 'title' => t('Add')), 'html' => true)) . '</span>';

This is incorrect; the span is inside the l() for the CSS sprites. Did you test this change? It should have visibly broken the add and rearrange links.

I'm not sure how this change affects internationalization issues, either; you didn't affect the strings at all, you just re-organized the tags incorrectly.

Comment #3

merlinofchaos CreditAttribution: merlinofchaos commented 4 October 2008 at 16:56

In response to #1, while I appreciate the effort, it'd be nice if you did a little research. I had a long discussion with Goba about this, and while Views still needs to find a way to communicate this data to potx, there are, in fact, areas where core does the t() dynamic translation. Right now, there is no way for Views to be dynamically translated. This fairly hacky method allows for it to happen, albeit with some amount of difficulty.

I put out a proposal 6-8 months ago for software that could help facilitate Views and internationalization, and nobody took me up on it. SO the items in #1 are very much "won't fix".

Comment #4

merlinofchaos CreditAttribution: merlinofchaos commented 4 October 2008 at 17:09

Status:

Needs review

» Needs work

Comment #5

stella CreditAttribution: stella commented 4 October 2008 at 19:48

Status:

Needs work

» Needs review

File	Size
views_i18n_0.patch	2.95 KB

Ok, here's a re-rolled patch without the span changes. I did wonder why the spans were included in the link and did test my changes, but it was quite late last night when I did it, so I obviously missed something, my apologies.

Regarding the potx and translation of dynamic variables, I just wanted to flag it and alert you to the g.d.o discussion. I didn't really expect any changes to be made :) I agree there is no good way to handle this at present and it's something I would like to see improved. I wasn't aware of your discussion with Goba or your proposal, but I've done a quick search since and can't find it, would you mind pointing me at it? I'd be very interested in reading it.

Cheers,
Stella

Comment #6

hass CreditAttribution: hass commented 4 October 2008 at 19:53

@Stella: I think you should be aware about this monster patch #279073: Context sensitive translation issues. Please focus on this issue and make this thread here a duplicate.

Comment #7

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

@hass: assuming that suits merlinofchaos, then I think it's a good idea and we should do that (though I do prefer a few smaller patches rather than one monster one). However, my three changes for includes/admin.inc aren't included in your patch (as far as I can see), so it'd be great if you could work them in or suggest alternatives.

Cheers,
Stella

Comment #8

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

If they are not already included in my patch - it's ok and can go in as extra patch. I might have overseen some more... the patch is the 4-5th of this big patches and every time I find something more :-(.

Comment #9

merlinofchaos CreditAttribution: merlinofchaos commented 7 October 2008 at 22:05

Status:

Needs review

» Fixed

stella: The issue in question is this one http://drupal.org/node/262879 and there isn't a full resolution, though I believe the remaining details, really, are ensuring Views can give potx the right data so that it can do what needs to be done. However, I am not likely to work on this myself until there's someone in the translation community that can help do the work. I am too much of an outsider and even though the ability to translate Views seems to me to be a very important thing, nobody actually has pitched in to help finish this. I understand that people like Gabor are entirely too busy to do this, but I really need someone from within the community to help out here and make sure this is taken to the next step.

#279073 has been CNW for 7 weeks so I haven't looked at it in a long time.

The patch here has been committed.