Problem/Motivation

Currently whenever we need to use string translation in class context, the class needs to have a t() method and call $this->t() otherwise our string extractor won't be able to find the strings (unless of course you use the global/procedural t() function, but that makes your code untestable)

Proposed resolution

Instead of requiring to write a t() method over and over again whenever you need the translation and your base class does not provide it, use a trait!

Remaining tasks

  1. Write the trait
  2. Review/RTBC
  3. commit

User interface changes

None

API changes

New trait available: Drupal\Core\StringTranslation\StringTranslationTrait

Original report by @chx

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.

CommentFileSizeAuthor
#88 trait-t-2079797-88.patch87.33 KBParisLiakos
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,323 pass(es). View
#78 trait-t-2079797-77.patch87.36 KBParisLiakos
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,330 pass(es). View
#78 interdiff-t.txt1.07 KBParisLiakos
#76 trait-t-2079797-75.patch87.39 KBParisLiakos
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#75 interdiff-t.txt736 bytesParisLiakos
#75 trait-t-2079797-74.patch87.51 KBParisLiakos
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,065 pass(es). View
#73 interdiff-t.txt2.26 KBParisLiakos
#72 trait-t-2079797-72.patch86.79 KBParisLiakos
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#70 trait-t-2079797-70.patch85.5 KBParisLiakos
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#69 interdiff.txt1.48 KBParisLiakos
#69 trait-t-2079797-69.patch77.45 KBParisLiakos
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,815 pass(es). View
#66 trait-t-2079797-66.patch77.41 KBParisLiakos
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,804 pass(es). View
#63 interdiff.txt21.14 KBParisLiakos
#63 drupal_2079797_63.patch71.51 KBParisLiakos
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,401 pass(es), 0 fail(s), and 1 exception(s). View
#48 drupal_2079797_48.patch70.45 KBParisLiakos
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,702 pass(es). View
#46 interdiff.txt607 bytesXano
#46 drupal_2079797_46.patch70.05 KBXano
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,714 pass(es). View
#43 interdiff.txt2.92 KBXano
#43 drupal_2079797_43.patch70.02 KBXano
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,720 pass(es), 0 fail(s), and 15 exception(s). View
#42 interdiff-t.txt1.89 KBParisLiakos
#42 trait-t-2079797-42.patch70.01 KBParisLiakos
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,720 pass(es). View
#41 interdiff-t.txt23.81 KBParisLiakos
#41 trait-t-2079797-41.patch69.59 KBParisLiakos
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,720 pass(es). View
#38 interdiff-t.txt19.79 KBParisLiakos
#38 trait-t-2079797-38.patch41.97 KBParisLiakos
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,712 pass(es). View
#35 interdiff-t.txt1.27 KBParisLiakos
#35 trait-t-2079797-36.patch22.71 KBParisLiakos
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,641 pass(es), 68 fail(s), and 22 exception(s). View
#33 trait-t-2079797-34.patch22.33 KBParisLiakos
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#33 interdiff-t.txt605 bytesParisLiakos
#31 trait-t-2079797-32.patch22.38 KBParisLiakos
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#28 trait-t-2079797-28-do-not-test.patch29.15 KBamateescu
#28 interdiff.txt8.07 KBamateescu
#27 trait-t-2079797-27-do-not-test.patch34.65 KBamateescu
#27 interdiff.txt3.22 KBamateescu
#23 trait-t-2079797-23-do-not-test.patch33.71 KBtim.plunkett
#20 t-trait-20-do-not-test.patch4.31 KBParisLiakos
#19 t-trait-19-do-not-test.patch4.35 KBParisLiakos
#19 interdiff.txt2.5 KBParisLiakos
#17 t-trait-17-do-not-test.patch4.31 KBParisLiakos
#17 interdiff.txt2.17 KBParisLiakos
#5 t-trait-5-do-not-test.patch4.19 KBParisLiakos
#5 interdiff.txt1.32 KBParisLiakos
#3 t-trait.patch4.19 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-base

Tagging up.

dawehner’s picture

+1

We could think about other onces as well.

ParisLiakos’s picture

Status: Active » Needs review
FileSize
4.19 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php. View

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

Status: Needs review » Needs work

The last submitted patch, t-trait.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
4.19 KB

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?

chx’s picture

Aye, there's core/vendor/psr/log/Psr/Log/LoggerTrait.php, core/vendor/psr/log/Psr/Log/LoggerAwareTrait.php, and a couple tests.

Gábor Hojtsy’s picture

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

RobLoach’s picture

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.

chx’s picture

PHP is single hierarcy so we can't use one class just for a few utility methods. An interface for t() might be workable, true.

Edit: however the implementation won't vary from class to class so a Trait is very helpful too.

ParisLiakos’s picture

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

effulgentsia’s picture

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.

webchick’s picture

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.

ParisLiakos’s picture

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

effulgentsia’s picture

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.

RobLoach’s picture

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.

Crell’s picture

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

ParisLiakos’s picture

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

dawehner’s picture

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

ParisLiakos’s picture

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.

ParisLiakos’s picture

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

jhedstrom’s picture

dawehner’s picture

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.

tim.plunkett’s picture

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

tim.plunkett’s picture

Issue summary: View changes

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

tim.plunkett’s picture

Xano’s picture

