Views field rewrite replacement subtoken yields double encoded HTML entities

Verified that this issue exists in Drupal 8.3.x as well
and Re-rolled patch in #4 for 8.3.x

Confirmed that the patch works as expected.
However, it does need a test. Just working my head around it how to test it, should be easy enough. I hope.

New test & test only patch.
Notes: Due to adding an HTML entity in the test data, a couple other tests had to be adjusted. I believe this is reasonable but an extra look should be given there.

hmm that had unexpected side effects on a lot of other tests. Worse yet, I found that it didn't fail when it should have.
New test created that doesn't interfere and actually works.

Fixed CS errors.

Updated patch will fix test failures

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. 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.

The issue summary should be updated with the problem and proposed solution specified using the default template. Also since this is causing a UI issue before/after screenshots would be useful.

Updating version to 10.1.x.-dev (which still has this problem), since D9 is EOL.

These patches reroll #22 for Drupal 10.1.x. Tests only patch is expected to fail. The full patch includes the fix and should succeed.

I'm not familiar with the GitLab system, so I've fallen back to patches.

Fyi current development branch is 11.x

What's the issue with gitlab? Eventually patches will be fully phased out so if there's a bug with gitlab we should know

I don't think there's anything wrong with gitlab. I wanted to run the test suite on the commit with just the new test, to show that it failed. In looking around for how to do that, I found this Patch or merge request section of the docs, which seemed to suggest that I should stick with patches here, since that's how the issue started. I'm happy to be corrected.

I'll reroll again for 11.x and open another merge request - is a test-only commit and test run not really necessary?

Gitlab actually has a test-only feature automatically there. Takes about 1-2 minutes to run.

I did see this test-only job, but it deleted the test view configuration the rerolled patch included, so the test couldn't run and fail the expected way.

I think I'm going to remove the new test view configuration and use an existing one, so this shouldn't be an issue, here, but what happens when a merge request does need to include new test configuration?

If the test only doesn’t fail it’s not a show stopper. Reviewer can always run locally.

I ended up making a new test class and view, after all. I updated their naming to make what they're testing more clear.
Updating the issue title and description to indicate that this is only a problem for field plugin generated subtokens.

Added standard template to the issue summary but left TBD for sections that were missing. If not applicable just change TBD to NA.

Filled out the issue template.

\Drupal\views\Plugin\views\field\FieldPluginBase::renderText() calls FieldPluginBase::getRenderTokens(), calls addSelfTokens(), which for \Drupal\views\Plugin\views\field\EntityField::addSelfTokens() calls Xss:filterAdmin(), encoding special characters.
Then renderText() calls FieldPluginBase::renderAltered(), calls \Drupal\views\Plugin\views\PluginBase::viewsTokenReplace(), calls Xss:filterAdmin() (either explicitly or as a post-render callback), encoding special characters a second time.

The proposed encode/decode process also takes place elsewhere, e.g. in \Drupal\views\Plugin\views\field\FieldPluginBase::renderText() at 1331-1333.
However, other core field plugins omit encoding the value, and so don't need to decode it. See \Drupal\taxonomy\Plugin\views\field\TaxonomyIndexTid::addSelfTokens() and \Drupal\user\Plugin\views\field\Roles:addSelfTokens(). Maybe that is a better route, to avoid the extra back-and-forth calls. The value does get filtered in the call to viewsTokenReplace(), anyway.

Removing the encoding seemed a better solution. I've updated the merge request to do that, instead.

Issue summary appears to have been updated

Ran the test-only feature

1) Drupal\Tests\views\Kernel\Handler\FieldSelfTokensTest::testSelfTokenEscaping
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<p>Questions &amp; Answers</p>'
+'<p>Questions &amp;amp; Answers</p>'
/builds/issue/drupal-2856598/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/issue/drupal-2856598/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
/builds/issue/drupal-2856598/core/modules/views/tests/src/Kernel/Handler/FieldSelfTokensTest.php:66
/builds/issue/drupal-2856598/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 1, Assertions: 8, Failures: 1.

Which does show the double encoding.

Change in MR matches issue summary and may change is small so easy to review.

Think this is good.

I'm triaging RTBC issues. I read the IS, the MR and the comments. I didn't find any unanswered questions.

I did leave some comments in the MR. Setting to NW.

Suggested changes applied. Setting to Needs Review.

Feedback from @quietone appears to be addressed.

Double checked that views token replacement does in fact call filterXssAdmin() again and the test coverage looks good.

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!