Provide a trait for $this->t() and $this->formatPlural()

Tagging up.

+1

We could think about other onces as well.

oh, forgot this one.
here it is with a test. yay for the first trait in core!

some nits..well i guess if we get this in, it wont break anything since php -l only run on files touched by the patch right?

Let's celebrate this would be the first *added by Drupal core*, ie. not a vendor :) *party*

Quick note that you could actually accomplish what this does with a class...

class StringTranslation {
 /* ... */
}

class StringTranslationMock extends StringTranslation {
  public function testT() {
    /* ... */
  }
}

If you want to get extra fancy, could even throw a StringTranslationInterface in there so that you're not dependent on the implementation. Food for thought. But, I'm likely completely missing the goal of this.

an interface wont work cause t() is protected. and i doubt making it public is a good idea

Why did we ever introduce $this->t() into Drupal 8 to begin with? It's resulted in controllers calling it, but non-controller classes calling Drupal::t() or using some other crazily verbose pattern. Is there any actual use case for swapping out the translation manager used by one object separately from just swapping out the one in the DIC? Given that traits aren't available for D8, I suggest we step back and become more practical, and use the $this->SERVICE() pattern only for services that have a practical reason for being injected, and otherwise, stick to \Drupal::SERVICE().

In D9, when we can use traits, I think the $this->SERVICE() pattern for everything will be great.

I'm pretty sure "BECAUSE UNIT TESTING" but if we can get away with only doing injections on things that have a pragmatic reason to be swappable, that's far, far preferable to me, too.

there is no Drupal::t(). it is either t() or $this->t(). anything else is a bug
Background : #2018411: Figure out a nice DX when working with injected translation

I'd be fine with just t() :)
But I'm also making a pitch to use PHP 5.4 and traits in D8: #1498574-69: [policy, no patch] Require PHP 5.4.

I see two steps?

1. Introduce Drupal::t(), using the DIC's string_translation
2. Keep t() but make it just return Drupal::t($stuff)

Reasoning for above:

1. It keeps BC
2. Still works when t() is not available
3. $this->t() is just a wrapper around Drupal::service('string_translation')->translate() anyway. Let's stop beating around the bush. There is no reason to complicate it more than it needs to be. $this->t() doesn't solve anything.

I'm pretty sure "BECAUSE UNIT TESTING" but if we can get away with only doing injections on things that have a pragmatic reason to be swappable, that's far, far preferable to me, too.

"Because Unit Testing" is actually just a ploy to move things into the class loader space, but don't tell anyone that.

Rob: No, it's a ploy to make using PHPUnit easier/saner.

I'm favorable toward this sort of trait, as it has a nice balance between pragmatism (I just want to be able to call $this->t(), screw wiring up a service) with cleanliness and testability (I can do a true-unit test without much hoop jumping). Even if we don't require PHP 5.4, having a few of these is a good thing (and helps push people to use 5.4 in contrib!)

Shouldn't $translationManager be defined in the trait, though? If not, it becomes a dynamic public variable, which is leaky and uses more memory than a defined property. (I think traits can define properties, no?)

why the \Drupal::t() thing is worse than what we have now:

1st: It requires me to set up a container anytime i want to unit test a class that uses t(). I have to type more things to do this instead of just doing

$translation->setTranslationManager($this->getStringTranslationStub());

2nd and more importanly: There are side effects with other unit tests..you have to kill the container before ending the test to make sure you wont screw other unit tests, that have to use the container too.

No thank you, i would rather we require 5.4

Shouldn't $translationManager be defined in the trait, though? If not, it becomes a dynamic public variable, which is leaky and uses more memory than a defined property. (I think traits can define properties, no?)

You are completely right..lets do this and also get rid one of the testT() methods:P

+++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
index 0000000..87003d3
--- /dev/null

--- /dev/null
+++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTraitMock.php

+++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTraitMock.php
+++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTraitMock.php
@@ -0,0 +1,17 @@

@@ -0,0 +1,17 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Core\StringTranslation\StringTranslationTraitMock.
+ */
+
+namespace Drupal\Core\StringTranslation;
+
+/**
+ * A mock class that uses the StringTranslationTrait for unit tests.
+ */
+class StringTranslationTraitMock {
+
+  use StringTranslationTrait;
+
+}

If this is just for tests place it in some test directory or maybe even better put it into the actual test file.

we cant put it in the same file cause that would cause tests to fail in 5.3.
the same will happen if we move it in tests/ namespace..seems the autoloader picks it up..i just tried it in a 5.3 install, so having it in lib/ namespace is the only viable approach for now.

duh, i forgot to remove the attachments. disregard patch above, like i said it will cause tests to fail under 5.3.
re-uploading #17