amateescu’s picture

FileSize
3.22 KB
34.65 KB

Rerolled and cleaned up a little.

amateescu’s picture

FileSize
8.07 KB
29.15 KB

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

effulgentsia’s picture

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

Gábor Hojtsy’s picture

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

ParisLiakos’s picture

FileSize
22.38 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

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

Status: Needs review » Needs work

The last submitted patch, 31: trait-t-2079797-32.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
605 bytes
22.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Oops

Status: Needs review » Needs work

The last submitted patch, 33: trait-t-2079797-34.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
22.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,641 pass(es), 68 fail(s), and 22 exception(s). View
1.27 KB

setter should return $this

tstoeckler’s picture

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

Status: Needs review » Needs work

The last submitted patch, 35: trait-t-2079797-36.patch, failed testing.

ParisLiakos’s picture

Title: Provide a trait for $this->t() » Provide a trait for $this->t() and $this->formatPlural()
Status: Needs work » Needs review
FileSize
41.97 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,712 pass(es). View
19.79 KB

yes, we need to do formatPlural here too

ParisLiakos’s picture

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

dawehner’s picture

  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

ParisLiakos’s picture

FileSize
69.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,720 pass(es). View
23.81 KB

Fixed #39

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

ParisLiakos’s picture

FileSize
70.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,720 pass(es). View
1.89 KB

some doc love

Xano’s picture

FileSize
70.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,720 pass(es), 0 fail(s), and 15 exception(s). View
2.92 KB

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.

Status: Needs review » Needs work

The last submitted patch, 43: drupal_2079797_43.patch, failed testing.

YesCT’s picture

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

Xano’s picture

Status: Needs work » Needs review
FileSize
70.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,714 pass(es). View
607 bytes

'scuse me.

ParisLiakos’s picture

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

ParisLiakos’s picture

Assigned: ParisLiakos » Unassigned
FileSize
70.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,702 pass(es). View

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

dawehner’s picture

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

ParisLiakos’s picture

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

tim.plunkett’s picture

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.

dawehner’s picture

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

jhodgdon’s picture

There's an issue
#2206175: [policy, no patch] Figure out how to document trait methods
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() { }
ParisLiakos’s picture

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

jhodgdon’s picture

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

jibran’s picture

Well #49 and #51 is discussed in #2134513: [policy, no patch] Trait strategy. Doc standard is discussed in #2206175: [policy, no patch] Figure out how to document trait methods. Anything left other then RTBC because I haven't found anything related to doc or coding standard.

jhodgdon’s picture

Status: Needs review » Needs work

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?

ParisLiakos’s picture

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.

Crell’s picture

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

ParisLiakos’s picture

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.

jhodgdon’s picture

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?

Crell’s picture

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.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
71.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,401 pass(es), 0 fail(s), and 1 exception(s). View
21.14 KB

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

Status: Needs review » Needs work

The last submitted patch, 63: drupal_2079797_63.patch, failed testing.

tstoeckler’s picture

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.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
77.41 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,804 pass(es). View

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

ParisLiakos’s picture

jibran’s picture

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.

ParisLiakos’s picture

FileSize
77.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,815 pass(es). View
1.48 KB

oops indeed! also wrong namespace for the formatPlural method

ParisLiakos’s picture

FileSize
85.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

reroll.

Status: Needs review » Needs work

The last submitted patch, 70: trait-t-2079797-70.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
86.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
ParisLiakos’s picture

FileSize
2.26 KB

forgot interdiff

Status: Needs review » Needs work

The last submitted patch, 72: trait-t-2079797-72.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
87.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,065 pass(es). View
736 bytes
ParisLiakos’s picture

FileSize
87.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

reroll...

Status: Needs review » Needs work

The last submitted patch, 76: trait-t-2079797-75.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.07 KB
87.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,330 pass(es). View

oops messed up the arguments when merging

Damien Tournoud’s picture

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.

tim.plunkett’s picture

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

tstoeckler’s picture

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.

ParisLiakos’s picture

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.

tstoeckler’s picture

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.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

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.

Crell’s picture

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.

catch’s picture

Status: Reviewed & tested by the community » Needs work

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.

Xano’s picture

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

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
87.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,323 pass(es). View

reroll

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

back

alexpott’s picture

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

ParisLiakos’s picture

Issue summary: View changes
ParisLiakos’s picture

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

ParisLiakos’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b0821d8 and pushed to 8.x. Thanks!

  • Commit b0821d8 on 8.x by alexpott:
    Issue #2079797 by ParisLiakos, Xano, amateescu, tim.plunkett: Provide a...
jhodgdon’s picture

Title: Provide a trait for $this->t() and $this->formatPlural() » [Change record] Provide a trait for $this->t() and $this->formatPlural()
Priority: Normal » Critical
Status: Fixed » Needs work
Issue tags: +Needs change record updates, +beta blocker

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.

ParisLiakos’s picture

Title: [Change record] Provide a trait for $this->t() and $this->formatPlural() » Provide a trait for $this->t() and $this->formatPlural()
Priority: Critical » Normal
Status: Needs work » Fixed
Issue tags: -Needs change record updates, -beta blocker
jhodgdon’s picture

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

YesCT’s picture

Issue tags: +Traits

Status: Fixed » Closed (fixed)

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