Improve docs for the Xss::filter() $html_tags parameter

Adding patch.

We can make step further and rename the parameter to $allowable_tags (same as in strip_tags PHP function).

Discussed this issue at a documentation triage meeting, myself and ambermatz were present. We agreed to un-assign this since it has not been worked on since 2016, that this is a task, that "An array of allowed html tags" is preferred. I have updated the IS.

I like the idea of changing the variable name to $allowable_tags as suggested in #4.

Made changes as per comment #14.

I think we could also use $allowable_html_tags instead of $allowable_tags.

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

Think a good middle ground would be to use allowable_html_tags per #16.

If someone could make the quick change. I can mark it.

Tried to address the changes mentioned in #18.
Changed only for first method.

Thanks!

+++ b/core/lib/Drupal/Component/Utility/Xss.php
@@ -45,8 +45,8 @@ class Xss {
+   * @param array $allowable_html_tags

The value can also be Null, so "array|null". But I believe the array is a sting array, if that is correct, this is then "string[]|null"

Updated the patch as per #23

Thanks

Random

Update the #23 try to resolve the assertion failure.

27 was not needed as the failure in 23 was random

#23 looks good but no longer applies.

#23 was not applying. I have tried creating a new patch. It works for me, please review the same.

Raised an MR for the same https://git.drupalcode.org/project/drupal/-/merge_requests/3946

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

LGTM, but needs an issue summary update in proposed resolution and remaining tasks. Tagging for that.

I also wonder, since we now use "allowable" in the variable name, shouldn't we also use "allowable" in the parameter description instead of "allowed"? But I'm not a native speaker, so I'm not really sure about that one. Also, as a non-native speaker, I had to look up "allowable" in a dictionary whereas I knew what "allowed" was, even though I think my English is not that bad actually. But that's just me. After having done that, I agree that it is probably the more accurate term to use, though.

I think allowed_html_tags reads better.

Updated the patch as per #34 and #35

Thanks. Let's update the issue status to "Needs review" so that the tests will run.

Updated the IS.

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Thanks. The code looks good, #34 and #35 are addressed, the tests pass. I also tested the patch manually on 10.1.x-dev. I think this is RTBC.

Random

Triggered a re-test.

@smustgrave You know there is a typo in your issue template? :-) #need-reveiw-queue (reveiw -> review)

Another random test failure, retest was green, see #3361121: [random test failure] InstallerExistingConfig[SyncDirectory]MultilingualTest::testConfigSync, back to RTBC per #39.

Triggered retest

Triaging the RTBC queue.

Thank you @FeyP, for asking for an Issue Summary update. That helps a great deal!

I see that catch thought the patch in #23 was good and after that it was rerolling. It is confusing when there is both a patch workflow and an MR on issue. :-( It looks like the patch in #36 is the one to use. However, there are no interdiffs for that or the previous patch. I have added one.

All the issues have been addressed and this is good to go. I have updated credit, including myself and @Amber Himes Matz due to the triage work done in a documentation meeting.

@bharath-kondeti, Thanks for helping on this issue. To help reviewers always add an interdiff, or a diff, whichever is appropriate. There are instructions for creating an interdiff.

+++ b/core/lib/Drupal/Component/Utility/Xss.php
@@ -45,8 +45,8 @@ class Xss {
+   *   An array of allowed html tags.

"HTML" in the comment should be in upper case.

Paused to consider the backport here; while this is largely just a docs fix, we are also renaming a variable, and if someone is using named arguments this will break their code. But I think the chances of anyone actually using named arguments here is slim, and so I opted for the backport to keep the branches in sync and improve documentation for 10.1.x users.

Committed and pushed 8624038a9a to 11.x and 02c6babe8b to 10.1.x. Thanks!