Problem/Motivation

t() is one of the last places (other than SafeMarkup::format()) where we still use the safe list. Currently, use of the safe list in t() leads to bugs as well as security issues (see #2549943: [plan] Remove as much of the SafeMarkup class's methods as possible).

Proposed resolution

Instead of a string, make t() return a TranslationWrapper object implementing SafeStringInterface. This object will return the translated string upon casting to string using the __toString() magic method.

Doing this will allow us to get rid of the safe list entirely as the patch to get rid of SafeMarkup::format() will be trivial once the current issue is solved (see #2559971: Make SafeMarkup::format() return a safe string object to remove reliance on a static, unpredictable safe list).

Remaining tasks

Review current patch
Review draft change record at https://www.drupal.org/node/2564451
Commit

User interface changes

No

API changes

This will be disruptive to contrib in that the return value of t() will need to be explicitly cast to string in strpos() and in array keys. The conversion will be easy however as all we need to do is (string) t('String'), and the array key conversions will mostly be about labels in #options.

CommentFileSizeAuthor
#225 2557113-3-225.patch54.64 KBalexpott
#225 223-225-interdiff.txt765 bytesalexpott
#223 2557113-3-223.patch54.69 KBalexpott
#223 214-223-interdiff.txt8.63 KBalexpott
#214 2557113-3-214.patch50.06 KBalexpott
#214 213-214-interdiff.txt1.17 KBalexpott
#213 2557113-3-213.patch48.89 KBalexpott
#213 208-213-interdiff.txt2.39 KBalexpott
#208 interdiff.txt756 bytesdawehner
#208 2557113-208.patch47.08 KBdawehner
#204 2557113-204.patch46.35 KBdawehner
#204 interdiff.txt590 bytesdawehner
#200 2557113-3-200.patch45.77 KBalexpott
#200 197-200-interdiff.txt13.88 KBalexpott
#197 2557113-196.patch44.92 KBstefan.r
#197 interdiff-192-196.txt849 bytesstefan.r
#192 interdiff.txt1.82 KBdawehner
#192 2557113-192.patch45.54 KBdawehner
#191 2557113-189.patch45.58 KBstefan.r
#191 interdiff-188-189.txt1.05 KBstefan.r
#188 2557113-3-188.patch45.01 KBalexpott
#188 187-188-interdiff.txt1.06 KBalexpott
#187 2557113-3-187.patch45.1 KBalexpott
#187 185-187-interdiff.txt2.09 KBalexpott
#185 2557113-3-185.patch45.1 KBalexpott
#185 175-185-interdiff.txt2.09 KBalexpott
#175 2557113-3-175.patch46.25 KBalexpott
#175 169-175-interdiff.txt6.08 KBalexpott
#169 2557113-3-169.patch48.35 KBalexpott
#169 164-169-interdiff.txt813 bytesalexpott
#169 PATCH_t_list.png162.77 KBalexpott
#169 HEAD_t_list.png175.52 KBalexpott
#164 2557113-3-162.patch47.76 KBalexpott
#164 160-162-interdiff.txt3.06 KBalexpott
#160 2557113-3-160.patch45.52 KBalexpott
#160 156-160-interdiff.txt14.69 KBalexpott
#156 make_t_return_a-2557113-156.patch43.74 KBcilefen
#156 interdiff-2557113.txt1.64 KBcilefen
#135 2557113-133.patch44.15 KBstefan.r
#135 interdiff-130-133.txt611 bytesstefan.r
#130 2557113-130.patch44.11 KBstefan.r
#130 interdiff-128-130.txt1.15 KBstefan.r
#129 2557113-128.patch44 KBstefan.r
#129 interdiff-122-128.txt5.54 KBstefan.r
#122 2557113-122.patch41.74 KBstefan.r
#122 interdiff-120-122.txt2.45 KBstefan.r
#120 2557113-2-120.patch39.74 KBalexpott
#120 106-120-interdiff.txt2.58 KBalexpott
#106 2557113-105.patch37.16 KBstefan.r
#106 interdiff-103-105.txt1.1 KBstefan.r
#104 2557113-103.patch37.07 KBstefan.r
#104 interdiff-100-103.txt3.31 KBstefan.r
#101 2557113-100.patch36.9 KBstefan.r
#101 interdiff-96-100.txt13.89 KBstefan.r
#96 2557113-96.patch35.95 KBstefan.r
#95 2557113-95.patch34.17 KBstefan.r
#82 changes.txt38.74 KBstefan.r
#81 2557113-81.patch78.48 KBstefan.r
#75 2557113-74.patch68.55 KBstefan.r
#75 interdiff.txt13.84 KBstefan.r
#73 interdiff.txt6.69 KBlauriii
#73 make_t_return_a-2557113-73.txt35.62 KBlauriii
#67 changes.txt27.85 KBstefan.r
#67 changes.txt27.85 KBstefan.r
#63 2557113-62.patch79.59 KBstefan.r
#58 2557113-58.patch82.86 KBstefan.r
#58 2561273-2561259-2566447-stilltobecommitted.patch53.62 KBstefan.r
#58 differences.txt26.59 KBstefan.r
#53 2557113-53.patch71.59 KBstefan.r
#53 stilltobecommitted.patch42.09 KBstefan.r
#53 differences.txt26.85 KBstefan.r
#32 interdiff-26-32.txt1.52 KBstefan.r
#32 2557113-32.patch62.78 KBstefan.r
#27 2557113-26.patch62.83 KBstefan.r
#27 interdiff-12-26.txt19.55 KBstefan.r
#25 2557113-25.patch48.02 KBstefan.r
#25 interdiff-21-25.txt3.53 KBstefan.r
#21 2557113-21.patch46.31 KBstefan.r
#21 interdiff-18-21.txt775 bytesstefan.r
#18 2557113-18.patch46.3 KBstefan.r
#18 interdiff-12-18.txt3.38 KBstefan.r
#12 a_crazy_idea_for_t-2557113-12.patch45.74 KBjoelpittet
#12 interdiff.txt33.83 KBjoelpittet
#8 2557113-8.patch29.75 KBstefan.r
#4 2557113.4.patch7.21 KBalexpott
#4 2-4-interdiff.txt1.4 KBalexpott
#2 2557113.2.patch4.69 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
4.69 KB

Let's see what breaks. Drupal at least installs

Status: Needs review » Needs work

The last submitted patch, 2: 2557113.2.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
7.21 KB

This is going to be too painful... :(

Status: Needs review » Needs work

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

joelpittet’s picture

@alexpott I think we worked around this somehow in the original green SafeMarkup(as an object). May take a bit of digging to get at it but could prove to have some gems. #1825952-132: Turn on twig autoescape by default #132 is the passing version as an object before the switch to $safe_strings list

+++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
@@ -92,14 +92,16 @@ protected function hasPdoDriver() {
-    $this->results[$message] = TRUE;
+    // @todo fix this
+    $this->results[(string) $message] = TRUE;

Lol, this looks very, very familiar:) I wondered where this would pop it's head again with the direction we are taking. Was kinda hoping it would have magically been fixed by some other issue... anyways maybe the work we did originally will prove fruitful.

alexpott’s picture

@joelpittet yeah - this doesn't feel like something we can do before 8.0.0 - I wish it was.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
29.75 KB

I had a look at this yesterday to see if I could get rid of the most common test failures as they were mostly about the tests themselves. This breaks everything but reduces the 494 failures to a more reasonable number, maybe looking into this further will unearth some bugs or at least unnecessary t()s?

claudiu.cristea’s picture

Why not perform the cast inside t()?

function t($string, array $args = array(), array $options = array()) {
  return (string) \Drupal::translation()->translate($string, $args, $options);
}

Status: Needs review » Needs work

The last submitted patch, 8: 2557113-8.patch, failed testing.

alexpott’s picture

re #9 the whole point of this issue is to not do that. TranslationWrapper objects are safe for Twig to print - if we cast to a string we lose the safe-ness.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
33.83 KB
45.74 KB

maybe looking into this further will unearth some bugs or at least unnecessary t()s?

My thoughts exactly! :)

Here's a failing patch with less failures.

Status: Needs review » Needs work

The last submitted patch, 12: a_crazy_idea_for_t-2557113-12.patch, failed testing.

alexpott’s picture

Wow only 122 fails... I just worry about contrib given what we have to do fix stuff here...

  1. +++ b/core/lib/Drupal/Core/Config/StorableConfigBase.php
    @@ -156,6 +156,9 @@ protected function getSchemaWrapper() {
       protected function validateValue($key, $value) {
    +    if ($value instanceof SafeStringInterface) {
    +      $value = (string) $value;
    +    }
         // Minimal validation. Should not try to serialize resources or non-arrays.
         if (is_array($value)) {
           foreach ($value as $nested_value_key => $nested_value) {
    @@ -191,7 +194,7 @@ protected function castValue($key, $value) {
    
    @@ -191,7 +194,7 @@ protected function castValue($key, $value) {
           return $value;
         }
         // @todo fix this
    -    if ($value instanceof TranslationWrapper) {
    +    if ($value instanceof SafeStringInterface) {
           $value = (string) $value;
         }
    

    I think only the second hunk is needed...

  2. +++ b/core/modules/ckeditor/ckeditor.admin.inc
    @@ -113,6 +113,7 @@ function template_preprocess_ckeditor_settings_toolbar(&$variables) {
    +      $group_name = (string) $group_name;
    

    This shouldn't be necessary now you've changed Html::getClass right?

  3. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
    @@ -585,7 +585,7 @@ public function testViewsTranslationUI() {
    -    $this->assertTrue(strpos($json[$ids[0]], 'Translate view'), 'Translate view contextual link added.');
    +    $this->assertTrue(strpos($json[$ids[0]], (string) t('Translate view')), 'Translate view contextual link added.');
    

    I think in this and other cases we should just remove t()

catch’s picture

Hmm the actual changes in actual modules are pretty slim here - looking at shortcut, locale, entity reference each having to make a change or so each.

catch’s picture

Title: A crazy idea for t() and SafeMarkup » Make t() return a translation wrapper
Priority: Normal » Major
stefan.r’s picture

@joelpittet nice work! Just wondering how did we go from 54 failures to 93 failures on CI? Looking at the interdiff that number should have gone down instead of up, did the fixing of fatals unveil more failures? :)

stefan.r’s picture

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/BooleanItem.php
@@ -33,9 +33,8 @@ class BooleanItem extends FieldItemBase implements OptionsProviderInterface {
-      // @todo fix this if needed
-      'on_label' => 'On',
-      'off_label' => 'Off',
+      'on_label' => t('On'),
+      'off_label' => t('Off'),

This here was causing a lot of those new failures ;)

stefan.r’s picture

Status: Needs work » Needs review

Let's see how many fails we have now

Status: Needs review » Needs work

The last submitted patch, 18: 2557113-18.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
775 bytes
46.31 KB

Status: Needs review » Needs work

The last submitted patch, 21: 2557113-21.patch, failed testing.

joelpittet’s picture

@stefan.r oh yeah I should have left that in, but I was going to next find out the source of that :) I've been making slow headway in an ignore issue. Feel free to use it too if you expect the patch to fail #2465399-64: IGNORE: Patch testing issue

joelpittet’s picture

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/BooleanItem.php
@@ -72,13 +72,13 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
-      '#default_value' => $this->getSetting('on_label'),
+      '#default_value' => $this->t($this->getSetting('on_label')),
...
-      '#default_value' => $this->getSetting('off_label'),
+      '#default_value' => $this->t($this->getSetting('off_label')),

@@ -97,8 +97,8 @@ public function getPossibleValues(AccountInterface $account = NULL) {
-      0 => $this->getSetting('off_label'),
-      1 => $this->getSetting('on_label'),
+      0 => $this->t($this->getSetting('off_label')),
+      1 => $this->t($this->getSetting('on_label')),

These changes will need to be removed somehow. We don't pass variables(from the docs on t() say) in t().
https://api.drupal.org/api/drupal/core!includes!bootstrap.inc/function/t/8

stefan.r’s picture

Status: Needs work » Needs review
FileSize
3.53 KB
48.02 KB

Ouch, had missed that! Let's discuss that one on IRC - translating the label could be tricky.

Status: Needs review » Needs work

The last submitted patch, 25: 2557113-25.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
19.55 KB
62.83 KB

Let's see if this fixes another couple of fails

stefan.r’s picture

So now that this is green this this doesn't look horribly bad after all! Now we need to get rid of all the hacks/@todos...

Some points that will need addressing:

  1. +++ b/core/modules/search/src/Plugin/SearchPluginBase.php
    @@ -124,7 +124,9 @@ public function suggestedTitle() {
    -      return $this->t('Search for @keywords', array('@keywords' => Unicode::truncate($this->keywords, 60, TRUE, TRUE)));
    +      // This will already be auto-escaped on output, so we use a !placeholder to prevent double escaping.
    +      // @todo see if there is no better solution here, if we use @placeholder the head_title will be double-escaped in template_preprocess_html().
    +      return $this->t('Search for !keywords', array('!keywords' => Unicode::truncate($this->keywords, 60, TRUE, TRUE)));
    

    We had double escaping here.

  2. +++ b/core/modules/system/tests/modules/plugin_test/src/Plugin/MockBlockManager.php
    @@ -102,8 +102,8 @@ public function __construct() {
    -        'user' => new ContextDefinition('entity:user', t('User')),
    -        'node' => new ContextDefinition('entity:node', t('Node')),
    +        'user' => new ContextDefinition('entity:user', 'User'),
    +        'node' => new ContextDefinition('entity:node', 'Node'),
    

    var_export choked on this during assertEquals() because of the translation wrapper in the label property of the context definition.

  3. +++ b/core/modules/views/src/Plugin/Derivative/ViewsBlock.php
    @@ -93,7 +94,9 @@ public function getDerivativeDefinitions($base_plugin_definition) {
    -              $desc = t('!view', array('!view' => $view->label()));
    +              // @todo fix this hack, it should not be needed -- we should sanitize
    +              // during rendering instead.
    +              $desc = t('!view', array('!view' => Xss::filterAdmin($view->label())));
    

    Without this we had XSS on output in XssBlockTest, maybe #2558791: "!"-prefixed tokens should Xss::filterAdmin() but not affect safeness would solve this?

  4. +++ b/core/modules/comment/src/CommentForm.php
    @@ -294,7 +294,9 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
    -        $comment->setSubject($this->t('(No subject)'));
    +        // @todo fix this. We cast to string so this will pass the
    +        // primitive type constraint validation.
    +        $comment->setSubject((string) $this->t('(No subject)'));

    TranslationWrappers are not considered a primitive type, so this failed the constraint on the subject field.

  5. If we continue with this we may want to do decide whether to keep the string casts on the test assert methods or whether we cast to string in the tests itself. Right now the patch does a bit of both...
joelpittet’s picture

Awesome work! if the power returns to my part of Vancouver tomorrow I'll see if I can't tackle some of those todos with you.

alexpott’s picture

I think this might mean we can get rid of the safe list - since converting SafeMarkup::format to return a SafeString will be way simpler.

alexpott’s picture

Created #2559969: Make SafeStringInterface extend \JsonSerializable so we don't have to do so much string casting that uses the JsonSerializable trick discovered here for all SafeStrings.

stefan.r’s picture

stefan.r’s picture

dawehner’s picture

  1. +++ b/core/lib/Drupal/Component/Utility/Html.php
    @@ -54,10 +54,10 @@ class Html {
    +    if (!isset(static::$classes[(string) $class])) {
    +      static::$classes[(string) $class] = static::cleanCssIdentifier(Unicode::strtolower($class));
    ...
    +    return static::$classes[(string) $class];
    

    General question, should we document why we have to string casts in some places?

  2. +++ b/core/lib/Drupal/Core/Cache/CacheCollector.php
    @@ -145,6 +146,10 @@ public function has($key) {
    +    // @todo fix this
    +    if ($key instanceof SafeStringInterface) {
    

    @todo = new issue or @todo as part of this issue?

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/BooleanItem.php
    @@ -33,8 +33,8 @@ class BooleanItem extends FieldItemBase implements OptionsProviderInterface {
    -      'on_label' => t('On'),
    -      'off_label' => t('Off'),
    +      'on_label' => 'On',
    +      'off_label' => 'Off',
    

    +1 for this change

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/BooleanItem.php
    @@ -72,13 +72,13 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state) {
    -      '#default_value' => $this->getSetting('on_label'),
    +      '#default_value' => $this->t('!on_label', ['!on_label' => $this->getSetting('on_label')]),
    ...
    -      '#default_value' => $this->getSetting('off_label'),
    +      '#default_value' =>  $this->t('!off_label', ['!off_label' => $this->getSetting('off_label')]),
    
    @@ -97,8 +97,8 @@ public function getPossibleValues(AccountInterface $account = NULL) {
    -      0 => $this->getSetting('off_label'),
    -      1 => $this->getSetting('on_label'),
    +      0 => $this->t('!off_label', ['!off_label' => $this->getSetting('off_label')]),
    +      1 => $this->t('!on_label', ['!on_label' => $this->getSetting('on_label')]),
    

    mh I don't get why we need t() here for !on_label / !off_label

  5. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
    @@ -41,6 +41,24 @@
       /**
    +   * 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 $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.
    +   *
    +   * @internal
    +   */
    +  public function doTranslate($string, array $options = array());
    +
    

    What about using translateWithoutArgs? Mh I see we want to express that we return an actual string

  6. +++ b/core/modules/simpletest/src/AssertContentTrait.php
    @@ -178,6 +179,10 @@ protected function getUrl() {
    +      // @todo fix this
    +      if ($value instanceof SafeStringInterface) {
    +        $value = (string) $value;
    +      }
    

    IMHO for xpath we can always cast to string, right? anything else would not pack anyway, given how xpath works

  7. +++ b/core/modules/simpletest/src/TestBase.php
    @@ -654,6 +655,18 @@ protected function assertNotNull($value, $message = '', $group = 'Other') {
    +    if (is_array($first)) {
    +      array_walk_recursive($first, [$this, 'translationWrapperToString']);
    +    }
    +    if (is_array($second)) {
    +      array_walk_recursive($second, [$this, 'translationWrapperToString']);
    +    }
    
    @@ -683,6 +696,15 @@ protected function assertNotEqual($first, $second, $message = '', $group = 'Othe
    +   * @todo remove this
    +   */
    +  public function translationWrapperToString(&$value) {
    +    if ($value instanceof SafeStringInterface) {
    +      $value = (string) $value;
    +    }
    +  }
    +
    

    What about using strval for iteration?

stefan.r’s picture

mh I don't get why we need t() here for !on_label / !off_label

lol... yes that was wrong :)

joelpittet’s picture

Re #34.3

-      'on_label' => t('On'),
-      'off_label' => t('Off'),
+      'on_label' => 'On',
+      'off_label' => 'Off',

I'm going to go with -1 on this change. Because it causes the other !on_label stuff. Those values though must be used someplace in an inappropriate way for things to fail. I think that is where we need to focus on finding where the source of that inappropriate use(may it be using the values flipped as keys or something like that).

chx’s picture

We have shied away from doing this because DX. Having to (string) every translated string to be usable as a string is a nuisance. Not sure whether big enough. Also, wouldn't this make Drupal a hair slower?

catch’s picture

I think anything we lose we could more than make up by fixing issues like #2497275: ~50 calls to t() for two strings in LanguageManager() on every request.

Also for issues like that, the translated string is never used, so the creation of the TranslationWrapper object should be cheaper than doing the actual translation - so should be a net improvement if we're still doing things like that.

Then on most pages with SmartCache we shouldn't really be calling t() even once for cache hits.

So overall it'll be a net improvement when we unnecessarily translate strings that won't be rendered, or no change for warm caches, and possible a small hit on cold caches but I'd expect very minimal there.

stefan.r’s picture

We could split out some of the smaller issues here:

- The test helper methods that cast the assertEquals() etc comparisons to string
- The boolean label issue
- The casting of array keys to string
- Some of the unnecessary t()'s in tests
- The storing of translated strings in array keys in Tasks.php

On IRC it was also mentioned that TranslatedString may be a better name than TranslationWrapper here (we could alias and deprecate).

But first all core committers may need to be on board with this issue, otherwise not sure how we'll get the smaller issues in?

alexpott’s picture

@stefan.r I've written to all the other committers about the possibility of removing the safe list and why I think it is a good idea. I think the list of issues you propose breaking this up into make sense. Some of them are worth doing regardless, eg, the boolean label issue, the removal of some of the unnecessary t()'s in tests, and the storing of translated strings in array keys in Tasks.php.

joelpittet’s picture

+1 to the split up and TranslatedString sounds way more specific to me so +1 there but may be an unnessasary BC break?

stefan.r’s picture

it's not a BC break if we alias and keep the deprecation until 9.0.0...

stefan.r’s picture

Title: Make t() return a translation wrapper » Make t() return a TranslatedString object
joelpittet’s picture

Ah yeah good call:)

stefan.r’s picture

I just reviewed this patch again to see if it could be split up into further sub-issues, but it doesn't look like that will be needed. Once the current sub issues land this will be quite reasonable / review-able in terms of size.

stefan.r’s picture

Title: Make t() return a TranslatedString object » Make t() return a TranslationWrapper object
Issue summary: View changes
Issue tags: +Security improvements

Updating the issue summary, and let's leave the rename for later

stefan.r’s picture

Issue summary: View changes
stefan.r’s picture

Issue summary: View changes

Adding a link to the draft CR in the issue summary

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-ui, +sprint
stefan.r’s picture

@Gabor I htink this is just waiting on the children (#2561273: "Text on demand" for "input required" exposed form plugin is not displayed, #2561259: Cast (optgroup) array keys that may hold translated strings to string) and a decision from #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand but that shouldn't have to block further work on this - technically we could already do a reroll with those three patches included, and as soon as they are solved we can do a reroll that should result in a much smaller patch.

stefan.r’s picture

Reroll of #32 that includes some test fixes and the following 3 issues:

#2561273: "Text on demand" for "input required" exposed form plugin is not displayed
#2561259: Cast (optgroup) array keys that may hold translated strings to string
#2566447: Cast safe string objects to string or use assertEqual() in some assertIdentical() comparisons

Also posting a combined patch that includes just those 3 issues as well as an interdiff with this patch, just so we can continue working on this issue and see what would be left as soon as the other 3 went in.

Out for the evening so just posting my progress in case anyone wants to look any test failures or @todos

The last submitted patch, 53: stilltobecommitted.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 53: 2557113-53.patch, failed testing.

The last submitted patch, 53: stilltobecommitted.patch, failed testing.

The last submitted patch, 53: 2557113-53.patch, failed testing.

stefan.r’s picture

Let's try this again

stefan.r’s picture

OK so this is green now, just to reiterate the actual patch that would be for this issue is in 'differences.txt'. Some 6 remaining @todos:

  1. +++ b/core/modules/comment/src/CommentForm.php
    @@ -294,7 +294,9 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
    +        // @todo fix this. We cast to string so this will pass the
    +        // primitive type constraint validation.
    +        $comment->setSubject((string) $this->t('(No subject)'));
    

    Discussed this with @plach and we thought it'd be reasonable for objects with __toString() to pass the primitive type validation as the object can be cast to a primitive... So we could do something like this in PrimitiveTypeConstraintValidator instead:

    - if ($typed_data instanceof StringInterface && !is_scalar($value)) {
    + if ($typed_data instanceof StringInterface && !is_scalar($value) && !($value instanceof SafeStringInterface)) {
  2. +++ b/core/modules/search/src/Plugin/SearchPluginBase.php
    @@ -124,7 +124,9 @@ public function suggestedTitle() {
    +      // !!!! This will already be auto-escaped on output, so as a temporary workaround we use a !placeholder to prevent double escaping.
    +      // @todo fix this elsewhere, if we use @placeholder the head_title will be double-escaped in template_preprocess_html().
    +      return $this->t('Search for !keywords', array('!keywords' => Unicode::truncate($this->keywords, 60, TRUE, TRUE)));
    

    This just needs fixing - it may have been fixed already in HEAD so we can try taking it out and see if it passes tests.

  3. +++ b/core/modules/shortcut/shortcut.module
    @@ -311,7 +311,8 @@ function shortcut_preprocess_page(&$variables) {
    +      // @todo document why we cast the title to string.
    +      'name' => (string) $variables['title'],
    

    Needs documentation.

  4. +++ b/core/lib/Drupal/Core/Cache/CacheCollector.php
    @@ -145,6 +146,11 @@ public function has($key) {
       public function get($key) {
         $this->lazyLoadCache();
    +    // As a development aid, we allow calling code to use keys that are objects
    +    // implementing SafeStringInterface.
    +    if ($key instanceof SafeStringInterface) {
    +      $key = (string) $key;
    +    }
    

    This may not be needed? It is only making 1 test fail: https://www.drupal.org/pift-ci-job/31711

  5. +++ b/core/modules/user/user.module
    @@ -991,7 +991,8 @@ function user_user_role_insert(RoleInterface $role) {
    -      'label' => t('Add the @label role to the selected users', array('@label' => $role->label())),
    +      // @todo investigate
    +      'label' => (string) t('Add the @label role to the selected users', array('@label' => $role->label())),
    
    @@ -1004,7 +1005,8 @@ function user_user_role_insert(RoleInterface $role) {
    -      'label' => t('Remove the @label role from the selected users', array('@label' => $role->label())),
    +      // @todo investigate
    +      'label' => (string) t('Remove the @label role from the selected users', array('@label' => $role->label())),
    

    This may not be needed anymore.

  6. +++ b/core/modules/views/src/Plugin/Derivative/ViewsBlock.php
    @@ -93,7 +94,9 @@ public function getDerivativeDefinitions($base_plugin_definition) {
    -              $desc = t('!view', array('!view' => $view->label()));
    +              // @todo fix this, we should sanitize during rendering instead.
    +              $desc = t('!view', array('!view' => Xss::filterAdmin($view->label())));
    

    Needs investigation, something seems wrong here.

The last submitted patch, 58: 2561273-2561259-2566447-stilltobecommitted.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 58: 2557113-58.patch, failed testing.

joelpittet’s picture

Edit: LOL whoops wrong issue...

stefan.r’s picture

Status: Needs work » Needs review
FileSize
79.59 KB

Just posting an updated patch (combined with the patches from the 2 child issues). As to the previous 6 @todos, just one left (number 6):

1. Fixed the constraint validator
2. Already fixed in HEAD.
3. Not needed anymore.
4. Was not needed. We were translating an already translated string ("The site's default language (@language)"). The nice thing about t() returning an object is we can now check if a string has already been translated - so the patch now does that in Drupal\views\Plugin\views\PluginBase::listLanguages()
5. Not needed anymore.
6. Will investigate.

stefan.r’s picture

Discussed with @joelpittet and that last @todo is an XSS in the tests when $view->label() == '<script>alert('view');</script>':

          if (empty($desc)) {
            if ($display->display['display_title'] == $display->definition['title']) {
              $desc = t('!view', array('!view' => $view->label()));
            }
            else {
              $desc = t('!view: !display', array('!view' => $view->label(), '!display' => $display->display['display_title']));
            }
          }

He'll work on this in #2567159: Fix improper usage of t() in ViewsBlock

chx’s picture

@stefan.r asked me whether a SafeStringInterface object should pass as a string for StringItem .

Well, look at that. What is StringItem? It's an attempt to add metadata and behavior to a PHP data type that doesn't have it because PHP data types are utter shit. What is SafeStringInterface? It's an attempt to add metadata and behavior to a PHP data type that doesn't have it because PHP data types are utter shit.

Um. On what grounds should StringItem not acknowledge its brother in the struggle for better data types? Obviously it should.

stefan.r’s picture

+++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/PrimitiveTypeConstraintValidator.php
@@ -49,7 +50,7 @@ public function validate($value, Constraint $constraint) {
-    if ($typed_data instanceof StringInterface && !is_scalar($value)) {
+    if ($typed_data instanceof StringInterface && !is_scalar($value) && !($value instanceof SafeStringInterface)) {
       $valid = FALSE;

So per #65 this looks to be OK then. Thanks @chx!

stefan.r’s picture

FileSize
27.85 KB
27.85 KB

The 3 child issues will be dealt with in:

#2567159: Fix improper usage of t() in ViewsBlock
#2561259: Cast (optgroup) array keys that may hold translated strings to string
#2566447: Cast safe string objects to string or use assertEqual() in some assertIdentical() comparisons

So excluding those, what is left of the patch is the attached txt, which is ready to be reviewed now as there are no further @todos left, other than deciding what to do with the placeholder replacement code in TranslationWrapper::render().

xjm’s picture

Title: Make t() return a TranslationWrapper object » Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list
Priority: Major » Critical

Based on discussion with @alexpott and others, I'm tentatively promoting this issue to critical. It helps address numerous problems with SafeMarkup simultaneously, including:

lauriii’s picture

Status: Needs review » Postponed

There is nothing that could be worked on right now. Feel free to unpostpone this if you want to keep reiterating patch provided on #67 before child issues are fixed.

#2567159: Fix improper usage of t() in ViewsBlock
#2561259: Cast (optgroup) array keys that may hold translated strings to string
#2566447: Cast safe string objects to string or use assertEqual() in some assertIdentical() comparisons

In the latest patch there is one @todo which is blocked by decision made in here: #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand

stefan.r’s picture

Thanks. In the mean time #67 could still use further review (or iterations).

dawehner’s picture

Just some small comment here and there

  1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
    @@ -41,6 +41,24 @@
       /**
    +   * Translates a string to the current language or to a given language.
    +   *
    

    Can we document that you should use ::translate() instead?

  2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
    @@ -41,6 +41,24 @@
    +   */
    +  public function doTranslate($string, array $options = array());
    +
    

    We could name it a bit more explicit and use translateToString for example

  3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -103,7 +105,36 @@ public function getOptions() {
       public function render() {
    -    return $this->t($this->string, $this->arguments, $this->options);
    +    $string = $this->getStringTranslation()->doTranslate($this->string, $this->options);
    +    // @todo fix this pending a decision on https://www.drupal.org/node/2506427
    +    if (!empty($this->arguments)) {
    +      // Transform arguments before inserting them.
    +      $args = $this->arguments;
    +      foreach ($args as $key => $value) {
    +        switch ($key[0]) {
    +          case '@':
    +            if (!SafeMarkup::isSafe($value)) {
    +              // Escaped only.
    +              $args[$key] = Html::escape($value);
    +            }
    +            break;
    +
    +          case '%':
    +          default:
    +            // Escaped and placeholder.
    +            if (!SafeMarkup::isSafe($value)) {
    +              $value = Html::escape($value);
    +            }
    +            $args[$key] = '<em class="placeholder">' . $value . '</em>';
    +            break;
    +
    +          case '!':
    +            // Pass-through. This should be safe.
    +        }
    +      }
    +      $string = strtr($string, $args);
    +    }
    +    return $string;
    

    I'm curious whether the part of SafeMarkup::format() and this which is identical could/should be moved to something?

  4. +++ b/core/modules/system/tests/modules/plugin_test/src/Plugin/MockBlockManager.php
    @@ -87,7 +87,7 @@ public function __construct() {
           'context' => array(
    -        'user' => new ContextDefinition('entity:user', t('User'), FALSE),
    +        'user' => new ContextDefinition('entity:user', 'User', FALSE),
           ),
         ));
    

    I'm curious whether its really the best idea to remove the t() here and in other places ... just imagine that people take code like this as example

  5. +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -552,7 +553,14 @@ protected function listLanguages($flags = LanguageInterface::STATE_ALL, array $c
    +      // The language name may have already been translated, no need to
    +      // translate it again.
    +      // @see Drupal\Core\Language::filterLanguages().
    +      if (!$name instanceof TranslationWrapper) {
    +        $name = $this->t($name);
    +      }
    

    Did we discussed whether we should take care of that internally?

stefan.r’s picture

@dawehner regarding 5, IMO translating an already translated string is almost always a bug... So we could do an assert/exception/trigger_error and/or cast to string in the TranslationWrapper constructor when it is passed in an already translated string, but this feels a bit out of scope in a critical so the conservative thing to leave that as is, just fixing the calling code in order to fix the test fail, what do you think?

lauriii’s picture

dawehner’s picture

Stefan_r I agree, it should fail somehow early

stefan.r’s picture

Status: Postponed » Needs review
FileSize
13.84 KB
68.55 KB

This addresses @dawehner's feedback other than point 4 which would still need either a comment or some refactoring so we can use a t() in those unit tests.

Status: Needs review » Needs work

The last submitted patch, 75: 2557113-74.patch, failed testing.

The last submitted patch, 75: 2557113-74.patch, failed testing.

alexpott’s picture

@lauriii / #69 I don't see how #2567159: Fix improper usage of t() in ViewsBlock blocks this. It looks like just a regular followup.

stefan.r’s picture

@alexpott without that patch we have an xss test fail due to the !placeholders

stefan.r’s picture

stefan.r’s picture

Status: Needs work » Needs review
FileSize
78.48 KB
stefan.r’s picture

FileSize
38.74 KB
xjm’s picture

Issue tags: +needs profiling

Regarding #37 and #38, I think this is probably important enough to merit profiling.

xjm’s picture

dawehner’s picture

Ran some quick profiling for the frontpage, no page cache, no dynamic page cache.
So given that I think its pretty much the same at least for this arbitrary example, given that xhprof has sometimes a bad time profiling function call overheads.

### FINAL REPORT

=== 8.0.x..8.0.x compared (55f65b8d416ce..55f65be3eb379):

ct  : 38,119|38,119|0|0.0%
wt  : 89,288|88,912|-376|-0.4%
mu  : 18,156,512|18,156,512|0|0.0%
pmu : 18,190,792|18,190,792|0|0.0%

=== 8.0.x..2557113 compared (55f65b8d416ce..55f65bf3df334):

ct  : 38,119|38,149|30|0.1%
wt  : 89,288|89,930|642|0.7%
mu  : 18,156,512|18,309,248|152,736|0.8%
pmu : 18,190,792|18,343,648|152,856|0.8%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

As you see, we gain 30 function calls (so probably ~10 things which are translated (the new() call + render() + getStringTranslation())

chx’s picture

  1. I thought we do not care about anything outside of the critical path performance because it went to shit already and only caching can save the day.
  2. We do not have an alternative solution, do we.

So what are we profiling and why? I'm just asking for future criticals and policy and stuff.

dawehner’s picture

IMHO even if it doesn't change anything, having some knowledge about how much each particular patch changes is valueable.

dawehner’s picture

+++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
@@ -0,0 +1,65 @@
+ */
+trait PlaceholderTrait {

<3

+++ b/core/modules/locale/src/Tests/LocaleTranslationUiTest.php
@@ -64,7 +64,7 @@ public function testStringTranslation() {
-    t($name, array(), array('langcode' => $langcode));
+    (string) t($name, array(), array('langcode' => $langcode));

@@ -302,7 +302,7 @@ public function testStringValidation() {
-    t($name, array(), array('langcode' => $langcode));
+    (string) t($name, array(), array('langcode' => $langcode));

@@ -361,7 +361,7 @@ public function testStringSearch() {
-    t($name, array(), array('langcode' => $langcode));
+    (string) t($name, array(), array('langcode' => $langcode));

Those calls IMHO should call out to the TranslationManager directly and translateToString() as this makes it clearer what the test is about.

benjy’s picture

+++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
@@ -0,0 +1,65 @@
+namespace Drupal\Component\Utility;
...
+trait PlaceholderTrait {

It seems weird to me that a trait lives in the utility namespace. A utility for me is a final class with static methods.

PlaceholderTrait looks and feels like a utility, why does it have to be a trait? So we can do static:: vs Placeholder:: ?

dawehner’s picture

It seems weird to me that a trait lives in the utility namespace. A utility for me is a final class with static methods.

Well, its near SafeMarkup ...

PlaceholderTrait looks and feels like a utility, why does it have to be a trait? So we can do static:: vs Placeholder:: ?

Fair, well it is used probably by SafeMarkup and TranslationWrapper, but yeah the question is whether you would ever want to extend the behaviour simply by subclassing.

stefan.r’s picture

@benjy the documentation on that trait still needs some work, it wasn't actually intended to be called directly through a static call other than from tests, SafeMarkup::format() and TranslationWrapper::render()

benjy’s picture

Fair, well it is used probably by SafeMarkup and TranslationWrapper, but yeah the question is whether you would ever want to extend the behaviour simply by subclassing.

I'm not sure the difference between a trait and a class with static methods should be if you ever want to extend it. I think traits offer a benefit if they help you fulfil an interface, if all the methods are static it should just be a utility class.

catch’s picture

The trade off on function calls will be between t() calls that never get translated (we still have several in HEAD including on warm caches), vs. overhead of translating the ones that do.

This happens both on warm and cold caches to varying extents, so is probably why it doesn't look like much change, although in reality it's likely to be quite a big change in what's actually happening.

Another thing to note here is that if we eventually get to the point on warm caches of not translating anything in a t() call at all (because all the ones that are rendered are render cached), we'll end up saving the locale cache get for non-English requests. So this could end up a genuine net improvement eventually.

plach’s picture

  1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
    @@ -41,6 +41,24 @@
    +   * @internal This method is for internal use only. Use ::translate() instead.
    

    An internal method on an interface looks very weird, this makes me wonder whether it would make sense to extract the ::translateToString() logic to another trait so it can be protected and does not need to be part of the public API.

  2. +++ b/core/lib/Drupal/Core/Validation/DrupalTranslator.php
    @@ -73,8 +75,13 @@ public function getLocale() {
    +      // We allow the values in the parameters to be safe string objects. This can be
    +      // useful when we want to use parameter values that are TranslationWrappers.
    

    80 chars

stefan.r’s picture

reroll now that all children have been committed

stefan.r’s picture

The last submitted patch, 95: 2557113-95.patch, failed testing.

The last submitted patch, 95: 2557113-95.patch, failed testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
    @@ -0,0 +1,65 @@
    +   *
    +   * @see \Drupal\Component\Utility::SafeMarkup::format()
    +   * @see \Drupal\Core\StringTranslation::render().
    

    I think for documentation its fine to reference a different package

  2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -21,6 +24,7 @@
      */
     class TranslationWrapper implements SafeStringInterface {
     
    
    +++ b/core/modules/views/src/Plugin/views/PluginBase.php
    @@ -562,7 +563,14 @@ protected function listLanguages($flags = LanguageInterface::STATE_ALL, array $c
    +      // The language name may have already been translated, no need to
    +      // translate it again.
    +      // @see Drupal\Core\Language::filterLanguages().
    +      if (!$name instanceof TranslationWrapper) {
    +        $name = $this->t($name);
    +      }
    

    It is a big question whether the translation wrapper should handle that internally. What about adding an assert() into the TranslationWrapper now? This could allow us to find potential issues much faster, especially in contrib in code

  3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -103,7 +107,11 @@ public function getOptions() {
    +    if (!empty($this->arguments)) {
    +      list($string, $safe) = static::placeholderFormat($string, $this->arguments);
    +    }
    

    This just feels wrong, can we at least try to get rid of that madness in a follow up? The placeholder handling does not belong into this value object. We could easily move it into the TranslationManager itself

  4. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/PrimitiveTypeConstraintValidator.php
    @@ -49,7 +50,7 @@ public function validate($value, Constraint $constraint) {
    -    if ($typed_data instanceof StringInterface && !is_scalar($value)) {
    +    if ($typed_data instanceof StringInterface && !is_scalar($value) && !($value instanceof SafeStringInterface)) {
           $valid = FALSE;
    

    I'm curious how this ever happened. Does that mean there is code doing $entity->title->value = t('Foo')? Seems like a bug to do that in the first place.

  5. +++ b/core/modules/system/tests/modules/plugin_test/src/Plugin/MockBlockManager.php
    @@ -118,4 +118,17 @@ public function __construct() {
    +   * Creates a new context definition with a label that is cast to string.
    +   *
    +   * @return \Drupal\Core\Plugin\Context\ContextDefinition
    +   */
    +  protected function createContextDefinitionWithStringLabel($data_type, $label, $required = TRUE) {
    

    Missing docs, sorry

alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
    @@ -0,0 +1,65 @@
    +   * @return array
    +   *   Array with the formatted string and whether the string is safe.
    +   *
    +   * @see \Drupal\Component\Utility::SafeMarkup::format()
    +   * @see \Drupal\Core\StringTranslation::render().
    +   */
    +  protected static function placeholderFormat($string, array $args) {
    ...
    +    return [$output, $safe];
    

    Considering we're removing !placeholder I would do this differently and have $safe be a parameter passed in by reference. Rather than a multi value return.

  2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -7,8 +7,11 @@
    +use Drupal\Component\Utility\Html;
    +use Drupal\Component\Utility\SafeMarkup;
    

    Not used

  3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -103,7 +107,11 @@ public function getOptions() {
    +      list($string, $safe) = static::placeholderFormat($string, $this->arguments);
    

    I don't think we need to use static here - $this-> should be fine.

  4. +++ b/core/modules/locale/src/Tests/LocaleTranslationUiTest.php
    @@ -361,7 +361,7 @@ public function testStringSearch() {
    +    (string) t($name, array(), array('langcode' => $langcode));
    

    Let's call ->render to make it more explicit - the random string cast looks very odd.

  5. +++ b/core/tests/Drupal/Tests/Core/Annotation/TranslationTest.php
    @@ -46,8 +46,8 @@ public function testGet(array $values, $expected) {
    -      ->with($values['value'], $arguments, $options);
    

    $arguments is no longer used.. can be removed above.

stefan.r’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
13.89 KB
36.9 KB

@dawehner

1. It is :)
2. Added that assert(), along with a string cast.
3. Moved it into TranslationManager::translateToString() - letting you refactor this further if you like
4. We need this for default subjects, ie t('No subject;)
5. Docs added.

addressed #100 as well

Status: Needs review » Needs work

The last submitted patch, 101: 2557113-100.patch, failed testing.

dawehner’s picture

This looks really great. Thanks for fixing this!

  1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -146,7 +149,7 @@
    -  public function translateToString($string, array $options = array()) {
    +  public function translateToString($string, array $args = array(), array $options = array()) {
         // Merge in defaults.
    

    Nice!

  2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -63,7 +59,8 @@
    +    assert('!($string instanceof TranslationWrapper)', 'Already translated value was passed in.');
    +    $this->string = (string) $string;
    

    Do we want both the assert and the cast? I think we should actually check that we don't translate something which was already marked as safe, don't you think so? It should be just pure string allowed to be passed in here.

  3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -107,11 +105,7 @@
       public function render() {
    -    $string = $this->getStringTranslation()->translateToString($this->string, $this->options);
    -    if (!empty($this->arguments)) {
    -      list($string, $safe) = static::placeholderFormat($string, $this->arguments);
    -    }
    -    return $string;
    +    return $this->getStringTranslation()->translateToString($this->string, $this->arguments, $this->options);
       }
    

    nice!

  4. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -107,11 +105,7 @@
    diff -u b/core/modules/locale/src/Tests/LocaleTranslationUiTest.php b/core/modules/locale/src/Tests/LocaleTranslationUiTest.php
    
    diff -u b/core/modules/locale/src/Tests/LocaleTranslationUiTest.php b/core/modules/locale/src/Tests/LocaleTranslationUiTest.php
    --- b/core/modules/locale/src/Tests/LocaleTranslationUiTest.php
    
    --- b/core/modules/locale/src/Tests/LocaleTranslationUiTest.php
    +++ b/core/modules/locale/src/Tests/LocaleTranslationUiTest.php
    
    +++ b/core/modules/locale/src/Tests/LocaleTranslationUiTest.php
    +++ b/core/modules/locale/src/Tests/LocaleTranslationUiTest.php
    @@ -361,7 +361,7 @@
    
    @@ -361,7 +361,7 @@
         $this->drupalPostForm('admin/config/regional/language/add', $edit, t('Add custom language'));
     
         // Add string.
    -    (string) t($name, array(), array('langcode' => $langcode));
    +    t($name, array(), array('langcode' => $langcode))->render();
         // Reset locale cache.
         $this->container->get('string_translation')->reset();
         $this->drupalLogout();
    

    Aren't there more cases in that particular file?

stefan.r’s picture

Status: Needs work » Needs review
FileSize
3.31 KB
37.07 KB

test fixes

Status: Needs review » Needs work

The last submitted patch, 104: 2557113-103.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
1.1 KB
37.16 KB

#103.4 - the others were already converted, that was just one last stray (string) t(). Also removed the cast from the TranslationWrapper constructor which does a stricter is_string() check now.

I haven't fixed that last test failure yet, was it introduced by #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests?

Status: Needs review » Needs work

The last submitted patch, 106: 2557113-105.patch, failed testing.

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
+++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
@@ -59,6 +59,9 @@ class TranslationWrapper implements SafeStringInterface {

This patch is looking great, but I think the above is now a problem. Because it means that all t() results (which with this patch, now return the wrapper) are considered safe. But whether a TranslationWrapper::__toString() is actually safe depends on the arguments, at least until #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand is resolved. Technically, TranslationWrapper shouldn't even implement SafeStringInterface until then, but that would be a pain to make this patch have to change and then later change back, so maybe an interim solution can just be to add something to SafeMarkup::isSafe() to handle the possibility of a TranslationWrapper being unsafe? Maybe an @internal TranslationWrapper::isSafe() method that we can remove later?

The last submitted patch, 101: 2557113-100.patch, failed testing.

The last submitted patch, 104: 2557113-103.patch, failed testing.

The last submitted patch, 106: 2557113-105.patch, failed testing.

joelpittet’s picture

  1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
    @@ -41,6 +41,26 @@
    +  public function translateToString($string, array $args = array(), array $options = array());
    

    This feels like a step too far. Is this needed? It's a value object. It's a string wrapped in an object. We aren't translating anything to a string we are translating one string to another string and holding those in a wrapper.

  2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -140,38 +143,13 @@ public function getStringTranslation($langcode, $string, $context) {
    -  protected function doTranslate($string, array $options = array()) {
    +  public function translateToString($string, array $args = array(), array $options = array()) {
    

    can't see a reason for this change.

  3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -59,6 +59,9 @@ class TranslationWrapper implements SafeStringInterface {
    +    assert('!is_string($string)', 'Non-string value passed in.');
    

    Seems like a nice addition +1

  4. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -103,7 +107,10 @@ public function getOptions() {
    +    if ($this->string === '') {
    +      return '';
    

    Performance I assume?

  5. +++ b/core/modules/locale/src/Tests/LocaleTranslationUiTest.php
    @@ -64,7 +64,7 @@ public function testStringTranslation() {
    -    t($name, array(), array('langcode' => $langcode));
    +    t($name, array(), array('langcode' => $langcode))->render();
    

    Why is this needed?

  6. +++ b/core/tests/Drupal/Tests/Core/Menu/ContextualLinkDefaultTest.php
    @@ -74,7 +74,7 @@ public function testGetTitle() {
    -      ->method('translate')
    +      ->method('translateToString')
    

    hopefully an unnecessary change.

Edit: 1, 2 and 6 are the same thing I noticed.

stefan.r’s picture

@effulgentsia yes that was a last remaining @todo - it was made explicit in earlier code comments but those have been removed.

But maybe the result of today's call about the !placeholder problem will be a solution that doesn't involve having to deal with safeness to begin with? I.e. if the result of SafeMarkup::format() / t()->render() is always "safe", this all ceases to be a problem.

stefan.r’s picture

@joelpittet regarding 4, I just couldnt think of any reason for us to want an empty string in a translationwrapper. It think empty(t('')) would be FALSE otherwise. Just noticed it actually still would be - I don't think there is any way to make an object instantiation return a string, so do we need a TranslationWrapper::create() instead?

5 is needed because we want to retain the old behavior (of performing the actual translation)

dawehner’s picture

This feels like a step too far. Is this needed? It's a value object. It's a string wrapped in an object. We aren't translating anything to a string we are translating one string to another string and holding those in a wrapper.

... this method is on the TranslationManager, which for sure, should be able to also return a string. Its the responsibility of an API to be easily used in other places, which aren't part
of the direct Drupal ecosystem, in which we know using those things as array keys don't work, but for external APIs you might not be able to change. At the same time for those, you don't care about safety at all, like for REST API wrappers as example.

joelpittet’s picture

We expect the translated string to be safe. So if in all cases we expect it safe REST API doesn't need to care and just treat it as a string. No?

The test failures seemed to be a result of the assert() and maybe getLabel in derivatives in cache. Didn't dig down the callstack but I guess we do have some wrappers coming in from some systems.

dawehner’s picture

We expect the translated string to be safe. So if in all cases we expect it safe REST API doesn't need to care and just treat it as a string. No?

Right, this is the reason we need an API to do that. translateToString() is the one.

stefan.r’s picture

stefan.r’s picture

Further regarding the comment about the safeness of the output in #108, #2559971: Make SafeMarkup::format() return a safe string object to remove reliance on a static, unpredictable safe list has the same problem. If output from "!" were always "safe", or use of it triggered an error, the problem would be gone.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.58 KB
39.74 KB

We also need to look at for things like format_size(). This code is completely broken in head since the str_replace will lose safeness - so if translators included any HTML for instance changing the direction then HEAD is broken.

Status: Needs review » Needs work

The last submitted patch, 120: 2557113-2-120.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
2.45 KB
41.74 KB

Seems the test fail in #104 was because of a test that was fixed without fixing this in the config_translation module itself as well:

--- a/core/modules/config_translation/src/Tests/ConfigTranslationOverviewTest.php
+++ b/core/modules/config_translation/src/Tests/ConfigTranslationOverviewTest.php
@@ -116,7 +116,7 @@ public function testMapperListPage() {
       $entity_type = \Drupal::entityManager()->getDefinition($test_entity->getEntityTypeId());
       $this->drupalGet($base_url . '/translate');

 -      $title = t('!label !entity_type', array('!label' => $test_entity->label(), '!entity_type' => $entity_type->getLowercaseLabel()));
+      $title = t('@label @entity_type', array('@label' => $test_entity->label(), '@entity_type' => $entity_type->getLowercaseLabel()));

Is this already being dealt with in some other issue?

I also wonder if the assert() is not a bit disruptive, it breaks drush which has double translated strings.

dawehner’s picture

Is this already being dealt with in some other issue?

Yes, see #2568781: Replace remaining !placeholder for Non-URL HTML outputs only for $entity->label()

stefan.r’s picture

@dawehner should we mark that one as a child and blocker of this issue?

Status: Needs review » Needs work

The last submitted patch, 122: 2557113-122.patch, failed testing.

almaudoh’s picture

Really great work going on in this issue!! Just a few comments and nitpicks:

  1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -59,6 +59,9 @@ class TranslationWrapper implements SafeStringInterface {
    +    // We disallow passing in non-string values such as when we translate
    +    // an already translated value.
    +    assert('is_string($string)', 'Non-string value passed in: ' . (string) $string);
    

    I'm afraid casting this to string may not be very helpful in debugging.

  2. +++ b/core/modules/system/tests/modules/plugin_test/src/Plugin/MockBlockManager.php
    @@ -118,4 +118,24 @@ public function __construct() {
    +   * Creates a new context definition with a label that is cast to string.
    ...
    +  protected function createContextDefinitionWithStringLabel($data_type, $label, $required = TRUE) {
    

    Nit: the function name implies label is a string when actually the label is a TranslationWrapper being cast to string.

  3. +++ b/core/modules/user/src/Tests/UserCancelTest.php
    @@ -535,7 +535,7 @@ function testMassUserCancelByAdmin() {
    +      $status = $status && (strpos($this->content,  $account->getUsername() . '</em> has been deleted.') !== FALSE);
    

    I'm wondering where the opening <em> is at. Is it not needed for the substring test?

dawehner’s picture

I'm afraid casting this to string may not be very helpful in debugging.

It totally is if you have a backtrace available, AFAIK. Once you have that you could put a breakpoint in there and be pretty much done, aren't you?

The last submitted patch, 120: 2557113-2-120.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
5.54 KB
44 KB

Adding a BC layer for !placeholders to return a string along with a unit test. Create a postponed issue for whenever "!" is deprecated: #2569007: Remove "!" backward compatibility layer for t()

@almadoh:

1. Actually it is quite helpful as it outputs the string, it seems we have a couple of them in core as well, see the test fails: https://www.drupal.org/pift-ci-job/34109

Which is making me wonder if we should take out the assert() and address those in a follow-up, @dawehner what do you think?
2. Changed the method name.
3. Not needed.

stefan.r’s picture

The last submitted patch, 129: 2557113-128.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 130: 2557113-130.patch, failed testing.

stefan.r’s picture

I assume the test fails are about the ! BC compatibility layer - we have a problem when we combine ! and %, for instance in the FormValidator:

"!name cannot be longer than %max characters but is currently %length characters long."

dawehner’s picture

Which is making me wonder if we should take out the assert() and address those in a follow-up, @dawehner what do you think?

Mh, well, I think it would still add value, people might just not know how assert() actually works.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
611 bytes
44.15 KB

Maybe making this conditional on safeness of the value of the !placeholder will help

dawehner’s picture

Issue tags: +Needs tests

Proper unit tests in order to know exactly what how works (basically for documentation purposes) would be really nice!

xjm’s picture

The CR at https://www.drupal.org/node/2566447 is a great start. I think we can add a bit more. E.g. the strpos() item should also apply to virtually any other PHP function that operates on a string, right? explode() or what have you. (That search_excerpt() thing from #2568611: Replace remaining !placeholder for Non-URL HTML outputs only in search_excerpt() comes to mind.) I also think a few examples for fixing tests would be good; I added #2566447: Cast safe string objects to string or use assertEqual() in some assertIdentical() comparisons to the issue list for the CR to start.

Also, we should clarfy when the developer doesn't have to change anything. Would it be true to say "In most cases, PHP will cast the TranslationWrapper to a string automatically when needed."? And then we could give some examples of code that doesn't need to be changed?

alexpott’s picture

alexpott’s picture

At least on PHP5.6 that strpos example in https://www.drupal.org/node/2564451 is bogus.... and PHP 5.5.18. Are we sure about this?

alexpott’s picture

Yep - it is only when used as the second argument - which is quite often hardcoded... https://3v4l.org/sjtsH

alexpott’s picture

I've fixed the CR wrt to strpos and added a bit more detail.

alexpott’s picture

@xjm no that the strpos thing does not apply to explode... https://3v4l.org/E2C1G

alexpott’s picture

The thing with strpos is that $needle does not represent quite what people think it does - from https://secure.php.net/manual/en/function.strpos.php

If needle is not a string, it is converted to an integer and applied as the ordinal value of a character.

plach’s picture

I was skimming through the change record and I wondered whether it would make sense to make TW implement ArrayAccess to mimic string array access.

chx’s picture

alexpott’s picture

@plach I don't think so... we've got this far without it. It would also force translation to occur at that point. I think it is better to be more explicit about this with a string cast.

chx’s picture

Added https://www.drupal.org/node/2564451/revisions/view/8885025/8885099 -- notably there was a subtitle there " Array access to a string" it's not, it just happens to use the same syntax but it's really not an array access, it's string access. I have seen this recently elsewhere. Is there a tutorial or heaven forbid a PHP manual page spreading this madness? If the latter, I can fix it, just point it out please. Regarding emulating this syntax with ArrayAccess, what for? Getting a character from a t()'d string makes no sense since there's no way to guess what it would be as it gets translated. I added notes both to string offset access and strpos regarding the futility of these exercises.

The last submitted patch, 122: 2557113-122.patch, failed testing.

The last submitted patch, 130: 2557113-130.patch, failed testing.

plach’s picture

Very good points, thanks :)

Status: Needs review » Needs work

The last submitted patch, 135: 2557113-133.patch, failed testing.

Status: Needs work » Needs review

stefan.r queued 135: 2557113-133.patch for re-testing.

The last submitted patch, 129: 2557113-128.patch, failed testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
    @@ -0,0 +1,64 @@
    +   * @param array $args
    +   *   An associative array of replacements to make.
    

    I think its okay to not document possible placeholder types in here, given that its just an internal api?

  2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -140,38 +143,24 @@ public function getStringTranslation($langcode, $string, $context) {
    +    foreach (array_keys($args) as $arg_key) {
    +      // If the string has arguments that start with '!' we consider it unsafe
    +      // and return the translation as a string for backward compatibility
    +      // purposes.
    +      // @todo remove this temporary workaround.
    +      if (0 === strpos($arg_key, '!') && !SafeMarkup::isSafe($args[$arg_key])) {
    +        $safe = FALSE;
    +        break;
    +      }
         }
    

    So given that the patch continues to be green, does that mean we will have no double escaping caused by converting from ! to @?

  3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -59,6 +59,8 @@ class TranslationWrapper implements SafeStringInterface {
    +    // We disallow passing in non-string values such as when we translate
    +    // an already translated value.
    

    Nitpick: wrap 80 chars could be done different.
    Does that comment btw. still makes sense atm.?

  4. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -103,7 +106,10 @@ public function getOptions() {
    +    if ($this->string === '') {
    +      return '';
    +    }
    

    Should we document why we need this bit?

  5. +++ b/core/modules/comment/comment.module
    @@ -299,8 +299,6 @@ function comment_form_field_ui_field_storage_add_form_alter(&$form, FormStateInt
    diff --git a/core/modules/config_translation/config_translation.module b/core/modules/config_translation/config_translation.module
    
    diff --git a/core/modules/config_translation/config_translation.module b/core/modules/config_translation/config_translation.module
    index 3c81f78..7644f3e 100644
    
    index 3c81f78..7644f3e 100644
    --- a/core/modules/config_translation/config_translation.module
    
    --- a/core/modules/config_translation/config_translation.module
    +++ b/core/modules/config_translation/config_translation.module
    
    +++ b/core/modules/config_translation/config_translation.module
    +++ b/core/modules/config_translation/config_translation.module
    @@ -115,7 +115,7 @@ function config_translation_config_translation_info(&$info) {
    
    @@ -115,7 +115,7 @@ function config_translation_config_translation_info(&$info) {
             $info[$entity_type_id . '_fields'] = array(
               'base_route_name' => "entity.field_config.{$entity_type_id}_field_edit_form",
               'entity_type' => 'field_config',
    -          'title' => '!label field',
    +          'title' => '@label field',
               'class' => '\Drupal\config_translation\ConfigFieldMapper',
               'base_entity_type' => $entity_type_id,
               'weight' => 10,
    @@ -144,7 +144,7 @@ function config_translation_config_translation_info(&$info) {
    
    @@ -144,7 +144,7 @@ function config_translation_config_translation_info(&$info) {
         $info[$entity_type_id] = array(
           'class' => '\Drupal\config_translation\ConfigEntityMapper',
           'base_route_name' => $base_route_name,
    -      'title' => '!label !entity_type',
    +      'title' => '@label @entity_type',
           'names' => array(),
           'entity_type' => $entity_type_id,
           'weight' => 10,
    diff --git a/core/modules/config_translation/src/ConfigEntityMapper.php b/core/modules/config_translation/src/ConfigEntityMapper.php
    
    diff --git a/core/modules/config_translation/src/ConfigEntityMapper.php b/core/modules/config_translation/src/ConfigEntityMapper.php
    index 4a6d275..d454bb8 100644
    
    index 4a6d275..d454bb8 100644
    --- a/core/modules/config_translation/src/ConfigEntityMapper.php
    
    --- a/core/modules/config_translation/src/ConfigEntityMapper.php
    +++ b/core/modules/config_translation/src/ConfigEntityMapper.php
    
    +++ b/core/modules/config_translation/src/ConfigEntityMapper.php
    +++ b/core/modules/config_translation/src/ConfigEntityMapper.php
    @@ -158,7 +158,7 @@ public function getTitle() {
    
    @@ -158,7 +158,7 @@ public function getTitle() {
         // current page language. The title placeholder is later escaped for
         // display.
         $entity_type_info = $this->entityManager->getDefinition($this->entityType);
    -    return $this->t($this->pluginDefinition['title'], array('!label' => $this->entity->label(), '!entity_type' => $entity_type_info->getLowercaseLabel()));
    +    return $this->t($this->pluginDefinition['title'], array('@label' => $this->entity->label(), '@entity_type' => $entity_type_info->getLowercaseLabel()));
       }
     
    

    Do we really need those "conversions" as part of this patch?

  6. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/TranslationManagerTest.php
    @@ -59,6 +60,43 @@ public function testFormatPlural($count, $singular, $plural, array $args = array
    +      $this->assertTrue($actual instanceof SafeStringInterface);
    

    Nitpick: Let's use assertInstanceOf

larowlan’s picture

the circularity in making translateToString public concerns me $string_translation->translate() returns $wrapper $wrapper->render loops back to $string_translate, can't quite articulate my exact concerns, will mull over it, wonder if we can somehow use a closure to resolve it..and avoid making translateToString public…

cilefen’s picture

#154-3: Removed the comment. It makes no sense in the context of the function.
#154-6: done

The others are still in play.

Status: Needs review » Needs work

The last submitted patch, 156: make_t_return_a-2557113-156.patch, failed testing.

stefan.r’s picture

@dawehner

1. I think so too.
2. The patch shouldn't introduce any new double escaping in strings with !. Not following what this has to do with @?
3. Removed in last patch.
4. This was to prevent having a TranslationWrapper of an empty string as that should never be needed - but it is not the right solution as (empty(new TranslationWrapper('')) == false), still. Can object instantiations return a string somehow? Do we need a helper for this?
5. Not sure, they were causing test fails earlier but maybe the BC layer fixed this now.
6. Fixed in last patch.

Status: Needs work » Needs review
alexpott’s picture

The patch attached inverts the dependency of TranslationWrapper to the translation service by adding a renderTranslationWrapper method to the translation service.

Status: Needs review » Needs work

The last submitted patch, 160: 2557113-3-160.patch, failed testing.

dawehner’s picture

I certainly like the new idea, it helps to get rid of the crazy dependency chain. Its still calling kinda out from one to the other ...

  1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -154,22 +154,18 @@
        * {@inheritdoc}
        */
    -  public function translateToString($string, array $args = array(), array $options = array()) {
    -    // Merge in defaults.
    -    if (empty($options['langcode'])) {
    

    I still thing you should be able to do it without going through the object itself, but well, I'm okay with not fighting this, as I don't have the power / time to do so.

  2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -154,22 +154,18 @@
    +  public function renderTranslatedString(TranslationWrapper $translated_string) {
    +    $value = $this->doTranslate($translated_string->getUntranslatedString(), $translated_string->getArguments());
    +
    +    // Handle any replacements.
    +    $args = $translated_string->getArguments();
    

    Nice!

The last submitted patch, 160: 2557113-3-160.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.06 KB
47.76 KB

Fixing most test fails - this includes #1789090: Warnings when exporting templates: addcslashes() expects parameter 1 to be string which this patch has somehow uncovered... interesting.

Status: Needs review » Needs work

The last submitted patch, 164: 2557113-3-162.patch, failed testing.

The last submitted patch, 164: 2557113-3-162.patch, failed testing.

dawehner’s picture

In case someone tries to debug this failure. The problem seems to be that on saving translations and _locale_refresh_translation() is called.
In there though for some reason \Drupal::service('locale.storage')->getStrings(array('lid' => $lids, 'type' => 'javascript')) is empty.

catch’s picture

With HEAD I can't see any strings with 'javascript' context at all.

Enabled locale, enabled Arabic, switched to arabic.

Browsed around including the views UI.

MariaDB [d8]> SELECT DISTINCT(context) FROM locales_source;
+------------------+
| context          |
+------------------+
|                  |
| PHP date format  |
| Plural           |
| View entity type |
+------------------+
4 rows in set (0.01 sec)

Reminds me of #2421323: Parse js files from library definitions during rebuild to minimize variable_set() calls although clearly the test coverage breaks.

However in the test:

 // Retrieve the source string of the first string available in the
    // {locales_source} table and translate it.
    $source = db_select('locales_source', 'l')
      ->fields('l', array('source'))
      ->condition('l.source', '%.js%', 'LIKE')
      ->range(0, 1)
      ->execute()
      ->fetchField();

Nothing in {locales_source}.source has .js in it, so this returns FALSE, which means that when the strings get filtered it shows all strings, which means that the first string translated is not JavaScript, which means no javascript file should be created - so the test in HEAD ought to fail afaik.

I have no idea how the test passes in HEAD either, but I think it's the test that's broken, and looks likely that js translation is also broken.

Didn't debug all the way to the end though so take this with a pinch of salt.

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
175.52 KB
162.77 KB
813 bytes
48.35 KB

Ok wow... Drupal\locale\Tests\LocaleTranslationUiTest::testJavaScriptTranslation() in HEAD...

    $source = db_select('locales_source', 'l')
      ->fields('l', array('source'))
      ->condition('l.source', '%.js%', 'LIKE')
      ->range(0, 1)
      ->execute()
      ->fetchField();

Return false... this query is complete nonsense the location is stored in a completely different table. But the first string it can translate is a javascript string because format_size is completely broken. This patch fixes that... so the first string in not a JS string. Fixing the query fixes the tests. This is also why #1789090: Warnings when exporting templates: addcslashes() expects parameter 1 to be string is required because we are detecting a plural translation we were before. Nuts.

Here's how the test looks when it is doing

    $this->drupalPostForm('admin/config/regional/translate', $search, t('Filter'));

    $textarea = current($this->xpath('//textarea'));
    $lid = (string) $textarea[0]['name'];
    $edit = array(
      $lid => $this->randomMachineName(),
    );
    $this->drupalPostForm('admin/config/regional/translate', $edit, t('Save translations'));

HEAD

Patch

Status: Needs review » Needs work

The last submitted patch, 169: 2557113-3-169.patch, failed testing.

The last submitted patch, 169: 2557113-3-169.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

Something is up with DrupalCI

Mixologic’s picture

Sorry bout that. Bungled the rebasing of my pull request for a drupalci dependency.. basically I forgot to commit the lock file. Should be well now.

dawehner’s picture

Ok wow

Indeed.

  1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
    @@ -41,6 +41,17 @@
    +   * @param TranslationWrapper $translated_string
    

    FQCN

  2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -140,21 +143,42 @@ public function getStringTranslation($langcode, $string, $context) {
    +    $wrapper = new TranslationWrapper($string, $args, $options);
    +    return $safe ? $wrapper : $this->renderTranslatedString($wrapper);
    +  }
    

    Did we considered the alternative which is adding an UnsafeTranslationWrapper without implementing SafeStringInterface but __toString?

  3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -140,21 +143,42 @@ public function getStringTranslation($langcode, $string, $context) {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function formatPlural($count, $singular, $plural, array $args = array(), array $options = array()) {
    +    $translatable_string = implode(LOCALE_PLURAL_DELIMITER, array($singular, $plural));
    +    $translated_strings = $this->doTranslate($translatable_string, $options);
    +    return $this->formatPluralTranslated($count, $translated_strings, $args, $options);
       }
    
    @@ -172,13 +196,11 @@ public function translate($string, array $args = array(), array $options = array
    -    // Merge in defaults.
    -    if (empty($options['langcode'])) {
    -      $options['langcode'] = $this->defaultLangcode;
    -    }
    -    if (empty($options['context'])) {
    -      $options['context'] = '';
    -    }
    +    // Merge in options defaults.
    +    $options = $options + [
    +      'langcode' => $this->defaultLangcode,
    +      'context' => '',
    +    ];
    
    @@ -186,15 +208,6 @@ protected function doTranslate($string, array $options = array()) {
        */
    -  public function formatPlural($count, $singular, $plural, array $args = array(), array $options = array()) {
    -    $translatable_string = implode(LOCALE_PLURAL_DELIMITER, array($singular, $plural));
    -    $translated_strings = $this->doTranslate($translatable_string, $options);
    -    return $this->formatPluralTranslated($count, $translated_strings, $args, $options);
    -  }
    -
    -  /**
    -   * {@inheritdoc}
    -   */
    

    In case you like to, we could revert those hunks, they should not be needed for this issue, aren't they?

  4. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/TranslationManagerTest.php
    @@ -59,6 +60,43 @@ public function testFormatPlural($count, $singular, $plural, array $args = array
    +      ['foo @bar', ['@bar' => 'bar'], 'foo bar', TRUE],
    +      ['bar !baz', ['!baz' => 'baz'], 'bar baz', FALSE],
    

    Just in case you like, we could add one test case which has multiple placeholders, @bar and !baz. If we truely don't think of the implementation this might be a good idea

  5. +++ b/core/tests/Drupal/Tests/UnitTestCase.php
    @@ -214,7 +218,17 @@ public function getStringTranslationStub() {
         $translation->expects($this->any())
           ->method('translate')
    -      ->will($this->returnCallback('Drupal\Component\Utility\SafeMarkup::format'));
    +      ->willReturnCallback(function ($string, array $args = array(), array $options = array()) use ($translation) {
    +        $wrapper = new TranslationWrapper($string, $args, $options);
    +        $wrapper->setStringTranslation($translation);
    +        // Pretend everything is not safe.
    +        return (string) $wrapper;
    +      });
    

    Why is pretending everything not being safe the right choice? At least we should explain that

alexpott’s picture

  1. Fixed
  2. This is temporary whilst there is the concept of translating or SafeMarkup::format() returning an unsafe thing. Once that is done this code is gone. Add @todo and filed #2570037: Remove the ability to return unsafe string from Translation->translate()
  3. Well I like the options simplification so let's keep that. One executable line of code vs 4.
  4. Done
  5. The followup for #2 will fix this added @todo - the reason not to do it here is that then we'll be inundated with unit test change.

So one thing I think we've missed is formatPlural and friends. These return the output of SafeMarkup::format and hence will be objects once #2559971: Make SafeMarkup::format() return a safe string object to remove reliance on a static, unpredictable safe list lands but should we introduce a PluralString object to separate it out? Obviously if we decide to do that it should be a new issue. Also removing the test added by incorporating #1789090: Warnings when exporting templates: addcslashes() expects parameter 1 to be string since as the test fails show this patch introduces implicit test coverage. If this issue lands first (and it probably will) then we should repurpose that issue to add explicit test coverage.

Status: Needs review » Needs work

The last submitted patch, 175: 2557113-3-175.patch, failed testing.

alexpott’s picture

pift at pifr! GET http://ec2-52-88-182-40.us-west-2.compute.amazonaws.com/checkout/user/login returned 0 (0 bytes).

Status: Needs work » Needs review

alexpott queued 175: 2557113-3-175.patch for re-testing.

dawehner’s picture

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

Thank you alex!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 175: 2557113-3-175.patch, failed testing.

stefan.r’s picture

Very nice work, this looks great!

+++ b/core/modules/config_translation/config_translation.module
@@ -115,7 +115,7 @@ function config_translation_config_translation_info(&$info) {
-          'title' => '!label field',
+          'title' => '@label field',

@@ -144,7 +144,7 @@ function config_translation_config_translation_info(&$info) {
-      'title' => '!label !entity_type',
+      'title' => '@label @entity_type',

+++ b/core/modules/config_translation/src/ConfigEntityMapper.php
@@ -158,7 +158,7 @@ public function getTitle() {
-    return $this->t($this->pluginDefinition['title'], array('!label' => $this->entity->label(), '!entity_type' => $entity_type_info->getLowercaseLabel()));
+    return $this->t($this->pluginDefinition['title'], array('@label' => $this->entity->label(), '@entity_type' => $entity_type_info->getLowercaseLabel()));

I think these were being addressed in another issue (#2568781: Replace remaining !placeholder for Non-URL HTML outputs only for $entity->label())

dawehner’s picture

I think these were being addressed in another issue

#2568781: Replace remaining !placeholder for Non-URL HTML outputs only for $entity->label() its adressed different there, because, well, this fix is also IMHO not the right thing to do. We should not need it just to concat those two bit.

plach’s picture

Looks great, I have only minor complaints:

  1. +++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
    @@ -0,0 +1,64 @@
    +   * TranslationManager::translateToString().
    

    ::renderTranslatedString()

  2. +++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
    @@ -0,0 +1,64 @@
    +   * @see \Drupal\Core\StringTranslation::translateToString().
    

    This should be \Drupal\Core\StringTranslation\TranslationManager::renderTranslatedString(). Also, unnecessary trailing dot.

  3. +++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
    @@ -0,0 +1,64 @@
    +  protected static function placeholderFormat($string, array $args, &$safe = TRUE) {
    

    Since the goal is to remove !, can we remove the $safe parameter and temporarily move the related check in SafeMarkup::format()?

  4. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -140,21 +143,34 @@ public function getStringTranslation($langcode, $string, $context) {
    +      // If the string has arguments that start with '!' we consider it unsafe
    +      // and return the translation as a string for backward compatibility
    +      // purposes.
    +      // @todo https://www.drupal.org/node/2570037 remove this temporary
    +      // workaround.
    +      if (0 === strpos($arg_key, '!') && !SafeMarkup::isSafe($args[$arg_key])) {
    +        $safe = FALSE;
    

    Oh, above I suggested to implement exactly this also for SafeMarkup :)

  5. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -140,21 +143,34 @@ public function getStringTranslation($langcode, $string, $context) {
    +      $value = $this->placeholderFormat($value, $args);
    

    Can we use static::placeholderFormat() here?

  6. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -172,13 +188,11 @@ public function translate($string, array $args = array(), array $options = array
    diff --git a/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    
    diff --git a/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    index 10e9b12..b800b8a 100644
    
    index 10e9b12..b800b8a 100644
    --- a/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    
    --- a/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    
    +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -96,6 +96,17 @@ public function getOption($name) {
    

    It would be nice to add a note to the class doc block about the new use cases this patch addresses.

  7. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -96,6 +96,17 @@ public function getOption($name) {
    +   * @return mixed[]
    

    Shouldn't this be string[]?

alexpott’s picture

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.09 KB
45.1 KB

#2568781: Replace remaining !placeholder for Non-URL HTML outputs only for $entity->label() landed - I resolved the conflicts and used all the change from there.

Thanks for the review @plach

  1. Fixed
  2. Fixed
  3. This will be removed in #2570037: Remove the ability to return unsafe string from Translation->translate() but we need this to address some the issues with removing !placeholder
  4. SafeMarkup::format() will be fixed after !placeholder is removed in #2559971: Make SafeMarkup::format() return a safe string object to remove reliance on a static, unpredictable safe list
  5. There is no difference afaik. If some one extended the class and implemented there own placeholderFormat() it would use that one.
  6. Done - i think this will be even clearer once #2569069: Replace TranslationWrapper with TranslatableString and deprecate TranslationWrapper lands
  7. Hmm... no arguments can be SafeStrings or other scalars
plach’s picture

+++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
@@ -29,7 +29,7 @@
+   * @see \Drupal\Core\StringTranslation::renderTranslatedString()

We are still missing the class name (TranslationManager).

There is no difference afaik. If some one extended the class and implemented there own placeholderFormat() it would use that one.

Well, it was for consistency: we normally call static methods with the static:: form, also in this patch we do it.

alexpott’s picture

@plach but that static::placeholderFormat() is in a static class it has to be. And there is no perf benefit either - https://3v4l.org/ecbvS

Fixed the other thing.

alexpott’s picture

Ignore #187 it is the same as #185 oops.

The last submitted patch, 187: 2557113-3-187.patch, failed testing.

lauriii’s picture

+++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
@@ -0,0 +1,61 @@
+   *   A boolean indicating whether the string is safe or not (optional).

Nit: shouldn't (optional) be in the beginning?

stefan.r’s picture

FileSize
1.05 KB
45.58 KB

Adding two assertions and removing the bit @joelpittet and @plach had questions about earlier (don't know that we need to be testing assertions, but anyway:
https://3v4l.org/WvWvG)

If they cause a whole bunch of test fails it may be fine to add them in the follow-up mentioned in #129 instead, otherwise it could be good to add them here.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
45.54 KB
1.82 KB

Just a couple of nitpicks.

  1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -140,21 +143,34 @@ public function getStringTranslation($langcode, $string, $context) {
    +      if (0 === strpos($arg_key, '!') && !SafeMarkup::isSafe($args[$arg_key])) {
    

    we do really use yoda style, I think its actually better.

  2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -140,21 +143,34 @@ public function getStringTranslation($langcode, $string, $context) {
    +    $args = $translated_string->getArguments();
    +    if (!empty($args)) {
    

    Could be written in one line: if ($args = $tran...->getArguments())

  3. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/TranslationManagerTest.php
    @@ -59,6 +60,45 @@ public function testFormatPlural($count, $singular, $plural, array $args = array
    +      $this->assertInstanceOf('Drupal\Component\Utility\SafeStringInterface', $actual);
    

    For future lazyness reference, you can use SafeStringInterface::class instead

  4. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/TranslationManagerTest.php
    @@ -59,6 +60,45 @@ public function testFormatPlural($count, $singular, $plural, array $args = array
    +      $this->assertTrue(is_string($actual));
    

    There is assertInternalType('string', $actual);

The last submitted patch, 191: 2557113-189.patch, failed testing.

The last submitted patch, 191: 2557113-189.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 192: 2557113-192.patch, failed testing.

plach’s picture

I've posted a POC patch at #2509218-40: Ensure that SafeString objects can be used in non-HTML contexts, which is relying on this. However it changes things quite a bit wrt the translation wrapper, it would mainly remove/deprecate TranslationManager::renderTranslatedString() in favor of a TranslationManager::getTranslatedPattern() method. Since @catch thinks it's worth to explore that solution, would you please have a look to it and think whether there's any change we can do here to alleviate potential BC breaks if we end-up going that way?

stefan.r’s picture

Reverting #191 and created a new issue for those asserts and the one in #129: #2570285: Make sure TranslatableMarkup accepts string values only

Right now empty(t('')) === FALSE which is a break with previous behavior, but discussed with alexpott on IRC and this is a limitation we can live with.

stefan.r’s picture

Status: Needs work » Reviewed & tested by the community

per #192 reverting to previous status

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I think we can mostly remove placeholdering from TranslationManager and that'll make work in #2509218: Ensure that SafeString objects can be used in non-HTML contexts easier.

alexpott’s picture

Status: Needs work » Needs review
FileSize
13.88 KB
45.77 KB

Moved placeholdering to TranslationWrapper and also self injected the TranslationManager into TranslationWrapper which makes TranslationManager into the object factory it should be and way less \Drupal::container magic. Also we now store the result of translation manager on the object so printing the same TranslationWrapper object twice is cheap especially when we context switch.

plach’s picture

Interdiff looks awesome to me :)

If we happen to reroll this...

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
@@ -60,11 +69,14 @@ class TranslationWrapper implements SafeStringInterface {
+  public function __construct($string, array $arguments = array(), array $options = array(), \Drupal\Core\StringTranslation\TranslationInterface $string_translation = NULL) {

...can we skip the FQCN here?

Status: Needs review » Needs work

The last submitted patch, 200: 2557113-3-200.patch, failed testing.

dawehner’s picture

Temporarily this adds more logic into TranslationWrapper but once we have #2509218: Ensure that SafeString objects can be used in non-HTML contexts this is moved into its own logical instance, yeah!

dawehner’s picture

Status: Needs work » Needs review
FileSize
590 bytes
46.35 KB

PHP :(

<?php

$array = [
    'foo' => 1,
    'bar' => 2,
];

array_splice($array, -1, 0, 'Meh');

print_r($array);

vs

<?php

class TranslationWrapper {
    protected $foo = 'H';
    protected $bar = 'E';
}

$t = new TranslationWrapper();

$array = [
    'foo' => 1,
    'bar' => 2,
];

array_splice($array, -1, 0, $t);

print_r($array);
stefan.r’s picture

@dawehner that's crazy... let's update the CR as well? Are other array manipulation functions affected?

dawehner’s picture

I don't think so. Its behaviour is documented:

Note: If replacement is not an array, it will be typecast to one (i.e. (array) $parameter). This may result in unexpected behavior when using an object or NULL replacement.

Status: Needs review » Needs work

The last submitted patch, 204: 2557113-204.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
47.08 KB
756 bytes

Let's fix the rest of the failures.

Status: Needs review » Needs work

The last submitted patch, 208: 2557113-208.patch, failed testing.

dawehner queued 208: 2557113-208.patch for re-testing.

dawehner’s picture

Drupal\taxonomy\Tests\TermKernelTest 22 passes
zend_mm_heap corrupted
FATAL Drupal\taxonomy\Tests\Migrate\d7\MigrateNodeTaxonomyTest: test runner returned a non-zero error code (1).

The last submitted patch, 208: 2557113-208.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.39 KB
48.89 KB

Storing TranslationWrapper objects in session does not seem like a good idea. Also there's no need for StringTranslationTrait in TranslationWrapper anymore.

alexpott’s picture

A here's a nice new KernelTestBase (tng) to test this behaviour.

The last submitted patch, 213: 2557113-3-213.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 214: 2557113-3-214.patch, failed testing.

The last submitted patch, 192: 2557113-192.patch, failed testing.

The last submitted patch, 208: 2557113-208.patch, failed testing.

The last submitted patch, 213: 2557113-3-213.patch, failed testing.

The last submitted patch, 200: 2557113-3-200.patch, failed testing.

The last submitted patch, 204: 2557113-204.patch, failed testing.

The last submitted patch, 214: 2557113-3-214.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
8.63 KB
54.69 KB

This really is a find all the issues in core type patch... now I discovered we are passing render arrays to drupal_set_message :)

Status: Needs review » Needs work

The last submitted patch, 223: 2557113-3-223.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
765 bytes
54.64 KB

Messed up the namespacing of the new test.

plach’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready now.

xjm’s picture

+++ b/core/includes/common.inc
@@ -343,7 +335,26 @@ function format_size($size, $langcode = NULL) {
+        return new TranslationWrapper('@size KB', $args, $options);

Do these get scanned for translations the same way that t() itself does? I assumed it would, but then found no mention of the class on https://www.drupal.org/developing/api/8/localization.

I left @Gábor Hojtsy a message asking to clarify.

effulgentsia’s picture

Do these get scanned for translations the same way that t() itself does?

I believe so, since new TranslationWrapper is a pattern that's existed in HEAD for a while now. Gábor himself authored https://www.drupal.org/node/2540620 when a recent thing got converted to use it.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 225: 2557113-3-225.patch, failed testing.

effulgentsia’s picture

Looks like potx support for TranslationWrapper was added a year ago: #2280843: Add support for D8 TranslationWrapper, so we should be all good there.

Status: Needs work » Needs review

effulgentsia queued 225: 2557113-3-225.patch for re-testing.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Bot happy again, so back to RTBC.

effulgentsia’s picture

I read through this patch again top to bottom and think it's absolutely terrific! Here are the only things I found to complain about, but none of them need to block commit. In fact, I'll probably commit this in the next hour or two if no one beats me to it or asks me not to.

  1. +++ b/core/lib/Drupal/Component/Gettext/PoItem.php
    @@ -193,7 +193,7 @@ public function setFromArray(array $values = array()) {
    -      $this->setPlural(count($this->_translation) > 1);
    +      $this->setPlural(count($this->_source) > 1);
    

    I didn't investigate this to see if it makes sense. I'm guessing it does, but if someone thinks otherwise, please say so or open a follow-up if it's after this lands.

  2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
    @@ -41,6 +41,17 @@
    +   * 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);
    

    I don't think it's clear from the method name or docs that this only translates the first parameter of t(), and does not do any placeholder replacement. A follow-up to improve these docs would be good.

  3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -172,13 +178,11 @@ public function translate($string, array $args = array(), array $options = array
    +    $options = $options + [
    

    Can be shortened to $options +=.

  4. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -7,21 +7,25 @@
    + * This is useful for using translation in very low level subsystems like entity
    + * definition and stream wrappers.
    

    I think we can remove this comment now, since after this patch, we're pretty much using TranslationWrapper everywhere, not just in low-level subsystems.

  5. +++ b/core/lib/Drupal/Core/Template/Attribute.php
    @@ -109,7 +109,8 @@ protected function createAttributeValue($name, $value) {
    -    elseif (!is_object($value)) {
    +    // As a development aid, we allow the value to be a safe string object.
    +    elseif (!is_object($value) || $value instanceof SafeStringInterface) {
    

    I'm not clear on what "development aid" means here. This is now pretty much required, since $attributes[$foo] = t($bar) is quite common.

  6. +++ b/core/lib/Drupal/Core/Validation/DrupalTranslator.php
    @@ -73,8 +75,14 @@ public function getLocale() {
    +      // We allow the values in the parameters to be safe string objects. This
    +      // can be useful when we want to use parameter values that are
    +      // TranslationWrappers.
    +      if ($value instanceof SafeStringInterface) {
    +        $value = (string) $value;
    +      }
           if (is_object($value)) {
    -        // t() does not work will objects being passed as replacement strings.
    +        // t() does not work with objects being passed as replacement strings.
           }
    

    Must we cast to a string (and thereby do the translation right now)? Could we instead keep it an object, and change the if statement to if (is_object($value) && !$value instanceof SafeStringInterface) {?

  7. +++ b/core/modules/locale/locale.module
    @@ -641,7 +641,7 @@ function locale_form_language_admin_overview_form_alter(&$form, FormStateInterfa
    -  array_splice($form['languages']['#header'], -1, 0, t('Interface translation'));
    +  array_splice($form['languages']['#header'], -1, 0, ['translation-interface' => t('Interface translation')]);
    

    This could be another good thing to add to the change record. The CR already mentions the $needle param in strpos(), but perhaps that can be expanded to also say that any invocation of a PHP function that treats scalar strings and stringifiable objects differently is affected.

  8. +++ b/core/modules/locale/src/Tests/LocaleTranslationUiTest.php
    @@ -64,7 +64,7 @@ public function testStringTranslation() {
    -    t($name, array(), array('langcode' => $langcode));
    +    t($name, array(), array('langcode' => $langcode))->render();
    

    Here and the other two places: why an explicit ->render() instead of casting to a string?

  9. +++ b/core/tests/Drupal/Tests/Core/Menu/ContextualLinkDefaultTest.php
    @@ -103,11 +101,10 @@ public function testGetTitleWithContext() {
       public function testGetTitleWithTitleArguments() {
         $title = 'Example @test';
    -    $this->pluginDefinition['title'] = (new TranslationWrapper($title, array('@test' => 'value')))
    -      ->setStringTranslation($this->stringTranslation);
    +    $this->pluginDefinition['title'] = (new TranslationWrapper($title, array('@test' => 'value'), [], $this->stringTranslation));
         $this->stringTranslation->expects($this->once())
    -      ->method('translate')
    -      ->with($title, array('@test' => 'value'), array())
    +      ->method('translateString')
    +      ->with($this->pluginDefinition['title'])
           ->will($this->returnValue('Example value'));
    

    Here and the other tests with this same test method: we should probably also change the returnValue to 'Example @test' (or maybe even 'Example translated @test').

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
@@ -7,21 +7,25 @@
+  use PlaceholderTrait;
@@ -103,7 +135,18 @@ public function getOptions() {
+    // Handle any replacements.
+    // @todo https://www.drupal.org/node/2509218 Note that the argument
+    //   replacement is not stored so that different sanitization strategies can
+    //   be used in different contexts.
+    if ($args = $this->getArguments()) {
+      return $this->placeholderFormat($this->translatedString, $args);
+    }

Also, in some earlier comment, @dawehner thought this wasn't ideal because it ends up putting logic (the details of each sanitization strategy implementation for each placeholder type) into what is otherwise a pure value object. If anyone wants to open a follow-up to move that into a service or whatever, please go for it.

effulgentsia’s picture

Adding credit to a few more people who helped drive some aspect of this issue forward. Apologies if I left someone out.

  • effulgentsia committed e7a7de9 on 8.0.x
    Issue #2557113 by stefan.r, alexpott, dawehner, joelpittet, cilefen,...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Pushed to 8.0.x and published the change record. Thanks everyone!

alexpott’s picture

This is just fantastic... What started out as a crazy idea has come a long way quickly. See you in the SafeMarkup::format() issue to remove the static safe list.

webflo’s picture

This patch broke site install via Drush. I created a quick fix for Drush in https://github.com/drush-ops/drush/pull/1619

tstoeckler’s picture

The Drush problem points to a larger issue. Because of http://php.net/manual/en/language.oop5.object-comparison.php#71623 basically in lots of places where we used to be comparing to strings we might now be comparing objects and are potentially subject to these very weird fatals. Not good...

tstoeckler’s picture

The Drush problem points to a larger issue. Because of http://php.net/manual/en/language.oop5.object-comparison.php#71623 basically in lots of places where we used to be comparing to strings we might now be comparing objects and are potentially subject to these very weird fatals. Not good...

webflo’s picture

Berdir’s picture

Another follow-up: #2571593: Fatal error: Cannot use object of type Drupal\Core\GeneratedLink as array in Tableselect.php on line 220, I think this was caused by this issue too. This broke a bunch of contrib tests, most were easy to fix with some string casting( t() in array keys, t() used with strpos) but that one has to be fixed in core.

Gábor Hojtsy’s picture

It is unfortunate that this fixed #1789090: Warnings when exporting templates: addcslashes() expects parameter 1 to be string too but the commit credit from there (reyero, vijaycs85 and droplet) did not end up in this commit. (Not that this does not happen from time to time, just taking a note here).

stefan.r’s picture

Allowing this to take objects as a first parameter seems to have introduced bugs such as:

Recoverable fatal error: Object of class __PHP_Incomplete_Class could not be converted to string in Drupal\Component\Utility\SafeMarkup::isSafe() (line 70 of core/lib/Drupal/Component/Utility/SafeMarkup.php).
Drupal\Component\Utility\SafeMarkup::isSafe(Object)
Drupal\Component\Render\FormattableMarkup::placeholderEscape(Object)
Drupal\Component\Render\FormattableMarkup::placeholderFormat('%type: @message in %function (line %line of %file).', Array)
Drupal\Core\StringTranslation\TranslatableMarkup->render()
Drupal\Core\StringTranslation\TranslatableMarkup->__toString()
strip_tags(Object)
Drupal\dblog\Controller\DbLogController->overview()
call_user_func_array(Array, Array)

Seems we had an object pointing to a nonexistent class there from and older drupal version - casting to string would at least make this fail earlier.

This issue was always supposed to either cast $this->string to string or throw some kind of error if it's not (see #101) - sadly we did neither and we won't have time to get the error check into 8.0.0. Can we at least get the string cast into the RC? Wrapping other objects as input is not well tested and IMO asking for problems.

The relevant issue is #2579121: Cast TranslatableMarkup input values to string, it is inconsistent with FormattableMarkup

Gábor Hojtsy’s picture

Issue tags: -sprint

Yay for security and consistency! Thanks all for your amazing job on this one!

Status: Fixed » Closed (fixed)

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