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
Discovered escaping in the render pipeline with views-view-field.html.twig templates. #2348729-42: Convert theme_views_view_field to twig
https://qa.drupal.org/pifr/test/1157318
TaxonomyFieldAllTermsTest failing
CustomBooleanTest failing.
Proposed resolution
Resolve failures in
- CustomBooleanTest
Remaining tasks
Add twig template for views-view-field.html.twig to a test theme.
Apply fixes.
Steps to reproduce
- Setup a content View with fields.
- Add a status/publisehd as a boolean field.
- Setup custom values for the boolean display with markup ('
<p>Yay</p>
' and '<em>Nay</em>
' - #59 contains a txt file diff that converts the field plugin to Boolean instead of BooleanFormatter thanks to (@damiankloip)
- The values should display and in HEAD they are escaped.
User interface changes
API changes
Data model changes
Follow-up to #2348729: Convert theme_views_view_field to twig
Comment | File | Size | Author |
---|---|---|---|
#62 | interdiff.txt | 1.78 KB | joelpittet |
#62 | views_render_pipeline-2560553-62.patch | 6.05 KB | joelpittet |
#59 | help-status-for-test.txt | 636 bytes | joelpittet |
#54 | views_render_pipeline-2560553-54.patch | 6.03 KB | joelpittet |
#54 | interdiff.txt | 5.69 KB | joelpittet |
Comments
Comment #2
joelpittetThese tests probably don't test everything but it should give this a kickstart. This should fail.
Comment #4
joelpittetHaving a hard time writing these tests on something that doesn't exist:S
Tried to get all the broken tests covered in one shot... much easier with actual twig template. Needs heavy overhaul or refactor, even if this does what I want.
Comment #6
subhojit777Comment #7
subhojit777This will reduce the number of fails (somewhat..)
This was returning 404
So I added `test_page_display`
Can you please explain why `@todo` here. Everything starting with `test` itself is a test and is executed by the system. Why are you executing them explicitly.
Comment #8
joelpittetOh I put that @todo to remind myself this is probably a bad approach to writing tests (aka hijacking another test that was failing).
I just did that to get a quick test that could show the problem, likely needs to be refactored.
@subhojit777 becareful not the fix things outside the scope of this issue. There may be another issue out there trying to fix that problem explicitly and will have to needlessly reroll, also makes this patch a bit harder to review. Though your change is minor. I have to catch myself when I start fixing all the things too!
Comment #10
subhojit777Patch with fixes in #2348729: Convert theme_views_view_field to twig
Comment #12
joelpittetThanks @subhojit777. I'm removing the theme function conversion from the patch in #10.
Likely the tests I wrote in there aren't quite right, so needs work on that for sure. I swear this issue will start to make more sense when the test are better:)
Comment #14
dawehnerDon't we seriously need some profiling on this issue?
Oh so you have to ensure that the $separator is safe even if you use a template? This somehow feels counter intuitive, to be honest.
Don't you want a renderPlain here?
Comment #15
joelpittet@dawehner re #14.1 I copied that from something else that does the same thing in HEAD for consistency and fixes the problem. We don't have to ensure the separator is safe and actually I may just remove that from the other place as well in this patch. Thanks for noticing.
#14.2 Yes, but but kinda hoping I can fix that without early rendering the values. (pretty sure this can be done, just need to sit with it for a bit)
Thanks for the review.
Comment #16
joelpittetAnybody feel free to improve the tests or resolve notes in #14
Comment #17
RainbowArrayThis fixes point 2 in in #14. I'm not clear on what the next step is for point 1. Just remove the XSS:filterAdmin()? Is that needed because it's used both in the template and the context below that? I'm not quite sure what's going on there.
Comment #20
dawehnerIts a bit odd that this kind of test coverage lives in a file related with taxonomy term fields and not some other test more specific test class
This is the same as Field.php does, so this is fine.
Are you sure we need renderPlain() and cannot just use render()?
Comment #21
RainbowArray3. I used renderPlain because you suggested it. If we use straight up render, don't we need to define a render context? I don't fully understand how that works, but thought I spotted that when I was working on the page title block.
Comment #22
joelpittetre #20.1 yes super weird. I'm just trying to reproduce and fix the bugs we found in the other issue, we can make these tests better by moving them into a better location too.
20.2 ok
20.3 renderPlain() seems appropriate.
Comment #23
joelpittetComment #24
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedCan we get this in short array syntax?
+1
Comment #25
RainbowArrayThis adds the short array syntax. That's the only feedback I felt I could take action on right now.
Time to review again, and if it's looking good, move forward?
Comment #27
joelpittetNeed to debug and sort out what's up with CustomBooleanTest.
Comment #29
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedRerolling
Comment #30
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedNot entirely sure of this re-roll... pls have a look -=)
Comment #33
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedComment #34
joelpittetI'm going to tackle this some more
Comment #35
iMiksuThe #30 patch was failing to apply against latest HEAD:
patching file core/modules/views/src/Plugin/views/field/FieldPluginBase.php
Hunk #1 FAILED at 1308.
Here's a reroll.
Comment #36
lauriiiComment #37
iMiksuPlease ignore #35, it was missing one of the files. Here's a with the previously missing file.
Comment #38
joelpittetDon't need to assign myself, I'm working on it with @iMiksu, he's helping me find a better place for these tests and more consistent structure to follow other views + twig render pipeline tests.
Comment #43
iMiksuComment #44
joelpittetComment #45
iMiksuI've been digging around different views tests and found out that these escaping issues needs a bit of testing planning which I think somebody more experienced should take a look (I'm also quite new to SafeMarkup). There is a test
\Drupal\views\Tests\ViewsEscapingTest
which could be abstracted.I also think that this may be more child issue of #2297711: Fix HTML escaping due to Twig autoescape rather than #2348729: Convert theme_views_view_field to twig, should we relink this?
I'm gonna drop on this and leave this to somebody who knows lots of about testing and lots of about SafeMarkup and let myself move on to work on another issue :)
Comment #46
joelpittetHandlerFieldRoleTest is not a problem
Comment #47
joelpittetChanging the scope of this issue to only the
CustomBooleanTest
because it's the only failing issue in the other issue now.Also not "double escaping" just unexpected HTML single escaping.
Comment #48
joelpittetComment #51
joelpittetComment #52
iMiksuShouldn't the tests-only patch fail?
Comment #53
joelpittet@iMiksu yes it should:)
Comment #54
joelpittetWas having troubles with the views preview using the theme.
Went with
ViewsRenderPipelineSafeString
to make this more discoverable.Comment #57
joelpittetComment #59
joelpittetFound a way to test this manually.
Attached is patch make sure the boolean field uses Boolean class instead of BooleanFormatter.
core/modules/node/src/NodeViewsData.php
Comment #60
mr.baileysNitpick: I would defer UtilityXss::filterAdmin() until the return statement (to avoid having it twice, since we are doing it for $custom_value regardless of value).
Should this explain *why* we want to override the theme function for this test (to force usage of twig auto-escaping on rendering the field values, so we can ensure there is no double-escaping going on), or is this not necessary?
Comment #61
mr.baileysRemoving the manual testing tag since manual testing was done in #60.
Comment #62
joelpittetThanks for the review I've updated those two points @mr.baileys. Slightly more generic because it can be used for things other than escaping and it's to test the template version of the same field template.
Comment #63
mr.baileysManual testing and review completed, all issues addressed, RTBC.
Comment #64
xjmComment #65
joelpittetThis is blocking a theme function conversion.
Comment #66
alexpottCommitted f26b065 and pushed to 8.0.x. Thanks!