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
form_select_options() is a recursive markup generating function. It uses SafeMarkup::set() to get around the fact that it's basically a theme function in disguise, hiding inside template_preprocess_select().
Commit credit: also add cilefen, porchlight, lbainbridge.
Proposed resolution
refactor to actually printing the options markup in select.html.twig.
Remaining tasks
- (done) Update docs of form_select_options().
- (done) change return to mixed[]
- (done - not needed. See #72) We also need to 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.
Manual testing steps (for XSS and double escaping)
In head
- edit form.inc to take out the checkplains
$options .= '<option value="' . $key . '"' . $selected . '>' . SafeMarkup::checkPlain($choice) . '</option>';
- add a text field to basic page content type
- trick it into putting script tag by closing the quotes
- use these allowed values
"><script>alert('XSS');</script><"|one two|two three|three
- load a create content page and see the alert. (or load the field settings page)
API changes
Removes form_select_options(), however this was an internal function only called (in core at least) by template_preprocess_select().
Comment | File | Size | Author |
---|---|---|---|
#71 | interdiff-54-71.txt | 598 bytes | alimac |
#71 | form_select_options-2501481-71.patch | 5.32 KB | alimac |
| |||
#67 | do-not-commit.checking-for-test-coverage.patch | 582 bytes | YesCT |
#54 | interdiff-2501481-47to54.txt | 1.07 KB | davidhernandez |
#54 | form_select_options-2501481-54.patch | 5.32 KB | davidhernandez |
Comments
Comment #1
star-szrComment #2
star-szrWorking on this, so far so good.
Comment #3
star-szrInitial patch, worked on this with @kfriend. It will have fails.
The Classy template removal is only temporary to avoid having to update multiple template files while iterating.
Comment #5
kfriend CreditAttribution: kfriend commentedComment #7
kfriend CreditAttribution: kfriend as a volunteer commentedProgress on work. Passed option widget tests.
Comment #8
kfriend CreditAttribution: kfriend as a volunteer commentedComment #10
davidhernandezTaking a look at this now, and will work on the tests. I think we can remove form_select_options since I don't find anything else calling it in core. I'd possibly want framework manager confirmation on that since maybe it is needed for some other, broader api reason?
Comment #11
star-szrYeah I think it can be removed. Could potentially grep D7 contrib if we want to see if anyone is using it for stuff.
Comment #12
davidhernandezQuestion about the conditions me by this 'if'. What exactly is the functionality this is satisfying? All I'm seeing is selected being set, but it isn't printed anywhere. It is only checked later on and if it is true
' selected="selected"'
gets printed.Comment #13
star-szrIt's just a Boolean, never printed. Just used to determine whether to print as you said.
Comment #14
davidhernandezRight, what I mean is why the various elseifs; checking if the element value equals the key, versus it just existing, and this set
{% set selected = ((element['#value'] ~ "") == key) %}
?Comment #15
davidhernandezIgnore my previous comment. I see now what that is doing, and the replication from the original PHP function.
I'm uploading a patch with a few things.
1) Added comment to the template about the macro with twig reference link. We've done this in other macros and I think it is helpful.
2) Removed form_select_options(). I did not see it used anywhere else in core.
3) Removed the "defined test accommodates..." comment from the template. I don't see what it is in reference to. Leftover from testing something?
4) Moved the spaceless tag higher, because the markup was still creating a large amount of empty space that I feel made the markup more annoying to read.
5) Fixed a mistake in the code around
choice.option
. This should beoptions
. This is what caused the php errors and failed tests for me. It is also singular in the original PHP code, which means that code is likely not working at all in HEAD. The fail was on dblog, because the select list created there seems to be one of the few that would even satisfy that condition.6 ) Included the Classy template.
cilefen should be included in the commit credit. We had to yell at each other for a good while to figure out 5).
Comment #17
tim.plunkettTempted to switch this to form system...
Sure it abuses SafeMarkup::set(), but it also is a self-contained recursive function.
This change generates the world's most complex twig file, that cannot be unit tested and clearly wishes it were just PHP.
Comment #18
tim.plunkettYeah this is not a change to the theme system. It changes the forms system.
Comment #19
star-szrTim that sounds great too. Thanks!
Comment #20
star-szrAnd what I mean is I would love a solution to this that isn't tied up with the theme system if at all possible.
Comment #21
tim.plunkettActually, this seems to be duplicating a lot of #342316: Introduce proper Form API #types for 'option' and 'optgroup', and make #options consistent., which seems like a reasonable approach.
Comment #22
star-szrWhat if we sort out all the crazy edge cases in PHP (pre render probably) and build out the structured data as an array or similar to pass to the template? Middle ground to be found there? I do think we're stretching Twig a bit here, but not because it's recursive. Menus are recursive too.
Comment #23
tim.plunkett+100 to 22
Comment #24
star-szrThanks Tim! I'm thinking since it wouldn't be preparing a render array then just in the preprocess may actually make more sense than pre render here. Definitely seems to be one of the more complicated form API elements.
Comment #25
star-szrRough prototype. Not all that recursive, seeing what fails for now. Early feedback definitely welcome!
No interdiff because it's a new approach.
Comment #27
star-szrOops, a couple obvious fixes :)
Comment #28
star-szrOne thing that's not super clear to me is whether or not HEAD supports nested optgroups. It seems like the HTML5 spec does not: http://www.w3.org/TR/2012/WD-html5-20121025/the-optgroup-element.html#th...
Comment #30
davidhernandezCool that's what I was just about to do last night. The template is simpler, and thank you for moving the whitespace modifier further out. The resultant markup was making me twitch. As long as we are getting markup out of php and removing the SafeMarkup calls, it should be an improvement.
The taxonomy uid test is the same that was failing for me. You can see the ui fail when you try to add a taxonomy filter in a view. For me it looked like an issue with the 'if' checking the option, but that is completely gone in this patch.
Comment #31
star-szrSo I tried to run
Drupal\taxonomy\Tests\Views\TaxonomyIndexTidUiTest
locally and 2 hours later I took my headphones off and wondered why my fans were spinning. Test was still running, yikes. Manual testing steps are super helpful though thanks @davidhernandez! I'll comment before I start working on this again.Edit: And that means anyone else is also welcome to jump in of course!
Comment #32
star-szrWorking on this at Drupal North.
Comment #33
star-szrSo far I can't see any problems with this approach.
@porchlight and @lbainbridge also helped.
Comment #34
star-szrComment #35
davidhernandezDid that reset really just fix everything? Good catch.
Looking this over now. Here is a before and after screenshot of some markup. Looks the same. Is there a place in core to check an optgroup? I don't find one anywhere and it doesn't look like any of the UIs let you make one.
Comment #36
davidhernandezFound an optgroup. "Add a new field" when adding a field to a content type uses it.
This looks good. I didn't see any issues and did some manual testing. I did not have any issues with the safety of the strings.
Setting back to needs work, because we need the Classy template.
Comment #37
star-szrHere's that. As far as the template itself, do we want to make it DRYer by putting the
<option>
output into a macro or something? Or is that just unneeded complexity?Comment #38
davidhernandezI thought about that, but the way the template is right now it is very easy to understand, and I don't think we gain anything with the added complexity.
Comment #39
davidhernandezLooks pretty good.
Comment #40
star-szrThanks, still has todos though.
Comment #41
tim.plunkettThis reverts a recent security fix :D
Comment #42
star-szrLooks like I forgot to rebase. I'll fix that tonight, thanks @tim.plunkett!
Comment #43
davidhernandezOh, I didn't even notice that. interdiff-- davidhernandez-- :(
Comment #44
star-szrDidn't get to it last night so here's the rebased patch. No other changes from #37.
Comment #45
dawehnerI really like the fact that we move HTML into the template, where it belongs to.
On the other hand I don't feel to be confident to RTBC this patch, given my limited knowledge.
Comment #46
lauriiiThere is still @todo comment which needs to be solved
Comment #47
davidhernandezSee if this makes sense.
Comment #48
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedMinor, but I might use current() here:
Looks like some of the other cases specifying an array return value seem to use a : like "containing the following keys:", but it's not especially consistent.
Comment #49
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedActually, better still would probably be to use
array_merge()
Comment #50
davidhernandezDoes anyone know the use-case that satisfies the elseif where $choice is an object?
Comment #51
star-szrThat one test that was failing via Views UI I'm pretty sure uses that method.
The docs are an awesome start thanks @davidhernandez. They should probably say that it can be a nested array, and options applies to non-optgroup selects as well.
Comment #52
davidhernandezI don't think that is correct. This is the only place I see 'options' set, so it only applies to an optgroup?
Comment #53
star-szrSorry, yes of course you're right.
Comment #54
davidhernandezComment #55
lauriiiPatch itself looks quite good. We have @todo inside the IS which needs to be addressed.
s/array/mixed[]
Comment #56
davidhernandezIs it mixed if it's always an array, regardless of the depth of the array?
Which todo are you referring to? The testing? I assume someone will do that when reviewing. I did some manual testing myself, and did not find any issues.
Comment #57
lauriiiIts array but mixed[] means its an array containing mixed items.
It would be helpful to have it there so its easier to review & move forward.
We also need to 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.
Comment #58
dawehnerWell technically its array[], its an array of arrays, of which array is not further defined.
Comment #59
lauriiiI disagree because according to the documentation it is possible there to be string values too which makes it mixed.
Comment #60
davidhernandezWhich documentation? This should always return an array. The structure of the array might be different though.
Comment #61
YesCT CreditAttribution: YesCT commentedI will review this and work on the todo in the issue summary.
Comment #62
YesCT CreditAttribution: YesCT commented@davidhernandez is clever. :)
adding steps to reproduce an alert in head...
Comment #63
YesCT CreditAttribution: YesCT commentedmanually tested with the patch, and there is no alert. and when I turn off twig autoescape (thanks @tim.plunkett) by:
and running drush cr
and reloading the page, the alert does happen.
----
next I'm going to read the entire patch.
Comment #64
YesCT CreditAttribution: YesCT commentedI think @return array is fine the way it is. it is accurate. (if we really want to be more specific, then I think it would be mixed[])
Comment #65
YesCT CreditAttribution: YesCT commentedso... I read the whole thing, and could go either way on the return. so I think it is fine the way it is.
Comment #66
YesCT CreditAttribution: YesCT commenteddarn. forgot about checking for test coverage. noting in the issue summary so it is not missed again, but I'll also look for it right now.
Comment #67
YesCT CreditAttribution: YesCT commentedlet's try this. if there is test coverage, then this should fail.
Comment #68
lauriii@davidhernandez: thats why its mixed[], array containing mixed types of items. You can also say stdClass[] which would mean array containing stdClass type of items and string[] if all the items are strings
Comment #69
lauriiiComment #70
YesCT CreditAttribution: YesCT commentedok. needs work for mixed[] and tests.
Comment #71
alimac CreditAttribution: alimac at University of Illinois at Chicago commentedChanged
array
tomixed[]
.I would like to write the automated test for this, but I am not sure where to start. Any hints?
Comment #72
davidhernandezThere is already test coverage for the functionality of select form items in
Drupal\system\Tests\Form
and we are now changing the functionality to output through Twig instead of directly in form_select_options. Don't we now have test coverage through Twig? I understand that we do not have proper test coverage in head, but I think the functional change here gives us new test coverage by changing the way this outputs.The mixed[] change is fine.
Comment #73
YesCT CreditAttribution: YesCT commentedok. that addresses the remaining todo's.
Comment #74
YesCT CreditAttribution: YesCT commentedComment #75
cilefen CreditAttribution: cilefen commentedRe #15 - oh that was at the NJ sprint.
Comment #76
lbainbridge CreditAttribution: lbainbridge at Northern Commerce commentedAs @cottser mentioned in #32/33 we worked on this together with @porchlight at Drupal North, I was asked to comment so I could get credit.
Comment #77
alexpottThis is a nice fix of a SafeMarkup::set(). Nice work everyone! Committed 8aa9b3a and pushed to 8.0.x. Thanks!