Problem/Motivation

Now that TranslationManager::translate() no longer translates, but rather returns
a TranslationWrapper we could simplify the call chain of t() and $this->t()
quite a lot.

Proposed resolution

convert t() and $this->t() to return a TranslationWrapper

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#68 2600118-68.patch13.79 KBstefan.r
#68 interdiff.txt642 bytesstefan.r
#66 2600118-66.patch13.74 KBstefan.r
#66 interdiff.txt670 bytesstefan.r
#65 2600118-65.patch13.93 KBstefan.r
#65 interdiff.txt1.06 KBstefan.r
#64 2600118-64.patch13.64 KBandypost
#57 interdiff.txt0 bytesdawehner
#57 2600118-57.patch13.96 KBdawehner
#55 interdiff.txt3.29 KBdawehner
#55 2600118-55.patch12.26 KBdawehner
#36 2600118-36.patch9.28 KBanil280988
#34 2600118-34.patch9.82 KBanil280988
#32 interdiff-2600118-28-32.txt8.16 KBaerozeppelin
#32 2600118-32.patch11.29 KBaerozeppelin
#29 2600118-29.patch8.69 KBanil280988
#28 2600118-28.patch12.43 KBaerozeppelin
#28 interdiff-2600118-13-28.txt1.48 KBaerozeppelin
#22 2600118-22.patch7.28 KBanil280988
#16 interdiff-2600118-5-13.txt9.94 KBaerozeppelin
#13 2600118-13.patch11.25 KBaerozeppelin
#13 interdiff-2600118-5-13.patch9.94 KBaerozeppelin
#7 drupal-TranslationWrapper-issue-2600118-5-8.patch602 bytesramirez.gerardo
#2 2600118-2.patch1.27 KBsdstyles
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

sdstyles’s picture

Status: Active » Needs review
FileSize
1.27 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2600118-2.patch, failed testing.

dawehner’s picture

+++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
@@ -43,7 +43,7 @@
   protected function t($string, array $args = array(), array $options = array()) {
-    return $this->getStringTranslation()->translate($string, $args, $options);
+    return new TranslatableMarkup($string, $args, $options);
   }

This should ideally set the translation translation manager on the newly created object, something like $translatable_markup = new TranslatableMarkup($string, $args, $options, $this->getStringTranslation())

sdstyles’s picture

Status: Needs work » Needs review
FileSize
1.3 KB
582 bytes

@dawehner thanks for the hint

Status: Needs review » Needs work

The last submitted patch, 5: 2600118-5.patch, failed testing.

ramirez.gerardo’s picture

Per @dawehner comments. Patch should reflect code provided in comment 4.

dawehner’s picture

#5 is IMHO the right patch but it maybe needs some proper fixes inside those tests ...

anil280988’s picture

Status: Needs work » Needs review

Hi Dawehner,
Could you drop some hint regarding this.

Status: Needs review » Needs work

The last submitted patch, 7: drupal-TranslationWrapper-issue-2600118-5-8.patch, failed testing.

The last submitted patch, 2: 2600118-2.patch, failed testing.

dawehner’s picture

So a) certainly start with #5, #7 is wrong.

Let's have a look at one of those failure:

\Drupal\Tests\Core\Controller\TitleResolverTest::testStaticTitle

there you see the following code:

    $this->translationManager->expects($this->once())
      ->method('translate')
      ->with('static title', array(), array())
      ->will($this->returnValue('translated title'));

    $this->assertEquals('translated title', $this->titleResolver->getTitle($request, $route));

instead use something like:

    $this->assertEquals(new TranslatableMarkup('static title'), $this->titleResolver->getTitle($request, $route)));

I kind of believe that a similar approach can be applied to all of those instances.

aerozeppelin’s picture

Based on comment #12. Here are the some of the changes.

aerozeppelin’s picture

Status: Needs work » Needs review
aerozeppelin’s picture

aerozeppelin’s picture

FileSize
9.94 KB

The last submitted patch, 7: drupal-TranslationWrapper-issue-2600118-5-8.patch, failed testing.

The last submitted patch, 2: 2600118-2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 13: 2600118-13.patch, failed testing.

The last submitted patch, 13: interdiff-2600118-5-13.patch, failed testing.

dawehner’s picture

Thank you for trying to get the patch green!

Some points

a) We don't need to change / convert $this->t() to new TranslationWrapper
b) In order to fix the tests we need to change two things:

1) \Drupal\Tests\Core\Controller\TitleResolverTest::testStaticTitleWithParameter should no longer expects call to $this->translationManager->translate ... so just remove that blob
2) The expected values in \Drupal\Tests\Core\Controller\TitleResolverTest::providerTestStaticTitleWithParameter are now TranslationWrapper objects, so set them up like:

      array('static title @test', new TranslatableMarkup('static title @test', ['@test' => 'value', '%test' => 'value', '@test2' => 'value2', '%test2' => 'value2'])),

