To get translation working, I had to change the following code on line 129 from:

$extension .= l(t('@more_text', array('@more_text' => $settings['more_text'])), $uri['path'], array('html' => TRUE, 'attributes' => array('class' => array('more-link'))));

to:

$extension .= l(t('@more_text', array('@more_text' => t($settings['more_text']))), $uri['path'], array('html' => TRUE, 'attributes' => array('class' => array('more-link'))));

CommentFileSizeAuthor
#12 smart-trim-i1i8n-string-2533032-2.patch1.6 KBBerdir
#8 smart_trim-translate-read-more-2533032-8-D7.patch1.05 KBmarkie
#1 smart_trim-translate-read-more-2533032-2-D7.patch911 bytesdragos-dumi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dragos-dumi’s picture

dragos-dumi CreditAttribution: dragos-dumi commented
FileSize
smart_trim-translate-read-more-2533032-2-D7.patch911 bytes

This bug was introduced in the commit f7e80ade63f962f4d96337fb27bbb18ae9009b1f
The line should be restored to it's previous state because t function replaces the placeholder after locale. (see the attached patch)

dragos-dumi’s picture

dragos-dumi CreditAttribution: dragos-dumi commented
Status: Active » Needs review
rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
CreditAttribution: rodrigoaguilera as a volunteer commented
Status: Needs review » Needs work

I can confirm this.

I feel that both approaches (after and before patch) need improving.
I'm using features and potx and the 'more_text' is a setting for each view mode inside the field instance settings.

Maybe it should use something like i18n_field to translate each instance and fallback to the method you are trying to use.

my-family’s picture

my-family CreditAttribution: my-family commented

I can confirm the issue. After the #1 patch the string is available for translation and the translation works.

Ahmad Abbad’s picture

Ahmad Abbad CreditAttribution: Ahmad Abbad at Vardot for Vardot commented

I can confirm the issue.

Patch #1 worked for me

akalam’s picture

akalam CreditAttribution: akalam at Atenea tech commented
Status: Needs work » Reviewed & tested by the community

Worked for me too! Let's make this RTBC. In my opinion #3 sould be a diferent issue, this one is for translatable feature

Khumbu’s picture

Khumbu CreditAttribution: Khumbu commented

works like a charm...

markie’s picture

markie CreditAttribution: markie at Mediacurrent commented
Status: Reviewed & tested by the community » Needs review
FileSize
smart_trim-translate-read-more-2533032-8-D7.patch1.05 KB

So I really don't like the smell of a t() inside of a t(). It doesn't seem to make sense to me. Also, coding standards wants to pass a string literal where possible. (I hate the squiggly lines in phpstorm so I do as much as I can to get rid of them.

I created a new version of the patch for your review. Thanks for your help

rodrigoaguilera’s picture

rodrigoaguilera
Primary language Spanish
Location Madrid
CreditAttribution: rodrigoaguilera as a volunteer commented

I feel we are trying too hard to avoid the use of i18n_string.
t() is supposed to be for fixed strings translation. Using t() for this seems like an abuse.

I don't see clearly how the text ends up translated in the latest patch. It is the same case as the code was before patches but with more wrapping.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

I concur with @rodrigoaguilera regarding the latest patch. I think the first patch should be used. Regarding i18n_string I think that can be done later, but that requires a dependency to the i18n_module (also for users not using it). The site we are working on uses translations but doesn't use i18n_string, since that causes other problems.

elondaits’s picture

elondaits CreditAttribution: elondaits at IMAGINARY commented

Translation of the "read more" is really necessary and I'd say it's too little to justify installing a whole extra module even if it goes against the philosophy of t(). I do use i18_strings and wouldn't want this module to use that because importing strings from a file (for code versioning and deploying in dev, test and production) it's a real pain because it requires a file per language and context.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
FileSize
smart-trim-i1i8n-string-2533032-2.patch1.6 KB

Using t() on user-provided text is a strict no-go. Translations get lost if you edit the original, and it can even result in security issues.

In fact, this is basically what https://www.drupal.org/node/2480321 was about. That could be prevent by running it through a check_plain(), the new code is also very pointless as there's nothing to translate anymore. And it even explicitly sets html => TRUE, meaning it allows HTML. We can just drop that, and l() does a check_plain() for us, making it secure.

As a workaround, I think you could actually translate "@read_more" to whatever you want, it simply is global then and not configurable anymore per field.

This is a first patch using i18n_string(), but it's not very well tested yet. It has the opposite downside that you need to translate it per-field as it is depending on the context, and it actually can't be translated per view mode as the formatter doesn't have the view_mode string available as far as I see.

barraponto’s picture

barraponto CreditAttribution: barraponto commented
Status: Needs review » Reviewed & tested by the community

Looks good, works for me.

markie’s picture

markie CreditAttribution: markie at Mediacurrent commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +dcco2019

Sorry folks. #12 doesn't apply for me cleanly. I have added a few commits before it. Please re-roll