Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Problem/Motivation
See #2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only
ConfigSync
is using !placeholder
.
Proposed resolution
!change
is the result of a render and therefore will not be escaped by the@placeholder
!collection
is a configuration collection. This is defined in code and results in directories being created on the file system - can not contain HTML. Therefore safe to change to@placeholder
Remaining tasks
Review
Commit
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#4 | 2569419-2-4.patch | 3.93 KB | alexpott |
#4 | 3-4-interdiff.txt | 1.14 KB | alexpott |
#3 | 2569419-2-3.patch | 4.04 KB | alexpott |
#2 | 2569419-2.patch | 1.45 KB | alexpott |
Comments
Comment #2
alexpottComment #3
alexpottActually we should use an inline template here. Unfortunately this does not work because it is too long :( we need to fix #2558885: TwigEnvironment is unable to cache inline templates because it sends invalid filenames to MTimeProtectedFastFileStorage
Comment #4
alexpottThere is always another way :)
Comment #5
lauriiiNice workaround! :P Tested this manually on the config sync page and everything seems to work like before so RTBC if passes.
Comment #7
dawehnerLooks perfect, indeed
Comment #9
alexpottThis is part of the critical meta.
Comment #10
xjmA good improvement, and consistent with other places we're adding item lists to status messages.
I asked myself whether this was sufficiently specific to ensure that it does not match
system.site
somewhere else on the page now and in the future (probably). At first I thought it might be inappropriately reliant on the markup or risk a false positive if we change the config UI in the future, but OTOH, it is good to test that the<li>
tags do not get escaped.Something like an xpath or cssSelect() would be more robust, but this should be sufficient. I would however add an inline comment explaining why we're testing for the
<li>
tag specifically so that someone doesn't "clean it up" in the future and regress the coverage.So I did that on commit:
Committed and pushed to 8.0.x.