t() function is used to translate strings without page context. Some unnecessary white-spaces removed, and other cosmetic white-spaces for different sentence concatenation moved outside the t() function.

CommentFileSizeAuthor
#8 t-new.patch5.32 KBTR
t-function-spaces.patch6.1 KBoriol_e9g
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Component: language system » locale.module
Issue tags: +Novice

This should be an easy one to review.

oriol_e9g’s picture

Issue tags: -Novice

t-function-spaces.patch queued for re-testing.

aspilicious’s picture

Lol at the tag

oriol_e9g’s picture

Issue tags: +Novice

Ops! O.o WTF!

skottler’s picture

Issue tags: -Novice

t-function-spaces.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice

The last submitted patch, t-function-spaces.patch, failed testing.

TR’s picture

Status: Needs work » Needs review
FileSize
5.32 KB

Patch failed to apply because commit 1ce52d92fa19def removed the t() call from field.form.inc.

Attached is a re-rolled patch which excludes the change to field.form.inc.

Niklas Fiekas’s picture

Status: Needs review » Needs work

Maybe we should include this one:

./modules/book/book-navigation.tpl.php: <a href="<?php print $next_url; ?>" class="page-next" title="<?php print t('Go to next page'); ?>"><?php print $next_title . t(' ›'); ?></a>
That space should better be outside the t(), shouldn't it?

TR’s picture

Assigned: oriol_e9g » Unassigned
Status: Needs work » Needs review

@ Niklas Fiekas: That use of t() in the book-navigation.tpl.php seems wrong to me. ">" is not a character that needs translating, with or without whitespace included. Yes, I understand that it's desirable to allow users to customize this "arrow" symbol, much as breadcrumb separators may be customized, but that's not something that should be done via t(). Thus I feel that the issue you raise is a theming issue more appropriately addressed in a separate bug report filed against the book module.

Changing back to needs review. Patch still applies cleanly in D8 and D7.

TR’s picture

Actually, the use of t() in book-navigation.tpl.php is already being addressed in #736604: book module now without clearfix & better markup + accessibility love, so there's no need to deal with it here.

Niklas Fiekas’s picture

Status: Needs review » Reviewed & tested by the community

Thanks TR, I just grepped for more whitespace in t's and didn't think about that. Since it was the only thing I criticized: RTBC.

Gábor Hojtsy’s picture

Agreed, looks good.

Dries’s picture

Version: 8.x-dev » 7.x-dev

Committed to 8.x. Thanks!

Should we backport this to 7.x?

Niklas Fiekas’s picture

Issue tags: +Needs backport to D7

Yes, I guess.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Overall this looks *pretty* safe. The string changes are all in admin pages or tests, so no unwitting end users should stumble across them. And these are legitimate bugs with the strings, so I think they're ok for D7.

It's interesting that file.test has a hunk like this:

-    $message = t('Only files with the following extensions are allowed: ') . '<em class="placeholder">' . $extensions . '</em>';
+    $message = t('Only files with the following extensions are allowed:') . ' <em class="placeholder">' . $extensions . '</em>';

...but yet there's no corresponding change in file.module. I'm wondering how that could possibly not be failing tests right now. Hm.

At any rate, committed and pushed to 7.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.