Problem/Motivation

  • The rendering of select elements and their options is inconsistent with how checkboxes and radios are rendered
  • The markup of <option> and <optgroup> cannot be modified per-element

Proposed resolution

  • Introduce a proper #type optgroup and #type option
  • Use identical expansion of #options into sub-elements as is being used for #type checkboxes and radios except for the fact that it supports arrays as value which get expanded into optgroup elements
  • \Drupal\Core\Render\Element\Select::preRender() is duplicated by template_preprocess_select() and the former is not called by #type language_select elements

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Update the issue summary noting if allowed during the beta Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

None.

API changes

  • \Drupal\Core\Form\OptGroup::flattenOptions() is moved to \Drupal\Core\Render\Element\Optgroup as it would be confusing to have two different OptGroup classes doing similar things; as part of the moving the separate protected method doFlattenOptions() is removed as it is unnecessary and can be replaced by a recursive call to flattenOptions()
  • form_flatten_options() is removed; this is not technically necessary, but was easy enough to do here
  • form_select_options() is removed
  • It is no longer supported to explicitly set #options to NULL as this is rather pointless
  • It is no longer supported to put objects which have a options member variable in #options as this is rather pointless
Files: 
CommentFileSizeAuthor
#120 342316-bench--do-not-test.patch5.16 KBtstoeckler
#115 342316-115-select-options.patch43.64 KBtstoeckler
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,673 pass(es). View

Comments

lilou’s picture

FileSize
19.73 KB
1.45 KB
Failed: 10267 passes, 1 fail, 0 exceptions View

stBorchert’s picture

Title: Add classes for select list » Add attributes to option elements
FileSize
962 bytes
Failed: Failed to apply patch. View

This patch adds the ability to add attributes to option elements in a more general way. Thus you can add a class or title to option in select lists.

Status: Needs review » Needs work

The last submitted patch failed testing.

stBorchert’s picture

Status: Needs work » Needs review
FileSize
1016 bytes
Failed: Failed to apply patch. View

Ups, patch isn't working as expected. One minor change.

Wim Leers’s picture

Status: Needs review » Needs work

If you fix the patch, I'll review it. I need this for Hierarchical Select, so it'd save me an override function.

Your code style is wrong: you should now put spaces around dots!

stBorchert’s picture

Status: Needs work » Needs review
FileSize
1.42 KB
Failed: Failed to apply patch. View

Ah, true. I tried it with d5 and d7 and forgot to add the spaces. Additionally there was something missing in the last patch so here is a new one.

edit: argh, it failed again. Unfortunately I couldn't checkout HEAD from here and have to use latest d7-dev package. Any hints whats wrong now?

stBorchert’s picture

Well, seems latest dev-tarball is not really up to date with head so the patch fails

Hunk #1 FAILED at 1429.
Hunk #2 FAILED at 1440.
Hunk #3 FAILED at 1448.
3 out of 3 hunks FAILED -- saving rejects to file includes/form.inc.rej

Will try again later (if I can access CVS again).

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.29 KB
Failed: 6625 passes, 16 fails, 24 exceptions View

Here's a reroll :)

stBorchert’s picture

Thanks for doing this. I must admit, I simply forgot about it :-/

lilou’s picture

Status: Needs work » Needs review

