Figure out a nice DX when working with injected translation

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.)

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

$this->translate();

I'd be OK with #2 as well.

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

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.

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.

+++ 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?

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.

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.

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.

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)?!

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

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.

Related: #2049159: Create a ControllerBase class to stop the boilerplate code madness

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...).

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

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?

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

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.

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.

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

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

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

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

@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.

A few fixes.

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.

+++ 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.

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.

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).

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?

Sold.

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"?

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

Works for me.

Committed and pushed to 8.x. Thanks!

Good call. Yes!

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.

Follow-up added #2120059: Cleanup temporary t() functions in phpunit tests.