Properly deprecate AllowedTagsXssTrait

ok working on this with stmh at drupalcon dublin

Ok 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?

Sorry the code i'm talking about is in here: core/modules/options/options.module

/**
 * Returns the array of allowed values for a list field.
 *
 * The strings are not safe for output. Keys and values of the array should be
 * sanitized through \Drupal\Core\Field\AllowedTagsXssTrait::fieldFilterXss()
 * before being displayed.
 *

Hi,

Looks like you do need to change it to FieldFilteredMarkup::create.

Using 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!

Working 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

Updated issue summary.

Applying patch

#17 solves the test failures. dmitryl is re-rolling a patch that removes the trait's php file.

Remove AllowedTagsXssTrait.php from the codebase.

Remove AllowedTagsXssTrait.php from the codebase.
Add missing file deletion from #19

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.

Mentoring this at DrupalEurope with @iampuma

Indeed, 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.

Reverted deletion of the AllowedTagsXssTrait.php in the codebase.

The changes look better now, Thanks!

--- a/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php

--- a/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
+++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php

+++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
+++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
@@ -2,7 +2,6 @@

@@ -2,7 +2,6 @@
 
 namespace Drupal\options\Plugin\Field\FieldType;
 
-use Drupal\Core\Field\AllowedTagsXssTrait;
 use Drupal\Core\Field\FieldDefinitionInterface;
 use Drupal\Core\Field\FieldItemBase;
 use Drupal\Core\Form\FormStateInterface;
@@ -15,8 +14,6 @@

@@ -15,8 +14,6 @@
  */
 abstract class ListItemBase extends FieldItemBase implements OptionsProviderInterface {
 
-  use AllowedTagsXssTrait;
-
   /**
    * {@inheritdoc}
    */

I'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?

Yeah 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.

Tagging 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.

Removing assigned property.

I'm working on it on DrupalCon Amsterdam2019

I tested and applied this patch on DrupalCon Amsterdam 2019 on 8.9.x-dev and it's working fine.

I 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.

Here's a fixed up test.

1. @louisnagtegaal great catch and good call to add a test.

2. +++ b/core/tests/Drupal/Tests/AllowedTagsXssTraitDeprecateTest.php
   @@ -0,0 +1,28 @@
   +namespace Drupal\Tests;
   

   This needs to go in a better namespace - for example Drupal\Tests\Core\Field

3. +++ b/core/tests/Drupal/Tests/AllowedTagsXssTraitDeprecateTest.php
   @@ -0,0 +1,28 @@
   +
   +class FieldDeprecateAllowedTagsXssTraitTest extends UnitTestCase {
   

   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.

4. +++ b/core/tests/Drupal/Tests/AllowedTagsXssTraitDeprecateTest.php
   @@ -0,0 +1,28 @@
   +
   +  public function testDeprecation(){
   

   We need to add @expectedDeprecation annotations to test the deprecation.

5. +++ b/core/tests/Drupal/Tests/AllowedTagsXssTraitDeprecateTest.php
   @@ -0,0 +1,28 @@
   +    $deprecated->fieldFilterXss('Test string');
   +    $deprecated->allowedTags();
   +    $deprecated->displayAllowedTags();
   +    $this->assertTrue(TRUE);
   

   Instead of doing $this->assertTrue(TRUE); we can make assertions against the return values of the deprecated calls.

This should go in so we can remove these in Drupal 9.

Committed 3791e77 and pushed to 9.0.x. Thanks! Cherry-picked to 8.8.x and 8.9.x