Problem/Motivation

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...

The attribute description "(string, optional) A string containing the textual contents of the tag" does not tell the reader whether HTML escaping is performed. I'd prefer instead:

#value: (HTML string, optional) The markup content of the tag. No escaping is performed on output.

Steps to reproduce

Proposed resolution

See text in #25

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#27 3215627-25.patch933 bytesguilhermevp
#27 interdiff_24-25.txt1019 bytesguilhermevp
#24 interdiff_3215627_22-24.txt868 bytesankithashetty
#24 3215627-24.patch820 bytesankithashetty
#22 drupal_core-htmltag-doc-improvement-3215627-22.patch820 bytesilgnerfagundes
#20 evidence-tag.jpeg143.1 KBilgnerfagundes
patch_apply-2.png42.13 KBvikashsoni
patch_apply.png27.62 KBvikashsoni
#17 interdiff_10-17.txt935 bytesvarshith
#17 drupal_core-htmltag-doc-improvement-3215627-17.patch869 bytesvarshith
#14 drupal_core-htmltag-doc-improvement-3215627-14.patch868 bytesvarshith
#10 interdiff_7-10.txt735 bytesrahulkhandelwal1990
#10 3215627-10.patch738 bytesrahulkhandelwal1990
#7 3215627-7.patch708 bytesthiagorw
#4 3215627_4.patch707 bytespragati_kanade

Issue fork drupal-3215627

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mrclay created an issue. See original summary.

cilefen’s picture

cilefen commented
Issue tags: -html output, -escaping HTML, -xss +Novice
pragati_kanade’s picture

pragati_kanade commented
Assigned: Unassigned » pragati_kanade
pragati_kanade’s picture

pragati_kanade commented
Status: Active » Needs review
StatusFileSize
new 3215627_4.patch707 bytes

Hi @mrclay
I created patch.Please review.

thiagorw’s picture

thiagorw commented
Status: Needs review » Reviewed & tested by the community

Hi
I did the review the comment is as suggested on this Issue

cilefen’s picture

cilefen commented
Status: Reviewed & tested by the community » Needs work

I think we need a space after periods.

thiagorw’s picture

thiagorw commented
Status: Needs work » Needs review
StatusFileSize
new 3215627-7.patch708 bytes

I insert a space after periods.
Please review it.

pragati_kanade’s picture

pragati_kanade commented
Assigned: pragati_kanade » Unassigned
Status: Needs review » Reviewed & tested by the community

I reviewed patch #7 It looks good to me.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -16,7 +16,7 @@
- *   the tag.
+ *   the tag. No escaping is performed on output.

This is not accurate, if you look at ::preRenderHtmlTag you can see it does Xss::filterAdmin

rahulkhandelwal1990’s picture

rahulkhandelwal1990 commented
StatusFileSize
new 3215627-10.patch738 bytes
new interdiff_7-10.txt735 bytes

Description updated in the new patch, please verify.

rahulkhandelwal1990’s picture

rahulkhandelwal1990 commented
Status: Needs work » Needs review
cilefen’s picture

cilefen commented
Status: Needs review » Needs work

I think it would make sense to mention \Drupal\Component\Utility\Xss::filterAdmin() precisely. Also more clarity on "if it is not safe" is needed here. What does that actually mean?

cilefen’s picture

cilefen commented

I think that means "if it is not marked as a safe string".

varshith’s picture

varshith commented
StatusFileSize
new drupal_core-htmltag-doc-improvement-3215627-14.patch868 bytes

Updating it to me more clear based on similar comments found in core elsewhere.

varshith’s picture

varshith commented
Status: Needs work » Needs review
rahulkhandelwal1990’s picture

rahulkhandelwal1990 commented

@varshith Please upload interdiff along with patch(in #14 it is missing), It will help to understand the changes you have done. See how to create interdiff here https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

varshith’s picture

varshith commented
StatusFileSize
new drupal_core-htmltag-doc-improvement-3215627-17.patch869 bytes
new interdiff_10-17.txt935 bytes

@rahulkhandelwal1990
I didn't add interdiff as this is a very small change. Adding it now.
Also updating patch to add grammatical fix.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

ilgnerfagundes’s picture

ilgnerfagundes commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new evidence-tag.jpeg143.1 KB

Patch #17 works correctly and changes the description correctly. Changing status to rtbc

cilefen’s picture

cilefen commented
Status: Reviewed & tested by the community » Needs work

The sentence is missing a definite article. How about instead “This will be XSS admin filtered unless it is an object implementing MarkupInterface.”?

Please stop posting screenshots of code and diffs.

ilgnerfagundes’s picture

ilgnerfagundes commented
Status: Needs work » Needs review
StatusFileSize
new drupal_core-htmltag-doc-improvement-3215627-22.patch820 bytes

updating patch

cilefen’s picture

cilefen commented
Status: Needs review » Needs work
ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented
Status: Needs work » Needs review
StatusFileSize
new 3215627-24.patch820 bytes
new interdiff_3215627_22-24.txt868 bytes

Just fixed custom command failure error in #22, thanks!

cilefen’s picture

cilefen commented

"#value: (string, optional) A string containing the textual contents of the tag." is actually wrong. That should probably be "#value: (string|MarkupInterface, optional) The textual contents of the tag. Strings will be XSS admin filtered."

guilhermevp’s picture

guilhermevp commented
Assigned: Unassigned » guilhermevp
guilhermevp’s picture

guilhermevp commented
StatusFileSize
new interdiff_24-25.txt1019 bytes
new 3215627-25.patch933 bytes

Sending patch following @celifen suggestions.

Please review.

guilhermevp’s picture

guilhermevp commented
Assigned: guilhermevp » Unassigned
cilefen’s picture

cilefen commented
Status: Needs review » Needs work

That’s close but it is slightly different.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone made their first commit to this issue’s fork.

quietone opened merge request !9138

quietone’s picture

quietone commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Novice

Let's finish this one.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems straight forward enough.

  • longwave committed 8069f003 on 10.3.x 
    Issue #3215627 by guilhermevp, varshith, rahulkhandelwal1990,...

  • longwave committed af2a2c5a on 10.4.x 
    Issue #3215627 by guilhermevp, varshith, rahulkhandelwal1990,...

  • longwave committed 658f18f9 on 11.0.x 
    Issue #3215627 by guilhermevp, varshith, rahulkhandelwal1990,...

  • longwave committed 11996c7a on 11.x 
    Issue #3215627 by guilhermevp, varshith, rahulkhandelwal1990,...

longwave closed merge request !9138

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Backported to 10.3.x as a docs only fix.

Committed and pushed 11996c7a16 to 11.x and 658f18f920 to 11.0.x and af2a2c5a14 to 10.4.x and 8069f0030a to 10.3.x. Thanks!

Status: Fixed » Closed (fixed)

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