After #1813762: Introduce unified interfaces, use dependency injection for interface translation landed, using an injected service instead of t() is now possible..
which itself is a great win in terms of unit-testability, but there is a DX drawback

t()
becomes
$this->translation->translate()

Files: 
CommentFileSizeAuthor
#50 translation-2018411-50.patch22.55 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,424 pass(es).
[ View ]
#50 interdiff.txt3.01 KBtim.plunkett
#48 translation-2018411-48.patch22.67 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#48 interdiff.txt5.09 KBtim.plunkett
#41 injected-t-2018411-41.patch22.74 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,210 pass(es).
[ View ]
#41 interdiff.txt5.57 KBtim.plunkett
#34 injected-t-2018411-34.patch18.91 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,158 pass(es), 85 fail(s), and 265 exception(s).
[ View ]
#18 drupal-translation-dx-2018411-18.patch10.83 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 57,662 pass(es).
[ View ]
#15 drupal-translation-dx-2018411-13.patch10.93 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 57,553 pass(es).
[ View ]
#15 interdiff.txt3.43 KBParisLiakos
#11 drupal-translation-dx-2018411-11.patch8.04 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 56,393 pass(es), 3 fail(s), and 7 exception(s).
[ View ]
#11 interdiff.txt1.88 KBParisLiakos
#10 translate-invoke-2018411-10.patch6.49 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 57,561 pass(es).
[ View ]
#8 translate-invoke-2018411-8.patch3.59 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 57,403 pass(es).
[ View ]
#2 translate-invoke.patch618 bytesmsonnabaum
PASSED: [[SimpleTest]]: [MySQL] 57,573 pass(es).
[ View ]

Comments

Crell’s picture

Simple option:

public function t($str) {
return $this->translate($str);
}

So the common case becomes:

$this->translate->t();

