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
The AllowedTagsXssTrait is deprecated in favor of FieldFilteredMarkup and will be removed in Drupal 9.
Proposed resolution
Trigger deprecation errors in its methods.
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
N/a
Comment | File | Size | Author |
---|---|---|---|
#38 | 2809237-38.patch | 6.89 KB | alexpott |
#38 | 37-38-interdiff.txt | 4.61 KB | alexpott |
#37 | 2809237-31.patch | 5.92 KB | louisnagtegaal |
#30 | 2809237-30.patch | 5.04 KB | alexpott |
#25 | 2809237-25.patch | 9.82 KB | iampuma |
Comments
Comment #2
aburrows CreditAttribution: aburrows as a volunteer commentedComment #3
aburrows CreditAttribution: aburrows as a volunteer commentedComment #4
aburrows CreditAttribution: aburrows as a volunteer commentedComment #5
muschpusch CreditAttribution: muschpusch at Factorial GmbH commentedok working on this with stmh at drupalcon dublin
Comment #6
muschpusch CreditAttribution: muschpusch at Factorial GmbH commentedOk the more easy part of removing the class and use statements is done. There is actually one file where the AllowedTagsXssTrait::fieldFilterXss should have been used but since it's deprecated maybe the values should be filtered by FieldFilteredMarkup. Could somebody confirm that?
Comment #7
muschpusch CreditAttribution: muschpusch at Factorial GmbH commentedSorry the code i'm talking about is in here: core/modules/options/options.module
Comment #12
nickmans CreditAttribution: nickmans at The Reference commentedHi,
Looks like you do need to change it to FieldFilteredMarkup::create.
Comment #13
twfahey CreditAttribution: twfahey as a volunteer commentedUsing muschpusch's previous patch as a base, and based on the feedback provided, I *believe* I have added the appropriate changes here to the comments. Hope I'm not stepping on any toes, I am just eager to become more proficient with Drupal patching and contributing, and hopefully I have done things correctly here, but please, any and all feedback at this point is welcome!
Comment #14
tstoecklerComment #15
Mike Lewis CreditAttribution: Mike Lewis commentedWorking on this at Drupalcon Nashville. This is a little more complicated than just removing the use statements. Looks like the patches in #6 and #13 tried that ok, but the reason the tests are failing is that the classes are still trying to use the functions from the trait. So we need to go through and refactor it a little further to replace those with the updated methodology per #12
Comment #16
Mike Lewis CreditAttribution: Mike Lewis commentedUpdated issue summary.
Comment #17
dmitryl CreditAttribution: dmitryl commentedApplying patch
Comment #18
Mike Lewis CreditAttribution: Mike Lewis commented#17 solves the test failures. dmitryl is re-rolling a patch that removes the trait's php file.
Comment #19
dmitryl CreditAttribution: dmitryl commentedRemove AllowedTagsXssTrait.php from the codebase.
Comment #20
dmitryl CreditAttribution: dmitryl commentedRemove AllowedTagsXssTrait.php from the codebase.
Add missing file deletion from #19
Comment #21
borisson_I don't think we can just remove the trait from core. As it's possible that this is used by contrib. We can remove the usages but removing the trait seems dangerous.
Comment #23
jcnventura CreditAttribution: jcnventura at Wunder commentedMentoring this at DrupalEurope with @iampuma
Comment #24
jcnventura CreditAttribution: jcnventura at Wunder commentedIndeed, as specified, it is "@deprecated in Drupal 8.0.x, will be removed before Drupal 9.0.0."
We can't remove it in the 8.x branch. But we can remove the usages of this in preparation of Drupal 9.
Comment #25
iampumaReverted deletion of the AllowedTagsXssTrait.php in the codebase.
Comment #26
borisson_The changes look better now, Thanks!
Comment #28
catchI'm not 100% about removing this from ListItemBase because it's designed to be extended. Could we split this hunk out to a separate issue?
Comment #30
alexpottYeah we can't remove the trait usage. We can only deprecate all the methods properly.
No interdiff because the approach is different and inline with current deprecated code best practices.
Comment #31
alexpottComment #33
mradcliffeTagging this for drupalcon amsterdam 2019.
We need to review @alexpott's code and update the issue summary to include the consensus in #28 and #30.
Comment #34
mradcliffeRemoving assigned property.
Comment #35
louisnagtegaal CreditAttribution: louisnagtegaal at LimoenGroen commentedI'm working on it on DrupalCon Amsterdam2019
Comment #36
nickmans CreditAttribution: nickmans at The Reference commentedI tested and applied this patch on DrupalCon Amsterdam 2019 on 8.9.x-dev and it's working fine.
Comment #37
louisnagtegaal CreditAttribution: louisnagtegaal at LimoenGroen commentedI have reviewed the patch in 2809237-30.patch. To get proper deprecation warnings in unit tests (like the one I added) an extra parameter must be added tot the @trigger_error-calls in core/lib/Drupal/Core/Field/AllowedTagsXssTrait.php
I have added a new patch which includes this extra parameter and added an unit test to check for the deprecation messages.
Comment #38
alexpottHere's a fixed up test.
This needs to go in a better namespace - for example Drupal\Tests\Core\Field
This classname needs to match the filename and it needs an @group annotation otherwise test discovery breaks and you see what you see on your patch's test run.
We need to add @expectedDeprecation annotations to test the deprecation.
Instead of doing
$this->assertTrue(TRUE);
we can make assertions against the return values of the deprecated calls.Comment #39
longwaveThis should go in so we can remove these in Drupal 9.
Comment #40
catchCommitted 3791e77 and pushed to 9.0.x. Thanks! Cherry-picked to 8.8.x and 8.9.x
Comment #44
catch