form_select_options() is a theme function in disguise and should not use SafeMarkup::set

Progress on work. Passed option widget tests.

Taking 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?

+++ b/core/modules/system/templates/select.html.twig
@@ -12,4 +12,55 @@
+        {% if element['#value'] is not defined %}
+          {% set selected = false %}
+        {% elseif element['#value'] is not iterable %}
+          {% set selected = ((element['#value'] ~ "") == key) %}
+        {% elseif key in element['#value'] %}
+          {% set selected = true %}
+        {% elseif element['#value'] is empty and element['#multiple'] is not empty and key == '_none' %}
+          {% set selected = true %}
+        {% endif %}

Question 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.

Right, 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) %}?

Ignore 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 be options. 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).

Tempted 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.

Yeah this is not a change to the theme system. It changes the forms system.

Actually, 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.

+100 to 22

Cool 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.

Did 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.

Found 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.

I 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.

Looks pretty good.

+++ b/core/modules/ckeditor/js/ckeditor.js
@@ -30,7 +30,7 @@
-      var label = $('label[for=' + element.getAttribute('id') + ']').html();
+      var label = $('label[for=' + element.getAttribute('id') + ']').text();

This reverts a recent security fix :D

Oh, I didn't even notice that. interdiff-- davidhernandez-- :(

I 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.

+++ b/core/includes/form.inc
@@ -78,13 +78,16 @@ function template_preprocess_select(&$variables) {
+ *   @todo Document the format of this array.

There is still @todo comment which needs to be solved

See if this makes sense.

Minor, but I might use current() here:

+      $option = form_select_options($element, $choice->option);
+      $options[] = reset($option);

$options[] = current(form_select_options($element, $choice->option));

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.

Actually, better still would probably be to use array_merge()

+++ b/core/includes/form.inc
@@ -94,29 +103,36 @@ function form_select_options($element, $choices = NULL) {
     elseif (is_object($choice) && isset($choice->option)) {

Does anyone know the use-case that satisfies the elseif where $choice is an object?

..and options applies to non-optgroup selects as well.

+++ b/core/includes/form.inc
@@ -94,29 +103,36 @@ function form_select_options($element, $choices = NULL) {
+        'options' => form_select_options($element, $choice),

I don't think that is correct. This is the only place I see 'options' set, so it only applies to an optgroup?

Patch itself looks quite good. We have @todo inside the IS which needs to be addressed.

+++ b/core/includes/form.inc
@@ -78,13 +78,22 @@ function template_preprocess_select(&$variables) {
+ * @return array

s/array/mixed[]

Is 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.

Its 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.

+++ b/core/includes/form.inc
@@ -78,13 +78,22 @@ function template_preprocess_select(&$variables) {
+ * @return array

Well technically its array[], its an array of arrays, of which array is not further defined.

I disagree because according to the documentation it is possible there to be string values too which makes it mixed.

+++ b/core/includes/form.inc
@@ -94,29 +103,35 @@ function form_select_options($element, $choices = NULL) {
+  $options = [];

Which documentation? This should always return an array. The structure of the array might be different though.

I will review this and work on the todo in the issue summary.

@davidhernandez is clever. :)
adding steps to reproduce an alert in head...

manually tested with the patch, and there is no alert. and when I turn off twig autoescape (thanks @tim.plunkett) by:

diff --git a/core/lib/Drupal/Core/Template/TwigEnvironment.php b/core/lib/Drupal/Core/Template/TwigEnvironment.php
index 346dcc2..6014ab1 100644
--- a/core/lib/Drupal/Core/Template/TwigEnvironment.php
+++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
@@ -58,7 +58,7 @@ public function __construct($root, \Twig_LoaderInterface $loader = NULL, $option
       'auto_reload' => NULL,
     );
     // Ensure autoescaping is always on.
-    $options['autoescape'] = TRUE;
+    $options['autoescape'] = FALSE;

     $this->loader = $loader;
     parent::__construct($this->loader, $options);

and running drush cr

and reloading the page, the alert does happen.

----

next I'm going to read the entire patch.

I 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[])

so... I read the whole thing, and could go either way on the return. so I think it is fine the way it is.

darn. 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.

let's try this. if there is test coverage, then this should fail.

@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

ok. needs work for mixed[] and tests.

Changed array to mixed[].

I would like to write the automated test for this, but I am not sure where to start. Any hints?

There 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.

ok. that addresses the remaining todo's.

Re #15 - oh that was at the NJ sprint.

As @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.

This is a nice fix of a SafeMarkup::set(). Nice work everyone! Committed 8aa9b3a and pushed to 8.0.x. Thanks!