Translation 'ts' usage fixes. #14971
Conversation
|
(Standard links)
|
| @@ -196,13 +196,13 @@ protected function addFieldsDefinedInSettingsMetadata() { | |||
| $this->$add( | |||
| $props['html_type'], | |||
| $setting, | |||
| ts($props['title']), | |||
| $props['title'], | |||
There was a problem hiding this comment.
This may break things, but it's up to the extension to call ts in their setting file. If this sometimes works, it's only because the string is already in CiviCRM elsewhere.
| ($options !== NULL) ? $options : CRM_Utils_Array::value('html_attributes', $props, []), | ||
| ($options !== NULL) ? CRM_Utils_Array::value('html_attributes', $props, []) : NULL | ||
| ); | ||
| } | ||
| elseif ($add == 'addSelect') { | ||
| $this->addElement('select', $setting, ts($props['title']), $options, CRM_Utils_Array::value('html_attributes', $props)); | ||
| $this->addElement('select', $setting, $props['title'], $options, CRM_Utils_Array::value('html_attributes', $props)); |
There was a problem hiding this comment.
same as above, the 'ts' should be in the extension.
| @@ -8,7 +8,7 @@ class CRM_Contribute_Exception_FutureContributionPageException extends Exception | |||
| * @param int $id | |||
| */ | |||
| public function __construct($message, $id) { | |||
| parent::__construct(ts($message)); | |||
| parent::__construct($message); | |||
There was a problem hiding this comment.
I'm not sure what the intention here was, or what it may break, but I would rather see the bug surface so that we can fix the source.
| if (!empty($processor['description'])) { | ||
| $this->_processors[$id] .= ' : ' . ts($processor['description']); | ||
| $this->_processors[$id] .= ' : ' . $processor['description']; |
There was a problem hiding this comment.
Again, these are things that may work in some circumstances, but only by chance. It should be fixed at the source.
| @@ -732,7 +731,7 @@ public static function formRule($fields, $files, $self) { | |||
| if (array_key_exists('membership_type_id', $fieldValue['options'][$key]) | |||
| && in_array($fieldValue['options'][$key]['membership_type_id'], $currentMemberships) | |||
| ) { | |||
| $errors['price_' . $fieldKey] = ts($errorText, [1 => CRM_Member_PseudoConstant::membershipType($fieldValue['options'][$key]['membership_type_id'])]); | |||
| $errors['price_' . $fieldKey] = ts('Your %1 membership was previously cancelled and can not be renewed online. Please contact the site administrator for assistance.', [1 => CRM_Member_PseudoConstant::membershipType($fieldValue['options'][$key]['membership_type_id'])]); | |||
There was a problem hiding this comment.
I understand that the author meant to reduce code duplication, but in this case, it meant that the string was not extracted for translation.
Might be worth creating a function for these conditions, so that it could be something along the lines of "if (wasPreviouslyCancelled()) { $errors[..] = ts('the message');".
| $prefix . ts($action . ' contributions of type ') . $type, | ||
| ts(ucfirst($action) . ' contributions of type ') . $type, | ||
| ts("CiviCRM: %1 contributions of type %2", [1 => $action_ts, 2 => $type]), | ||
| ts('%1 contributions of type %2', [1 => $action_ts, 2 => $type]), |
There was a problem hiding this comment.
This removes the ucfirst, because there isn't an easy mb/utf8 function for ucfirst, and it would badly break non-lating languages.
Also, I'm still not happy with the end result, because it makes assumptions in how infinitives work in a language, but worst case translators can use "CiviCRM: '%1' contribution of type..", so that it's somewhat less awkward.
mlutfy
force-pushed
the
ts-fixes-20190806
branch
from
921aafb to
5ba8307
Compare
| 'view' => ts('view'), | ||
| 'edit' => ts('edit'), | ||
| 'delete' => ts('delete'), | ||
| ]; |
There was a problem hiding this comment.
I believe all these strings should be uppercased, as that's the reason for the test failure. So
$actions = [
'add' => ts('Add'),
'view' => ts('View'),
'edit' => ts('Edit'),
'delete' => ts('Delete'),
];
There was a problem hiding this comment.
It would then break another part of the test, ex where it expects: "CiviCRM: edit contributions [...]", but then I could change that one for "CiviCRM: Edit contributions [..]" in the test?
|
@mlutfy test fail relates - looks like the test needs tweaking |
|
Actually fixing capitalisation on permissions - is that just the label? |
mlutfy
force-pushed
the
ts-fixes-20190806
branch
from
5ba8307 to
1f2a2a0
Compare
|
Yes, seems like the test is a bit of a code duplication, but I suppose there is still some value in it. I ported over the same changes. I opted not to capitalize the actions, because none of the other CiviCRM permissions are capitalized. .e. in Drupal they show as "CiviCRM: view my invoices" (i.e. 'view' is lowercase). It also means that the permission description won't be capitalized, which is not ideal, but I don't think it's worth implementing a new function that provides the equivalent to ucfirst() for utf8. ucfirst goes into my bucket of "don't try to be clever with translation". Also worth considering that very few people enable Financial Type permissions, so the impact is very limited. |
mlutfy
force-pushed
the
ts-fixes-20190806
branch
from
1f2a2a0 to
6dabf45
Compare
Overview
Fixes how the gettext 'ts' translation function is called. Reduces the amount of warnings from the string extraction script.
https://docs.civicrm.org/dev/en/latest/translation/#best-practices