3) We can remove the 'static title !test' testcase in that function, its no longer valid.

anil280988’s picture

Created the patch

anil280988’s picture

Status: Needs work » Needs review

Changing the status

The last submitted patch, 2: 2600118-2.patch, failed testing.

The last submitted patch, 7: drupal-TranslationWrapper-issue-2600118-5-8.patch, failed testing.

The last submitted patch, 13: 2600118-13.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 22: 2600118-22.patch, failed testing.

aerozeppelin’s picture

Status: Needs work » Needs review
FileSize
1.48 KB
12.43 KB

Thanks @dawehner for the hints.
In reference to #21, made changes to Drupal\Tests\Core\Controller\TitleResolverTest::testStaticTitleWithParameter. Point 'a' needs additional work.

anil280988’s picture

For the 'a' point , as stated by @dawehner , $this->t() need not be changed to new TranslationWrapper. So changing those line back.

Status: Needs review » Needs work

The last submitted patch, 29: 2600118-29.patch, failed testing.

dawehner’s picture

@anil280988
Did you tried to create a patch file by hand?

aerozeppelin’s picture

Status: Needs work » Needs review
FileSize
11.29 KB
8.16 KB

With reference to #21. Made some changes to accomplish this.

We don't need to change / convert $this->t() to new TranslationWrapper

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Datetime/DateFormatter.php
    @@ -14,6 +14,7 @@
     use Drupal\Core\StringTranslation\TranslationInterface;
     use Drupal\Core\StringTranslation\StringTranslationTrait;
     use Symfony\Component\HttpFoundation\RequestStack;
    +use Drupal\Core\StringTranslation\TranslatableMarkup;
     
     /**
      * Provides a service to handle various date related functionality.
    @@ -209,7 +210,7 @@ public function formatDiff($from, $to, $options = array()) {
    
    @@ -209,7 +210,7 @@ public function formatDiff($from, $to, $options = array()) {
    -      return $this->t('0 seconds');
    +      return $this->t('0 seconds', array(), array());
    
    @@ -289,7 +290,7 @@ public function formatDiff($from, $to, $options = array()) {
    -      $output = $this->t('0 seconds');
    +      $output = $this->t('0 seconds', array(), array());
    

    None of these changes are needed, these php arrays are optional, see StringTranslationTrait::t()

  2. +++ b/core/modules/breakpoint/src/Breakpoint.php
    @@ -8,6 +8,7 @@
     
     use Drupal\Core\Plugin\PluginBase;
    +use Drupal\Core\StringTranslation\TranslatableMarkup;
     
    

    Unneeded

anil280988’s picture

Rolled back the changes listed @dawehner.

dawehner’s picture

+++ b/core/includes/bootstrap.inc
diff --git a/core/lib/Drupal/Core/Controller/TitleResolver.php b/core/lib/Drupal/Core/Controller/TitleResolver.php
index 9adff8c..611fe24 100644

index 9adff8c..611fe24 100644
--- a/core/lib/Drupal/Core/Controller/TitleResolver.php

--- a/core/lib/Drupal/Core/Controller/TitleResolver.php
+++ b/core/lib/Drupal/Core/Controller/TitleResolver.php

+++ b/core/lib/Drupal/Core/Controller/TitleResolver.php
+++ b/core/lib/Drupal/Core/Controller/TitleResolver.php
@@ -11,6 +11,7 @@

@@ -11,6 +11,7 @@
 use Drupal\Core\StringTranslation\TranslationInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\Routing\Route;
+use Drupal\Core\StringTranslation\TranslatableMarkup;
 
 /**
  * Provides the default implementation of the title resolver interface.

this is now also not needed anymore

anil280988’s picture

Done Changes.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

The last submitted patch, 2: 2600118-2.patch, failed testing.

The last submitted patch, 7: drupal-TranslationWrapper-issue-2600118-5-8.patch, failed testing.

The last submitted patch, 29: 2600118-29.patch, failed testing.

The last submitted patch, 13: 2600118-13.patch, failed testing.

The last submitted patch, 22: 2600118-22.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 36: 2600118-36.patch, failed testing.

The last submitted patch, 36: 2600118-36.patch, failed testing.

snehi’s picture

Status: Needs work » Reviewed & tested by the community

@Anil thanks for the patch. Making it again to RTBC as it is working fine for me. Don't know what happened to testbot.

flyman@flyman-Vostro-3446:/var/www/d8-snehi$ git apply -v 2600118-36.patch
Checking patch core/includes/bootstrap.inc...
Checking patch core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php...
Checking patch core/modules/breakpoint/tests/src/Unit/BreakpointTest.php...
Checking patch core/tests/Drupal/Tests/Core/Controller/TitleResolverTest.php...
Checking patch core/tests/Drupal/Tests/Core/Datetime/DateTest.php...
Checking patch core/tests/Drupal/Tests/Core/Entity/EntityTypeTest.php...
Applied patch core/includes/bootstrap.inc cleanly.
Applied patch core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php cleanly.
Applied patch core/modules/breakpoint/tests/src/Unit/BreakpointTest.php cleanly.
Applied patch core/tests/Drupal/Tests/Core/Controller/TitleResolverTest.php cleanly.
Applied patch core/tests/Drupal/Tests/Core/Datetime/DateTest.php cleanly.
Applied patch core/tests/Drupal/Tests/Core/Entity/EntityTypeTest.php cleanly.

That is the output for Applying patch.

The last submitted patch, 2: 2600118-2.patch, failed testing.

The last submitted patch, 7: drupal-TranslationWrapper-issue-2600118-5-8.patch, failed testing.

The last submitted patch, 29: 2600118-29.patch, failed testing.

The last submitted patch, 13: 2600118-13.patch, failed testing.

The last submitted patch, 22: 2600118-22.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Let's do format_plural and formatPlural whilst we are at it to keep things consistent.

priya.chat’s picture

Assigned: Unassigned » priya.chat

@Alexpott, your suggestion/ comment is not clear to me. can you please explain a bit more.

dawehner’s picture

If you look at \Drupal\Core\StringTranslation\StringTranslationTrait::formatPlural you'll see that it calls to \Drupal\Core\StringTranslation\TranslationInterface::formatPlural which is just returning a new object, exactly the same as t() does on that class.

dawehner’s picture

Assigned: priya.chat » Unassigned

Unassigning and keeping the novice task to see whether someone else might pick up the issue.

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.26 KB
3.29 KB

format_plural()

this function no longer exists.

Fixed that and also adapted the test coverage to be a bit better.

Status: Needs review » Needs work

The last submitted patch, 55: 2600118-55.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.96 KB
0 bytes

There we go.

dawehner’s picture

Is this still valuable for 8.0.x as well, I would say so.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

If the bot's OK with it, this is RTBC. If the bot's not still OK with it, it can set it back.

catch’s picture

Version: 8.0.x-dev » 8.1.x-dev

Don't see what we get from this in 8.0.x, and it could potentially cause contrib test failures. But looks good to me for 8.1.x - moving there for the bot.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 57: 2600118-57.patch, failed testing.

dawehner’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Needs work » Reviewed & tested by the community

Don't see what we get from this in 8.0.x, and it could potentially cause contrib test failures. But looks good to me for 8.1.x - moving there for the bot.

Well, things are easier to understand. Previously its not obviously that t() is just a wrapper for the TranslatableMarkup object, which is kinda the point of the issue.

catch’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Needs work

Well looks like it needs a re-roll for 8.1.x at least.

andypost’s picture

re-roll against 8.1.x

+++ b/core/tests/Drupal/Tests/Core/Datetime/DateTest.php
@@ -279,6 +269,7 @@ public function providerTestFormatDiff() {
+    $string_translation = $this->getMock('Drupal\Core\StringTranslation\TranslationInterface');

removed in re-roll cos unused

stefan.r’s picture

removed in re-roll cos unused

Nice spot!

+++ b/core/tests/Drupal/Tests/Core/Datetime/DateTest.php
@@ -267,21 +261,18 @@ public function testformatDiff($expected, $max_age, $timestamp1, $timestamp2, $o
     // Mocks a simple formatPlural implementation.

Wrong comment got removed during that re-roll :)

stefan.r’s picture

andypost’s picture

+++ b/core/tests/Drupal/Tests/Core/Datetime/DateTest.php
@@ -264,24 +258,20 @@ public function testFormatTimeDiffSince() {
     // Mocks a simple translate implementation.
...
+      ->method('translateString')

s/translate/translateString

stefan.r’s picture

andypost’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice

  • catch committed deba295 on 8.1.x
    Issue #2600118 by aerozeppelin, anil280988, stefan.r, dawehner, andypost...
catch’s picture

Version: 8.1.x-dev » 8.0.x-dev

Committed/pushed to 8.1.x, thanks!

Leaving RTBC against 8.0.x.

Well, things are easier to understand. Previously its not obviously that t() is just a wrapper for the TranslatableMarkup object, which is kinda the point of the issue.

Again I struggle with this when there's just two months until 8.0.x EOL.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I agree with @catch here - it's not fixing a bug and the disruption to 8.0.x contrib does not seem worth it. Given that credit for working on issues is not given whilst it remains open I'm going to mark the issue as fixed. Also t() documents that it returns a TranslatableMarkup object so I'm not that convinced that the win is big enough for 8.0.x.

dawehner’s picture

Fair, I always forget that time moves on.

tstoeckler’s picture

Version: 8.0.x-dev » 8.1.x-dev

Status: Fixed » Closed (fixed)

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