Remove uses of deprecated XSS filter functions

reroll + update patch.
reverted two cases of filter_xss() → Xss:filterAdmin(), and fixed a mismatched route.

Updated issue summary.

The patch needs to be rebased because it doesn't apply.

reroll.

reroll

For some reason some of the conflicts didn't show in SourceTree. Here's a reroll with all the conflicts fixed.

Patch looks good, remaining instances of filter_xss_admin after applying are:

$ git grep filter_xss_admin
core/includes/common.inc:function filter_xss_admin($string) {
core/includes/errors.inc:    if (!function_exists('filter_xss_admin')) {

RTBC if bot runs green.

2089433-21.patch no longer applies.

error: patch failed: core/modules/forum/forum.module:6
error: core/modules/forum/forum.module: patch does not apply

Rerolled with updated context in forum.module following String::checkPlain() changes.

24: 2089433-24.patch queued for re-testing.

Testbot ran out of disk space

2089433-24.patch no longer applies.

error: patch failed: core/themes/seven/seven.theme:7
error: core/themes/seven/seven.theme: patch does not apply

rerolled (auto-merged).

2089433-29_0.patch no longer applies.

error: patch failed: core/modules/system/system.admin.inc:149
error: core/modules/system/system.admin.inc: patch does not apply

Straight reroll, back to RTBC

2089433-31.patch no longer applies.

error: patch failed: core/includes/ajax.inc:6
error: core/includes/ajax.inc: patch does not apply

Straight reroll. All applicable code in ajax.inc was removed in #2203239: Remove ajax_render and co.

Just for comparison I have ran my automated script from https://drupal.org/node/2089331 on this issue.

~/function_replacer.php 'Drupal\Component\Utility\Xss' 'UtilityXss' 'filter_xss_admin' 'filterAdmin'

Fixed bug with script adding use declaration when it already exists

Differences between 33 and 35:
- Use statements are reordered by the script. We don't have a coding standard for use statement order, so I think this is just noise and should be removed. There might be an argument to run a script to reorder all use statements in a single patch during the disruptive window. See also #1624564: Coding standards for "use" statements.
- The script doesn't change comments. This is probably a good thing as comments often need a bit more thought. A separate patch can be hand-rolled for them, either on the same issue (so you'd run the script, then apply the comments-only patch) or as a followup issue.
- 33 adds an unnecessary use statement to TextPlainUnitTest.
- 33 uses an alias in Drupal\views\Plugin\views\field\Boolean due to conflict with views plugin.
- 35 misses bartik.theme.

Rerolled #33

+++ b/core/modules/locale/locale.module
@@ -1058,9 +1058,9 @@ function locale_translation_use_remote_source() {
+ * The allowed tag list is like \Drupal\Component\Utility\Xss::filterAdmin(),
+ * but omitting div and img as not needed for translation and likely to cause
+ * layout issues (div) or a possible attack vector (img).
  */
 function locale_string_is_safe($string) {
   return decode_entities($string) == decode_entities(filter_xss($string, array('a', 'abbr', 'acronym', 'address', 'b', 'bdo', 'big', 'blockquote', 'br', 'caption', 'cite', 'code', 'col', 'colgroup', 'dd', 'del', 'dfn', 'dl', 'dt', 'em', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'hr', 'i', 'ins', 'kbd', 'li', 'ol', 'p', 'pre', 'q', 'samp', 'small', 'span', 'strong', 'sub', 'sup', 'table', 'tbody', 'td', 'tfoot', 'th', 'thead', 'tr', 'tt', 'ul', 'var')));

Would be cool to create an issue to add Xss::getDefaultAllowedTags() and Xss::getDefaultAdminAllowedTags() methods to the Xss class, so that code like this here does not have to duplicate the full set of admin tags manually, and instead, (1) just get the default admin tags, (2) remove the unwanted, and (3) use the resulting list.

There is still an unnecessary Use statement in TextPlainUnitTest.

However, rather than fixing and rerolling when the patch inevitably doesn't apply, I'd suggest leaving this to #2208607: Write script to automate replacement of deprecated functions where possible, or at the very least rerolling this at a time when we can get several of these removal patches in together.

https://drupal.org/comment/8582935#comment-8582935

Using Function Replacer Part of Write script to automate replacement of deprecated functions where possible.

$ function_replace.php filter_xss 'Drupal\Component\Utility\Xss' UtilityXss filter
$ function_replace.php filter_xss_bad_protocol 'Drupal\Component\Utility\UrlHelper' UtilityUrlHelper filterBadProtocol
$ function_replace.php filter_xss_admin 'Drupal\Component\Utility\Xss' UtilityXss filterAdmin

43: 2089433-43-filter_xss.patch queued for re-testing.

The error is: Fatal error: Cannot use Drupal\Component\Utility\Xss as Xss because the name is already in use in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views/lib/Drupal/views/Plugin/views/field/Boolean.php on line 10

This is because the patch adds "use Drupal\Component\Utility\Xss;" which conflicts with the existing \Drupal\views\Plugin\views\field\Xss class in the same namespace as \Drupal\views\Plugin\views\field\Boolean.

Hopefully Function Replacer can be modified to accommodate this situation? In the case where the class to be imported already exists in the same namespace we should use the fallback (UtilityXss in this case).

@Longwave thank you. Yes that is current weakness of the script I had not considered. I need to build an index of what classes exist so I can test the use declaration I want to insert against the classes in the current namespace.

Manually fixed core/modules/views/lib/Drupal/views/Plugin/views/field/Boolean.php to import as UtilityXss for now.

+++ b/core/includes/common.inc
@@ -3177,7 +3177,7 @@ function _drupal_bootstrap_code() {
-    // filter_xss_admin() is called by the installer and update.php, in which
+    // Xss::filterAdmin() is called by the installer and update.php, in which

That comment is wrong, I've fixed it to refer to the correct function and opened #2222875: Don't set default allowed protocols in _drupal_bootstrap_code() to fix this properly.

I've also corrected a few comments that weren't caught by the script, e.g. because they didn't have the brackets on the function name, and changed comments to fully qualify the class name.

#52 looks good to me.

2089433-52-filter_xss.patch no longer applies.

error: core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockTypeListController.php: No such file or directory
error: core/modules/menu/lib/Drupal/menu/MenuListController.php: No such file or directory

I have spoken to webchick and she suggested that she would be available to review/commit this next Monday, 31st March. There's little point in rerolling the patch until the weekend, but if there's a patch here on Monday that applies then I'll review it.

Now patch is re-rolled.

Sorry i forget to change the interdiff extension. Corrected interdiff are uploaded.

Sorry i forget to change the interdiff extension. Corrected interdiff are uploaded.

Please see #55 - there is no point rerolling this during Szeged as the patch will just cause other, more important, patches to need rerolls.

Reroll instructions are getting a bit lost in the noise, so I have added them to the issue summary. We should do that this weekend.

I compared #61 with #52, because that was the last patch to have been reviewed and was RTBC.

1. +++ b/core/modules/aggregator/aggregator.module
   @@ -6,6 +6,8 @@
   +//use Drupal\Component\Plugin\Exception\PluginException;
   

   Why are we adding this commented out line?

2. +++ b/core/modules/node/node.module
   @@ -8,6 +8,8 @@
   +//use Drupal\Component\Utility\String;
   

   Again

3. +++ b/core/themes/bartik/bartik.theme
   @@ -4,7 +4,7 @@
   -
   

   We still want this whitespace.

Please check your patches before you upload them to save everyone time. These mistakes could also be fixed by following the reroll instructions in the issue summary.

@ianthomas_uk You are really kind. :-(

These lines were unused:
use Drupal\Component\Plugin\Exception\PluginException;
use Drupal\Component\Utility\String;
I commented them out, and after run the tests I forgot to delete them.

@sweetchuck sorry if I was a bit blunt, but it's really frustrating to review rerolls with basic mistakes that are obvious when diffing patches. I always go over my patches line-by-line before uploading them, or when doing rerolls I run a diff between the old patch and the new patch.

These lines were unused

This is a common misunderstanding. In #52 these lines were there as context, so that git would know where to put the lines that we were adding. At some point the lines have been removed from HEAD, so git no longer knows where to add the new lines and must mark them as conflicted. The job of a reroller in this instance is to update the context so we are still inserting the same lines in the same place (and sometimes to verify that those lines are still valid).

If you had decided that extra lines could be removed, then that would make it more than a reroll so should be mentioned.

Rerolled

Whoops forgot to follow my own reroll instructions.

That's got a lot of differences compared to #52, I think it still needs to have the interdiff from #52 applied to it.

Here is #68 plus the appropriate changes from the interdiff on #52 (RTBC by longwave). I've rerolled that interdiff and attached it as manual-changes-70.txt and updated the reroll instructions accordingly.

diff-52-70.txt shows that this is a good reroll, plus some changes to fully qualified class name.

The patch in #52 was RTBC.

I have very carefully reviewed the differences between the patch in #52 and the patch in #70, and they are all fine. Therefore, based on the prior RTBC in #52, this patch is also RTBC.

Looks like it needs another reroll. :(

Simply re-rolled....

Thanks! Reroll looks good to me.

74: drupal8.remove-uses-of-deprecated-XSS-filter-functions.2089433-74.patch queued for re-testing.

it is still green.

74: drupal8.remove-uses-of-deprecated-XSS-filter-functions.2089433-74.patch queued for re-testing.

it was a random test failure, rather than anything wrong with the patch.

Ok, getting this in while it's hot!

Committed and pushed to 8.x. Thanks!

However, I somehow missed that there is mysteriously no change notice for this (or at least "filter_xss_admin" etc. yield no results at https://drupal.org/list-changes) before I pushed, so please get on that lickety split so I don't have to roll this back now that it finally applied. :)

Original issue was #1998466: Convert filter_xss_admin and similar function to an Xss component (really we should have written the CR for that, but better late than never)

Thanks @ianthomas_uk. Let's be sure to add both that issue and this one to the draft for the change record.

Explicitly tagging missing change records as beta blockers.

Created https://drupal.org/node/2239919 and updated https://drupal.org/node/2079005

Thanks @ParisLiakos!