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
FormCache::loadCachedFormState calls SafeMarkup::set() which is meant to be for internal use only.
Proposed resolution
- If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required.
Remaining tasks
- Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
- Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.
- If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.
User interface changes
N/A
API changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#21 | document-2501823-21.patch | 822 bytes | davidhernandez |
#19 | interdiff.txt | 867 bytes | joelpittet |
#19 | document-2501823-19.patch | 849 bytes | joelpittet |
#18 | interdiff.txt | 994 bytes | joelpittet |
#18 | document-2501823-18.patch | 851 bytes | joelpittet |
Comments
Comment #1
peezy CreditAttribution: peezy commentedComment #2
peezy CreditAttribution: peezy as a volunteer commentedComment #4
peezy CreditAttribution: peezy as a volunteer commentedResubmitting patch after pulling.
Comment #6
peezy CreditAttribution: peezy as a volunteer commentedCorrected core version.
Comment #7
joelpittetSorry we just need to keep these under 80 characters per line.
80 character limit.
Comment #9
peezy CreditAttribution: peezy as a volunteer commentedReduced length of comments to be fewer than 80 characters.
Comment #10
peezy CreditAttribution: peezy as a volunteer commentedComment #11
joelpittetspace pleezy
Comment #12
peezy CreditAttribution: peezy as a volunteer commentedSorry, it was obviously a late night.
Comment #13
joelpittetMerci bien! That works.
Comment #14
catchSorry nitpick but this should be 'ensures'
Comment #15
star-szrI'm wondering do we even need this sentence? Seems a tiny bit redundant the way I'm reading it.
Comment #16
joelpittetI thought there was $5M liability on this method, insures it correct to me:P
Comment #17
davidhernandezRemoved the redundant sentence and clarified the first one.
Comment #18
joelpittetThanks @davidhernandez. Just adding some pluralized pronouns and doc standards for methods in comments. AFAIK.
Comment #19
joelpittetScratch the first thing, it's 'the list' not 'the safe strings'.
Comment #20
cilefen CreditAttribution: cilefen commentedJust semantically, I would go with "Retrieve the safe strings and store them for this request. ..." if that conveys the proper meaning. Does it matter that they were "previously known"?
Comment #21
davidhernandezSimplified the sentence. Lets not bikeshed it.
Comment #22
star-szrI'm not sure if the ::setStrings() thing is going to mesh with api.d.o, but this looks good to me now. Small patch but it points to where the safeness of the strings has been determined.
Edit: And not sure it matters since it's an inline comment?
Comment #23
joelpittetOrange... I mean rtbc
Comment #29
star-szrComment #32
xjmThanks everyone! As with #2501447: Document SafeMarkup::setMultiple in _batch_page(), we're not going to be able to get around this usage right now, and we do have the existing @todo for the memory implications. So, committed and pushed to 8.0.x.
Re: the
::setCache()
bit, api.d.o will indeed not parse it, but I think we said it's okay to use such shorthands in paragraphs of text on the same class. So I think that's okay.Comment #33
tim.plunkett