Page title escaped with HTML markup when editing content translation

Needs to be added the the TWIG escape issues I think? Not sure about the tag? The [ ... ] is added in ContentTranslationHandler:

      if ($form_langcode != $entity_langcode) {
        $t_args = array('%language' => $languages[$form_langcode]->getName(), '%title' => $entity->label());
        $title = empty($source_langcode) ? $title . ' [' . t('%language translation', $t_args) . ']' : t('Create %language translation of %title', $t_args);
      }
      $form['#title'] = $title;

So looks like the form title ends up escaped in the page title here.

BTW reproduced the same issue with node translation. It seems to only happen on translation editing, not creation (and not original editing).

This is also happening in node translations.

However working alright for blocks

        if (empty($source_langcode)) {
          $title = array(
            '#type' => 'inline_template',
            '#template' => '{{title}} [{{language_translation}}]',
            '#context' => array(
              'title' => $title,
              'language_translation' => t('%language translation', $t_args),
            ),
          );
        }
        else {
          $title = t('Create %language translation of %title', $t_args);
        }

This fixes the issue, however it breaks the page title.

Well.. this simple change fixes the issue :)

@subhojit777
Do we really want to place the %title in <em></em> tags? Afaik @title would be the better placeholder to be used here.

@dawehner in both cases % or @, the strings are sanitized. Title was enclosed within <em></em> in the first place, therefore I just fixed its HTML escaping - rather than altering it in the base.

+++ b/core/modules/content_translation/src/ContentTranslationHandler.php
@@ -110,7 +110,7 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
-        $title = empty($source_langcode) ? $title . ' [' . t('%language translation', $t_args) . ']' : t('Create %language translation of %title', $t_args);
+        $title = empty($source_langcode) ? t('%title [%language] translation', $t_args) : t('Create %language translation of %title', $t_args);

I see this is supposed to fix the issue because $title is $this->entityFormTitle($entity) while %title is $entity->label(), so the later is not yet escaped and therefore will not be double escaped? However %title will escape it. So how does this fix it?

Also you modified the [] wrapping to only apply to the language name which is a problem, it should apply to the whole [%language translation].

Now $title and %title are both escaped. $title is not emphasized. Square brackets enclosing language and translation.

It is true that this is fixing the language name HTML bug (although not clear to me how) but the first part of the title is still escaped:

The day when I created the patch, create content page was not opening (whether article or page), it was giving error. I created the patch after testing in taxonomy page.

Finally figured out the problem. <em> tags are embedded inside t() in NodeTranslationHandler.php

  protected function entityFormTitle(EntityInterface $entity) {
    $type_name = node_get_type_label($entity);
    return t('<em>Edit @type</em> @title', array('@type' => $type_name, '@title' => $entity->label()));
  }

Rather than hardcoding the tags we should use string placeholders. We should create a follow up issue for this.

Opened a new issue in #2405397: Use placeholder formatter instead of embedding HTML tags in node translation. Changing this to needs review.

Thanks, we can consider that yet one more bug. That does not answer my question how $title . t(...) changed to t(@title...) fixes the escaping? You are not actually changing anything about the %language placeholder that used to generate the markup error this issue was opened to resolve.

@jhodgdon: I think the separation was the first part of the string and the second part. I agree it would likely be easier to resolve at once.

Ok I finally got up to speed on how the safemarkup registry works, so I know why the existing patch works. Yay for better educated Gábor :D Sorry for complaining about it. However fixing the visibe em tag is still todo.

herom gave nice explanation on what is causing the problem. See #2319233: Double escaped string on Available translation update page comment. Fixing the <em> tag issue.

I can confirm that the patch does the job but I'm unclear it's the right approach based on the discussion above.

Note that changing Spanish => espanol didn't change the result.

+++ b/core/modules/content_translation/src/ContentTranslationHandler.php
@@ -132,8 +132,8 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
+        $title = empty($source_langcode) ? t('!title [%language translation]', $t_args) : t('Create %language translation of %title', $t_args);

