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]
:
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.
Comment | File | Size | Author |
---|---|---|---|
#92 | page_title_escaped_with-2394951-92.patch | 11.36 KB | cilefen |
#92 | interdiff-68-92.txt | 3.99 KB | cilefen |
#88 | interdiff-52-68.txt | 10.26 KB | cilefen |
#76 | page_title_only_tests-2394951-75.patch | 11.01 KB | subhojit777 |
#68 | page_title_escaped_with-2394951-68.patch | 12.17 KB | subhojit777 |
Comments
Comment #1
Gábor HojtsyNeeds to be added the the TWIG escape issues I think? Not sure about the tag? The [ ... ] is added in ContentTranslationHandler:
So looks like the form title ends up escaped in the page title here.
Comment #2
jhodgdonYup, adding tags and related.
Comment #3
jhodgdonAh, I guess the [] is intentional, and the problem is just the usual twig safe markup escaping thing.
Comment #4
Gábor HojtsyBTW reproduced the same issue with node translation. It seems to only happen on translation editing, not creation (and not original editing).
Comment #5
subhojit777Comment #6
subhojit777This is also happening in node translations.
Comment #7
subhojit777However working alright for blocks
Comment #8
subhojit777This fixes the issue, however it breaks the page title.
Comment #9
subhojit777Well.. this simple change fixes the issue :)
Comment #10
dawehner@subhojit777
Do we really want to place the %title in
<em></em>
tags? Afaik @title would be the better placeholder to be used here.Comment #11
subhojit777@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.Comment #12
Gábor HojtsyI 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].
Comment #13
Gábor HojtsyComment #14
subhojit777Comment #15
subhojit777Now
$title
and%title
are both escaped.$title
is not emphasized. Square brackets enclosinglanguage
andtranslation
.Comment #16
Gábor HojtsyIt 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:
Comment #17
subhojit777Comment #18
subhojit777Comment #19
subhojit777The 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.
Comment #20
subhojit777Finally figured out the problem.
<em>
tags are embedded inside t() inNodeTranslationHandler.php
Rather than hardcoding the tags we should use string placeholders. We should create a follow up issue for this.
Comment #21
subhojit777Opened a new issue in #2405397: Use placeholder formatter instead of embedding HTML tags in node translation. Changing this to needs review.
Comment #22
Gábor HojtsyThanks, 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.
Comment #23
jhodgdonI do not understand why a new issue was added. The second issue seems like it is just fixing *this* issue?
Comment #24
Gábor Hojtsy@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.
Comment #25
jhodgdonOk let's just do it here. I'll close the other issue.
Comment #26
subhojit777Comment #27
Gábor HojtsyOk 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.
Comment #28
Gábor HojtsyComment #29
subhojit777herom 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.Comment #30
subhojit777Comment #31
Kristen PolI can confirm that the patch does the job but I'm unclear it's the right approach based on the discussion above.
Comment #32
Kristen PolNote that changing Spanish => espanol didn't change the result.
Comment #33
Gábor HojtsyAs 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?
Comment #34
Gábor HojtsyComment #35
DevElCuy CreditAttribution: DevElCuy commentedComment #36
DevElCuy CreditAttribution: DevElCuy commentedRemoved SprintWeekend2015Queue by mistake.
Comment #37
subhojit777@Kristen Pol Could you please explain what is the problem with this solution.
@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.
@Gábor Hojtsy
This make sure that the string is marked as safe.
So there will not be any problem if we use ! placeholder here.
Comment #38
Gábor HojtsySo 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.Comment #39
subhojit777Not 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.
Comment #40
subhojit777The 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:
%
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:Since the string within
!
placeholder is already marked as safe, soem
in%language
is not escaped.I hope I have explained it well.
Comment #41
subhojit777I had added some
var_dump()
messages insideString::format()
And this is what I got:
Comment #42
rodrigoaguileraThis 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?
Comment #43
Gábor HojtsyMakes sense. Thanks for the explanation in #40. Also updated the issue summary.
Comment #44
alexpottThis is testable bug.
Comment #45
subhojit777Comment #46
subhojit777I 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.
Comment #47
Gábor HojtsyContentTranslationUITest 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?
Comment #48
subhojit777Please 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.
Comment #50
subhojit777Can someone help me out here. I am trying to implement a new test case
doTestTranslationEdit()
inNodeTranslationUITest.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 inContentTranslationUITest.php
.This is just a suggestion. Suppose, we agree on this - exactly what we will test in
doTestTranslationEdit()
in the parent testContentTranslationUITest.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.Comment #51
cilefen CreditAttribution: cilefen commentedThe 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.
Comment #52
subhojit777Patch 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.
Comment #53
subhojit777Patch with only tests. I hope the bot catches this.
Comment #54
subhojit777Comment #55
cilefen CreditAttribution: cilefen commentedComment #56
subhojit777Terms and other entity type tests are not yet done :) See my comment #52
Comment #57
Gábor HojtsyThanks for working on this! Some notes:
Don't need to assert the 200 response, the expected title will not appear on an 500 page for example :)
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?
Expecting the wrong prior title is not needed.
Comment #58
subhojit777While 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" :(Same issue as this one
Comment #59
Gábor Hojtsy%edit adds the class no? See
String::placeholder()
.Comment #60
subhojit777Yes % adds the placeholder class. But the tag has been hard coded.
Comment #61
Gábor HojtsyRight. Why is that a problem?
Comment #62
subhojit777Embedding tags within string is not a good practice right?
That is why I have made such changes here. Otherwise, there is no problem.
Comment #63
Gábor HojtsyString::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.
Comment #64
subhojit777That code change does not affects string translation in any way, I am suggesting just for the sake of clean code.
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:
OR
Code 2:
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 approacht('<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: returnt('<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 @typeI hope you got what I was trying to make you understand :) I will revert back the old code. Sorry :)
Comment #65
Gábor HojtsyRight, 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().
Comment #66
subhojit777@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.
Comment #67
Gábor HojtsyI am sure. We don't need to reproduce the prior bogus behavior in the test. Asserting the fixed title is enough.
Comment #68
subhojit777Pattern of the title during translation edit varies for different entities:
<em>Edit @type</em> @title [%language translation]
Edit @type @title [%language translation]
@title [%language translation]
@title [%language translation]
@title [%language translation]
@title [%language translation]
There is test base class
ContentTranslationUITest
, and it provides a default test for checking HTML escape. There is also a testContentTestTranslationUITest
.Using the default test in
ContentTranslationUITest
we check the title inContentTestTranslationUITest
. Rest of the entities override the default class.Comment #69
subhojit777I 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.
Comment #70
subhojit777Now I will upload patch with "only tests".
Comment #71
subhojit777Comment #72
subhojit777What's with testbot? Does it follows some naming convention?
Comment #73
subhojit777Comment #74
subhojit777Comment #75
cilefen CreditAttribution: cilefen commentedMaybe the -test ending, but I thought it was only do-not-test https://www.drupal.org/node/332678?
Comment #76
subhojit777Comment #77
xjmThe problem is that these files are being uploaded as hidden files, with the "display" checkbox unchecked. This prevents testbot from picking them up. :)
Comment #78
rodrigoaguileraComment #79
rodrigoaguileraComment #80
rodrigoaguileraComment #84
cilefen CreditAttribution: cilefen commentedTrying to display the two relevant patches.
Comment #85
Gábor HojtsyLatest patches seem to be failing.
Comment #86
cilefen CreditAttribution: cilefen commented#68 is the patch, it applies, how is it failing?
Comment #87
Gábor Hojtsy@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.
Comment #88
cilefen CreditAttribution: cilefen commentedComment #89
Gábor HojtsyThanks a lot!
Why could this be moved back to a single t() now?
We don't use the Overrides ... scheme anymore, it should be @inheritdoc.
Comment #90
subhojit777@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?
Comment #91
Gábor HojtsyYeah, that makes sense. We only need to fix the code comments then! Thanks for keeping up the work on this one :)
Comment #92
cilefen CreditAttribution: cilefen commentedComment #93
Gábor HojtsyYay, thanks. Let's get this in then :) Thanks for sticking to it :)
Comment #94
alexpottNice 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!
Comment #96
Gábor HojtsySuperb! Thanks again for your attention to detail @subhojit777 and @cliefen!
Comment #97
subhojit777YAY!!