The following functions have been refactored into methods on the XSS and UrlHelper classes, all calls should be replaced with direct calls to the methods specified in their docblocks:
filter_xss()
filter_xss_admin()
filter_xss_bad_protocol()
This is part of #2093143: [meta] Remove calls to @deprecated and "backwards compatibility" procedural functions from core
Reroll instructions
Run these function replacer commands (see #2208607: 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
Then manually fix core/modules/views/lib/Drupal/views/Plugin/views/field/Boolean.php to import as UtilityXss if function replacer hasn't picked that up.
Finally apply manual-changes-70.txt
Comment | File | Size | Author |
---|---|---|---|
#74 | drupal8.remove-uses-of-deprecated-XSS-filter-functions.2089433-74.patch | 61.01 KB | visabhishek |
#70 | diff-52-70.txt | 10.27 KB | ianthomas_uk |
#70 | 2089433-70-xss.patch | 61.04 KB | ianthomas_uk |
#70 | manual-changes-70.txt | 9.87 KB | ianthomas_uk |
#52 | 2089433-52-filter_xss.patch | 60.76 KB | ianthomas_uk |
Comments
Comment #1
thedavidmeister CreditAttribution: thedavidmeister commentedpatch
Comment #3
thedavidmeister CreditAttribution: thedavidmeister commentedComment #5
thedavidmeister CreditAttribution: thedavidmeister commentedthere is no fatal for that test on simplytest.me
Comment #6
thedavidmeister CreditAttribution: thedavidmeister commented#3: 2089433-2.patch queued for re-testing.
Comment #8
thedavidmeister CreditAttribution: thedavidmeister commentedah, need to fix "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 14"
fair enough.
Comment #9
thedavidmeister CreditAttribution: thedavidmeister commentedComment #11
thedavidmeister CreditAttribution: thedavidmeister commentedComment #13
thedavidmeister CreditAttribution: thedavidmeister commentedComment #14
herom CreditAttribution: herom commentedreroll + update patch.
reverted two cases of
filter_xss()
→Xss:filterAdmin()
, and fixed a mismatched route.Comment #14.0
herom CreditAttribution: herom commentedUpdated issue summary.
Comment #15
areke CreditAttribution: areke commentedThe patch needs to be rebased because it doesn't apply.
Comment #16
herom CreditAttribution: herom commentedComment #17
herom CreditAttribution: herom commentedreroll.
Comment #18
sunComment #19
ianthomas_ukreroll
Comment #21
ianthomas_ukFor some reason some of the conflicts didn't show in SourceTree. Here's a reroll with all the conflicts fixed.
Comment #22
longwavePatch looks good, remaining instances of filter_xss_admin after applying are:
RTBC if bot runs green.
Comment #23
alexpott2089433-21.patch no longer applies.
Comment #24
ianthomas_ukRerolled with updated context in forum.module following String::checkPlain() changes.
Comment #26
ianthomas_uk24: 2089433-24.patch queued for re-testing.
Comment #27
ianthomas_ukTestbot ran out of disk space
Comment #28
alexpott2089433-24.patch no longer applies.
Comment #29
herom CreditAttribution: herom commentedrerolled (auto-merged).
Comment #30
alexpott2089433-29_0.patch no longer applies.
Comment #31
ianthomas_ukStraight reroll, back to RTBC
Comment #32
alexpott2089433-31.patch no longer applies.
Comment #33
longwaveStraight reroll. All applicable code in ajax.inc was removed in #2203239: Remove ajax_render and co.
Comment #34
grom358 CreditAttribution: grom358 commentedJust 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'
Comment #35
grom358 CreditAttribution: grom358 commentedFixed bug with script adding use declaration when it already exists
Comment #36
ianthomas_ukDifferences 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.
Comment #37
longwaveRerolled #33
Comment #38
sunWould be cool to create an issue to add
Xss::getDefaultAllowedTags()
andXss::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.Comment #39
ianthomas_ukThere 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.
Comment #40
ianthomas_ukComment #41
grom358 CreditAttribution: grom358 commentedhttps://drupal.org/comment/8582935#comment-8582935
Comment #42
ianthomas_ukComment #43
grom358 CreditAttribution: grom358 commentedUsing Function Replacer Part of Write script to automate replacement of deprecated functions where possible.
Comment #45
grom358 CreditAttribution: grom358 commented43: 2089433-43-filter_xss.patch queued for re-testing.
Comment #47
longwaveThe 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).
Comment #48
grom358 CreditAttribution: grom358 commented@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.
Comment #49
grom358 CreditAttribution: grom358 commentedManually fixed core/modules/views/lib/Drupal/views/Plugin/views/field/Boolean.php to import as UtilityXss for now.
Comment #50
grom358 CreditAttribution: grom358 commentedComment #51
grom358 CreditAttribution: grom358 commentedComment #52
ianthomas_ukThat 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.
Comment #53
longwave#52 looks good to me.
Comment #54
alexpott2089433-52-filter_xss.patch no longer applies.
Comment #55
ianthomas_ukI 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.
Comment #56
visabhishek CreditAttribution: visabhishek commentedNow patch is re-rolled.
Comment #58
visabhishek CreditAttribution: visabhishek commentedSorry i forget to change the interdiff extension. Corrected interdiff are uploaded.
Comment #59
visabhishek CreditAttribution: visabhishek commentedSorry i forget to change the interdiff extension. Corrected interdiff are uploaded.
Comment #60
ianthomas_ukPlease 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.
Comment #61
SweetchuckComment #62
ianthomas_ukI compared #61 with #52, because that was the last patch to have been reviewed and was RTBC.
Why are we adding this commented out line?
Again
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.
Comment #63
Sweetchuck@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.
Comment #64
ianthomas_uk@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.
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.
Comment #65
grom358 CreditAttribution: grom358 commentedComment #66
grom358 CreditAttribution: grom358 commentedRerolled
Comment #68
grom358 CreditAttribution: grom358 commentedWhoops forgot to follow my own reroll instructions.
Comment #69
ianthomas_ukThat's got a lot of differences compared to #52, I think it still needs to have the interdiff from #52 applied to it.
Comment #70
ianthomas_ukHere 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.
Comment #71
jhodgdonThe 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.
Comment #73
jhodgdonLooks like it needs another reroll. :(
Comment #74
visabhishek CreditAttribution: visabhishek commentedSimply re-rolled....
Comment #75
jhodgdonThanks! Reroll looks good to me.
Comment #77
visabhishek CreditAttribution: visabhishek commented74: drupal8.remove-uses-of-deprecated-XSS-filter-functions.2089433-74.patch queued for re-testing.
Comment #78
dawehnerit is still green.
Comment #80
visabhishek CreditAttribution: visabhishek commented74: drupal8.remove-uses-of-deprecated-XSS-filter-functions.2089433-74.patch queued for re-testing.
Comment #81
ianthomas_ukit was a random test failure, rather than anything wrong with the patch.
Comment #83
webchickOk, 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. :)
Comment #84
ianthomas_ukOriginal 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)
Comment #85
xjmThanks @ianthomas_uk. Let's be sure to add both that issue and this one to the draft for the change record.
Comment #86
xjmExplicitly tagging missing change records as beta blockers.
Comment #87
ParisLiakos CreditAttribution: ParisLiakos commentedCreated https://drupal.org/node/2239919 and updated https://drupal.org/node/2079005
Comment #88
xjmThanks @ParisLiakos!