As per #2399037: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument strings with ! placeholders will not end up as safe strings anymore (recent change). How is this not escaped then?

Removed SprintWeekend2015Queue by mistake.

I can confirm that the patch does the job but I'm unclear it's the right approach based on the discussion above. #31

@Kristen Pol Could you please explain what is the problem with this solution.

Note that changing Spanish => espanol didn't change the result. #32

@Kristen Pol I do not understand what you mean here. I was testing with German language and it said "some title [German translation]". Are you talking about translation the word Spanish as well.

As per #2399037: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument strings with ! placeholders will not end up as safe strings anymore (recent change). How is this not escaped then? #33

@Gábor Hojtsy

1. +++ b/core/modules/node/src/NodeTranslationHandler.php
   @@ -68,7 +69,10 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
   +    return String::format('%edit @title', array(
   +      '%edit' => t('Edit @type', array('@type' => $type_name)),
   +      '@title' => $entity->label(),
   +    ));
   

   This make sure that the string is marked as safe.

2. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -132,8 +132,8 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
   +        $t_args = array('%language' => $languages[$form_langcode]->getName(), '%title' => $entity->label(), '!title' => $title);
   +        $title = empty($source_langcode) ? t('!title [%language translation]', $t_args) : t('Create %language translation of %title', $t_args);
   

   So there will not be any problem if we use ! placeholder here.

So what I am not sure about is t('!title [%language translation]', $t_args) Based on TranslationManager::translate() this is passed on to String::format() after the string is translated. Due to #2399037: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument, String::format() will not mark it as a safe string due to the !title placeholder so the em in %language should be escaped, no? What am I missing? I am sure I am missing something based on my prior experience with these issues, so sorry to stretch the issue, not trying to give you a hard time, just trying to understand it, so I am confident we are not introducing a security issue.

Not any issue @Gábor Hojtsy :) I am very much newer to Drupal 8 system than you are. I have created the patch based on the changes I did in related issues. I will find out more on this and will let you know.

The string is already marked as secure.

Suppose we have this edit title: "Edit Basic page Wie geht es dir [German translation]".
"Edit Basic page Wie geht es dir" is the first part coming from $title = $this->entityFormTitle($entity);(!title). "[German translation]" is the second part coming from [%language translation].

This generates the first part of edit title:

  /**
   * {@inheritdoc}
   */
  protected function entityFormTitle(EntityInterface $entity) {
    $type_name = node_get_type_label($entity);
    return String::format('%edit @title', array(
      '%edit' => t('Edit @type', array('@type' => $type_name)),
      '@title' => $entity->label(),
    ));
  }

% and @ placeholders make the string "safe". So we have "Edit Basic page Wie geht es dir" as safe enclosed within placeholders.

So while generating the edit title, we already have "Edit Basic page Wie geht es dir" as safe string.
Since the string is passed through String::format(), we have:

case '!':
        // Pass-through.
        if (!SafeMarkup::isSafe($value)) {
          $safe = FALSE;
        }

Since the string within ! placeholder is already marked as safe, so em in %language is not escaped.

I hope I have explained it well.

I had added some var_dump() messages inside String::format()

  public static function format($string, array $args = array()) {
    $safe = true;

    // transform arguments before inserting them.
    foreach ($args as $key => $value) {
      switch ($key[0]) {
        case '@':
          // escaped only.
          $args[$key] = static::checkplain($value);
          break;

        case '%':
        default:
          // escaped and placeholder.
          $args[$key] = static::placeholder($value);
          break;

        case '!':
          // pass-through.
          if (!safemarkup::issafe($value)) {
            $safe = false;
          }
      }
    }

    $output = strtr($string, $args);
    if ($safe) {
      safemarkup::set($output);
    }
    var_dump($args);
    var_dump($safe ? 'true' : 'false');
    var_dump($output);

    return $output;
  }

