Problem/Motivation
We no longer need SafeMarkup::set(). All the work on documenting or removing has shown that removing is always possible - #2280965: [meta] Remove every SafeMarkup::set() call
Suggested commit
git commit -m 'Issue #2554889 by Berdir, Cottser, Daniel_Rose, JeroenT, RavindraSingh, Wim Leers, YesCT, akalata, alexpott, alimac, aneek, catch, cdulude, chx, cilefen, cmanalansan, crowdcg, cwells, davidhernandez, dawehner, dtraft, effulgentsia, harjotsingh, hass, ianthomas_uk, joelpittet, josephdpurcell, justAChris, kbaringer, lauriii, lduerig, leslieg, lokapujya, mpdonadio, peezy, pwolanin, realdream, sclapp, stefan.r, tim.plunkett, webchick, xjm: Remove SafeMarkup::set() from the codebase.'
This includes everyone from this issue, #2297703: [meta] Refactor and remove as many SafeMarkup::set() calls as possible and #2280965: [meta] Remove every SafeMarkup::set() call and people who joined a very long call about how to finish all the issues.
Proposed resolution
- Remove the method.
- Use reflection in a helper method for testing in SafeMarkupTest
Remaining tasks
- Write patch
- Review
- Commit
User interface changes
None
API changes
Remove SafeMarkup::set()
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#70 | interdiff.txt | 853 bytes | lauriii |
#70 | remove-2554889-70.patch | 10.49 KB | lauriii |
#69 | interdiff.txt | 1.36 KB | lauriii |
#69 | remove-2554889-69.patch | 10.35 KB | lauriii |
#67 | remove-2554889-67.patch | 10.56 KB | lauriii |
Comments
Comment #2
alexpottHere's the removal from SafeMarkupTest
Comment #3
Wim LeersWhew, must've felt … liberating, to be able to roll that patch! :D
Comment #4
cilefen CreditAttribution: cilefen commentedIs this postponed on #2280965: [meta] Remove every SafeMarkup::set() call?
Comment #5
dawehnerIt is so great that this is actually possible now!
Everytime I see a class reference not using ::CLASS a kitten dies :)
nitpick: true => TRUE
Comment #6
Wim LeersThe most important change is missing:
SafeMarkup::set()
is not being removed yet! But that's blocked on those other issues landing first, of course.:D
Comment #7
dawehnerOh for sure, but we are at a point where we can finally consider removing it.
Comment #8
Wim LeersYep, that's what I said :)
Comment #9
alexpottPostponing on all the other issues that are part of #2280965: [meta] Remove every SafeMarkup::set() call
Comment #10
alexpottComment #11
alexpottgit commit -m 'Issue #2554889 by kbaringer, mpdonadio, xjm, Cottser, alexpott, joelpittet, davidhernandez, cilefen, cwells, Wim Leers, akalata, stefan.r, josephdpurcell, sclapp, harjotsingh, justAChris, peezy, leslieg, lokapujya, RavindraSingh, cdulude, effulgentsia, chx, catch, crowdcg, webchick, aneek, JeroenT, dawehner, realdream, dtraft, YesCT, lauriii, hass, Daniel_Rose, alimac, cmanalansan, lduerig, pwolanin: Remove SafeMarkup::set() from the codebase.'
This includes everyone from #2297703: [meta] Refactor and remove as many SafeMarkup::set() calls as possible and #2280965: [meta] Remove every SafeMarkup::set() call and people who joined a very long call about how to finish all the issues.
Comment #12
stefan.r CreditAttribution: stefan.r commentedJust postponed on #2550981: Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages() now, let's try a combined patch
Comment #14
alexpottI think we should just remove the See self::set() it's not useful.
There is an issue to mark SafeMarkup::setMultiple as internal somewhere...
by a an? This text does not make much sense atm.
Comment #15
stefan.r CreditAttribution: stefan.r commented1. I think you mean ::setMultiple(), not ::set()? An explanation was added in ::setMultiple() of the escaping strategies... but I agree it's not hard to find anyway so let's remove.
2. Let's remove that as well and leave that for the other issue.
3. Rewrote this to "Every string in this list will be represented by a multidimensional array in which the keys are the string and the escaping strategy used for this string, and in which the value is the boolean TRUE."
Comment #17
xjmMoving this to #2549943: [plan] Remove as much of the SafeMarkup class's methods as possible instead since it will include a BC break, like the other children of that issue and unlike the other children of #2280965: [meta] Remove every SafeMarkup::set() call.
Comment #18
ianthomas_uksetMultiple issue is #2557893: Mark SafeMarkup::setMultiple as internal
Before this patch, the escaping strategies are documented on SafeMarkup::set(), but this removes that documentation. It would seem more sensible to move it to the remaining public API that uses it, SafeMarkup::isSafe().
Comment #20
stefan.r CreditAttribution: stefan.r commented@ianthomas_uk it wasn't removed, it was moved to ::setMultiple(). It does make more sense to put it on the single public API though, so let's move it there.
However even if it will be marked as internal it would still be good to have some kind of reference to the actual escaping strategies on ::setMultiple(), so I just added a "See self::isSafe() for the list of supported escaping strategies." there.
Comment #21
dawehnerJust grepped for
SafeMarkup::set()
and there are indeed also no references in documentation.I could not find anything to complain about. Read through SafeMarkup itself and it seems good and consistent now.
Comment #22
stefan.r CreditAttribution: stefan.r commentedreroll now that setMultiple() has been marked as @internal
Comment #23
Wim LeersLet's do this then!
s/mark string/mark a string/, can be fixed on commit.
Comment #24
alexpottDo we think there is any margin in warning contrib before doing this. @laurii has said that many contrib modules are using SafeMarkup::set() - i know composer_manager and devel are using it.
Comment #25
davidhernandezI would think at least.
Comment #26
cilefen CreditAttribution: cilefen commentedCan we just reference this one? https://www.drupal.org/node/2311123
Comment #27
ianthomas_ukHow much warning were you thinking? If you want modules to be working in Beta 14 and/or at Barcelona, then you'd want to remove this ASAP (which is a very obvious warning), or delay until after that date.
If contrib authors have been paying attention, they will have known this is coming. The method was documented "This method is for internal use. Do not use it to prevent escaping of markup;" when it was first committed (admittedly no @internal or underscore prefix) and there has been a big push to remove its use from core. That doesn't help the users of any broken modules of course...
Comment #28
lauriiiI'm +1 for doing this ASAP so modules / sites will have as much of time as possible to adapt for this change. I also referenced a CR for this one.
Comment #29
lauriiiComment #30
cilefen CreditAttribution: cilefen commentedI edited https://www.drupal.org/node/2549395 to refer to the other CR with strategies. Does that make sense for everyone?
Comment #31
davidhernandezThat change record might be enough but it was published a week ago. Do we assume everyone was paying attention or do we need something more attention grabbing? It does have set() mentioned with a link to the strategies. Thanks, Chris.
Comment #32
lauriiiI think we can republish the change record. xjm said that she will be posting a blog post about the changes. Beside these two I think we need extensive documentation page which contains all the information about autoescaping in one page. Right now its in multiple change records and its hard to find. My opinion is still that these should not be blocking this issue getting commited.
Comment #33
mpdonadioI think the other good idea is to make a Drupal 8 specific section (or maybe a separate version) of the Handle text in a secure fashion page.
Comment #34
joelpittetComment #35
joelpittetwoops
Comment #36
davidhernandezI agree with the sentiment that if this is to be done then do it sooner rather than later.
Comment #37
Berdir*If* contrib can be updated then removing it IMHO is OK.
The question is, are we sure that we covered all existing cases on contrib and custom now with better/alternative solutions?
A while ago I asked @alexpott about a use case I had with data attributes in #markup, just couldn't get that to work without SafeMarkup::set(). Not sure if that works now?
My code looks like this:
The problem is that the data attribute is destroyed. Obviously, #allowed_tags doesn't help as the problem is not a tag. Doing some testing, SafeString::create() works, but that's marked as internal. And it seems like the actual problem is the 8 in the data attribute.. data-np-adition-url works fine ;) seems like our data attribute detection has a bug?
Anyway, while I could remove that, there are likely valid use cases where you want to print out attributes that *will* be blocked? I guess #template should work then?
Comment #38
ianthomas_uk@berdir unless you've got a better way of identifying any cases that contrib needs SafeMarkup::set() for, that sounds like a good argument for removing ASAP. That way we've got maximum time to fix and fix any issues that do arise. Fixes could include reverting this patch if that's necessary.
Regarding your specific example, that sounds like a core bug that you've been able to use SafeMarkup to work around (I checked the html spec and couldn't see why numbers wouldn't be allowed). We should file and fix the core bug, in the meantime you can work around it by removing the 8.
Comment #39
xjmDiscussed with @alexpott, @catch, @effulgentsia, and @webchick. We agreed that this issue is critical, and removing
SafeMarkup::set()
as soon as possible will give modules that were already (mis)using as much chance to update as possible.Also, we need to update the change records for this before committing it, and also ideally announce this and the other method removals/deprecations on g.d.o/core.
Comment #40
catchYou can create your own class implementing SafeStringInterface. See the 'implemented by' list on https://api.drupal.org/api/drupal/core!lib!Drupal!Component!Utility!Safe... for examples.
Comment #41
lauriiixjm: I was wondering if you could explain which parts needs to be updated? I'm more than happy to do the actual work :)
Comment #42
dawehnerSafemarkup::set() was always considered as bad API, so I'm not sure why we need it for this particular method. I do agree for the other methods like checkPlain / format ... its way more important to inform people about those.
If you search for SafeMarkup::set() practically just https://www.drupal.org/node/2549395 appears.
I also searched for just SafeMarkup and a few issues appeared (https://www.drupal.org/node/2506757, https://www.drupal.org/node/2392803, https://www.drupal.org/node/2445441, https://www.drupal.org/node/2296163, https://www.drupal.org/node/2302363 and https://www.drupal.org/node/1312890)
but none of them call out to SafeMarkup::set(), so I'm not sure why we should update them as part of this issue.
Comment #43
xjmhttps://www.drupal.org/node/2311123 includes nothing about render arrays or the item list template, but leveraging render arrays was one of the ways we were able to get rid a lot of SafeMarkup::set() usage in core. Also the links in https://www.drupal.org/node/2549395 are not accessible; we should add actual link text (unlike what I did here) ;). Finally, we need a g.d.o/core post announcing this as well given the impact.
Edit: Also, we need to cover how to do things that are incompatible with Xss::filterAdmin() somewhere.
Comment #44
xjmAlso the SafeMarkup::format() example in https://www.drupal.org/node/2311123 arguably should be a Twig template. Anyway, I updated it with stubs for how it should be updated. I also think it should actually stop to exist as a separate thing entirely and be merged into https://www.drupal.org/node/2296163 because its title makes no sense now (I had filed an issue for this to happen several months ago but looks like it hasn't happened).
I can work on these things later if someone else doesn't get to them first.
Comment #45
catchTagging this as beta target since it's a deliberate API change that we want to expose to contrib as soon as possible.
Comment #46
webchickThe patch here looks straight-forward enough, but probably needs @xjm's sign-off on whether the docs gate is met.
Comment #47
xjmFound it -- this is the existing issue for the change records: #2494297: [no patch] Consolidate change records relating to safe markup and filtering/escaping to ensure cross references exist
I'm working on getting the CRs up to date and accurate enough so that at least 80% of reasons people might have added
SafeMarkup::set()
to their contrib or custom code are covered. When that's done, I'll prepare a short g.d.o/core post announcing this change as well as thecheckPlain()
deprecation, and then review and probably commit this patch within the next couple days.Comment #49
star-szrJust removing a hunk taken care of by #2557411: Remove obsolete reference to SafeMarkup::set() in LinkGeneratorTest documentation.
Comment #50
chx CreditAttribution: chx commentedYaaaaay! Thank you all so much for finally getting here!
Comment #51
alexpott@chx well the bigger dream is removing SafeMarkup::setMultiple() - #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list and #2559971: Make SafeMarkup::format() return a safe string object to remove reliance on a static, unpredictable safe list would actually allow us to do that :)
Comment #53
tim.plunkettConflicted with #2549393: Remove SafeMarkup::escape() and rely more on Twig autoescaping
Comment #54
stefan.r CreditAttribution: stefan.r commentedreroll looks sane
Comment #55
xjmThanks for the reroll! Just a note that I'm still working on the necessary docs updates for this. Getting close to having the minimum necessary information in https://www.drupal.org/node/2296163 (thanks @lauriii for also helping with this). I'll review the patch for commit once that's done and once I've posted to g.d.o/core announcing it.
Comment #56
xjmDraft for the related g.d.o/core announcement: https://docs.google.com/document/d/1OM33gYwcIXUB-QP2uiF-M-H54QyZdGyJHTj4...
Comment #57
xjmAlright, the announcement is out, and the CRs are not 100% ship-shape but I think good enough to commit this.
I think this documentation needs to be adapted and added to
setMultiple()
now. While it is marked@internal
, it is still a public method, and now the only way for other code to add stuff to the safe list, so we definitely need to warn people off. We could replace the words "This method is for internal use" with the stronger wording on the @internal for that method already.So, I kind of get that the intent here is to test the safe list without the explicit public setter. However, it's weird to say we test a helper method on the test. Maybe it would be better to use SafeMarkup::setMultiple()?
As far as I can tell we are totally losing this test coverage. We should move it to a test for
SafeMarkup::setMultiple()
I think.Finally, I think the proposed commit message also needs to be updated to merge in contributors who only commented here and not on one of the children (e.g. tim.plunkett). People who helped with the CR issues were @stefan.r, @effulgentsia, @alexpott, and @lauriii and they are already covered. Thanks!
Sorry for the delay in reviewing -- sorting out the docs issues took awhile!
Comment #58
alexpottComment #59
ianthomas_ukI'll do (1).
We can't just swap in setMultiple for set in 2 and 3, because setMultiple doesn't return the string.
Comment #60
ianthomas_ukComment #61
stefan.r CreditAttribution: stefan.r commented@ianthomas_uk per #57 could we replace "This method is for internal use" with "This is called by FormCache, StringTranslation and the Batch API. It should not be used anywhere else."? I think it's fine to repeat both on the method docs and in the internal notice.
Comment #62
stefan.r CreditAttribution: stefan.r commentedupdating suggested commit message to include everyone who commented in this issue
Comment #63
stefan.r CreditAttribution: stefan.r commentedI think this bit is irrelevant, people should never call this method to begin with.
Comment #64
ianthomas_uk#61: I prefer the text as it is now. Your suggestion makes it sound like it is fine to use setMultiple while translating strings, or during a batch operation, which is not the case. I don't think the existing documentation does that because that line starts @internal.
#63: Our audience includes developers writing for use cases we've documented. While those use cases are valid it's helpful to have this documentation as a reminder (once they are not we can just remove the function). If a contrib module does decide to use the method, against our advice, at least they might be a little safer if we keep these examples and I don't see that we gain anything by dropping them. If you're just trying to make life harder for people using the function, replace the whole docblock with "@internal Do not use" or similar.
Comment #65
ianthomas_ukComment #66
xjm@alexpott:
Hmm, I'm not sure I agree. Testing a method that is explicitly supposed to escape things unconditionally seems different from something that is not...
Is the first item in the provider actually bogus maybe? It looks like it may have been copied from the
checkPlain()
one without modification (and so the$message
parameter is not actually the message but the expected output from thecheckPlain()
provider).However, as far as I can tell, it is not explicitly tested that
setMultiple() handles objects with
the__toString()
(unless I missed that somewhere in the test; please tell me if so).@ianthomas_uk:
No, but we could test that all of these sequences are safe once set with
setMultple()
, which (edit) would confirm they were not altered during setting.Also:
Let's at least remove the words "$this->safeMarkupSet() and"?
Comment #67
lauriiiIt seems like it is actually not possible to make SafeMarkup::setMultiple mark a TextWrapper safe because it takes the string that is supposed to be marked as safe as an array key, but array key cannot be object.
Comment #68
alexpott@lauriii and all the other tests have equivalents elsewhere - I guess there is no harm in adding extra tests.
This should be SafeMarkup::setMultiple() ...
This is silly. There is no point in doing this.
Comment #69
lauriiiOk I removed the test cases that didn't make much sense.
Comment #70
lauriiiComment #71
alexpottEverything from #66 has been addressed - as pointed out #67 object support is not an issue because array keys don't support objects. All other test coverage is move to a test for
SafeMarkup::setMultiple()
.Comment #73
xjmRe: #67, lol doh :)
That addresses all my concerns. Committed and pushed to 8.0.x. Amazing work!
Comment #75
jmuzz CreditAttribution: jmuzz commentedA call to SafeMarkup::set() results in a full screen error when I try to use dpm(). Try the following from drupal's root dir to find the remaining calls:
My result is:
Comment #76
cilefen CreditAttribution: cilefen commentedLibraries is a contrib module.
Comment #77
cilefen CreditAttribution: cilefen commentedComment #78
rlmumfordAll three of those are in contributed modules. You should raise issues on those modules http://www.drupal.org/project/devel and http://www.drupal.org/project/libraries to fix the problem.
Comment #79
ianthomas_ukAs is devel, which is the module that provides dpm. Check you've got the latest version, and if the problem still happens search for, then open an issue in the devel queue.
Comment #80
jmuzz CreditAttribution: jmuzz commentedOh you got me, sorry I really thought it was core.