Translatable strings review, context senstive bugfixes [#1555036]

I've seen so many issues in the German translation that made me reviewing the strings and translations and to get some issues fixed I've created this patch.

Comment	File	Size	Author
#6	captcha_Translatable+string+review+context+sensitive+bugfixes3.patch	16.2 KB	hass
#6
#4	captcha_Translatable+string+review+context+sensitive+bugfixes2.patch	15.52 KB	hass
#4
#3	captcha_Translatable+string+review%2C+context+sensitive+bugfixes2.patch	15.52 KB	hass
#3
	captcha_Translatable+string+review%2C+context+sensitive+bugfixes.patch	15.52 KB	hass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

hass CreditAttribution: hass commented 30 April 2012 at 19:01

Status:

Active

» Needs review

Comment #2

30 April 2012 at 19:04

Status:

Needs review

» Needs work

The last submitted patch, captcha_Translatable+string+review%2C+context+sensitive+bugfixes.patch, failed testing.

File	Size
captcha_Translatable+string+review%2C+context+sensitive+bugfixes2.patch	15.52 KB

Comment #4

hass CreditAttribution: hass commented 30 April 2012 at 19:11

File	Size
captcha_Translatable+string+review+context+sensitive+bugfixes2.patch	15.52 KB

Buggy robot don't like percents in file names.

Comment #5

30 April 2012 at 19:13

Status:

Needs review

» Needs work

The last submitted patch, captcha_Translatable+string+review+context+sensitive+bugfixes2.patch, failed testing.

File	Size
captcha_Translatable+string+review+context+sensitive+bugfixes3.patch	16.2 KB

Comment #7

soxofaan CreditAttribution: soxofaan commented 1 May 2012 at 15:26

Status:

Needs review

» Needs work

Hi, thanks a lot for these suggestions.

I've already committed some parts in http://drupalcode.org/project/captcha.git/commit/bdfc108

I have some questions/doubts about others:

-    $captcha_types['none'] = '[' . t('none') . ']';
-    $captcha_types['default'] = '[' . t('default challenge type') . ']';
+    $captcha_types['none'] = t('- None -');
+    $captcha_types['default'] = t('Default challenge type');

I agree with the capitalization change. Did you change "none" to "- None -", because 'none' is too short to be unambiguously translated (e.g. in German)?. Why did you remove the square brackets? I've used these to indicate that it are special cases. Compromise proposal: '[No challenge]' and '[Default challenge type]'.

-        $captcha_types["$module/$type"] = t('@type (from module @module)', array('@type' => $type, '@module' => $module));
+        $captcha_types["$module/$type"] = t('@type (from module @module)', array('@type' => t($type), '@module' => $module));

I've always learned that the Drupal translation system is not made to do t($variable) and you should always do t('a hardcoded string'). Why do you want to translate it anyway? The challenge types are shown untranslated in a lot of other places.

In a lot of places you suggest things like:

-    t(' ... the help of the \'%CAPTCHA_admin_links\' option below.',
-      array('%CAPTCHA_admin_links' => t('Add CAPTCHA administration links to forms'))
-      ),
+    t(" ... the help of the <em>Add CAPTCHA administration links to forms</em> below."),

I don't think I agree here: if you have incomplete translations (e.g 'Add CAPTCHA administration links to forms' is translated, but the long string not), with the original construction you have consistent translations: 'Add CAPTCHA administration links to forms' is always translated, whether it is embedded in a translated string or not. With your construction, the user will see the string translated and untranslated in different places, which makes the UI harder to understand IMHO.

Flush versus clear cache: is there a guideline for this? I found usage of both in Drupal core.

-            '#title' => t('Challenge "%challenge" by module "%module"', array('%challenge' => $challenge, '%module' => $module)),
+            '#title' => t('Challenge %challenge by module %module', array('%challenge' => $challenge, '%module' => $module)),

I would keep the quotes around %challenge because this strings can come from other modules and can be a complete sentence, so dropping the quotes can be bad for readability. I have less firm feelings about the quotes around %module :)

