Inconsistent autogenerated values for user fields with options.

[ipokkel]

Describe the bug
User fields, when having multiple required select fields and setting the values for a select field with only the first value having a value:label the registration check returns all other fields need filling in if only one of the select fields were not filled in.

:Select 
Yes
No

It appears that if a select field has a value:label option set the rendered values are numerical and when the field does not have a value:label set the rendered values are as entered.

To Reproduce
Steps to reproduce the behavior:

1. Go to Memberships > Settings > User Fields and create a select type field with the following options:

:Select 
Yes
No

2. Create another select field with the following options:

Yes
No

3. Save all field settings.
4. Navigate to the checkout page and review the values set for these select type fields in the browser's tools.

Screenshots

Expected behavior
The automated value and label generated for options to be consistent.

Isolating the problem (mark completed items with an [x]):

• I have deactivated other plugins and confirmed this bug occurs when only Paid Memberships Pro plugin is active.
• This bug happens with a default WordPress theme active, or Memberlite.
• I can reproduce this bug consistently using the steps above.

WordPress Environment

Paid Memberships Pro 2.12.2

[ideadude]

Tricky.

There are likely other edge cases when labels are only sometimes give for the options. I wonder what happens if you e.g. have options like:

:Select
Yes
No:No

We have code here that checks if an array is non-associative (i.e. the keys for the array elements are 0, 1, 2, 3...). If so, we assume that the keys aren't important indexes and so we can update the array so all keys match the values... which is better for searching or later updating the list of options.

paid-memberships-pro/classes/class-pmpro-field.php

Lines 359 to 367 in d25351f

	//is a non associative array is passed, set values to labels
	$repair_non_associative_options = apply_filters("pmprorh_repair_non_associative_options", true);
	if($repair_non_associative_options && !$this->is_assoc($this->options))
	{
	$newoptions = array();
	foreach($this->options as $option)
	$newoptions[$option] = $option;
	$this->options = $newoptions;
	}

Because the Select value is set to "" here, the array of options is non-associative. So the code expects the keys to be set for each element in the array.

Here is the code that processes the list of options: https://github.com/strangerstudios/paid-memberships-pro/blob/dev/includes/fields.php#L1580-L1595

The work around here is to be explicit and use Yes:Yes, No:No for the option lines.

Adding an empty Select to the start of an array is a common feature. Do we want to support this format? Or address this in another way (some fields tools have a checkbox to include a "Select One" option... I like letting folks define the label themselves in the list.

Supporting this in general is pretty difficult.

First we have to recognize ":Select" for what it is, an extra empty option at the top of a list we otherwise want our keys and values to be equal. But folks say "Select" or "Choose One" or a different language. Do we just recognize when the first item in the string is set to ""? There are many reasons this may be the case. I don't think we can assume an empty-keyed first item is a special option that can be ignored.

Maybe we adjust the code that processes the list of options and if any of the options have keys set, then let's assume the intention was for the other options to use the values as keys. Is this a good assumption? I think possibly. This allows folks to use a shorthand where they only set a different key/label when needed, otherwise, we assume it's the same. If so, we should update the code in includes/fields.php to first loop through the items and look for a ":" separator. If found, then instead of the else condition there saying $options[] = $settings_option;, it should say $options[$settings_option] = $settings_option;

Side note, we should probably be trimming in the else condition there always.

Should we instead throw an error RE requiring all options have keys when only some do? Only if we think assuming "Yes" is "Yes:Yes" here is a bad assumption or if there is other trouble folks can get into by mixing and matching the formatting of their options.

[dparker1005]

To me, it does not feel like the flexibility that we are trying to give admins by making : optional is worth the amount of time we spend trying to eliminate every edge case. If we made a bold decision to require that all field options be key:label, we could enforce that requirement on user field save and "fix" user input that doesn't fit that pattern. Users can then see the correct way for the data to be formatted and understand what their user fields will look at on the frontend.

ex. User tries to save option Yes, after save if they edit the field again, they will see Yes:Yes.

Or, maybe we even enforce key:label in the UI by having two separate text fields for each option: one for key and a separate text field for label. In this case, we could automatically "guess" at option keys based on the label that the admin creates, similar to how we automatically try to guess field names from field labels that are entered.