@stBorchert : +1 for supports all kind of attributes, but your patch remove classes by default (the scope of this issue and my patch #2), like theme_table (odd, even) and theme_item_list (first, last).

stBorchert’s picture

Status: Needs work » Needs review

Hi.
The patch doesn't remove anything. It allows you to add the classes on the place you are building the options for the select list. :-)
Thus you are not sticked to "even" and "odd" but you are totally free to add any class to each option.
If it should be striped the modules have to do this on generating the options array. Otherwise there would (I'm pretty sure they would) be complaints about changing the classes, adding a third class, whatever :-)

 Stefan

stBorchert’s picture

Hm. While looking through the forms api and discussing the fact that using '#value' for the displayed text of an option probably isn't best with a friend (because the value is the one the option would return) I found '#return_value' would be better.
So options should be build like

$options = array(
  array(
    '#value' => t('first value to display'), 
    '#return_value' => 'first_value_to_return', 
    '#attributes' => array('class' => 'form-option')
  ),
);

Passing a simple array of string as options would convert the array key into '#return_value' and the array value into '#value'.
Thoughts?

lilou’s picture

Status: Needs review » Needs work

Yes, I quite agree with you, but for theme_item_lists() and theme_table(), drupal add classes by default.

Back to CNW because patch #10 failed tests.

stBorchert’s picture

Status: Needs work » Needs review
FileSize
1.34 KB
Failed: Invalid PHP syntax. View

Well I found a nasty bug with an unset '#value' that causes the testbot to fail (hopefully no more errors).

@lilou: as you said, its done by theme-functions. Unfortunately this isn't a theme function. Its worth thinking about adding a theme_option().

lilou’s picture

Status: Needs work » Needs review
FileSize
1.48 KB
Failed: 7003 passes, 87 fails, 18 exceptions View

Small typo (elseif)

stBorchert’s picture

Ok, I ran simpletest on my local machine to see which tests failed and why. The failed tests didn't have to do anything with this patch, they are all "Uncaught exception" (e.g. blog api)

INSERT INTO {simpletest} (test_id, test_class, status, message, message_group, function, line, file) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7) - Array ( [:db_insert_placeholder_0] => 1 [:db_insert_placeholder_1] => BlogAPITestCase [:db_insert_placeholder_2] => exception [:db_insert_placeholder_3] => Object of class stdClass could not be converted to string [:db_insert_placeholder_4] => Recoverable fatal error [:db_insert_placeholder_5] => d [:db_insert_placeholder_6] => d [:db_insert_placeholder_7] => d ) SQLSTATE[HY000]: General error: 1366 Incorrect integer value: 'd' for column 'line' at row 1

Only on taxonomy, trigger and especially user tests I'm not sure about the errors. E.g. "Failed to set field x to ...". This needs more review.

stBorchert’s picture

Wow, this gets more complicated as I initially thought :-)
User.module tests failed because of providing the permissions in user_filters() as optgroups. The way this is build has to be changed (maybe taxonomy and triggers, too).
Now it is:

  $options = array();
  foreach (module_implements('perm') as $module) {
    $function = $module . '_perm';
    if ($permissions = $function('perm')) {
      asort($permissions);
      foreach ($permissions as $permission => $description) {
        $options[t('@module module', array('@module' => $module))][$permission] = t($permission);
      }
    }
  }

proper usage after the patch lands:

  $options = array();
  foreach (module_implements('perm') as $module) {
    $function = $module . '_perm';
    if ($permissions = $function('perm')) {
      $options[t('@module module', array('@module' => $module))] = array('#options' => array());
      asort($permissions);
      foreach ($permissions as $permission => $description) {
        $options[t('@module module', array('@module' => $module))]['#options'][$permission] = t($permission);
      }
    }
  }