And this is what I got:

This issue was commented in today D8MI meeting but I don't know what is left to review. Anyone knows what to do to move this forward?

Makes sense. Thanks for the explanation in #40. Also updated the issue summary.

This is testable bug.

I have seen the existing tests of content translation module. For this issue, it seems like new tests cannot be build on the existing structure of the tests. The existing tests work with some custom entity created dynamically during the test. FYI, I was seeing ContentTranslationUITest.php.

I was saying that, we need to write a separate web test case for this. I am new to this, sorry if I am missing something obvious. Any help would be appreciated. Thanks.

ContentTranslationUITest has at least some DrupalPostForms to the content translation edit pages which are to be tested here, so that can be extended with a couple lines, no?

Please review the test case in the patch. I have added tests for node translation only. Term translation tests are not yet done, I will soon add them, but the logic will be same as I have used for node testing.

Just to see that the test logic is fullproof, I have also attached a patch that includes only the test and not the code fixes, that patch should fail testing.

I am not removing "needs tests" tag because term testing is not yet done.

Can someone help me out here. I am trying to implement a new test case doTestTranslationEdit() in NodeTranslationUITest.php. But it seems like other entities like term, block, etc. may also need this test. In that case we need to add this test case in ContentTranslationUITest.php.

This is just a suggestion. Suppose, we agree on this - exactly what we will test in doTestTranslationEdit() in the parent test ContentTranslationUITest.php, I mean the edit title for every entity is different. Yes, we will obviously override the parent test case in individual test, but what exactly will be the common test to carry out in the parent test, can it be say... 200 response test.

The problems in #48, the full patch, are just inheritance-related. Consider putting the test method on the parent class, to be used as necessary in the child tests.

Patch made as per #51. Please review this patch, I am not sure whether the new test added in parent class is the right approach. And yes, term tests have not been included, and we need to add tests for all other entity types.

Patch with only tests. I hope the bot catches this.

Terms and other entity type tests are not yet done :) See my comment #52

Thanks for working on this! Some notes:

