Translatable strings review [#1499442]

Patch attached. RTBC.

Comment	File	Size	Author
	filefield_paths_Translatable_strings_review.patch	4.01 KB	hass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Deciphered CreditAttribution: Deciphered commented 2 April 2012 at 09:06

Status:

Needs review

» Needs work

+++ b/filefield_paths.moduleundefined
@@ -63,7 +63,7 @@
+          '#title' => t('@title options', array('@title' => t($field['title']))),

Never wrap a variable with t().

+++ b/filefield_paths.moduleundefined
@@ -75,22 +75,22 @@
-          '#title' => t('Cleanup using Pathauto') . '.',

This is intentional, not to be changed

+++ b/filefield_paths.moduleundefined
@@ -75,22 +75,22 @@
+          '#description' => t('Cleanup %title using <a href="@pathauto">Pathauto settings</a>.', array('%title' => t($field['title']), '@pathauto' => url('admin/config/search/path/settings'))),

@@ -108,9 +108,7 @@
+          '#description' => t('Move and rename previously uploaded files.') . '<div>' . t('<strong class="warning">Warning:</strong> This feature should only be used on developmental servers or with extreme caution.') . '</div>',

@@ -119,9 +117,7 @@
+          '#description' => t('Actively move and rename previously uploaded files as required.') . '<div>' . t('<strong class="warning">Warning:</strong> This feature should only be used on developmental servers or with extreme caution.') . '</div>',

A translation string should not contain HTML.

+++ b/filefield_paths.moduleundefined
@@ -63,7 +63,7 @@
+          '#title' => t('@title options', array('@title' => t($field['title']))),

Variables should never be wrapped with t().

+++ b/filefield_paths.moduleundefined
@@ -75,22 +75,22 @@
-          '#title' => t('Cleanup using Pathauto') . '.',

This is intentional, should not be changed.

Some of the text strings you've suggested are nice, but the majority of this patch is a no go. Happy to review an future updates.

Cheers,
Deciphered.

Comment #2

hass CreditAttribution: hass commented 2 April 2012 at 14:29

Status:

Needs work

» Reviewed & tested by the community

Have you run coder review with this patch or why do you think this? Sounds like you are not very familiar with translatable strings... :-(. I'm creating translatable review patches for about 5 years and I'm a hardcore translator. I know the rules very well. Trust me this is all correct per latest rules. This patch is RTBC. Details below.

Never wrap a variable with t().

In general you are correct, but You have broken the rules. The main problem is that your $field['title'] is not translated if it is used here. The string is not translated in the UI today! If you can use t('File name') it would be better, but than you need to duplicate the foreach() code. I guess you don't like to do this. This is an exceptional situation, where we are allowed to use the variable wrapped into t() as you already have the string inside your module as t('File name') and we know it's safe here. Don't worry about coder complains. There is no problem at all, this is all 100% correct.

This is intentional, not to be changed

What is intentional here? You may say this is intentional, but this is a context sensitive bug and plain wrong. We place dots INSIDE translatable strings, not outside. My fix is 100% correct.

A translation string should not contain HTML.

I'm sorry, but this is incorrect. A translatable string is allowed to contain INLINE html code (EM, STRONG, A, etc.). The reason why I changed it, is the string "Warning:" is context sensitive with the full sentence and should be kept together in one string. This fix is 100% correct.

I will search for some reference documentation so you can read the DEV docs again and better understand.

Comment #3

Deciphered CreditAttribution: Deciphered commented 2 April 2012 at 23:03

Status:

Reviewed & tested by the community

» Needs work

Unfortunately you where right, I jumped ahead of myself based on my memory, and doing the appropriate research I have to eat my words, something I'm willing to do when I know I'm wrong. I don't appreciate your attitude, so I've opted to review some of your words for future disagreements:

- Have you run coder review with this patch or why do you think this? Sounds like you are not very familiar with translatable strings... :-(. I'm creating translatable review patches for about 5 years and I'm a hardcore translator. I know the rules very well. Trust me this is all correct per latest rules. This patch is RTBC. Details below.
+ I think you may be mistaken with some key points of your review, while I'm sure you have reasons to believe you are correct I assure you that I made the changes for the patch based on the following documentation: http://drupal.org/node/299085. I suggest giving that a quick skim over before you respond.

I was short with my review, but that's because I'd just been through quite a few other reviews and I was wrapping up for the night, all I wanted to do was point out what I understood to be wrong to continue the ball rolling. I still personally disagree with some of the decisions, I don't think a HTML element should be translatable as it's a structural or style element which should not be up to interpretation, and a few other minor things, but you do have documentation backing you up so I'm happy to concede.

Will commit shortly.

Comment #4

Deciphered CreditAttribution: Deciphered commented 2 April 2012 at 23:07

Status:

Needs work

» Reviewed & tested by the community

Marking back as RTBC, I changed it before coming to my final decision.

One other minor thing, it might help in the future if you actually put some details with the patch instead of just attaching a patch and saying that it's 'RTBC'. Not everyone knows your history and instantly trusts your opinion, it might be better to provide some justification for the patch. I think that would have sped things up in this case for sure.