Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Untranslatable fields are extended with a "all translations" hint to make sure the editor understands the affect of changing it.
Since the recent change in SafeMarkup, the UI with "all languages" output is broken.
#2506195: Remove SafeMarkup::set() from Xss::filter()
Proposed resolution
Follow the change, improve test coverage.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#25 | html_escaped_fix_revision.patch | 409 bytes | samimalik |
#23 | html_is_escaped_in_all-2532284-23.patch | 931 bytes | Peacog |
#22 | all_languages_markup_in_detail_title.patch | 1007 bytes | ayalon |
#21 | Screen Shot 2016-02-29 at 09.55.49.png | 10.88 KB | reekris |
#16 | html_is_escaped_in_all-2532284-16-interdiff.txt | 4.47 KB | sasanikolic |
Comments
Comment #1
cilefen CreditAttribution: cilefen commentedIt could be one of the methods in #2501975: Determine how to update code that currently joins strings in SafeMarkup::set() will fix it.
Comment #2
cilefen CreditAttribution: cilefen commentedComment #3
miro_dietikerThis does not resolve neither of both cases in screenshot above for me.
Comment #4
cilefen CreditAttribution: cilefen commentedHa - I didn't actually test it. Could you provide steps to reproduce. I know little about content translation. How do I get that HTML to appear?
Comment #5
joelpittet@cilefen I think your solution there is fine, but the real problem is further down the stack.
Xss::filterAdmin
intemplate_preprocess_form_element_label()
.Should be using
SafeMarkup
as they are now separated nicely for their appropriate domains:)Comment #6
joelpittetI didn't need the extra parens in there and twig with strict off doesn't need to initialize the variables, so this is a bit cleaner.
Since @alexpott worked hard on the other one, it would be good to get his opinion here. Also SafeMarkup::checkAdminXss() looks like a good use case here but it's been deprecated also.
Comment #7
joelpittetHere's some manual testing on simplytest.me:
Comment #8
alexpottI don't think we need to be doing the escaping here
['#markup' => $element['#title']]
should make the render system do this later.Comment #9
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedHere's the updated patch and a screenshot.
Comment #10
BerdirI think this addressed the feedback from @alexpott, but we still need review.
We can probably just add some assertRaw statements to the existing content translation UI tests?
Comment #11
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedAdded the test (as suggested in #10).
Comment #13
olli CreditAttribution: olli commentedHow is this different from #2501975: Determine how to update code that currently joins strings in SafeMarkup::set()? It looks like the source of this is the concatenation in ContentTranslationHandler::addTranslatabilityClue().
Comment #14
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedComment #15
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedFixed the failing tests.
Comment #16
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedRerolled and inverted the flag.
Comment #17
mErilainen CreditAttribution: mErilainen at Wunder commentedFixes the rendered html and tests pass now.
Comment #18
alexpottThis issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed bfb0580 and pushed to 8.0.x. Thanks!
Comment #21
reekris CreditAttribution: reekris commentedI'm still having this issue in the details field using Drupal 8.0.4. HTML is escaped in the details title. See attached screenshot.
Comment #22
ayalon CreditAttribution: ayalon commentedThe bug is not fully resolved as you forgot to also fix the template_preprocess_details(9 function. I made a patch. This should be fixed aswell
Comment #23
Peacog CreditAttribution: Peacog as a volunteer commentedThanks for the new patch @ayalon. I've re-rolled it and set version to 8.1.x. Let's see if it passes tests.
Comment #24
joelpittetTry to avoid re-opening issues, it will get confusing to what the issue was trying to solve. Instead create a follow-up. But first try to see if an existing issue exists.
This exact patch is here #2652850: Title for details form elements is not set as '#markup' and it will be escaped, but all other form elements use '#markup' and are not escaped. It's good to post a comment to cross-reference if they are related.
Comment #25
samimalik CreditAttribution: samimalik commentedThe last patch did not completely fix the issue, if you tried to go and modify content (a page with a form field) or edit any form an error occurs that causes the site to break. Fixed.
Comment #26
handkerchiefon date fields the label problem still exists. For more details follow the issue linked in #24