(We can probably do better, but that's nice and easy. Or just rename the method directly.)

msonnabaum’s picture

Status:Active» Needs review
StatusFileSize
new618 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,573 pass(es).
[ View ]

I'd do this. It'll allow for:

<?php
$this
->translate();
?>
Crell’s picture

I'd be OK with #2 as well.

plach’s picture

Somehow using ->t() probably would have the advantage of not requiring changes in the code scanning files to find translatable strings.

ParisLiakos’s picture

well, anyone can name the property however he/she wants, but we should standarize it here. i think calling it t would make more sense than calling it translate, since it is a property. we use verbs for methods.
but i would be fine with translate as well

Edit:
or

<?php
$this
->translation('my string');
?>

would be a good solution too

Crell’s picture

Assigned:Unassigned» Gábor Hojtsy

With the __invoke() approach we cannot control what the variable is named, that's true. I would prefer a more self-descriptive name, so $this->translate() is better than $this->t() as a convention for core. (That way it reads nicely as an imperative verb.) Contrib modules could do what they want, but I presume most would follow core's lead.

Let's get input from Gabor, too.

ParisLiakos’s picture

tagging

ParisLiakos’s picture

StatusFileSize
new3.59 KB
PASSED: [[SimpleTest]]: [MySQL] 57,403 pass(es).
[ View ]

and here it is with missing interface, so that we can use the interface for typehints

Gábor Hojtsy’s picture

Assigned:Gábor Hojtsy» Unassigned
Issue tags:+D8MI, +language-base

The coexistence of TranslationInterface and TranslatorInterface sounds tricky :D

Using ->t() would not help with not requiring changes in the string identification code, since this will result in different tokens I think compared to t() the function. (I did not yet check). Anyway, Drupal 8 does all kinds of funky things to not use t() at a lot more places, and people will need to learn a lot more than just "use t", so whatever works best should be used here. I think ->translate() is most descriptive.

ParisLiakos’s picture

StatusFileSize
new6.49 KB
PASSED: [[SimpleTest]]: [MySQL] 57,561 pass(es).
[ View ]

The coexistence of TranslationInterface and TranslatorInterface sounds tricky :D

well it has the getStringTranslation method, so why not be able to use it as translator.

Bad news is that __invoke wont work directly on properties..

<?php
// Throws fatal error
$this->translate();
// This works
$t = $this->translate;
$t();
?>

so, here is an alternative solution based on this

ParisLiakos’s picture

StatusFileSize
new1.88 KB
new8.04 KB
FAILED: [[SimpleTest]]: [MySQL] 56,393 pass(es), 3 fail(s), and 7 exception(s).
[ View ]

and you can do this of course

tim.plunkett’s picture

+++ b/core/modules/action/lib/Drupal/action/ActionListController.phpundefined
@@ -137,7 +137,7 @@ public function render() {
-    $build['action_admin_manage_form'] = drupal_get_form(new ActionAdminManageForm($this->actionManager));
+    $build['action_admin_manage_form'] = drupal_get_form(ActionAdminManageForm::create(\Drupal::getContainer()));

Nice trick, I like that.

+++ b/core/modules/action/lib/Drupal/action/Form/ActionAdminManageForm.phpundefined
@@ -11,12 +11,13 @@
+class ActionAdminManageForm extends TranslationBase implements FormInterface, ControllerInterface {

Are we also going to change the base of SystemConfigFormBase and ConfirmFormBase?

ParisLiakos’s picture

Are we also going to change the base of SystemConfigFormBase and ConfirmFormBase?

i would love too, i guess i should i do it here

Status:Needs review» Needs work

The last submitted patch, drupal-translation-dx-2018411-11.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new3.43 KB
new10.93 KB
PASSED: [[SimpleTest]]: [MySQL] 57,553 pass(es).
[ View ]

missed a use statement.
also converted base confirm forms

Status:Needs review» Needs work
Issue tags:-DX (Developer Experience), -D8MI, -language-base

The last submitted patch, drupal-translation-dx-2018411-13.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
Issue tags:+DX (Developer Experience), +D8MI, +language-base
ParisLiakos’s picture

StatusFileSize
new10.83 KB
PASSED: [[SimpleTest]]: [MySQL] 57,662 pass(es).
[ View ]

reroll
also made setTranslation public

catch’s picture

Priority:Normal» Major

Bumping this up. I'm seeing loads of patches not injecting the translation service and I'm loathe to commit them still using t() just because the API is so ugly at the moment. Haven't reviewed patches here yet.

Gábor Hojtsy’s picture

From the point of view of localize.drupal.org, so long as we include text pieces in code (and we do in Drupal 8), we need a reliable way to identify those text pieces. So the #1 requirement from there is that the injected translation service invocations of the translation method need to be identifiable in the same foolproof way that t() is. Eg. the method name should be one that no other object would use. Otherwise we'll end up with false positives, bugos warnings, etc. after parsing.

I'm not sure translate() is such a distinguishable method name for this that can be statically identified unique to this feature. t() has that characteristic by the nature of the function namespace requiring unique function names. Methods between different classes don't ensure that, so we need to be careful which method name we pick. t() may be a method name that would be unlikely to be used for anything else (even if it does sound like very much a Drupalism still). The other extreme is to go very verbose like translateSoftwareText(). Also a format_plural() equivalent would need to be made available.

Finally, this sounds like it will make sweeping API changes, eg. to what services certain controllers need injected, no? It would need to have the approved API change tag if that understanding is right.

tstoeckler’s picture

Can't the string parser check for "use Drupal\Core\StringTranslation\TranslationBase" and "extends TranslationBase" and then pick up all "$this->translate()"? That sounds pretty foolproof.

Gábor Hojtsy’s picture

I'm not sure it is feasible that everything that will need translation injected will be able to extend direct from TranslationBase (as opposed to a service injected into it)?!

tstoeckler’s picture

Yeah, that's true... Hmm...

tim.plunkett’s picture

I like the __invoke() idea.

In the meantime, #2049709: TranslationManager::translate() should be on an interface needs to happen. I see @ParisLiakos did something like that in #8, but while we determine the perfect solution for this we still need the interface fixed first.

dlu’s picture

Moving to language system per #2050763-16: Refine "base system" component (notes on refactoring of "base system" category here: https://docs.google.com/a/acquia.com/spreadsheet/ccc?key=0AusehVccVSq2dF...).

tim.plunkett’s picture

Component:base system» language system

See #2059245: Add a FormBase class containing useful methods for another suggestion similar to ControllerBase

plach’s picture

It's a difficult pick but so far we have always categorized interface-translation stuff in the Locale module queue. Architecturally this does not belong to that queue but not even to the language system one, which is about the installed languages and language negotiation.

What about moving this to the Locale module anyway?

Berdir’s picture

ControllerBase went in with $this->t(), The not yet committed FormBase is doing the same.

Crell’s picture

If we say controllers and forms have a base class that has $this->t(), but services have to use a traditional injected object (or not have translation at all, as we've generally been doing for exceptions), would that be "good enough"? Controllers and forms are the largest users of translatable forms, I suspect. Perhaps we could also have it on BlockBase, but that would round out all of the common cases.

tim.plunkett’s picture

We could also ship with a 5.4 trait for $this->t(), at least to make some of contrib happy.

Add Drupal\views\Plugin\views\PluginBase to the list of ones desperately needing this.

Gábor Hojtsy’s picture

Status:Needs review» Needs work

Sounds like $this->t() is the way to go. Patch needs update.

Gábor Hojtsy’s picture

Some places use ->translate() while others use ->t(). This will not end well... Can we get on the same ground?!

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new18.91 KB
FAILED: [[SimpleTest]]: [MySQL] 58,158 pass(es), 85 fail(s), and 265 exception(s).
[ View ]

Everything using ->translate() is wrong, thankfully there are only four classes.

I'm adding protected function t() to EntityListController, since we'll need it on all of those (copying from FormBase)

Unfortunately, neither StaticLocalActionDeriver nor AggregatorCategoryBlock have a base class, and DerivativeBase is in Drupal\Component anyway. Many derivatives will need translation, we might want to consider a Drupal\Core DerivativeBase, and have a protected function t() on there.

Basically, we really need traits :(

EDIT: Oh, I didn't even realize there was a preexisting patch, sorry. This is completely from scratch

ParisLiakos’s picture

instead of adding everywhere t(), (set|get)TranslationManager methods, why dont we stick with the TranslationBase in #18, and make everything (including formBase and controllerBase) extend it?

Gábor Hojtsy’s picture

Ok, it is good we have a decision here. I reopened #2019679-3: Support for Drupal 8 $this->t() then for potx, so we can figure out localize.drupal.org support before it is too late :D

tim.plunkett’s picture

@ParisLiakos So that is why traits are needed. Because we're going to make class FormBase extends DependencySerialization in #2004282: Injected dependencies and serialization hell .
It can't extend from both.

Status:Needs review» Needs work

The last submitted patch, injected-t-2018411-34.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review

a ha, yeah i didnt think about this scenario..i guess then #34 is the best we can do till we require 5.4

ParisLiakos’s picture

Status:Needs review» Needs work
tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new5.57 KB
new22.74 KB
PASSED: [[SimpleTest]]: [MySQL] 58,210 pass(es).
[ View ]

A few fixes.

Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:+blocker

I think ATM its of paramount importance to spread the availability of the t() method in base classes, so new code being written will use this as best practice. Figuring out better inheritance based solutions later once inheritance trees are reworked may be an option, but IMHO this is a blocker to apply the t() method properly everywhere. So let's do this ASAP.

webchick’s picture

+++ b/core/modules/image/lib/Drupal/image/Controller/ImageStyleDownloadController.php
@@ -181,7 +154,7 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
-      return new Response($this->translator->translate('Error generating image.'), 500);
+      return new Response($this->t('Error generating image.'), 500);

Well that definitely looks much nicer.

--- a/core/modules/image/lib/Drupal/image/Controller/ImageStyleDownloadController.php
...
--- a/core/modules/system/lib/Drupal/system/FileDownloadController.php

Also enjoy seeing all the - signs in these classes.

Not moving down from RTBC, bit...

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Derivative/AggregatorCategoryBlock.php
@@ -95,9 +95,29 @@ public function getDerivativeDefinitions(array $base_plugin_definition) {
+  /**
+   * Translates a string to the current language or to a given language.
+   *
+   * @param string $string
+   *   A string containing the English string to translate.
+   * @param array $args
+   *   An associative array of replacements to make after translation.
+   * @param array $options
+   *   An associative array of additional options, with the following elements:
+   *   - 'langcode': The language code to translate to a language other than
+   *      what is used to display the page.
+   *   - 'context': The context the source string belongs to.
+   *
+   * @return string
+   *   The translated string.
+   */
+  protected function t($string, array $args = array(), array $options = array()) {

My biggest concern is that we're starting to see these docs copy/pasted now to 4+ places in core. None of them are remotely as detailed as https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7, explaining the use of @ and ! and % placeholders, when to use it, when not to use it, etc. I'm not sure how people using this method are supposed to learn of them and view proper examples on how they work.

Would it be better to instead move this to a single interface that we could selectively implement on e.g. FormBase, ControllerBase, BlockBase, etc? This would allow us to centralize the work of creating and maintaining these docs and the function signature, logic, etc. so if we needed to make changes to it in the future, we'd do so once and it would propogate everywhere.

Berdir’s picture

Interfaces can only be added for public methods. This is about protected helper methods, so that doesn't work, we don't want them to be public.

Maybe we could agree on some kind of short-form, with a @see t()? I'd suggest to do that as a follow-up for all t() methods, though.

tim.plunkett’s picture

Yes, I didn't want to be the one to propose we skimp on docs and just say @see t() (or @see \Drupal\Core\StringTranslation\TranslationInterface::translate()).
But maybe that is better than having half-ass docs in 4+ places (and yes, there will be more places this will end up).

ParisLiakos’s picture

yes, just having

@see \Drupal\Core\StringTranslation\TranslationInterface::translate()

everywhere is much better, cause in the long run it will be more maintainable, than keeping/updating same docs everywhere...

webchick’s picture

Cool, we are in agreement.

Then since the point of this issue is to introduce these protected t() helper methods, doesn't it make sense to tackle this here?

tim.plunkett’s picture

StatusFileSize
new5.09 KB
new22.67 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Sold.

jhodgdon’s picture

Instead of just a @see WhateverItIs90, could we have a sentence explaining why, something like "See the WhateverItIs::method() documentation for a detailed explanation of placeholders blah blah blah"?

tim.plunkett’s picture

StatusFileSize
new3.01 KB
new22.55 KB
PASSED: [[SimpleTest]]: [MySQL] 58,424 pass(es).
[ View ]

After further discussion with @jhodgdon, we settled on this.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Works for me.

Committed and pushed to 8.x. Thanks!

ParisLiakos’s picture

Do we need a change notice here saying that $this->translationManager->translate() *won't* work, and $this->t() or t() should be used, else strings wont be picked up for translation?

webchick’s picture

Title:Figure out a nice DX when working with injected translation» Change notice: Figure out a nice DX when working with injected translation
Priority:Major» Critical
Issue tags:+Needs change record

Good call. Yes!

ParisLiakos’s picture

Status:Fixed» Needs review

i wrote https://drupal.org/node/2079611 it could use a review though ;)

Berdir’s picture

Title:Change notice: Figure out a nice DX when working with injected translation» Figure out a nice DX when working with injected translation
Priority:Critical» Major
Status:Needs review» Fixed
Issue tags:-Needs change record

Made some small adjustments (used instead of <?php for the example at the bottom, works imho better inline in a sentence and clarified what doesn't work.

Status:Fixed» Closed (fixed)

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

jhedstrom’s picture