Remove theme_options_none, use an alter hook instead for changing empty option label

Hmm, the only theming function is theme_options_none which only returns translatable strings. It seems extremely overhead to me to convert that no ?

@swentel - Possibly, but at least it's consistent :)

Yeah, let's convert it to get moved over to Twig, and then create a follow-up issue to do something better, maybe remove it entirely? :)

I would remove it immediately to be honest :)

I agree with swentel here and we should remove it immediately. Create some kind of preprocess that allows for the value manipulation by contrib. But I took a stab at it anyways for practice;)

#7: drupal-twig-options-1898434-7.patch queued for re-testing.

#7: drupal-twig-options-1898434-7.patch queued for re-testing.

Okay, the test failures look much more reasonable now :)

Cleaned up styles, fixed a bug with spaces causing tests to fail with {% spaceless %}

Maybe these values can be moved out of theme layer and into config?

Also this thing will fail test with twig debug turned on because it html encodes the label and those twig debug comments get jammed in there.

Ok I think I have plan...
a) Get rid of this theme layer bit
b) Move the code from the preprocessor up to the theme() call above in _options_get_options
c) Augment the allowed_values or equivalent by preprending the option before it hits hook_options_list
d) Possibly rewrite the tests
e) Docs update
f) profit?

Please talk me out of this if you know why this won't do the trick

Ok so here's what I came up with:

No more theme_options_none()

A theme function to change a label value was overkill. I moved the logic for this magical option_none label to the options_allowed_values() so that I didn't lose the ability to manipulate this value.

This is an API change. But now if you want to manipulate the none value of a field you can implement
hook_options_list() and that value will be prepended along with all the other values(just as it was before but previously prepended after the hook call and unavailable in hook_options_list()).

I had to move field "properties" along with the field to get consistent properties to match up like they did previously and it passed all the tests locally.

@todo Write an example to checking empty option and widget type and overriding it's _none label

This definitely looks like the way to go. Yeah, just an example in options.api.php would probably make things real clear. I imagine just a few lines of implementation like this:

function mytheme_options_list(...) {
  if (isset(options[_none])) {
    options[none] = '~~~~' . t('None') . '~~~~';
  }
}

Ok so take 2. This one I am confident will pass. *shakes fist at taxonomy module*

The big difference is I brought everything closer to what it was and added an API Change.

The idea would be if you needed to change the Empty Option Label you could do something like this

function HOOK_options_list(&$field, $instance, $entity) {
  // Set the Empty Option Label
  $field['empty_option_label'] = '--' . t('None') . '--';
}

the API change, as it wasn't super obvious above, would be the $field in the hook being called by reference.

Re-titling ala #1898060: Remove all (useless) theme functions from dblog module, so we don't have to convert to Twig.

This looks great @joelpittet! We should update options.api.php with an example to reflect the API change.

added the @code to docblock from #22

Thanks @webthingee!

+++ b/core/modules/options/options.api.phpundefined
@@ -13,6 +13,13 @@
+ *  // Set the Empty Option Label

This should be a complete sentence per http://drupal.org/node/1354#drupal, and I'm not sure about the capitalization, I would suggest something like:

// Override the default empty option label.

I would also change the example to use something other than 'None', the two dashes are not as obvious IMO :)

@Cottser ~

Taking a looks at it also, here's what I am thinking.

 * For a result of '-- None --', you could use the following:
 * @code
 * function HOOK_options_list(&$field, $instance, $entity) {
 *  // Set the empty option label
 *  $field['empty_option_label'] = '-- ' . t('None') . ' --';
 * }
 * @endcode

Yup, that looks better. Just make sure to end the comment in a period.

This is the re-roll taking the place of the patch from comment #26.

Looking at the change again in the context of the hook_options_list docblock, I think we should explain what the code sample is for, namely changing the empty option label.

Something like this maybe (would need to be wrapped at 80 chars)?

To change the empty option label to '-- None --', update $field['empty_option_label'] as in the following example:

This is the re-roll taking the place of the patch from comment #26 and overriding comment #30, based on #31.

I'm sorry to be saying this now as we're refining docs but after reviewing the patch again, only the module defining the options would be able to change the empty option label. Other modules or themes would not be able to change the empty option label, so that would be a regression.

We could probably just do an alter hook for the empty option label instead. Any objections to that idea?

+++ b/core/modules/options/options.moduleundefined
@@ -659,9 +648,31 @@ function _options_properties($type, $multiple, $required, $has_value) {
   // Get the list of options.
   $options = (array) module_invoke($field['module'], 'options_list', $field, $instance, $entity);

Add conversion summary table

Update remaining

@Cotter, no objections if it gets rid of the theme function:) I just don't know how to make one, could you provide a patch?

Yes.