1. +++ b/core/modules/node/src/Tests/NodeTranslationUITest.php
   @@ -373,4 +374,43 @@ protected function doUninstallTest() {
   +        $this->assertResponse(200, 'Translation edit page is alright');
   

   Don't need to assert the 200 response, the expected title will not appear on an 500 page for example :)

2. +++ b/core/modules/node/src/Tests/NodeTranslationUITest.php
   @@ -373,4 +374,43 @@ protected function doUninstallTest() {
   +        $title = String::format('%edit @title', array(
   +          '%edit' => t('Edit @type', array('@type' => $type_name)),
   +          '@title' => $entity->getTranslation($langcode)->label(),
   +        ));
   +        $title = t('!title [%language translation]', array(
   +          '!title' => $title,
   +          '%language' => $languages[$langcode]->getName(),
   +        ));
   +        $this->assertRaw($title);
   

   Reusing the $title variable here makes it a bit confusing...

   Also is this not the same title pattern expected for other entities too? It sounds like it would make sense to abstract the expected title then into a method and implement that for each type?

3. +++ b/core/modules/node/src/Tests/NodeTranslationUITest.php
   @@ -373,4 +374,43 @@ protected function doUninstallTest() {
   +        $title = t('<em>Edit @type</em> @title', array(
   +          '@type' => $type_name,
   +          '@title' => $entity->getTranslation($langcode)->label(),
   +        ));
   +        $title = t('!title [%language translation]', array(
   +          '!title' => $title,
   +          '%language' => $languages[$langcode]->getName(),
   +        ));
   +        $this->assertNoRaw(String::checkPlain($title));
   

   Expecting the wrong prior title is not needed.

While I was testing ContentTranslationUITest, and I clicked verbose message to check how translation edit page looks, and then I saw this:

Looks like the <em> tag has been hard-coded somewhere, because if % placeholder was used then there would be a class "placeholder" :(

+++ b/core/modules/node/src/NodeTranslationHandler.php
@@ -68,7 +69,10 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
-    return t('<em>Edit @type</em> @title', array('@type' => $type_name, '@title' => $entity->label()));
+    return String::format('%edit @title', array(
+      '%edit' => t('Edit @type', array('@type' => $type_name)),
+      '@title' => $entity->label(),
+    ));

Same issue as this one

%edit adds the class no? See String::placeholder().

Yes % adds the placeholder class. But the tag has been hard coded.

Right. Why is that a problem?

Embedding tags within string is not a good practice right?

+++ b/core/modules/node/src/NodeTranslationHandler.php
@@ -68,7 +69,10 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
-    return t('<em>Edit @type</em> @title', array('@type' => $type_name, '@title' => $entity->label()));
+    return String::format('%edit @title', array(
+      '%edit' => t('Edit @type', array('@type' => $type_name)),
+      '@title' => $entity->label(),
+    ));

That is why I have made such changes here. Otherwise, there is no problem.

String::placeholder() does not translate anything, so why is it a problem it has a tag included? I think we are wasting our time here. Can you post a step by step list of why is something wrong (not sure we it is clear to me which code you think is wrong where) and that causes what elsewhere? I'm totally puzzled.

That code change does not affects string translation in any way, I am suggesting just for the sake of clean code.

so why is it a problem it has a tag included?

You got this right. It has a tag included that is the problem, I mean just code level problem. You can consider this as code refactoring. Using <em> tag instead of % will not affect the purpose of this issue.

I am going to show you an example.

Code 1:

print t('<em>I am translatable</em>');

OR

Code 2:

print t('%translatable_string', array('%translatable_string' => t('I am translatable')));

My opinion choose Code 2, and that is what I am suggesting here.

While I was writing this.. timplunkett answered my query in IRC
<subhojit777> suppose I want to print a string within <em> tags, and also want the string to be translatable, I mean enclose it within t(). What would be the best approach t('<em>Translatable string</em>') OR t('%string', array('%string' => t('Translatable string')))
<subhojit777> I am asking this in reference to this issue https://www.drupal.org/node/2394951#comment-9602489 (this issue)
<timplunkett> subhojit777: no embedding tags is fine
<timplunkett> subhojit777: return t('<em>Edit @type</em> @title', array('@type' => $type_name, '@title' => $entity->label())); is fine
<timplunkett> subhojit777: <em> and <em class="placeholder"> are different
<subhojit777> timplunkett, Is there any significant difference between them, apart from the class
<timplunkett> subhojit777: that, and its harder for translators because they have less context
<timplunkett> <em>Edit @type</em> @title is different than Edit @type

I hope you got what I was trying to make you understand :) I will revert back the old code. Sorry :)

Right, context is good. If you include some context, that helps translators. If it requires some markup, that is doable. We try to avoid markup but not to the extend of wrapping a t() in a t().

@Gábor you sure we dont need #57.3? An assertNoRaw "pessimistic" check would be good here, that is a full-proof that the problem indeed is fixed.

I am sure. We don't need to reproduce the prior bogus behavior in the test. Asserting the fixed title is enough.

Pattern of the title during translation edit varies for different entities:

• Block: <em>Edit @type</em> @title [%language translation]
• Comment: Edit @type @title [%language translation]
• Menu link: @title [%language translation]
• Shortcut: @title [%language translation]
• Term: @title [%language translation]
• User: @title [%language translation]

There is test base class ContentTranslationUITest, and it provides a default test for checking HTML escape. There is also a test ContentTestTranslationUITest.

Using the default test in ContentTranslationUITest we check the title in ContentTestTranslationUITest. Rest of the entities override the default class.

I identified these entities from this failing test https://qa.drupal.org/pifr/test/970173, and I manually checked them - their edit translation title were indeed breaking.

Now I will upload patch with "only tests".

What's with testbot? Does it follows some naming convention?

Maybe the -test ending, but I thought it was only do-not-test https://www.drupal.org/node/332678?

The problem is that these files are being uploaded as hidden files, with the "display" checkbox unchecked. This prevents testbot from picking them up. :)

Trying to display the two relevant patches.

Latest patches seem to be failing.

#68 is the patch, it applies, how is it failing?

@cilefen/@subhojit777 ok I was majorly confused by the back and forths on this issue. Can we get an interdiff for the last patch then? I reviewed the one before.

Thanks a lot!

1. +++ a/core/modules/node/src/NodeTranslationHandler.php
   @@ -69,10 +68,7 @@
   +    return t('<em>Edit @type</em> @title', array('@type' => $type_name, '@title' => $entity->label()));
   -    return String::format('%edit @title', array(
   -      '%edit' => t('Edit @type', array('@type' => $type_name)),
   -      '@title' => $entity->label(),
   -    ));
   

   Why could this be moved back to a single t() now?

2. +++ b/core/modules/node/src/Tests/NodeTranslationUITest.php
   @@ -51,7 +50,7 @@
   +   * Overrides \Drupal\content_translation\Tests\ContentTranslationUITest::testTranslationUI().
   
   +++ b/core/modules/block_content/src/Tests/BlockContentTranslationUITest.php
   @@ -163,4 +163,28 @@ public function testDisabledBundle() {
   +   * Overrides \Drupal\content_translation\Tests\ContentTranslationUITest::assertTranslationEdit().
   
   +++ b/core/modules/comment/src/Tests/CommentTranslationUITest.php
   @@ -182,4 +182,28 @@ function testTranslateLinkCommentAdminPage() {
   +   * Overrides \Drupal\content_translation\Tests\ContentTranslationUITest::assertTranslationEdit().
   
   +++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentUITest.php
   @@ -94,4 +94,27 @@ function testTranslationLinkTheme() {
   +   * Overrides \Drupal\content_translation\Tests\ContentTranslationUITest::assertTranslationEdit().
   
   +++ b/core/modules/shortcut/src/Tests/ShortcutTranslationUITest.php
   @@ -79,4 +79,27 @@ protected function doTestBasicTranslation() {
   +   * Overrides \Drupal\content_translation\Tests\ContentTranslationUITest::assertTranslationEdit().
   
   +++ b/core/modules/taxonomy/src/Tests/TermTranslationUITest.php
   @@ -139,4 +139,27 @@ function testTranslateLinkVocabularyAdminPage() {
   +   * Overrides \Drupal\content_translation\Tests\ContentTranslationUITest::assertTranslationEdit().
   
   +++ b/core/modules/user/src/Tests/UserTranslationUITest.php
   @@ -54,4 +54,27 @@ protected function getNewEntityValues($langcode) {
   +   * Overrides \Drupal\content_translation\Tests\ContentTranslationUITest::assertTranslationEdit().
   

   We don't use the Overrides ... scheme anymore, it should be @inheritdoc.

@Gábor @cilefen Thanks! Could you please explain more here #89.1

Earlier I was thinking that embedding <em> tags in t() is a wrong thing to do. Therefore, I made the patch using % placeholder.
I asked in IRC regarding this and timplunkett said that, there is no problem using <em> tags inside t(), and they should be used otherwise the translator will lose context (see comment #64). And therefore I reverted back to the old code.

Is this what you are asking?

Yeah, that makes sense. We only need to fix the code comments then! Thanks for keeping up the work on this one :)

Yay, thanks. Let's get this in then :) Thanks for sticking to it :)

Nice amount of tests! This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 87ba2da and pushed to 8.0.x. Thanks!

Superb! Thanks again for your attention to detail @subhojit777 and @cliefen!

YAY!!