Modernize form select option helpers

This issue also came up in #2426781-9: Custom OptionWidget have no empty option label.1. Let's fix it in 8.0.x first and then backport it to D7. Nothing is broken here so I think it's not a bug it's a task. Also added beta evaluation and improved issue summary.

It's a simple search and replace so it's a novice task. We also need a draft change record for this change.

Please find a patch.
I defined a constant OPTIONS_EMPTY_OPTION_KEY and replaced '_none' everywhere in the options module.

- Should we replace it everywhere in the core ? (form.inc, Entity, Field, User)
- Does "need a draft change record" means create a new row here ?

Renamed the constant to OPTIONS_EMPTY_NONE in order to follow current naming from #2426781: Custom OptionWidget have no empty option label

Hmmm, I think OPTIONS_EMPTY_OPTION would be much better but it should be defined in OptionsWidgetBase just like existing constants.

With the patch applied I still see multiple places where _none is used.

Given the first search result, doesn't that mean, this constant belongs into the FAPI?

core/includes/form.inc
121:      $empty_choice = $empty_value && $key == '_none';

core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php
116:        'field' => '_none',
172:          '_none' => $this->t('- None -'),
185:      if ($selection_handler_settings['sort']['field'] != '_none') {
348:      if ($sort_settings['field'] != '_none') {

core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php
116:        'field' => '_none',
172:          '_none' => $this->t('- None -'),
185:      if ($selection_handler_settings['sort']['field'] != '_none') {
348:      if ($sort_settings['field'] != '_none') {

core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsWidgetBase.php
84:    if ($element['#required'] && $element['#value'] == '_none') {

Please find a patch defining the constant into OptionsWidgetBase and replacing _none everywhere in the code (not only in options module).
This is my first patch using POO so please review it carefully and give your input so I can fix it if it's not the appropriate way (I'm definitely not sure the way I access the variable is appropriate).

Thank you very much for the patch. There are still some _none left in OptionsWidgetsTest and one in docs options.api.php.

You are right. Let me update this patch.

Please find an updated patch with missing "_none" replacements.

I don't see any more missing '_none' matches. Replacing '_none' with a constant seems like a great idea to me. I'm going to mark this RTBC.

--- a/core/includes/form.inc
+++ b/core/includes/form.inc

@@ -118,7 +119,7 @@ function form_select_options($element, $choices = NULL) {
-      $empty_choice = $empty_value && $key == '_none';
+      $empty_choice = $empty_value && $key == OptionsWidgetBase::OPTIONS_EMPTY_OPTION;

I doubt that its a good idea to have FAPI depend on OptionsWidgetBase

So either a constant in form.inc or maybe a new class, which would also handle form_select_options() in the future, would be a better place.

Let's move it to form.inc then.

Sorry, missing replacement. Working on it.

(facepalm)

1. +++ b/core/includes/form.inc
   @@ -19,6 +19,11 @@
   +const OPTIONS_EMPTY_OPTION = '_none';
   

   How about OPTIONS_EMPTY_CHOICE as a name for this constant? OPTIONS_EMPTY_OPTION sounds a bit weird :) Not saying that we *have* to rename it, just bringing it up as an option (hah!)

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php
   @@ -169,7 +169,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   -          '_none' => $this->t('- None -'),
   +            OPTIONS_EMPTY_OPTION => $this->t('- None -'),
   

   We need to remove the two extra spaces here.

1. I initially think about OPTIONS_EMPTY_OPTION_KEY as it's only used as a key in a select-list/radio.

2. Find an updated patch to remove extra-space. (I'm not drinking coffee, maybe I should regarding the number of wrong patch posted this morning)

+++ b/core/includes/form.inc
@@ -19,6 +19,11 @@
+const OPTIONS_EMPTY_OPTION = '_none';

Are we certain we want to introduce another global constant that will limit our ability to do phpunit tests? Should it be called OPTIONS_EMPTY_NONE, that implies it is from options.module?

@larowlan the constant seems like a good idea and a bit of progress to keep that value consistent so I'm +1 on it.

Lack of PHP unit testing could be a concern but wouldn't it be better to abstract that into a class with a static class constant to do it all in? If so maybe that can be a follow-up?

OPTIONS_EMPTY_KEY seems short and to the point maybe? OPTIONS_EMPTY_NONE is a bit strange because we are trying to equate empty to _none don't you think?

I'm not adverse to just leaving it OPTIONS_EMPTY_OPTION either. OPTIONS_EMPTY_OPTION_KEY is getting long but don't mind it either.

OPTIONS_EMPTY_CHOICE is another suggestion (from #24) :)

Please don't put it in form.inc.

Ideally form_select_options() would have been moved to a static method by now, the constant should go with it.

@amateescu whoops didn't see your suggestion, just looked at the last few comments.

@tim.plunkett can we do the ideal at the moment? Or do you have a suggestion that would not be ideal but maybe better than form.inc for this constant to live and move this issue a further?

I spent zero time on figuring out good names, but this is what I had in mind.

IMHO this seems like followup. We can't really backport #31 to D7.

#25 can absolutely be backported. We're not adding a new global constant to form.inc in D8 only to move it in a follow-up.

This issue is still classed as:

• 8.0.x-dev
• Needs review

But it's not completely clear from the comment thread what's still required: there's a patch on #25 and a patch on #33, and discussion after the two of them. As this patch is also tagged as "Novice" then it would be good to clarify the next steps so anyone can pick it up.

I propose that what's remaining is:

1. RTBC the patch on #31 (or whatever is finally agreed during testing) for 8.0.x-dev.
2. Write change record for this.
3. Change this issue to 7.x-dev.
4. RTBC patch on #25 (or whatever etc. as 1. above.) for 7.x-dev.
5. Get patches committed.

If this is correct, the issue summary should be updated with these tasks and we can proceed to "proper" review.

(Triaged as part of Sprint Weekend 2016.)

Wrote a draft change record

Working on as part of global sprint

Re-rolled patch from https://www.drupal.org/node/2448545#comment-9771769

There was a lot of conflicts with:
CONFLICT (content): Merge conflict in core/modules/system/src/Tests/Entity/EntityCacheTagsTestBase.php
CONFLICT (content): Merge conflict in core/modules/options/src/Tests/OptionsWidgetsTest.php
CONFLICT (modify/delete): core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php deleted in 3caa7b355ec2630758044cb9bdb5d7ed8a202196 and modified in Applying patch from issue 2448545 comment 9771769. Version Applying patch from issue 2448545 comment 9771769 of core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php left in tree.
CONFLICT (content): Merge conflict in core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsWidgetBase.php
CONFLICT (content): Merge conflict in core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php
CONFLICT (content): Merge conflict in core/includes/form.inc

Made it like in previous patch

This reroll was not perfect, let's add the tag back.

rerolled

This probably needs a re-roll. Unassigning as no work done on it within a few months.

The needs issue summary update still needs to be done to reflect comments from #25.

I'm working on this at DrupalCon.

Patch #46 applies cleanly against 8.3.x

Updated title and issue summary to reflect the patch in it's current state.

Here's another re-roll with some of the extras that got in there removed and I put form_get_options() in the helper as well.

I'm not sure on the naming, maybe that can be shorter for things considering they are in an Options helper the methods and constant don't need to repeat that, what do you think?

This would need a re-roll.

Re-rolled.

I'm reviewing this patch.

I'm mentoring @Skabbkladden on this issue.

The patch looks OK. I could not find any remaining uses of '_none'.

A quick fix, if there are anybody up for it, maybe @Skabbkladden, is to update the deprecation notices to 8.5?
@deprecated in Drupal 8.3.x... should be @deprecated in Drupal 8.5.x...

Here is an updated patch and interdiff as per comment mentioned in #70

Thank you @Dinesh18

1. +++ b/core/lib/Drupal/Core/Form/FormOptionsHelper.php
   @@ -0,0 +1,154 @@
   +/**
   + * @todo.
   + */
   

   whoops :)

2. +++ b/core/lib/Drupal/Core/Form/FormOptionsHelper.php
   @@ -0,0 +1,154 @@
   +   * Converts an array of options into HTML, for use in select list form elements.
   +   *
   +   * This function calls itself recursively to obtain the values for each optgroup
   +   * within the list of options and when the function encounters an object with
   +   * an 'options' property inside $element['#options'].
   +   *
   +   * @param array $element
   +   *   An associative array containing the following key-value pairs:
   +   *   - #multiple: Optional Boolean indicating if the user may select more than
   +   *     one item.
   +   *   - #options: An associative array of options to render as HTML. Each array
   +   *     value can be a string, an array, or an object with an 'option' property:
   +   *     - A string or integer key whose value is a translated string is
   +   *       interpreted as a single HTML option element. Do not use placeholders
   +   *       that sanitize data: doing so will lead to double-escaping. Note that
   +   *       the key will be visible in the HTML and could be modified by malicious
   +   *       users, so don't put sensitive information in it.
   +   *     - A translated string key whose value is an array indicates a group of
   +   *       options. The translated string is used as the label attribute for the
   +   *       optgroup. Do not use placeholders to sanitize data: doing so will lead
   +   *       to double-escaping. The array should contain the options you wish to
   +   *       group and should follow the syntax of $element['#options'].
   +   *     - If the function encounters a string or integer key whose value is an
   +   *       object with an 'option' property, the key is ignored, the contents of
   +   *       the option property are interpreted as $element['#options'], and the
   +   *       resulting HTML is added to the output.
   +   *   - #value: Optional integer, string, or array representing which option(s)
   +   *     to pre-select when the list is first displayed. The integer or string
   +   *     must match the key of an option in the '#options' list. If '#multiple' is
   +   *     TRUE, this can be an array of integers or strings.
   +   * @param array|null $choices
   +   *   (optional) Either an associative array of options in the same format as
   +   *   $element['#options'] above, or NULL. This parameter is only used internally
   +   *   and is not intended to be passed in to the initial function call.
   

   lots of >80 here

Good to see this moving though, blast from the past (I asked dasha to file it...back when we were building stuff with D7)

Addressed @larowlan's concerns from #73.

Due to the time that has passed, this issue would be good to do a once over the code to ensure it's being used in all the places it can be.

I found two instances of '_none' in:
- drupal/core/modules/field/src/Plugin/migrate/process/d7/FieldInstanceSettings.php
- drupal/core/modules/system/tests/src/Functional/Entity/EntityCacheTagsTestBase.php

Attached is a rerolled patch and interdiff.

Sorry, I removed my interdiff as It was incorrect.

@pfrilling:

I created an interdiff for you. For future reference: https://www.drupal.org/documentation/git/interdiff

We usually use "reroll" when updating a patch that no longer applies cleanly. I would say that you "updated" the patch from #74.

Just reroll.
There was moved simpletest test into the javascriptfunctional test.

Reviewed the patch. Found no errors using simplytest.me.

1. Created a list field using the select field widget.
2. Added a list field to the basic page content type.
3. Checked the field on both the edit and the view tabs of a basic page node.

This looks good, but we're not doing the deprecation as per the policy

1. +++ b/core/includes/form.inc
   @@ -33,149 +34,27 @@ function template_preprocess_select(&$variables) {
   + * @deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0.
   

   This needs to be 8.8.x now

2. +++ b/core/includes/form.inc
   @@ -33,149 +34,27 @@ function template_preprocess_select(&$variables) {
    function form_select_options($element, $choices = NULL) {
   

   we need a trigger_error here for the deprecation and a deprecation test

3. +++ b/core/includes/form.inc
   @@ -33,149 +34,27 @@ function template_preprocess_select(&$variables) {
   + * @deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0.
   ...
    function form_get_options($element, $key) {
   

   same here, need to update the version, add a trigger_error and add a deprecation test

We also missed a few spots:

Targets
    Occurrences of '_none' in Directory core with mask '*.php' (In String Literals)
Found Occurrences  (8 usages found)
    Usage in string constants  (8 usages found)
            app/core/modules/block_content/tests/src/Kernel  (1 usage found)
                BlockContentEntityReferenceSelectionTest.php  (1 usage found)
                    99 'sort' => ['field' => '_none'],
            app/core/modules/field/src/Plugin/migrate/process/d7  (1 usage found)
                FieldInstanceSettings.php  (1 usage found)
                    30 'field' => '_none',
            app/core/modules/options/tests/src/Functional  (4 usages found)
                OptionsWidgetsTest.php  (4 usages found)
                    284 _none
                    306 $this->assertFalse($this->xpath('//select[@id=:id]//option[@value="_none"]', [':id' => 'edit-card-1']), 'A required select list with an actual value has no "none" choice.');
                    318 $this->assertTrue($this->xpath('//select[@id=:id]//option[@value="_none" and text()=:label]', [':id' => 'edit-card-1', ':label' => '- None -']), 'A non-required select list has a "None" choice.');
                    568 $this->assertTrue($this->xpath('//select[@id=:id]//option[@value="_none" and text()=:label]', [':id' => 'edit-card-1', ':label' => '- None -']), 'A test select has a "None" choice.');

Re-rolled for 8.9.x (prior to addressing @larowlan's comments in #85.

Addressed @larowlan's comments in #85.

Thank you for the patch, @jofitz.

I think there may have been some issue with your patch in #88 as it contains git merge conflict data. I think that the next step would be to go back to #83, and try to re-roll this one again. It will most likely have many merge conflicts that you will need to resolve by checking what was intended to be added and putting that back into the relevant spot in the code base as it is today.

After that's done, do as @jofitz did in #89 and apply @larowlan's reviews in #85.

I could apply the patch, so no reroll is needed

Was re-rolled the latest patch

Lets try again

Fixed coding standard error

+++ b/core/includes/form.inc
@@ -34,148 +35,29 @@ function template_preprocess_select(&$variables) {
+ * @deprecated in drupal:8.9.0 and is removed from drupal:9.0.0. Use
+ *   \Drupal\Core\Form\FormOptionsHelper::formSelectOptions() instead.

As per #3088246: [policy, no patch] How to handle Drupal 8.9.x deprecations we cannot deprecate in 8.9 for removal in 9.0, unfortunately as this is not critical I think this will have to be deferred until 9.1 now.

Back to correct target branch.

Sorry, The Target branch is 9.1.x-dev.

As the #94 patch getting applied on the latest version . So I have rerolled the patch for 9.1.x-dev.

@meena-bisht
thanks for your help.
But we already on 9.x version. so we no need these deprecated function at all.
So, I've rerolled patch from #94 and removed these deprecation functions

D10 version needed
At this time we would need a D10.1.x patch or MR for this issue.

Re Rolled patch 2448545-100.patch to 10.1.x Branch.

Unhappy TestBot.

Updated few more occurrences of '_none' .

There is already a constant defined for '_none' in media module any suggestions on how to proceed with replacement of that constant ? should we replace it now or deprecate it now and remove in 11.x or ignore it ? do we have any policy in regards that ?

core/modules/media/src/MediaSourceInterface.php:
  74     */
  75:   const METADATA_FIELD_EMPTY = '_none';
  76  

Created follow up issue #3324012 to replace METADATA_FIELD_EMPTY constant.

Rebased against 11.x Branch.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased branch to 11.x, but getting some Functional tests failures

Tests pass

Title + MR title mention deprecating form_select_options but that doesn't appear to be the case.

You're right, I'll put it back with the trigger_error.
Shall I create a new CR for the deprecation or add it to the current draft?

Lets do a new one for the deprecation.

Current one could use some love but think adding a constant deserves to be separate.

OK. And one more question. Is core/tests/Drupal/KernelTests/Core/Form a good place to put the deprecation test?

Think that should be good since the function isn't in a module. Least I can't think of a better spot

Ughh the code removes form_get_options() too

Don't the new methods need tests for themselves?

Please ignore the change in .gitlab-ci.yml, which is for debugging purposes, all other changes and reported feedback are ready for this issue

Left some feedback.

Thanks @smustgrave.
Moreover in #3290812: Deprecate form_get_options() it was proposed to remove form_get_options() as there is no usage in core and (then) in contrib. We need to see if we still want to do that here

In https://drupal.slack.com/archives/C1BMUQ9U6/p1707853736065729 , catch agreed to remove form_get_options() completely

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Still appears to have some open threads in the test. But rest is looking real good.

Fixed Tests and updated depreciations.

Tests are passing. Seems its ready for review.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Bot false positive.

Rebased to latest head, seems it is ready for review.

Only did a light review but you get the flow.

Overall looks good though.

Review comments addressed.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Feedback appears to be address here. I see nothing outstanding.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Commented on MR with suggestion to meet CS line length issue flagged by NR bot.

Rebased. I applied the comment line length suggestion, but I'm restoring the RTBC status anyway since it's a very minor change.

The title reads like a list of changes instead it should be a description of what is being fixed or improved. Can this return to the simpler, original title? Remember, The title is used as the git commit message so it should be meaningful and concise. See List of issue fields.

There are two change records here and neither has been reviewed. That needs to be done. However, I don't think we need a change record to announce the new constant. That doesn't fit the criteria in the policy at Change records.

I reviewed the comments and left suggestions and questions.

There are a bunch of suggestions to clean up.

Took a chance on a new title and this feels like it belongs in the form system.

This also need a rebase since the template preprocess fixings have been converted.

Edit: applied the comment suggestions since they make sense to me.

Edit 2:

I'll be honest I think this should be split into two issues.

Issue 1
Deprecate and move form_select_options
Deprecate form_get_options($element

Issue 2
Replace _none with a constant.

Ok I reduced scope here to just deprecating the two functions and creating one replacement.

I created a follow up for the documentation here: #3548829: [pp-1] Add examples for FormSelectOptions::process it is postponed on this.
I also created an issue for the _none bit here #3548830: Add enum for _none and replace uses it is not postponed, but it should be an enum and can be done in parallel.

I did this on 11.x and went through and addressed all comments and moved the one I did not.

I reviewed the CRs and will move the enum one.

I updated the Issue Summary too.

Assuming tests pass this is ready for review and should be more straightforward to get in now.

I left removal at drupal 12 since form_select_options is not used all that much and it's an easy replacement and form_get_options is not used period.

This is ready for review, one comment on the MR that might need updating.

Greatly reduced scope this is not likely to conflict again and I created follow ups.

@nicxvan, I'm glad you liked my suggestion from #164.

The reduction in scope is helpful here, this is now just one task. There are issues for the remaining tasks.

My points have been addressed so restoring the RTBC.

Let's not change and move the documentation.. if we're move code and docs let's keep everything the same and then file follow-up issues to improve if necessary but on a quick review many of the changes being made here do not feel like improvements of the no-brainer kind that might be allowed during a code move.

Ok I rebased this and updated the deprecation versions.

I addressed the comment feedback and reset it to the original, we can do a follow up to modify it.

I added @final, I know nobody should really instantiate this or extend it, but what is the harm if someone does that? Do we really need to fully lock this down?

Ok I added it.

Needs a rebase please

Wring MR sorry

Apologize, I rebased this yesterday as it was 700+ commits back and then got super distracted by work (not fun).

Anyway I bumped the versions to 11.5 since 11.4 is out.

Believe latest feedback has been addressed, CR is basic and straight forward.

All LGTM. My bad again promise wasn't drive by rebasing.

Thanks, rebase and version bump looks great!

I did another check on this issue. I found errors in the deprecation messages and made suggestions for those, which I have applied. I also updated credit and reviewed the change record.

Leaving at RTBC

Thank you for those updates, they look great!