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
This issue is a result of #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests where we uncovered that ConfigImporter is more difficult than other instances of !placeholder which is well documented in #2514044: Do not use SafeMarkup::format in exceptions.
See #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand for complete motivation on removal of !placeholder
This issue addresses !placeholder in Config
File Scope is:
core/lib/Drupal/Core/Config/*
core/modules/config/*
Remaining tasks
- Replace !placeholder with @placeholder. Refer to patch in #2506445-85: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests as that patch should have related update
- Ensure tests come back clean
- Manually test the update and post screen shot after patch, review source for any difference in escaping.
User interface changes
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | replace_placeholder-2551743-15.patch | 3.8 KB | Sutharsan |
| #12 | 2551743-Config-SafeString.jpg | 203.33 KB | justAChris |
| #10 | replace_placeholder-2551743-10.patch | 10.43 KB | lauriii |
| #10 | interdiff.txt | 5.07 KB | lauriii |
| #7 | replace_placeholder-2551743-7.patch | 15.67 KB | joelpittet |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedRyan Weal created an issue. See original summary.
Comment #2
pguillard CreditAttribution: pguillard commentedComment #3
pguillard CreditAttribution: pguillard commentedComment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedThis is probably no longer necessary, it looks like it has been successfully patched in the other issue.
Comment #5
joelpittetWe started splitting the other issue so this is being re-purposed.
Comment #6
justAChris CreditAttribution: justAChris as a volunteer commentedUpdated IS to include template from similar issue.
Some useful information related to ConfigImporter: #2506445-85: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests
@joelpittet: can we include any related tests here?
Comment #7
joelpittetAh probably why things break, thanks @justAChris
Added all config.
Comment #9
lauriiiI tried testing all different scenarios where the escaping of placeholders inside messages have changed and there was no visual difference before/after. I didn't test the exception where SafeString object has been created because I didn't manage to throw an exception which would trigger that. Maybe @joelpittet has insight how to trigger that? Anyway otherwise the patch seems fine for me.
Comment #10
lauriiiRemoved hook_help() from the patch
Comment #11
justAChris CreditAttribution: justAChris as a volunteer commentedIn comment #7 we included all of config, simply updating file bounds in IS and issue title to comply.
Comment #12
justAChris CreditAttribution: justAChris as a volunteer commentedRegarding SafeString in #9, Looks good to me:
A bit of a pain to reproduce, I've outlined the steps at the bottom of #2506445-85: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests, reran these steps with the patch in #10
The only thing I would consider on this patch is cleaning up the tests to remove unecessary functions from the asserts, but I think that's out of scope here.
RTBC++
Comment #13
catchPostponed on #2558791: "!"-prefixed tokens should Xss::filterAdmin() but not affect safeness/
Comment #14
justAChris CreditAttribution: justAChris as a volunteer commentedClosing this, splitting by module was not the ideal approach to removing !placeholder. Marking as duplicate of #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand, since the chosen approach is / will be outlined there, please refer to it for any additional action.
Comment #15
Sutharsan CreditAttribution: Sutharsan as a volunteer commentedRerolling patch for easy migration into single patch at #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests.
Changing status for test bot. Do revert status after test.
Comment #18
Sutharsan CreditAttribution: Sutharsan as a volunteer commentedStatus back to 'Closed (duplicate)'. Patch now included in #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests.
Comment #19
xjm