Problem/Motivation

Follow-up to #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list. Now that this is done TranslationWrapper does not make much sense. Since it is no longer wrapping t() - t() is wrapping it :).

Proposed resolution

We should create a new TranslatedString class and deprecate TranslationWrapper for Drupal 9.

Remaining tasks

Review & commit patch

User interface changes

None

API changes

API addition

Data model changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because just replacing a class and deprecating another one.
Issue priority Major because followup to a critical which changes the meaning of the TranslationWrapper class
Unfrozen changes Unfrozen because it is a followup to a critical was is only separated to keep scope sane.
Disruption Not disruptive TranslationWrapper will extend TranslatedString and it will be deprecated.
CommentFileSizeAuthor
#28 interdiff.txt6.15 KBmr.baileys
#28 2569069-28-rename-translation-wrapper.patch84.4 KBmr.baileys
#24 2569069-24-rename-translation-wrapper.patch83.79 KBmr.baileys
#22 2569069-22-rename-translation-wrapper.patch83.71 KBmr.baileys
#6 2569069-5.patch73.98 KBstefan.r
#5 interdiff-4-5.txt2.14 KBstefan.r
#4 translatedstring.patch72.26 KBstefan.r
#4 translatedstring.patch72.26 KBstefan.r
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Title: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list » Replace TranslationWrapper with TranslatedString and deprecate TranslationWrapper
Priority: Critical » Major
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue summary: View changes
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Status: Postponed » Needs review
Issue tags: -Security improvements, -D8MI, -language-ui, -sprint, -D8 Accelerate, -Needs tests
FileSize
translatedstring.patch72.26 KB
translatedstring.patch72.26 KB
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
FileSize
interdiff-4-5.txt2.14 KB
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
FileSize
2569069-5.patch73.98 KB
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

CR: https://www.drupal.org/node/2571255

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Issue summary: View changes

The last submitted patch, 4: translatedstring.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: 2569069-5.patch, failed testing.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

Cannot reproduce this failure locally :/

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Status: Needs work » Needs review
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented

Hmm, has there been discussion somewhere about the name TranslatedString or should we include that in this issue?

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

@lauriii I had added it in the parent after @dawehner or @alexpott's suggestion and @joelpittet +1ed, but then took it out so it could be its own issue.

TranslatedString or TranslatableString works for me, considering we already have SafeString as well and we may have more *String classes later on

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented

But isn't that more like a TranslatableString because on string will be only translated when its actually being printed? So its not always necessarily translated.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

Yes, if I had to pick between the two I'd go with that as well.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Well it always translated when you print it but I guess TranslatableString is more correct so let's do that.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Status: Needs review » Needs work

Yeah TranslatedString is super confusing when creating new instance

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Title: Replace TranslationWrapper with TranslatedString and deprecate TranslationWrapper » Replace TranslationWrapper with TranslatableString and deprecate TranslationWrapper
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Let's also fix the param name on TranslationInterface::translateString()

/**
   * Translates a TranslationWrapper object to a string.
   *
   * @param \Drupal\Core\StringTranslation\TranslationWrapper $translated_string
   *   A TranslationWrapper object.
   *
   * @return string
   *   The translated string.
   */
  public function translateString(TranslationWrapper $translated_string);

The param should be $translatable_string

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at Calibrate commented
Assigned: Unassigned » mr.baileys

Working on this.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at Calibrate commented
Assigned: mr.baileys » Unassigned
Status: Needs work » Needs review
FileSize
2569069-22-rename-translation-wrapper.patch83.71 KB

* Now using TranslatableString instead of TranslatedString
* No interdiff since that doesn't make sense in this case
* Also manually renamed TranslationWrapper to TranslatableString in "core/modules/system/tests/fixtures/update/drupal-8.language-enabled.php" (both class names have the same string lenght and are essentially the same object, so I'm assuming this is won't screw up the serialized data).

Status: Needs review » Needs work

The last submitted patch, 22: 2569069-22-rename-translation-wrapper.patch, failed testing.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at Calibrate commented
Status: Needs work » Needs review
FileSize
2569069-24-rename-translation-wrapper.patch83.79 KB

Patch didn't apply since core/modules/system/tests/fixtures/update/drupal-8.language-enabled.php is a binary file, and the patch was created without the --binary flag.

The last submitted patch, 22: 2569069-22-rename-translation-wrapper.patch, failed testing.

cpj’s picture

cpj CreditAttribution: cpj commented
Status: Needs review » Reviewed & tested by the community

I've reviewed the changes and it looks fine to me. So I'm going to mark it RTBC

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableString.php
@@ -0,0 +1,183 @@
+  protected $TranslatableString;

    translatedString 

  2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslatableString.php
@@ -0,0 +1,183 @@
+  public function getUnTranslatableString() {

    getUntranslatedString

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at Calibrate commented
Status: Needs work » Needs review
FileSize
2569069-28-rename-translation-wrapper.patch84.4 KB
interdiff.txt6.15 KB

Thx, fixed.

Status: Needs review » Needs work

The last submitted patch, 28: 2569069-28-rename-translation-wrapper.patch, failed testing.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented 
 .../fixtures/update/drupal-8.language-enabled.php  | Bin 6316 -> 6316 bytes
...
       $spec = array(
...
diff --git a/core/modules/system/tests/fixtures/update/drupal-8.language-enabled.php b/core/modules/system/tests/fixtures/update/drupal-8.language-enabled.php
index 6be5c24f5c101bc7fea607af617b4bc135a4401e..d80e74ef8e62ec9a730045cae83e4af09d8f932d 100644
GIT binary patch
delta 73
zcmZ2uxW;gU8!uO4Qch}cNl|8A`s4uKYe?M927E2-NPM8Q_~fgi7m<WEZxH*#2mts-
B93KDx
 
delta 73
zcmZ2uxW;gU8!uO8eqMM{VnIP_(c}Q$Ye?M927E2-NPM8Q_~fgi7m<WEZxH*#2mlE+
B9FG71
 

What is this change?

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at Calibrate commented

@lauriii

What is this change?

/fixtures/update/drupal-8.language-enabled.php contains a couple of references to TranslationWrapper that have been renamed to TranslatableString. The file is marked as binary though (contains null bytes in an otherwise plain-text file), hence the strange output in the patch.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys at Calibrate commented
Status: Needs work » Needs review
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

Patch looks good to me! The change in fixtures/update/drupal-8.language-enabled.php shouldn't technically be needed, but if tests are green it's fine to keep anyway.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Status: Needs review » Reviewed & tested by the community

Looked the changes inside the fixtures/update/drupal-8.language-enabled.php and there actually is something about the TranslationWrapper so it makes sense. DrupalCI is green so RTBC!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

A simple rename leaving the original class as a deprecated BC later. Looks great. Committed 5c943b5 and pushed to 8.0.x. Thanks!

  • alexpott committed 5c943b5 on 8.0.x 
    Issue #2569069 by stefan.r, mr.baileys, alexpott, lauriii: Replace...
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

Published the CR at https://www.drupal.org/node/2571255

Status: Fixed » Needs work

The last submitted patch, 28: 2569069-28-rename-translation-wrapper.patch, failed testing.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Druid commented
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: +Security improvements, +D8MI, +language-ui, +D8 Accelerate

Not sure how this lost all its tags in #4, restoring :)