Since #2018411: Figure out a nice DX when working with injected translation is fixed, is this the place to remove http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...?

Since #2018411: Figure out a nice DX when working with injected translation is fixed, is this the place to remove http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...?

Not really to be honest. Please open another issue.

Because the impact of this change is not immediately clear, I've expanded it to actually show what core would look like if #1498574: [policy, no patch] Require PHP 5.4 went in.

Not counting the tests:
109 insertions(+), 330 deletions(-)
Not counting the tests or trait:
44 insertions(+), 330 deletions(-)

Dries has made a decision on #1498574: [policy, no patch] Require PHP 5.4, we just need to wait for the testbots now.

Rerolled and cleaned up a little.

As discussed in IRC, services should still receive the string_translation service injected and set it on the trait.

+++ b/core/core.services.yml
@@ -384,7 +384,7 @@ services:
   controller.page:
     class: Drupal\Core\Controller\HtmlPageController
-    arguments: ['@http_kernel', '@controller_resolver', '@title_resolver']
+    arguments: ['@http_kernel', '@controller_resolver', '@string_translation', '@title_resolver']
+++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
@@ -8,6 +8,7 @@
+use Drupal\Core\StringTranslation\TranslationInterface;
+++ b/core/lib/Drupal/Core/Controller/HtmlPageController.php
@@ -47,12 +48,15 @@ class HtmlPageController {
+   * @param \Drupal\Core\StringTranslation\TranslationInterface $translation_manager
+   *   The translation manager.
    */
-  public function __construct(HttpKernelInterface $kernel, ControllerResolverInterface $controller_resolver, TitleResolver $title_resolver) {
+  public function __construct(HttpKernelInterface $kernel, ControllerResolverInterface $controller_resolver, TranslationInterface $translation_manager, TitleResolver $title_resolver) {
+    $this->translationManager = $translation_manager;

Instead of all that, can we instead use setter injection everywhere that needs string_translation injected? It results both in less code in the class, and per #2019055-187: Switch from field-level language fallback to entity-level language fallback, makes subclasses less brittle.

In general, I would recommend only using constructor injection for injecting services that are close collaborators for the domain logic of the class, not for general services like string translation.

I think this would need format plural coverage as well now that that is a service.

time for some testbot action here again :)
the patch was severily outdated, rerolled what i could.

@Gabor: agreed, but it might be good to split this to another issue if it requires many changes..i ll check and open a new issue, if they make this patch really big

Oops

setter should return $this

Looks great from a cursory review, awesomesauce. This is very exciting :-)
Could use another pair of eyes, though.

yes, we need to do formatPlural here too

+++ b/core/lib/Drupal/Core/Datetime/Date.php
@@ -85,7 +88,7 @@ class Date {
-    $this->stringTranslation = $translation;
+    $this->translationManager = $translation;

i actually like stringTranslation better for property name, since thats the name of the service.
translationManager is the name of the class, which if you swap it, it wont be correct anymore

1. +++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
   @@ -0,0 +1,67 @@
   +   *
   +   * See the t() documentation for details.
   +   */
   

   Actually the documentation should be on TranslationINterface::translate, shouldn't it?

2. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/StringTranslationTraitTest.php
   @@ -0,0 +1,40 @@
   +
   +  public static function getInfo() {
   +    return array(
   

   Needs an inheritdoc, because it is people will always look at it.

3. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/StringTranslationTraitTest.php
   @@ -0,0 +1,40 @@
   +    $translation = new StringTranslationTraitMock();
   

   You can also use $this->getObjectForTrait

Fixed #39

#40:
1. Because the t() docs are far richer:)
2. Done
3. Didnt know about that..Awesomeness!

some doc love

I corrected some errors in the code comments and made them a bit easier to understand, mostly by moving around the subordinate clause. I also added PHPUnit annotations to the test, and clarified the test metadata. For approval on the format of that last bit, see @dawehner. The trait itself looks good and is otherwise very clearly documented.

+++ b/core/tests/Drupal/Tests/Core/StringTranslation/StringTranslationTraitTest.php
@@ -39,8 +40,7 @@ class StringTranslationTraitTest extends UnitTestCase {
-      'description' => 'Tests the string translation trait.',

I think description is required.
https://api.drupal.org/api/drupal/core%21tests%21Drupal%21Tests%21UnitTe...

Probably an accidental delete of that line.

'scuse me.

thanks for the annotations

+++ b/core/tests/Drupal/Tests/Core/StringTranslation/StringTranslationTraitTest.php
@@ -39,8 +40,7 @@ class StringTranslationTraitTest extends UnitTestCase {
-      'name' => 'String translation trait',
-      'description' => 'Tests the string translation trait.',
+      'name' => '\Drupal\Core\StringTranslation\StringTranslationTrait unit test',

this change is just wrong:)

Why we need the full namespace in the name that appears in the simpletest UI?
Also the description should not be empty.
Please revert, thanks

renamed the trait to StringTranslationAwareTrait and fixed #47
sorry i messed up interdiff

+++ b/core/core.services.yml
@@ -421,7 +421,7 @@ services:
   controller.page:
     class: Drupal\Core\Controller\HtmlPageController
-    arguments: ['@controller_resolver', '@string_translation', '@title_resolver']
+    arguments: ['@controller_resolver', '@title_resolver']
   controller.ajax:
     class: Drupal\Core\Controller\AjaxController
     arguments: ['@controller_resolver', '@ajax_response_renderer']
@@ -549,7 +549,7 @@ services:

@@ -549,7 +549,7 @@ services:
     arguments: ['@config.manager', '@config.storage', '@config.storage.snapshot']
   exception_controller:
     class: Drupal\Core\Controller\ExceptionController
-    arguments: ['@content_negotiation', '@string_translation', '@title_resolver', '@html_page_renderer', '@html_fragment_renderer']
+    arguments: ['@content_negotiation', '@title_resolver', '@html_page_renderer', '@html_fragment_renderer']
     calls:
       - [setContainer, ['@service_container']]

Maybe it is just me but I would still love to have proper constructor based DI for this really important services.

those services dont need the translation..they are just dead properties:)

Actually, both of those services were only using it because the base class (HtmlControllerBase) had it in the constructor.
But neither that class, nor these services, actually use it.
You'll see no trait is added there, it's just an unrelated cleanup.

But yes, I would generally like to avoid using traits for our true services, and stick to DI. That's more for the "trait strategy" issue though.

Well, at least the exception controller will need a way to translate things in the future, as it adds stuff. (just saying)

There's an issue
#2206175: Document traits that use methods on interfaces
which is proposing a standard for how to document Traits.

The suggestions there (not official standards yet but probably a good idea):

a) Add a @see in the main Trait doc block referencing the interface whose methods it is implementing.

b) Document each method as:

/**
* Implements \Full\Namespaced\Interface\Name::methodName().
*/
function methodName() { }

the trait in this issue does not implement any interface:)

