Problem/Motivation

When editing translations of content entities, the page title is transformed with an added note on the translation language. This results in HTML double-escaping issues, and the title appears as gatos [<em class="placeholder">Espanol</em> translation]:

Screen shot of unescaped HTML at top of translation page

Proposed resolution

Apply proper safe markup handling to avoid this issue.

Remaining tasks

Commit.

User interface changes

Title is not double escaped:

API changes

None.

Files: 
CommentFileSizeAuthor
#92 page_title_escaped_with-2394951-92.patch11.36 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,212 pass(es). View
#92 interdiff-68-92.txt3.99 KBcilefen
#88 interdiff-52-68.txt10.26 KBcilefen
#76 page_title_only_tests-2394951-75.patch11.01 KBsubhojit777
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,736 pass(es), 15 fail(s), and 0 exception(s). View
#68 page_title_escaped_with-2394951-68.patch12.17 KBsubhojit777
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,742 pass(es). View

Comments

Gábor Hojtsy’s picture

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.

jhodgdon’s picture

jhodgdon’s picture

Ah, I guess the [] is intentional, and the problem is just the usual twig safe markup escaping thing.

Gábor Hojtsy’s picture

Title: Bad page title with [placeholder=...] after translating taxonomy term » Page title escaped with HTML markup when editing content translation
Issue summary: View changes
FileSize
19.91 KB

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

subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

subhojit777’s picture

subhojit777’s picture

        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.

subhojit777’s picture

Assigned: subhojit777 » Unassigned
Status: Active » Needs review
FileSize
1011 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,784 pass(es). View

Well.. this simple change fixes the issue :)

dawehner’s picture

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

subhojit777’s picture

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

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ 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].

Gábor Hojtsy’s picture

Issue tags: +language-content
subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

Assigned: subhojit777 » Unassigned
Status: Needs work » Needs review
FileSize
1.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,388 pass(es). View
1.15 KB

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

Gábor Hojtsy’s picture

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:

subhojit777’s picture

Issue tags: +dcdelhi
subhojit777’s picture

Status: Needs review » Needs work
subhojit777’s picture

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.

subhojit777’s picture

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('Edit @type @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.

subhojit777’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Postponed (maintainer needs more info)

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’s picture

I do not understand why a new issue was added. The second issue seems like it is just fixing *this* issue?

Gábor Hojtsy’s picture

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

jhodgdon’s picture

Status: Postponed (maintainer needs more info) » Needs work

Ok let's just do it here. I'll close the other issue.

subhojit777’s picture

Assigned: Unassigned » subhojit777
Gábor Hojtsy’s picture

Issue tags: +sprint

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.

Gábor Hojtsy’s picture

Issue tags: +SprintWeekend2015Queue
subhojit777’s picture

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.

subhojit777’s picture

Assigned: subhojit777 » Unassigned
Status: Needs work » Needs review
FileSize
2.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,253 pass(es). View
2.12 KB
Kristen Pol’s picture

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

Kristen Pol’s picture

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

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Issue tags: +SprintWeekend2015
develCuy’s picture

Issue tags: -SprintWeekend2015Queue
develCuy’s picture

Issue tags: +SprintWeekend2015Queue

Removed SprintWeekend2015Queue by mistake.

subhojit777’s picture

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.

Gábor Hojtsy’s picture

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.

subhojit777’s picture

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.

subhojit777’s picture

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.

subhojit777’s picture

FileSize
38.38 KB

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:

rodrigoaguilera’s picture

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?

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

This is testable bug.

subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

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.

Gábor Hojtsy’s picture

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?

subhojit777’s picture

Status: Needs work » Needs review
FileSize
2.76 KB
4.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,122 pass(es), 7 fail(s), and 0 exception(s). View
2.76 KB

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.

The last submitted patch, 48: page_title_escaped_with-2394951-48.patch, failed testing.

subhojit777’s picture

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.

cilefen’s picture

Status: Needs review » Needs work

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.

subhojit777’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
5.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,279 pass(es). View

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.

subhojit777’s picture

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

subhojit777’s picture

Status: Needs review » Needs work
cilefen’s picture

Status: Needs work » Needs review
subhojit777’s picture

Status: Needs review » Needs work

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

Gábor Hojtsy’s picture

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.

subhojit777’s picture

Issue summary: View changes
FileSize
72.43 KB

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

Gábor Hojtsy’s picture

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

subhojit777’s picture

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

Gábor Hojtsy’s picture

Right. Why is that a problem?

subhojit777’s picture

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.

Gábor Hojtsy’s picture

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.

subhojit777’s picture

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('I am translatable');

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

Gábor Hojtsy’s picture

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

subhojit777’s picture

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

Gábor Hojtsy’s picture

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

subhojit777’s picture

Status: Needs work » Needs review
FileSize
12.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,742 pass(es). View

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.

subhojit777’s picture

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.

subhojit777’s picture

Status: Needs review » Needs work

Now I will upload patch with "only tests".

subhojit777’s picture

Status: Needs work » Needs review
FileSize
11.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,669 pass(es), 14 fail(s), and 0 exception(s). View
subhojit777’s picture

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

subhojit777’s picture

Status: Needs review » Needs work
subhojit777’s picture

Status: Needs work » Needs review
FileSize
11.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,665 pass(es), 14 fail(s), and 0 exception(s). View
cilefen’s picture

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

subhojit777’s picture

FileSize
11.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,736 pass(es), 15 fail(s), and 0 exception(s). View
xjm’s picture

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

rodrigoaguilera’s picture

rodrigoaguilera’s picture

Status: Needs review » Needs work
rodrigoaguilera’s picture

Status: Needs work » Needs review

The last submitted patch, 71: page_title_escaped_with-2394951-71-only-tests.patch, failed testing.

The last submitted patch, 74: page_title_escaped_with_only_tests-2394951-74.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 76: page_title_only_tests-2394951-75.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review

Trying to display the two relevant patches.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Latest patches seem to be failing.

cilefen’s picture

Status: Needs work » Needs review

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

Gábor Hojtsy’s picture

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

cilefen’s picture

FileSize
10.26 KB
Gábor Hojtsy’s picture

Status: Needs review » Needs work

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.

subhojit777’s picture

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

Gábor Hojtsy’s picture

Issue tags: -Needs tests

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

cilefen’s picture

Status: Needs work » Needs review
FileSize
3.99 KB
11.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,212 pass(es). View
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed 87ba2da on 8.0.x
    Issue #2394951 by subhojit777, cilefen, Gábor Hojtsy, Kristen Pol,...
Gábor Hojtsy’s picture

Issue tags: -sprint

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

subhojit777’s picture

YAY!!

Status: Fixed » Closed (fixed)

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