Unfortunately there are some more points to take care of (e.g. the filter on admin/user/user isn't working) so this would take a while until the testbot is happy.

stBorchert’s picture

Status: Needs work » Needs review
FileSize
6.28 KB
Failed: Invalid PHP syntax. View

Ok bot, here's some food for you.
I've ran all tests and didn't see any errors caused by this patch (hopefully).

stBorchert’s picture

PHP syntax error?
I've run the syntax checks of pifr (pifr_review_check_syntax()) on my local machine without getting any errors:

check syntax of file ./modules/user/user.module
check syntax of file ./modules/user/user.admin.inc
check syntax of file ./modules/trigger/trigger.admin.inc
check syntax of file ./includes/form.inc
output of php -l -f:
Array
(
    [0] => No syntax errors detected in ./modules/user/user.module
    [1] => No syntax errors detected in ./modules/user/user.admin.inc
    [2] => No syntax errors detected in ./modules/trigger/trigger.admin.inc
    [3] => No syntax errors detected in ./includes/form.inc
)

Any ideas why tesbot fails?

stBorchert’s picture

Status: Needs work » Needs review
FileSize
6.35 KB
Failed: 7889 passes, 5 fails, 3 exceptions View

Next try with clean HEAD.
Again, no parse errors found in these 4 files.

stBorchert’s picture

FileSize
7.21 KB
Failed: 9335 passes, 6 fails, 113 exceptions View

Argh, this happens when you're editing with vi over a web interface :-/
user.admin.inc misses 4 "s". Therefore the user list filter could not be set and the tests failed. Additionally locale.inc has to be modified to use the new array('#options' => ...).
Hope this passes now (local test succeeded without any errors or exceptions).

stBorchert’s picture

FileSize
7.21 KB
Failed: Failed to install HEAD. View

Hm, testbot doesn't recognize the patch so here it is again.

stBorchert’s picture

Category: task » feature
Priority: Minor » Normal

Making this a feature request and pushing priority to get more attention.

stBorchert’s picture

Status: Needs work » Needs review
FileSize
8.17 KB
Passed: 11117 passes, 0 fails, 0 exceptions View

Modified patch to work with current HEAD.

dawehner’s picture

one question:

the handling of for examples checkboxes doesn't change?

<?php
$options['value1'] = t('display1');
$options['value2'] = t('display2');

'#options' => $options
?>

If yes its confusing for me, why there is additional syntax for select, but not for checkboxes.

stBorchert’s picture

Status: Needs review » Needs work

You're right. The patch changes the syntax for <option /> elements only.
I will re-roll the patch to give the ability adding attributes to all children of #options.

Thanks for the hint.

dawehner’s picture

also

i would like to have documentation for it in the patch, this adds quite a bit of functionality, but hard to see in code.

stBorchert’s picture

FileSize
9.86 KB
Failed: 11088 passes, 23 fails, 62 exceptions View

Well, extended the patch to allow adding attributes to checkboxes and radio buttons.
The only thing that has to be done eventually is adding (for example) the class attribute of the checkbox to its surrounding div ("form-item").

Documentation has to be done within the forms-api docu, but here's a short summary of the new behavior.

<option> elements can now be defined using the following syntax:

<?php
$form['optiontest'] = array(
  '#type' => 'select',
  '#title' => t('Option test'),
  '#options' => array(
    array(
      '#return_value' => 0,
      '#value' => t('First option'),
      '#attributes' => array('class' => 'first', 'title' => t('First option')),
    ),
    array(
      '#value' => t('Option group'),
      '#attributes' => array('class' => 'group', 'title' => t('This is an optgroup')),
      '#options' => array(
        array('#return_value' => 2, '#value' => t('1st sub-option')),
        array('#return_value' => 4, '#value' => t('2nd sub-option')),
      ),
    ),
  ),
);
?>

The code for radio buttons and checkboxes looks similar.
(btw.: the old syntax (key-value-pair) is still valid.)

stBorchert’s picture

For the log: we had a small discussion in IRC about the use of '#return_value' instead of simply using the array key as the return value. As #return_value is standard FAPI (for checkboxes and radios) it is advisable to use it for options also. (Btw: using the array key as return value is working, too)

Changing status back to needs review.

sun’s picture

Title: Add attributes to option elements » FAPI: Turn options and optgroups into proper #types
Category: feature » task

+1

But let's remove the entire, ugly magic here. Why complicated if it can be simple? Let's introduce:

#type = 'optgroup'
#type = 'option'

So, when iterating over #options, just check whether we're dealing with one of those #types, and handle them accordingly. Fall back to keyed, non-recursive arrays for trivial cases.

The resulting logic will work similarly to #type = 'radios' and #type = 'checkboxes' (depending on whether the select is multiple or not). Additionally, this will allow to support #disabled and other stuff like #id and #attributes (which was the original trigger).

Yes. That will make option groups in form arrays a bit more verbose (but then again, it's questionable whether it's that more verbose, compared to using weird, recursive, anonymous objects in #options). However, it removes all of the current limitations and implements those form elements as proper Form API elements.

casey’s picture

Version: 7.x-dev » 8.x-dev
xjm’s picture

Tracking; this would be very useful and prevent the need for a lot of crazy workarounds. I'd find #disabled, #access, and #attributes all especially valuable.

theunraveler’s picture

+1

@sun: so, would the array to create checkboxes look something like this?

$form['checkboxes'] = array(
  '#type' => 'checkboxes',
  '#options' => array(
    'option1' => array(
      '#type' => 'option',
      '#title' => t('Option 1'),
    ),
    'option2' => array(
      // and so on...
    ),
  ),
);

If that is the case, I would like to make sure that this does not introduce a regression. We should still be able to define #options as key => value as well. sun already covered this in their comment and I agree completely.

casey’s picture

I would say:

$form['checkboxes'] = array(
  '#type' => 'checkboxes',
  'option1' => array(
    '#type' => 'option',
    '#title' => t('Option 1'),
  ),
  'option2' => array(
    // and so on...
  ),
);

Like this, we could still allow for #options

theunraveler’s picture

@casey: Would whatever is in '#options' then supersede the option definitions as arrays? I think I like it better in '#options', personally.

-or-

in 'options', like most theme functions in D7 work.

casey’s picture

My proposal is exactly how Drupal handles its render trees. you could use element_children() and 'option1' and 'option3' would be automatically available in #children.

And to keep backward compatibility form_process_checkboxes() could check for #options and if available expose them as element children.

I guess this is what sun is proposing too.

sun’s picture

Hrm. To me it sounds like a lot more solid architectural planning is required here. Actually tempted to move that architectural discussion into a g.d.o wiki page and mark this issue as postponed...

But since we have new issue summaries, let's try whether that works out. I've updated the issue summary with an architectural proposal.

theunraveler’s picture

Perhaps I'm being dense, but it seems like the way Drupal 7 handles parametrized theme functions is by prefixing required params with '#'. For example:

$page['some_link'] = array(
  '#theme' => 'links',
  '#links' => array(),
);

It works the same way with theme_item_list(). Using '#options' to define both complex and trivial option sets just seems a bit cleaner to me. If we start defining options as un-prefixed array keys, then it seems like the '#options' key is just sticking around for legacy reasons. I'd rather use one way or the other.

theunraveler’s picture

Nice issue summary, sun. I believe it's missing an example of '#type' => 'option' though. All if the '#options' are simple one-dimensional arrays.

theunraveler’s picture

Issue summary: View changes

New architectural summary.

sun’s picture

#type 'option' cannot be used on its own. Option elements are automatically expanded from #options into sub-elements for #type 'select' and 'optgroup'.

theunraveler’s picture

Why can't '#type' => 'option' be used on its own? It seems silly to write it and not use it.

That would allow us to do things like:

$form['checkboxes'] = array(
  '#type' => 'checkboxes',
  '#title' => t('A thing'),
  '#options' => array(
    'opt1' => array(
      '#type' => 'option',
      '#title' => t('Option 1'),
      '#disabled' => TRUE,
    ),
    // and so on...
  ),
);
sun’s picture

Sorry, I meant that #type 'option' (as well as 'optgroup') cannot be used outside of a #type 'select' on its own. Of course, the new expansion algorithm for #options would produce elements of #type 'option'.

I've updated the issue summary accordingly to clarify.

Actually, we have a concrete architectural proposal, so let's try to get it signed off or hear about objections now.

theunraveler’s picture

Sorry to ask so many questions, but should we assume that this issue is particular to '#type' => 'select'? It seems that, if we want to do the same thing for '#type' => 'radios' and '#type' => 'checkboxes', it would be a simple patch, since there are already '#type's for 'radio' and 'checkbox'.

Basically, what we are proposing here is to give '#type' => 'select' the same treatment as 'radios' and 'checkboxes', right? That is to say, breaking out it's children into their own '#type's.

xjm’s picture

#61: type=#checkboxes and type=#radio already support #options, same as type=#select. I believe you are conflating the markup with the FAPI implementation. There is the HTML tag <option>, and then there is the FAPI #options array, which is a property of a FAPI element (that may or may not be rendered as an <option> tag depending on context). The second is what we are discussing here.

The idea is that we make FAPI support adding #disabled, #access, #attributes, etc. to #options--regardless of the #type--and then FAPI takes care of applying these in the way appropriate to the specific markup.

Edited for clarity.

sun’s picture

Status: Needs work » Needs review

The idea is that we make FAPI support adding #disabled, #access, #attributes, etc. to #options--regardless of the #type--and then FAPI takes care of applying these in the way appropriate to the specific markup.

Not to #options itself. I don't support the idea of replacing the current whack hack of #type select's #options with another whack hack of #options.

#options should remain as a simple, one-dimensional array of key/value pairs. If you want optgroups, use optgroups.

This introduces consistency across all #types. Wherever #options appear, they will be expanded into sub-elements of #type 'option'. Hence, the behavior will be identical for #type 'checkboxes', 'radios', 'select', and 'optgroup'.

If you want an attribute for a particular option, then apply it on the sub-element.

In D7+, form_process_checkboxes() and form_process_radios() already support you in doing so by allowing for partial sub-element pre-definitions -- the following is working code to apply a class to option "Foo" and disable option "Bar":

$form['myboxes'] = array(
  '#type' => 'checkboxes',
  '#options' => array(
    'foo' => t('Foo'),
    'bar' => t('Bar'),
    'baz' => t('Baz'),
  ),
);
$form['myboxes']['foo'] = array(
  '#attributes' => array('class' => array('ok')),
);
$form['myboxes']['bar'] = array(
  '#disabled' => TRUE,
);
sun’s picture

Title: FAPI: Turn options and optgroups into proper #types » Introduce proper Form API #types for 'option' and 'optgroup', and make #options consistent.
Status: Needs review » Active

Actually, there's no patch for this yet. Also, better title.

theunraveler’s picture

Thanks for the clarification. This seems very well-defined to me now.

Also, cool little pointer, sun!

theunraveler’s picture

Issue summary: View changes

Added info about expansion into #type 'option'.

sun’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

Automatic expanding could bring some overhead to rendering & processing forms.

HendrikM’s picture

I´m trying to get this working inside a hook_form_alter. The following code hat so effect at all:

$form['field_publishing_status']['und']['#options']["draft"]['#disabled'] = TRUE;

where "draft" is the key for the field.

<input type="radio" class="form-radio" checked="checked" value="draft" name="field_publishing_status[und]" id="edit-field-publishing-status-und-draft">

Any idea why? Thanks!

doublejosh’s picture

Status: Needs work » Needs review
FileSize
3.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 342316_option_add_attributes_69.patch. Unable to apply patch. See the log in the details link for more information. View

Here's how I added #attributes to options.
It also leaves normal form structures intact, including: normal option lists, opt groups, array of option items arrays, plus empty opt groups for placeholder items (like special menu items).

I overwrote the form_select_options() with a theme_registry_alter() and theme_select() to get class #attributes into options for hierarchical jump selects. Here's that code in place: jump_menu.module.

franz’s picture

Status: Needs work » Needs review
FileSize
3.26 KB
FAILED: [[SimpleTest]]: [MySQL] 36,523 pass(es), 361 fail(s), and 66 exception(s). View

Simply re-rolled

barraponto’s picture

Status: Needs work » Needs review
FileSize
3.26 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

re-roll, will address the test results later.

andypost’s picture

Status: Needs work » Needs review

needs re-roll

andypost’s picture

Issue summary: View changes

Updated issue summary.

Neo13’s picture

Is it still impossible to add attributes to an option?

svenssson’s picture

The last submitted patch, 69: 342316_option_add_attributes_69.patch, failed testing.

andypost’s picture

this needs to be landed before release, otherwise will require bc

tstoeckler’s picture

Assigned: Unassigned » tstoeckler

Was bitten by this recently, will take a stab at this.

tstoeckler’s picture

Assigned: tstoeckler » Unassigned
Issue tags: +Needs tests
FileSize
10.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Here's a first stab. Tested manually that options work. Did not test optgroups yet.

I documented a few open questions in @TODOs in code.

Status: Needs review » Needs work

The last submitted patch, 88: 342316-88-select-options.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
1.33 KB
10.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

This should be better.

Optgroups in core aren't converted yet, so those will fail, but regular selects should be fine now.

Status: Needs review » Needs work

The last submitted patch, 90: 342316-90-select-options.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
Issue tags: +VDC
FileSize
3.88 KB
12.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

OK, so those fatals were in fact from the lack of optgroup support in #options. Let's see if this is green.

The reason that is problematic and that - like the issue summary - I think this support should be removed is that optgroups themselves need to be #process-ed to turn themselves into multiple #options. However, selects #process themselves into multiple optgroups at that stage and thus the #process callbacks of the newly created optgroups does not run. This is a shortcoming of Form API but not one that is solved easily. Also optgroups aren't that common so I think it's fine to require a little bit more work.

Tagging this VDC because I realized that Views exposed filters have optgroup support and I'm not quite sure where to adapt the code to account for the new structure. I need some help with that.

Status: Needs review » Needs work

The last submitted patch, 92: 342316-92-select-options.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
2.99 KB
11.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,673 pass(es), 337 fail(s), and 51 exception(s). View

Wow, it's quite a WTF that #input => FALSE is ignored for FormElements.

Let's see.

Status: Needs review » Needs work

The last submitted patch, 94: 342316-94-select-options.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
578 bytes
12.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,113 pass(es), 121 fail(s), and 7 exception(s). View

Fixed Weight render element.

Status: Needs review » Needs work

The last submitted patch, 96: 342316-96-select-options.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
2.5 KB
14.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,293 pass(es), 3 fail(s), and 1 exception(s). View

Fix form option validation.

Status: Needs review » Needs work

The last submitted patch, 98: 342316-98-select-options.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
Issue tags: -VDC
FileSize
2.27 KB
14.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,332 pass(es). View

Unless I managed to break something else with this, this should be green. If that's the case, the next steps are:
1. Write tests for Select, Optgroup, and Option
2. Clean-up code and remove lots of legacy stuff.

At this point I would suggest to remove the optgroup support in #options in a follow-up (if at all) because that seems like a non-trivial undertaking and the patch will already be big enough without it.

tstoeckler’s picture

Issue tags: -Needs architectural review, -Needs tests
FileSize
33.09 KB
40.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,278 pass(es), 102 fail(s), and 21 exception(s). View
9929101 DEV-342316: (Patch #101) Remove lots of legacy option handling code
ff15d11 DEV-342316: Fix weights not being applied correctly and expand tests
d6e2d52 DEV-342316: Use a data provider for SelectTest
be3cb4c DEV-342316: Add tests for options, optgroups and adding attributes
b96a7a8 DEV-342316: Fix duplicate attributes being set for select elements
0fb6417 DEV-342316: Add a SelectTest

If this is still green I consider this patch "done", as in ready for some serious reviews.

Removing the "Needs architectural review" tag as this is pretty much inline with what is done with radios/checkboces. Of course it can't hurt ;-)

tim.plunkett’s picture

+++ b/core/tests/Drupal/Tests/Core/Render/Element/OptGroupTest.php
@@ -31,19 +31,12 @@ public function testFlattenOptions($options) {
-    $object1 = new \stdClass();
-    $object1->option = array('foo' => 'foo');
-    $object2 = new \stdClass();
-    $object2->option = array(array('foo' => 'foo'), array('foo' => 'foo'));
-    $object3 = new \stdClass();
+    $object = new \stdClass();
     return array(
       array(array('foo' => 'foo')),
       array(array(array('foo' => 'foo'))),
-      array(array($object1)),
-      array(array($object2)),
-      array(array($object1, $object2)),
-      array(array('foo' => $object3)),
-      array(array('foo' => $object3, $object1, array('foo' => 'foo'))),
+      array(array('foo' => $object)),
+      array(array('foo' => $object, array('foo' => 'foo'))),

So this is the API change? (Aside from the class moving)

We're losing support for all those other usages?

tstoeckler’s picture

FileSize
6.33 KB
40.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,251 pass(es), 135 fail(s), and 26 exception(s). View

Forgot to mention that I also think in terms of test coverage the added test is sufficient IMO. Here is the list of test cases, taken from the inline comments:

    // Test an empty select element.
    // Test a simple select element.
    // Test a select element with a default value.
    // Test a required select element.
    // Test a required select element with a default value.
    // Test a select element with option groups.
    // Test a select element with option groups and a default value.
    // Test a select element with attributes on the option elements.
    // Test a select element with option groups that have attributes.
    // Test a select element with both options and option groups.

Feel free to suggest additional test cases.

I noticed there were a bunch of @TODOs which I added in order to clean them up before commit. Did that now. The only one that remains is one that I've long wondered about: Why select elements use #value internally instead of #default_value:

    // @TODO This is ported from form_select_options(). But instead of
    //   checking #value, #default_value should be used here and below, right?

So that needs to be considered before commit. It might be that we want to leave it that way and clean it up later, then we can just convert the @TODO into a @todo.

Oh, also I should mention that I removed all support for putting objects into #options. That seems utterly pointless to me at this point.

tstoeckler’s picture

Ahh X-post:

Re #102: Yes, that is an API change. We drop support for passing objects that happen to have a option member variable into #options. Consequently, that support is also dropped from OptGroup::flattenOptions().

The last submitted patch, 101: 342316-101-select-options.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 103: 342316-102-select-options.patch, failed testing.

andypost’s picture

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
4.71 KB
43.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,612 pass(es). View
3a7b0b5 DEV-342316: Remove invalid #options => NULL from FormTestEmptySelectForm
70b8436 DEV-342316: ThemeHandler::enable() is now ThemeHandler::install()
0fe037f DEV-342326: #type 'language_select' should add select's pre-render callback
aa9828d DEV-342316: Retain $element['#options'] for select elements

The retaining of #options is now consistent with checkboxes and radios and in general less error-prone.

Let's see.

tstoeckler’s picture

Forgot to mention this: In fixing FormTest I realized it duplicates a lot of the new SelectTest. Will have to see how to proceed. I do find it rather weird to test *every* form element in one single test, but perhaps that should be left to a follow-up? OTOH I wrote a DrupalKernelTest and FormTest is a WebTest so I would prefer to remove the duplicated parts from FormTest instead. Thoughts?

tstoeckler’s picture

Issue summary: View changes
FileSize
656 bytes
43.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 342316-110-select-options.patch. Unable to apply patch. See the log in the details link for more information. View

So I looked into FormTest and in fact there's no duplication. FormTest tests the form submission of select elements (among other things) and SelectTest just tests the rendering. The former could also be moved to a KernelTestBase in theory, but that's not for this issue.

So this just adapts the test method name.

I also revamped the issue summary.

I would like some feedback first before writing a change notice.

Status: Needs review » Needs work

The last submitted patch, 110: 342316-110-select-options.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
826 bytes
43.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,660 pass(es), 43 fail(s), and 25 exception(s). View

Here we go.

Removed the remaining @TODO. It's not at all related to this issue (it just bugged when I was refactoring the code) and I can't be bothered to open an issue right now.

Status: Needs review » Needs work

The last submitted patch, 113: 342316-113.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
4.41 KB
43.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,673 pass(es). View

Let's see if this is green.

joelpittet’s picture

Issue tags: +Twig, +sprint

Putting this on our radar for BADCamp to review.

dawehner’s picture

Problem/Motivation

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it improves the consisteny and DX
Issue priority Normal because it affects only a small amount of people
Disruption Disruptive just for really advanced usecases.
  1. +++ b/core/includes/form.inc
    @@ -107,34 +106,10 @@ function form_set_value($element, $value, FormStateInterface $form_state) {
    -function form_options_flatten($array) {
    -  return OptGroup::flattenOptions($array);
    -}
    

    Afaik we should try to remove the BC breaks if possible ...

  2. +++ b/core/includes/form.inc
    @@ -143,138 +118,43 @@ function form_options_flatten($array) {
     function template_preprocess_select(&$variables) {
       $element = $variables['element'];
    -  Element::setAttributes($element, array('id', 'name', 'size'));
    -  Element\RenderElement::setAttributes($element, array('form-select'));
     
       $variables['attributes'] = $element['#attributes'];
    -  $variables['options'] = form_select_options($element);
    +  $variables['options'] = drupal_render_children($element);
     }
     
    

    So we replaced a simple function call with a drupal_render_children ... did anyone did some benchmarks whether this scales for a complex form?

  3. +++ b/core/lib/Drupal/Core/Render/Element/Option.php
    @@ -0,0 +1,51 @@
    +    return array(
    +      '#process' => array(
    +        array($class, 'processAjaxForm'),
    +      ),
    +      '#pre_render' => array(
    +        array($class, 'preRenderOption'),
    +      ),
    +      '#theme' => 'option',
    +      '#selected' => FALSE,
    +      '#return_value' => NULL,
    +    );
    

    Do we really need to run those for every option? This could result in a performance problems for select fields with a lot of options.

  4. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
    @@ -137,9 +137,7 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
    -            $choice = new \stdClass();
    -            $choice->option = array($term->id() => str_repeat('-', $term->depth) . String::checkPlain(\Drupal::entityManager()->getTranslationFromContext($term)->label()));
    -            $options[] = $choice;
    +            $options[$term->id()] = str_repeat('-', $term->depth) . String::checkPlain(\Drupal::entityManager()->getTranslationFromContext($term)->label());
    

    This is to be honest somehow a great DX improvement.

tstoeckler’s picture

Thanks for the review, this was very, very helpful!

  1. I'm not 100% up to date with the current state of release management, but the function docs are currently marked with
    * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
    * Use \Drupal\Core\Form\OptGroup::flattenOptions().

    So it seems that is fine? Again, feel free to correct me here.

Will now try to do some micro-benchmarks to be able to respond to 2. and 3. Thanks for bringing that up, this is really important and something I would have overlooked.

tstoeckler’s picture

Status: Needs review » Needs work

Wow, you were spot on. The current pach is a massive regression. Attached patch creates a form with 100 select elements with 100 options each.

In 8.0.x rendering it takes ~0.25 seconds. With the patch it takes ~4.65 seconds. Clearly this is not acceptable.

I will dig into this, when I find some time.

tstoeckler’s picture

Oh, forgot to attach the test-patch.

YesCT’s picture

#2501481: form_select_options() is a theme function in disguise and should not use SafeMarkup::set is duplicating some of this. And that is part of the #2280965: [meta] Remove every SafeMarkup::set() call critical. Maybe we should... use some of this in that issue, or come back to this and try and get this in first.

If we want to try and get this in, since it is a normal task, we should add a beta eval to make sure it is allowed now, and document why.

joelpittet’s picture

The option and option group really need to go inside the select template, loading a file for each option is a bit crazy IMO.

The #2501481: form_select_options() is a theme function in disguise and should not use SafeMarkup::set looks like the right direction. Just need to get that last failure knocked out.

Cottser’s picture

#117 (on phone, no Dreditor) has at least part of a beta evaluation. Haven't had a chance to look at the patch here yet.

stefan.r’s picture

stefan.r’s picture

I don't know how close this issue is, but @catch mentioned today this would be a nice issue to solve the ugliness of point 1 in https://www.drupal.org/node/2564451

andypost’s picture

+++ b/core/includes/theme.inc
@@ -2273,6 +2273,14 @@ function drupal_common_theme() {
+    'option' => array(
+      'render element' => 'element',
+      'template' => 'option',
+    ),
+    'optgroup' => array(
+      'render element' => 'element',
+      'template' => 'optgroup',
+    ),

let's do that in options template so group names will be rendered at the same context as options

lokapujya’s picture

Should this continue from #115, or is a rewrite?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xeM8VfDh’s picture

Thank you VERY much @ sun for the suggestion in #63. Its times like these when I wish we could 'like' comments (and also properly reference/link users).