Related issue: #1751234: Convert option widgets to Plugin system

Whoops, sorry that I only find this one now.

As much as I hate the current theme_options_none(), the proposed solution doesn't fly, since HOOK_options_list() is only invoked on the module providing the field type (i.e it's not a hook but a callback - we're slowly getting rid of those as we move to plugins).

theme('options_none') lets any custom code change the label of the 'none' option (that was a frequent feature request way back in the old CCK days). The proposed solution would only let the field type control it.

@yched - please see #33 which is our current plan (an alter hook) - would that work?

Ah, right, I only looked at the last patch.

Yes, an alter hook sounds good. But then, maybe an alter hook on the whole $options array ?
In OptionsWidgetBase::getOptions() (where the code lives now after #1751234: Convert option widgets to Plugin system) :
- Get the list of options from the field type (module_invoke($this->field['module'], 'options_list'))
- Add the empty option.
- Call the new alter hook,
- Sanitize the whole list (the array_walk_recursive(array($this, 'sanitizeLabel')); bit)
- Flatten if needed (the $this->flattenOptions($options); bit)

@yched - that sounds good. To satisfy the use case of theme_options_none(), when you implement the alter hook you would just target the '_none' key in the array. Thanks for explaining how the sanitize and flatten operations would need to be moved as well.

Code from #1751234: Convert option widgets to Plugin system:

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.phpundefined
@@ -0,0 +1,206 @@
+  protected function getOptions() {
+    if (!isset($this->options)) {
+      // Get the list of options from the field type module, and sanitize them.
+      $options = (array) module_invoke($this->field['module'], 'options_list', $this->field, $this->instance, $this->entity);
+      array_walk_recursive($options, array($this, 'sanitizeLabel'));
+
+      // Options might be nested ("optgroups"). If the widget does not support
+      // nested options, flatten the list.
+      if (!$this->supportsGroups()) {
+        $options = $this->flattenOptions($options);
+      }
+
+      // Add an empty option if the widget needs one.
+      if ($empty_option = $this->getEmptyOption()) {
+        $label = theme('options_none', array('option' => $empty_option, 'widget' => $this, 'instance' => $this->instance));
+        $options = array('_none' => $label) + $options;
+      }
+
+      $this->options = $options;
+    }
+    return $this->options;
+  }

Update API changes

Hello Guys

I did a small patch based in thread recommendations and support from Gnuget and Cottser on IRC, please check and let me know if works for you.

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.phpundefined
@@ -124,8 +124,17 @@ protected function getOptions() {
       // Add an empty option if the widget needs one.
       if ($empty_option = $this->getEmptyOption()) {
-        $label = theme('options_none', array('option' => $empty_option, 'widget' => $this, 'instance' => $this->instance));
-        $options = array('_none' => $label) + $options;
+        //Set empty optins + definitions from others modules
+        $options = array(t('- None -')) + (array) module_invoke($this->field['module'], 'options_list', $this->field, $this->instance, $this->entity);
+
+        //Sanitize options
+        array_walk_recursive($options, array($this, 'sanitizeLabel'));
+
+        // Options might be nested ("optgroups"). If the widget does not support
+        // nested options, flatten the list.
+       if (!$this->supportsGroups()) {
+         $options = $this->flattenOptions($options);
+       }
       }

Thanks for having a go at this @enzolutions! I think we need to move the logic from theme_options_none() to here and then provide the alter hook to change this; we can't just use '- None -' as the default, that would be a regression. Also, we don't need to duplicate the sanitize and flatten operations. Instead, we should move this 'Add an empty option if the widget needs one' code further up within the getOptions() method.

i did a new patch how Cotter suggested.

This patch add the things from #39 and take in account the clarifications from Cottser (#44).

Also, this is a example about how can be use the new hook alter:

function module_options_list_alter(&$options, $field) {

  if ($field->id == "field_name") {
    $options['_none'] = 'this is my default value';
  }
}

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.phpundefined
@@ -114,6 +114,24 @@ protected function getOptions() {
+            $label = t('N/A');

I'm curious whether we could improve this string, as N/A seems to be not easy for new users.

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.phpundefined
@@ -114,6 +114,24 @@ protected function getOptions() {
+      drupal_alter('options_list', $options, $this->field);

It feels wrong to add an alter hook. Do you think there is a need for that?

@dawehner - can you expand on 'feels wrong'? I'm not sure how else we would replicate the functionality of the theme function we're removing. Arguably we could make the alter hook specific to the empty option instead of all the options - I'm fine with either approach.

@dawehner check #33, 39 and #44

I'm not sure if using the drupal_alter is the best way to do the alter hook in this case but works fine.

Okay I'm fine :)

Looks good so far, thanks @gnuget!

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.phpundefined
@@ -114,6 +114,24 @@ protected function getOptions() {
+          case 'options_buttons':
+            $label = t('N/A');
+          break;
+
+          case 'options_select':
+            $label = ($empty_option == OptionsWidgetBase::OPTIONS_EMPTY_NONE ? t('- None -') : t('- Select a value -'));
+          break;

Please indent the 'break' lines by two spaces per http://drupal.org/coding-standards#controlstruct.

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.phpundefined
@@ -114,6 +114,24 @@ protected function getOptions() {
+      drupal_alter('options_list', $options, $this->field);

This should be ModuleHandler::alter() per http://drupal.org/node/1894902, see drupal_alter() API docs that note it is deprecated as well.

Followup to my last comment: I asked in #drupal-contribute and @tim.plunkett said that the alter hook would look like \Drupal::moduleHandler()->alter().

Re-titling.

Updated version of the patch, using \Drupal::moduleHandler()->alter() instead of drupal_alter and a change in the "break" lines for follow the coding standards

Thanks, looking good !

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.phpundefined
@@ -114,6 +114,24 @@ protected function getOptions() {
+          case 'options_select':
+            $label = ($empty_option == OptionsWidgetBase::OPTIONS_EMPTY_NONE ? t('- None -') : t('- Select a value -'));
+            break;

OptionsWidgetBase::OPTIONS_EMPTY_NONE should be static::OPTIONS_EMPTY_NONE

+++ b/core/modules/options/lib/Drupal/options/Plugin/field/widget/OptionsWidgetBase.phpundefined
@@ -114,6 +114,24 @@ protected function getOptions() {
+      \Drupal::moduleHandler()->alter('options_list', $options, $this->field);

If you're using moduleHandler for this alter hook, then it would probably make sense to use it for the module_invoke('options_list') call just above.

The new hook_options_list_alter() needs to be documented in options.api.php

Another version of the patch, taking in account #55

Also, seems to is missing a reference for ModuleHandler::invoke($module, $hook, $args = array()) in http://drupal.org/node/1894902

Thanks @gnuget :-)

Some more polish:

- hook_options_list_alter() hook should receive the same arguments as the hook_options_list() callback:
$this->field, $this->instance, $this->entity

- Use $module_handler = \Drupal::moduleHandler(); rather than fetching it twice in OptionsWidgetBase::getOptions()

+ Several language / formatting issues with the phpdoc for hook_options_list_alter() :

- "Alter the options that is displayed in a option list"
Wrong grammar + missing trailing point + verb needs to be 3rd person.
Since the description for hook_options_list() is "Returns the list of options to be displayed for a field.", I'd suggest "Alters the list of options to be displayed for a field.".

- "Called by getOptions() to allow modules..."
Just mentioning a method name is not enough if you don't specify the class (has to be the fully namespaced class name, including the leading "\".
I'm not really fond, however, of the trend of documenting where hooks are called, this is often impossible to maintain IMO.
I'd suggest simply:
"This hook can notably be used to change the label of the '_none' option."

- @param blocks should not be separated by a newline, and include type hints.

All in all, I'd suggest the following:

/**
 * Alters the list of options to be displayed for a field.
 *
 * This hook can notably be used to change the label of the '_none' option.
 *
 * @param array $options
 *   The array of options for the field, as returned by hook_options_list(). A 
 *   '_none' option might have been added, depending on the field properties.
 * @param \Drupal\field\Plugin\Core\Entity\Field $field
 *   The field definition.
 * @param Drupal\field\Plugin\Core\Entity\FieldInstance $instance
 *   The instance definition. It is recommended to only use instance level
 *   properties to filter out values from a list defined by field level
 *   properties.
 * @param \Drupal\Core\Entity\EntityInterface $entity
 *   The entity object the field is attached to.
 *
 * @see hook_options_list()
 */

I'm not a native english speaker, so is still hard for me write documentation, thanks for your corrections @yched.

I found a problem, i had to change:

$module_handler->alter('options_list', $options, $this->field, $this->instance, $this->entity);

with this:

$context = array(
  'field' => $this->field,
  'instance' => $this->instance,
  'entity' => $this->entity
);
$module_handler->alter('options_list', $options, $context);

this because \Drupal::moduleHandler()->alter() expects maximum 4 params (http://api.drupal.org/api/drupal/core!lib!Drupal!Core!Extension!ModuleHandler.php/function/ModuleHandler%3A%3Aalter/8) so i had to use a $context variable how is recommended in the drupal_alter in drupal 7 (http://api.drupal.org/api/drupal/includes!module.inc/function/drupal_alter/7)

The problem is i don't know how this will affect the documentation in options.api.php :-/

I attach the patch updated with this changes, and i did a little modifications at the options.api.php file but i'm not sure if is correct what i did there

Thanks for your time and corrections.

my last patch has a problem in the options.api.php =/ i attach a updated version

Yeah, I'm not a native english speaker either, and writing clean docs is always a painfiul process ;)

alter() / $context: Ah, right, that's the usual pattern for alters with many arguments.
You can have a look at hook_entity_field_access_alter() for an example of how this gets documented in api.php

Oks, i updated the documentation using as example the hook_entity_field_access_alter().

Thanks again @yched :-)

#61: 1898434-61-options_none.patch queued for re-testing.

Thanks @gnuget !
Looks good to me if testbot approves.

This is looking good, nice work @gnuget and @yched :)

1. +++ b/core/modules/options/options.api.phpundefined
   @@ -68,3 +68,31 @@ function hook_options_list($field, $instance, $entity) {
   + * This hook can notably be used to change the label of the '_none' option.
   ...
   +    // Change the label of the _none option
   

   In the comments for in options.api.php can we refer to 'the empty option' instead of 'the _none option'? A bit friendlier IMO :)

2. +++ b/core/modules/options/options.api.phpundefined
   @@ -68,3 +68,31 @@ function hook_options_list($field, $instance, $entity) {
   +  // Check if is the field where want be changed the label of the _none option
   

   Wording is a bit off here, how about "Check if this is the field we want to change."

   This comment and the inline comment noted above need to end in a period per http://drupal.org/node/1354#drupal.

3. +++ b/core/modules/options/options.api.phpundefined
   @@ -68,3 +68,31 @@ function hook_options_list($field, $instance, $entity) {
   + *   The array of options for the field, as returned by hook_options_list(). A ¶
   

   Trailing whitespace.

4. +++ b/core/modules/options/options.api.phpundefined
   @@ -68,3 +68,31 @@ function hook_options_list($field, $instance, $entity) {
   + * @param array $context
   + *   Context array on the performed operation with the following keys:
   + *   - field: The field definition (\Drupal\field\Plugin\Core\Entity\Field).
   

   On which performed operation? I'm confused by this description. Maybe it's copy + pasted from hook_entity_field_access_alter() but I'm not sure it makes sense here.

   What about just:

   @param array $context
     An associative array containing:
     - field: …
   

5. +++ b/core/modules/options/options.api.phpundefined
   @@ -68,3 +68,31 @@ function hook_options_list($field, $instance, $entity) {
   + *   - instance: The instance definition. It is recommended to only use
   + *     instance level properties to filter out values from a list defined
   + *     by field level properties (Drupal\field\Plugin\Core\Entity\FieldInstance).
   

   Re-wrap these lines at 80 characters please per http://drupal.org/coding-standards#drupal. The last line is 81 characters but this could work out:

    *   - instance: The instance definition. It is recommended to only use instance
    *     level properties to filter out values from a list defined by field level
    *     properties (Drupal\field\Plugin\Core\Entity\FieldInstance).

Oks, a updated version with the changes from #65

Changes look good, thanks @gnuget!

+++ b/core/modules/options/options.api.phpundefined
@@ -72,27 +72,27 @@ function hook_options_list($field, $instance, $entity) {
- *   The array of options for the field, as returned by hook_options_list(). A 
- *   '_none' option might have been added, depending on the field properties.
+ *   The array of options for the field, as returned by hook_options_list(). A
+ *   empty option might have been added, depending on the field properties.

For the param docs here maybe we can say 'An empty option (_none) might have been added…'? I think we should mention _none somewhere :)

Remove sandbox link

Updating summary to use template and reflect current status

Oks, done :-)

I'd say this is ready to go now, I did a manual test last night creating node_options_list_alter() and it worked great.

Just rolling in a tiny coding standards fix (http://drupal.org/coding-standards#array), RTBC! Great work @gnuget and thanks again @yched :)

Updated issue summary.

This has changed completely from my crazy start but I am so happy to see this theme function get the boot and get replaced by something better! Great work @yched @gnuget and @Cottser for taking the idea and running wild:)

Updated issue summary.

Much better! Committed/pushed to 8.x.

Change notice started at http://drupal.org/node/1987618

Change notice looks good. Thanks !

Thanks @swentel! I added an example of a comparable theme_options_none() override to the change notice as well.

@Cottser: right, makes sense :-).
$field->id might turn into a protected property at some point, so I switched it to $field->id() in the code sample.

@yched - should we create a small follow-up to adjust the sample code in options.api.php then?

Oh. Yes, I guess so :-/

Update API changes

Created #1988614: Adjust hook_options_list_alter() example in options.api.php to use $field->id() instead of $field->id as a follow-up issue.

Added #2426781: Custom OptionWidget have no empty option label as a follow-up issue.