Views field rewrite replacement subtoken yields double encoded HTML entities

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.

apkwilson → closed merge request !5628

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

Leaving at RTBC.

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!

Automatically closed - issue fixed for 2 weeks with no activity.