Ah, that's interesting. Let's discuss that possibility on the "how to document" issue.

Well #49 and #51 is discussed in #2134513: [policy, no patch] Trait strategy. Doc standard is discussed in #2206175: Document traits that use methods on interfaces. Anything left other then RTBC because I haven't found anything related to doc or coding standard.

The documentation of this trait needs a bit of work:

+/**
+ * Wrapper methods for \Drupal\Core\StringTranslation\TranslationInterface.
+ *
+ * Injected translation can be performed by using a protected method ::t(), so
+ * string extractor tools can find all translatable strings. This method must wrap
+ * \Drupal\Core\StringTranslation\TranslationInterface::translate().
+ * This trait provides this method in a re-usable way.
+ * Procedural code must use the global function t(). Any other approach will
+ * result in untranslatable strings, because the string extractor will not be
+ * able to find them.
+ */
+trait StringTranslationAwareTrait {

I'm not sure if this doc block was meant to be all one paragraph or multiple paragraphs? If it's all one, please wrap as a single paragraph. If it's two, leave blank lines between them.

I also don't really think this documentation makes a lot of sense. What is it really trying to say?

I also really really don't like the name of this trait "StringTranslationAwareTrait" ?!?? It seems to imply that it's providing "translation awareness", but isn't it really providing the translation service as a whole to classes that use the trait? Why was it renamed in #48 with no discussion as far as I can tell?

It seems to imply that it's providing "translation awareness", but isn't it really providing the translation service as a whole to classes that use the trait?

Yes, the class that uses this trait, is aware of the string translation service. The trait doesn't provide the service, but it provides a setter method, a property to store it (Like ContainerAware) and wrapper methods for the service's methods. If the class is not aware of the string translation it will blow up. (also inspired by the LoggerAwareTrait in vendor/psr)

I am open to any renames though.

"Aware" is not something I'd put on a trait, probably. Remember, this is a trait we're expecting people to use... a lot.

StringTranslationTrait or even just TranslationTrait seems fine to me.

i disagree. The "Aware" in the name informs me what this trait does pretty much. Without it i have to check the source code.
It also avoids potential name collisions with potential trait we want to add in the future for TranslationInterface implementations. (eg check LoggerTrait vs LoggerAwareTrait in Psr/Log)

But yes, maybe "StringTranslationAwareTrait" is too verbose.."TranslationAwareTrait" probably would be the best.
And when using it, it makes perfect sense:

class MyClass {
  use TranslationAwareTrait;
}

Which means that MyClass is aware of the translation service and can translate strings using it.
Exactly like you would use LoggerAwareTrait means that your class is aware of the logger service and can log stuff using it.

What does "being aware of the translation service" mean? If I have a class, I want to translate text, not be aware of a service. I also think "LoggerAwareTrait" is not a good name. Maybe both of them should be called SomethingServiceTrait, meaning that the trait provides access to the service?

LoggerAwareTrait from PSR-3 doesn't auto-wrap a service locator. It wraps a property and implements a standard method for setter-injection. In that sense it's the same as Symfony's ContainerAwareInterface and base class.

What we're talking about here isn't primarily a standardized setter injection method; it's a service locator wrapper. That it also includes a setter method for testability is incidental.

ok. i don't want to hold up the issue. lets go with the simplest approach here, and we can revisit if needed:)

Let's please stick to StringTranslation, though. There are various translation concepts in Drupal core right now (content translation, config translation, ...), so let's not be ambiguous here.

ok rerolled and renamed.
Also removed the newlines between the class declaration and trait's use statement to match what we do already in core.
No interdiff, i messed it up.

I hope the exception of #63 goes away cause i couldnt reproduce locally

I love this patch let's get this in.

+++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
@@ -0,0 +1,77 @@
+ * string extractor tools can find all translatable strings. This method must wrap

more then 80 chars.

oops indeed! also wrong namespace for the formatPlural method

reroll.

forgot interdiff

reroll...

oops messed up the arguments when merging

This is all very ugly. But if you really want to go that route, please make all those methods private, not protected.

Those are strictly helper functions (and as such, they should really not even be on the class to begin with...), they are clearly not part of the API that the class provides to its subclasses.

@Damien Tournoud, so when someone subclasses FormBase, itself an abstract class, then no actual form would be able to use $this->t()? Or they themselves would have to use the trait, lessening the usefulness of the base class.
+1 for leaving it as protected, it's a helper, let's be helpful.

so when someone subclasses FormBase, itself an abstract class, then no actual form would be able to use $this->t()? Or they themselves would have to use the trait,

Yes?! As far as I'm concerned that's the whole point of this issue. If your a form/controller/... and you need string translation -> use StringTranslationTrait;. I don't see how that lessens the usefulness of the base class. If the whole purpose of the base class is currently to aggregate various services into utility methods (basically ControllerBase/FormBase/PluginBase now) but there is no domain-specific "base-logic" anyway, then once we have converted all utility services to traits then that base class has no right to existance anyway. If that's what you mean by "lessening the usefulness", then, yes, we *should* lessen the "usefulness" of such classes.

So, we would have to use the trait on all FormBase, ConfirmFormBase and MyConfirmFormBase?
Then i dont see the usefullness of this issue..i would rather have a protected t() method in FormBase and reuse that instead.

Well instead you would simply use the StringTranslationTrait on your own form, i.e. MyEntityDeleteForm, MyModuleConfigForm, etc. That way forms that don't need string translation won't be tainted by the methods there.

What form would not need string translation?! How is that possible?
Also, "won't be tainted by the methods there" is a pretty funny thing to say about a form/controller. These are not services.

This is a long time coming, let's just finish it please.

I would rather see us dismantle the base classes in favor of letting classes piecemeal select the traits they care about. It's much more explicit that way about what their dependencies are. That said, that can be done in follow-ups and I don't see a need to make the methods private for that. I agree that would be self-defeating. So +1 on the RTBC for now.

Yeah I agree with Tim in #84. It really does not bother me if forms/controllers have translation built in.

On the other hand we have real dependency issues with plugin/discovery and translation that could do with more eyes on them #2244447: Translation of low-level info/annotations leads to circular dependencies.

Patch needs a re-roll though unfortunately.

What form would not need string translation?! How is that possible?

Those that require a form POST redirect to a remote server. This is a common method that payment service providers use.

Not that this is a good reason for not adding t() to FormBase, or anything... ;-)

reroll

back

Given that $this->t() is mandatory we should provide a Trait -- core won't use it but having it documented in the codebase is better than burying this in a change notice.

Hmmm... methinks we need an issue summary update :)

done.
for change record I will update https://drupal.org/node/2079611 once its in

Committed b0821d8 and pushed to 8.x. Thanks!

The change record update mentioned in #92 needs to take place, right?

ParisLiakos -- After you are done with that, please restore priority, status, and tags.

Its already done:)
https://drupal.org/node/2079611/revisions/view/7183121/7202909

Oh sorry, thanks! I looked but obviously not carefully enough.