-    drupal_set_message(t('Failed to set a CAPTCHA type for form %form_id: could not interpret value "@captcha_type"',
+    drupal_set_message(t('Failed to set a CAPTCHA type for form %form_id: could not interpret value %captcha_type.',
...
-          '%form_id post blocked by CAPTCHA module: challenge "%challenge" (by module "%module"), user answered "%response", but the solution was "%solution".',
+          '%form_id post blocked by CAPTCHA module: challenge %challenge (by module %module), user answered %response, but the solution was %solution.',

I prefer quotes in this kind of error handling cases, because if you have an empty string for some reason, you can clearly see this, while without quotes, it looks like your sentence is chopped off.

+  // "Level:" is a translatable string "context" for the options.
   $form['image_captcha_color_settings']['image_captcha_foreground_color_randomness'] = array(
     '#type' => 'select',
     '#title' => t('Additional variation of text color'),
     '#options' => array(
-      0 => t('none'),
-      50 => t('small'),
-      100 => t('moderate'),
-      150 => t('high'),
-      200 => t('very high'),
+      0 => t('Level: none'),
+      50 => t('Level: small'),
+      100 => t('Level: moderate'),
+      150 => t('Level: high'),
+      200 => t('Level: very high'),

I don't like the 'Level:' prefix here. Why not use real translation context as introduced in Drupal 7?. Or alternatively: 'No variation', 'Little variation', 'Moderate variation', ...

Comment #8

hass CreditAttribution: hass commented 1 May 2012 at 18:09

I agree with the capitalization change. Did you change "none" to "- None -", because 'none' is too short to be unambiguously translated (e.g. in German)?. Why did you remove the square brackets? I've used these to indicate that it are special cases. Compromise proposal: '[No challenge]' and '[Default challenge type]'.

I used the None string as we are using them everywhere in Drupal for selectboxes with a none option. In past many used "none" and <none> and this is extremely difficult to translate, The minus are just the way how it's done in all translations since D6. I would suggest to used dashes not brackets. - No challenge - sound also much better to me than "None". I don't like the brackets... it's uncommon.

I've always learned that the Drupal translation system is not made to do t($variable) and you should always do t('a hardcoded string'). Why do you want to translate it anyway? The challenge types are shown untranslated in a lot of other places.

Than I missed the others... In general you are absolutely correct, but there are a few exceptions. I made this because you have the strings already somewhere else wrapped in t(). As these strings are safe you can do the wrapping of a variable. You should not surround custom (unstrusted) text with t(). The custom captcha label is also one exception, as only admins have permission to change and you need to trust admins. :-)

I don't think I agree here: if you have incomplete translations (e.g 'Add CAPTCHA administration links to forms' is translated, but the long string not), with the original construction you have consistent translations: 'Add CAPTCHA administration links to forms' is always translated, whether it is embedded in a translated string or not. With your construction, the user will see the string translated and untranslated in different places, which makes the UI harder to understand IMHO.

I should better write documentation on d.o... you should never have any strings that use l() to create links. Translators cannot translate such strings in context. Very often the placeholder text is conditional to the surrounding text. Additional if there is only a placeholder - translators can only *guess* what is inside the placeholder. We don't do this since D6/7 any longer. You should always write the text in the translatable strings and keep only the links outside as they could change in D8, but the text may not. I understand that this is additional work for translators, but it's better than a placeholder as I have no idea if I need a verb before the placeholder starts of not (context sensitive issues). In localization server I have no clue about the context or lines of code.

Aside, something that is already a % placeholder don't need double quotes again. The % placeholder is perfect.

Flush versus clear cache: is there a guideline for this? I found usage of both in Drupal core.

The main idea was to bring it in one line with core core... I believe there is no written rule :-)

I would keep the quotes around %challenge because this strings can come from other modules and can be a complete sentence, so dropping the quotes can be bad for readability. I have less firm feelings about the quotes around %module :)

Hm... I just made it like everywhere; if we expose something we make it EM... if this are sentences, than ok, maybe better to say with double quotes, but than please don't use % placeholders... it's somewhat "double" exposed.

I prefer quotes in this kind of error handling cases, because if you have an empty string for some reason, you can clearly see this, while without quotes, it looks like your sentence is chopped off.

Your personal preference is not like the most common used. I would go with core and we use % placeholders and that's it. Doubles quotes in other languages are sometimes like nightmare... see http://en.wikipedia.org/wiki/Quotation_mark_glyphs... and now try to type them with a keyboard in a website textarea. In German we mostly use copy and paste as they are so difficult to type... :-)

I don't like the 'Level:' prefix here. Why not use real translation context as introduced in Drupal 7?. Or alternatively: 'No variation', 'Little variation', 'Moderate variation', ...

Funny... I know... contexts are cool, we can use them here, but how should be name the context? I tried to find a good name, but was not sure. It should be as generic as possible. Contexts should be used very exceptional. The list of contexts is still in heavy discussion as it's very difficult to reuse. The words are not really "sizes"... I don't know... please don't use "Variations" as context... this is what we could name - abuse of contexts. A context name should describe the content as good as possible and as generic as possible. I hope I explained this correctly...