Brand-new Drupal 7 installation, in DAMP, with a new site name (to prevent caching issues).

1. Add a new field to Basic page ("temp"). Type: List. Allowed values = 1, 2, 3, 4, 5.

2. Back on the "Basic page" setting page (Home » Administration » Structure » Content types » Basic page), click "List" link to change the field settings, but make no changes. All good.

3. Create node of Basic page type. Select "2" for the "temp" field.

4. Manage fields on the Basic page content type again. Click the List link to change field settings. Get the screen I attached here, with graphic error.

5. Remove "2" from the Allowed Values list. Save.

6. Return to the node you created. The value of that "temp" field has changed from "2" to "3".

Good luck,

--Tom

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

grendzy’s picture

hm. The help text says

The possible values this field can contain. Enter one value per line, in the format key|label. The key is the value that will be stored in the database, and must be a numeric value. The label is optional, and the key will be used as the label if no label is specified.

But in actuality the list position gets used as the key. (So if my list is 2,4,6,8 the keys are 0,1,2,3).

grendzy’s picture

Priority: Normal » Critical
FileSize
65.79 KB

More strangeness in the list module:

The "allowed values" textarea appears twice, under "Edit" and "Field settings". On the latter tab it tells you these values "cannot be changed" even though they can. (see attachment).

Even stranger, according to http://api.drupal.org/api/function/list_field_info/7 the list-position as keys behavior may have actually been intended. However the helpful description provided in hook_field_info() is never shown in the UI that I can see.

And more: list(number) stores floats. So what we really have is list(integer), list(float), and list(text), but they are labeled as list, list(number), list(text).

Even if list-position-as-keys was intended, I think it's a time bomb that should be fixed. Even experts are confused about the function of the 3 list types - so many users will simply chose the first option, "List", dash off some allowed values, and have no idea of the trouble that awaits.

I also reviewed the initial fields-in-core issue #361683, and I don't see any technical justification this list-position-as-keys. It's also a change from how integer/select CCK fields in D6 work.

Because of the myriad of inconsistencies here, and the fact that we now have Beta users out there losing data, I'm afraid this is critical.

grendzy’s picture

proposed solution:

  • If no label is given, use the key as the label like the help text says
  • Un-duplicate the allowed values list textarea and remove the "field settings" tab from list fields
  • Relabel the field types list(integer), list(float), and list(text). Make the text option first, since it's a safe choice for newbies confused by float vs int.
  • Validate the input on the allowed values list - if a user specifies non-integer keys on an integer field, the form should generate an error.

I'm not sure what's the best action to take if a key is removed. Ideas?

further reading:
#398928: Fixed vs. variable field settings
#553324: Create an actual interface for "allowed values"

Jeff Burnz’s picture

FYI the borked error messages are fixed in head.

yched’s picture

Title: Two field settings issues: one graphical, one related to changing values » Changing allowed values in "List" fields

re-scoping title + subscribe.

I thought there was some code to prevent changing allowed values once there is content for the field.

LaurentAjdnik’s picture

Tagging

jgraham’s picture

FileSize
632 bytes

The attached patch addresses the keys as values issue, as that appears straightforward and unquestionably a bug. This patch adjusts the logic to be inline with the documentation of the function assuming the inline documentation was accurate this should be safe.

Reviewing the rest of grendzy's proposed solutions in #3.

grendzy’s picture

yched: If there's an easy way to prevent removal of a key that's in use (and there might be, given EntityFieldQuery()) - that would be awesome. Preventing the allowed values list from changing at all seems unreasonable.

jgraham’s picture

Attached is a patch to disable the allowed values field when there is data. This is a naive fix but avoids clobbering data.

As grendzy says in #8, and given the comment in the header for list_field_settings_form() this can and should be smart about blocking an update.

I'm not sure what's the best action to take if a key is removed. Ideas?

This should have a validation routine as documented in the todo and fail validation if a key is removed that is in use.

header from list_field_settings_form()

* @todo: If $has_data, add a form validate function to verify that the
* new allowed values do not exclude any keys for which data already
* exists in the field storage (use EntityFieldQuery to find out).
* Implement the validate function via hook_field_update_forbid() so
* list.module does not depend on form submission.

Working on a patch described above that will replace this patch.

jgraham’s picture

Status: Active » Needs review
FileSize
2.77 KB

Updated patch to replace those in #7, #9. This now allows edits as long as keys with data remain intact.

This could be cleaned up with a fix to resolve the inline vs. tabbed edits as a couple control structures would go away. grendzy pointed this out in #3 above

Un-duplicate the allowed values list textarea and remove the "field settings" tab from list fields

Status: Needs review » Needs work

The last submitted patch, 932502_adjust_keys_allow_edits_if_no_data.patch, failed testing.

jgraham’s picture

Status: Needs work » Needs review
FileSize
2.3 KB

re-roll of 10

Status: Needs review » Needs work

The last submitted patch, 932505_12.patch, failed testing.

jgraham’s picture

Status: Needs work » Needs review
FileSize
2.99 KB

hopefully this gets it

Status: Needs review » Needs work

The last submitted patch, 932502_redo_again.patch, failed testing.

jgraham’s picture

FileSize
2.43 KB

wrapped to check for empty allowed_values before processing.

edit: This one will fail... working on another.

jgraham’s picture

FileSize
2.62 KB

this one should do it.

jgraham’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 932502_18.patch, failed testing.

jgraham’s picture

I think there needs to be more discussion around the following;

Even if list-position-as-keys was intended, I think it's a time bomb that should be fixed. Even experts are confused about the function of the 3 list types - so many users will simply chose the first option, "List", dash off some allowed values, and have no idea of the trouble that awaits.

I also reviewed the initial fields-in-core issue #361683, and I don't see any technical justification this list-position-as-keys. It's also a change from how integer/select CCK fields in D6 work.

If this is intended behavior and we keep it then I think we have to block edits entirely.

For example

  1. apple
  2. banana
  3. pear

and then change to

  1. apple
  2. pear
  3. orange

The values apple and pear remain, banana goes away and we have orange as a new value. But the key for pear changed from 2 to 1. What should happen?

  • Should we assume they knew what they were doing and allow it? Effectively all data points that said "pear" now say "orange"
  • Should we block it entirely?

I don't think either of the above are great options. Perhaps we could allow the initial input to be without keys and then subsequent edits would display as key|value. This is confusing too as it would display something other than what the user input.

Regarding the exceptions in the previous patch it looks like some bad calls to list_extract_allowed_values() in forum and taxonomy they pass in an array of allowed values rather than a string as the function documentation references http://api.drupal.org/api/function/list_extract_allowed_values/7

grendzy’s picture

FileSize
27.38 KB

From #20 you can see there's no good outcome for list-position-as-keys, which is why I think it needs to go. It's inconsistent with the other 2 list types, inconsistent with D6, contrary to user expectations, and just mayhem all around.

OK
10 | apple
20 | pear
30 | orange
OK
10
20
30
Not allowed - invalid keys
apple
pear
orange

For comparison, here's what list(number) type shows if you try to enter string keys:
each key must be a valid integer or decimal

chx’s picture

Status: Needs work » Postponed

http://drupal.org/node/553324#comment-3575244

Have a table of textfields and not a mess of a textarea. Each row has two textfields , one for key, one for the label. And then on editing we can, indeed, with EFQ can look for stuff with values and then the key would change from a textfield to just the text. On "add more" we can prefill key with the numeric values.
The current UI can only be fixed without throwing gigantic WTFs at every minute at the user by locking away the textarea or we will make the user go nuts with validate messages.

yched’s picture

chx : hm, good luck with that - that's basically bringing http://drupal.org/project/options_element in core.

Also, note that the "single textarea" input is handy for copy/pasting large lists of values (countries, languages, province codes...)

yched’s picture

side note - would be good to ping KarenS on this issue.
She did the initial "List / List (text) / List (numeric)" split and put some thought in there - more than I or anyone else did anymay.

catch’s picture

Priority: Critical » Major

This is major, not critical.

catch’s picture

Status: Postponed » Needs work
chx’s picture

If we are not providing a pretty UI in core, then open save we should add the keys omitted by the user and provide her with that on edit and if she changes an existing key then just throw a big fat validate error message on her.

webchick’s picture

Summary from IRC:

This "off-by-one" key issue has been a problem in CCK since the 4.7.x days. It bit us on the OSCMS site in 2007. So there is no way we're holding up release of Drupal 7 on this.

We are also not introducing fancy UIs at this stage. The Field UI we have is the Field UI we're going to ship with, for better or worse.

That said, this situation is definitely a UX wtf, and it can lead to awful data clobbering problems that take SQL chops to solve. No argument there.

The suggestion chx proposed is that if the user copy/pasted a country list in there, upon the next time someone edits the field, it should go from:

Canada
Mexico
United States

to

0|Canada
1|Mexico
2|United States

This would be a minor UI change, but I think doable.

He also proposes that you shouldn't be able to rename the keys from this point forward. I disagree there. I think, especially if you don't have data yet, you should be able to rename these to, say:

CA|Canada
MX|Mexico
US|United States

Same as you can rename content types, taxonomy vocabularies, etc.

If you do have data in this field, well, that's a bit more questionable. catch pointed out that we had to invent a hook_field_attach_rename_bundle() for this purpose, and we'd have to invent a similar hook here so that modules could react if the index of a referenced list item changed, and that sounds like a pain in the ass (and I know the exportables folks hate these random machine-name rename things too). OTOH, we could just document it and say "You're at your own peril if you change these when you have data in them," and trust people not to shoot themselves in the foot, as we do other places in core (to varying degrees of success in terms of foot shooting ;)). I'm rather ambivalent on this point, and could go either way.

grendzy’s picture

Priority: Major » Critical
FileSize
157.54 KB

I believe this is critical because getting the key-label relationship backwards for list(integer) fields appears to be a simple oversight, and one we'll be stuck with forever if it's not fixed now.

I don't think it's a big change, all that needs to be done is to make List, which is secretly List(integer)), consistent with List(number) which stores floats.

If it's already too late to fix, that's unfortunate but please, let's not let this slide under the misapprehension that it's always been this way. CCK in Drupal 6 does not exhibit this key/label reversal.

webchick suggested in IRC that a figure might help explain, so here goes:
field comparison

bcn’s picture

Priority: Critical » Major

chx : hm, good luck with that - that's basically bringing http://drupal.org/project/options_element in core.

And the the patch for that is at: #765338: Create an 'options' FAPI element.

bcn’s picture

Priority: Major » Critical

x-post...

grendzy’s picture

Status: Needs work » Needs review
FileSize
4.61 KB

Here's a patch that brings the List module in line with the user interface, and user expectations from D6.

One other thing I noticed, this position-as-keys business impacts List(text) too. This patch fixes that as well.

yched’s picture

Status: Needs review » Needs work

Again, I'd love to know what Karen thinks about this, but I have never been sure of the need for the "List" (automated position-as-keys) field type, compared to the other two.
It seems we could just go with "List (numeric)" and "List (text)", and have the current automated position-as-keys be just UX sugar on top of "List (numeric)", as proposed by chx :

Canada
Mexico
United States

is processed as

0|Canada
1|Mexico
2|United States

and presented that way next time the settings form is displayed.
I guess mixed entries like

Canada
0|Mexico
2|United States

need to be rejected - edge case.

and +1 for forbidding key deletion when existing data uses it.

yched’s picture

Status: Needs work » Needs review

sorry, x-post

KarenS’s picture

Sorry, late to this issue. There was some reason for including that type, but I can't remember what it was. Let me try to remember...

KarenS’s picture

Here's my best effort at remembering the history here.

Originally in CCK the only type of list was a list like:

First
Second
Third

There was no way to control the key, it was magically created and controlled, and if you re-ordered your options everything blew up without warning. That was obviously totally wrong, so we added a way to define both a key and a value. But by that time there was lots of the original stuff in the wild, so we supported an either/or option -- either define the keys explicitly or CCK would magically assign them. We also had an update to try to force the keys in, which turned out to be messy and unpredictable. Not a good solution, but it was a best attempt to handle the legacy situation.

When migrating to D7, the first thing we wanted to fix was the mess where you have to choose a 'text' or 'number' field and then turn it into a list, so we added the list type and removed 'allowed values' from text and number. That makes much more sense. Then we tried to think about how we were going to handle all the migrated data as well as do this in some logical way going forward. Whatever we do with this, we have to be sure that there will be some way to migrate D6 data in whatever form it may exist (text/number with/without keys).

I think we should no longer allow fields where you do not assign keys because it is totally non-intuitive that just re-arranging the values blows everything up. It's harder for people to understand, but safer since they are deciding (and they can see) what the key will be. I think I have always wanted to remove that option. Maybe they are both there because I didn't convince others of that. That part I don't remember.

Hmm, so now I guess I need to look at the patch to see what has been proposed.

grendzy’s picture

Just to be clear, List(numeric) stores floats. So it makes sense to me to have the third List type to store integers, although perhaps KarenS can clarify why it's not labeled as List(integer) and List(decimal).

grendzy’s picture

KarenS, thanks for the background! That helps a lot. It seems then, that this has been already been reversed more than once...

migrate D6 data in whatever form it may exist (text/number with/without keys).

In D6 all lists have explicit keys. It's the labels that are optional.

grendzy’s picture

Assigned: Unassigned » grendzy
Status: Needs review » Needs work

Discussed more with chx in IRC. We agreed #33 is the most reasonable compromise. I'll work on a new patch tonight / tomorrow.
The new patch will also hopefully incorporate jgraham's work that blocks destructive key deletion.

KarenS’s picture

For the suggestion in #33, I may be misunderstanding, but I would not let people input:

Canada
Mexico
United States

and have it processed as:

0|Canada
1|Mexico
2|United States

That's how the original code was set up and it caused no end of problems from people who didn't understand that the database was not storing the value they put in the list. If we do that, don't we also have to provide some ways to keep people from reversing items in that list without realizing what they're doing to their data. I strongly suggest that the keys be explicit and required.

grendzy’s picture

The proposal in #33 is kind of a hybrid... I'm not sure if this differs from the old (pre-D6) way. The notion is that keys are basically required, but we'd let it slide during initial field creation and auto-number them. Subsequent edit forms would display and require the keys. That reduces the risk of breakage, but I think you are right many users will be surprised at this point to discover their database didn't store "peaches".

If KarenS says make them required, I'm all for it. :-)

yched’s picture

Hm, automatically adding indexes on field creation also means we cannot really handle the case where people actually want no aliases, just

0
1
2
chx’s picture

required keys is , no doubt, the best way code wise. if the slight UX WTF is acceptable then go for it. (You no longer can copypaste a list just like without thinking into allowed values.)

KarenS’s picture

Yeah, this makes sense to people who understand how databases work, and those who don't have to get their heads around it. But it's better to guide them when they create this (the value on the left is what will actually be stored in the database, the value on the right is a user-friendly label that the user will see) so that they understand exactly what they are doing when they build the field than later when they have created lots of data that doesn't contain what they thought it would.

KarenS’s picture

Plus trying to explain the 'magical' stuff will be even harder (' ...and if you don't provide both a value for the database and a value for the label we will assume you have given us a list of *labels* and that will automatically be numbered -- and the numbering will start with zero, which is a total WTF for non-programmers all by itself -- and if you alter those values later the numbers we assign to them will change, so don't do that because your data will be trashed.') Explaining THAT is a much bigger UX problem than requiring keys is.

magnusk’s picture

If the behaviour were the same as with a taxonomy there'd be no problem, even if one removes labels, edits them, or changes their order. I think that would be a natural, default mental model of users. A key-less list would be an enumeration, completely hiding implementation details. Editing an existing enumeration would require a more elaborate interface than a textarea, so the system would know when a label is renamed, removed, or moved.

yched’s picture

re @46 : this scenario would mean each option is its own animal, saved and tracked separately - at this point, this would be no different than taxonomy terms :-).

KarenS’s picture

I think #46 would be a nice solution, if we weren't way way past code freeze. As webchick noted above, the idea now is to do the best we can with the UI we have. That could also potentially be done by a contrib module, with a hope to get this into core in D8. The current functionality for 'allowed values' was designed to be flexible so that other modules can do things with it. The current code in the D7 version of CCK injects a method of using php to create the allowed values list since that method was previously provided and needs an upgrade path but is not something that we thought we wanted in core.

magnusk’s picture

Yes, maybe it is better to not offer this problematic kind of key-less-seeming list -- instead, advise users to either use a taxonomy field or manage keys+labels explicitly.

Here is a possible scenario: upon an attempt to create a key-less list, warn the user that keys will be associated with each enumerated label, don't save but re-display the list as automatically generated key|label. (Maybe give a hint that possibly they'd rather use a taxonomy field.)

tgeller’s picture

Priority: Critical » Major

As the person who started this issue, I have to agree with KarenS and argue with the "critical" label. Yes, it's a terrible problem. But as webchick pointed out in #28:

This "off-by-one" key issue has been a problem in CCK since the 4.7.x days. It bit us on the OSCMS site in 2007. So there is no way we're holding up release of Drupal 7 on this.

Regardless of its label, I hope all of you -- particularly grendzy, whose concern and dedication are apparent -- continue to give it attention. Even if it doesn't make it into 7.0, perhaps it could be a top candidate for the 7.1 release.

webchick’s picture

Catching up on discussion here, I don't think requiring keys is an acceptable UX change at his point in the cycle. What's wrong with #33 again? Just that we need to explain better in the field description why suddenly there are numbers and pipes?

webchick’s picture

I read #45. I think that's way over-complicating the problem. The description just needs to say:

"List of values, specified in key|label format. If no key is provided, one will be auto-generated for you."

Or something like that.

sun.core’s picture

Status: Needs work » Needs review

#32: allowed_values.patch queued for re-testing.

grendzy’s picture

Assigned: grendzy » Unassigned

I may revisit this later, but I've gotten preoccupied lately with porting the securepages module. Sorry :-)

yched’s picture

Priority: Major » Critical

Last one of the 'field' issues that could use a pre-RC push.

According to webchick, approach #33 is still on the table.

MXT’s picture

Is this issue related in some way to http://drupal.org/node/954628#comment-3711274 ?
(Sorry if it isn't)

yched’s picture

@MXT : no, not related

EvanDonovan’s picture

I would definitely favor #33 if it's doable in the next 2 weeks. I don't think that most users of the Field UI will be familiar with the concept of keys, and it's a lot to explain in the help text for a select area.

I think that just have List (numeric) and List (text) types would make sense, although as an end user, I think that I would not understand why you would ever want to create a List (numeric) rather than a List (text).

In #33, if the keys are added and presented the second time around, is the textarea "locked down" from editing, or can people add more options, but now they will have to add them with the keys?

tstoeckler’s picture

FileSize
25.41 KB

I just had an idea. See screenshot attached.

Regardless of whether this gets implemented or not: Huge (!!!) kudos to sun for
'#type' => 'machine_name'
This took less than 10 minutes! (and could actually be rolled into a proper, reviewable patch in another 10).

tstoeckler’s picture

Notes on the screenshot.

This uses the 'type' => 'machine_name' of form api to turn each list item into a label -> machine_name pair of form items.
This is the established core pattern of subtly notifying users that they are doing something that they can't change later.
This would be a form of auto-filling the keys for the user, but without the destructiveness of using numeric integers. Maybe we could implement this UI for the "List (text)" field type, and hence merge the "List" and "List (text)" types.

EvanDonovan’s picture

@tstoeckler: I think that is a great idea. I haven't yet used the new '#type' => 'machine_name' property, although I think a few of Development Seed's modules have something similar that uses JavaScript to generate the machine name from the text field.

Not sure if something different would be necessary for "List (numeric)", though, but I'm not actually sure of the use case for that, unless you are creating a strictly numeric list.

grendzy’s picture

"List (numeric)" stores floats, which is what distinguishes it from "List".

tstoeckler’s picture

Well '#type' => 'machine_name' is actually quite flexible, so it would (I think, not sure though) allow to auto-create "machine names" that are integers.

Before we go into that detail, we should let some UI experts rip this apart, though.

webchick’s picture

Priority: Critical » Major

Hm. Even though the use if machine name a very clever approach, I really think that's too drastic of a UX change for this stage. :(

Also, this is not critical. It doesn't prevent someone from using Drupal. It is a pretty bad problem we should try and fix by RC1 though.

EvanDonovan’s picture

@webchick: So you would accept an approach like #33 but not #59?

yched’s picture

I don't favor #59 either. This has nothing to do with machine names, only kind of remotely similar

webchick’s picture

Yes, I said I wanted #33 like 50 replies ago. ;)

EvanDonovan’s picture

I remember that; I just wanted confirmation since it's still a UI change, but I guess not as major.

yched’s picture

Assigned: Unassigned » yched

Taking a crack at this.

yched’s picture

Side note, starting to dive in the code : long ago, we should have moved from storing allowed values in the infamous the multiline+pipe string format

$field['settings']['allowed_values'] = "0|Zero\n1|One"

to a saner

$field['settings']['allowed_values'] = array(0 => 'Zero', 1 => 'One');

Doing so now would break existing code creating list fields programmatically :-(.

yched’s picture

[server hiccup, duplicate post edited out]

yched’s picture

Here we are. Patch implements the approach in #33.

a) Removes the 'List' field type. No more "do I want List, List (numeric) or List (text) ?" head scratching.
We have 'List (numeric)' and 'List (text)'
b) Lets 'List (numeric)' field type accept a list of unkeyed strings as 'allowed values', generating numeric keys from positions, which was the intent of 'list' field type. You cannot mix explicit keys and implicit, auto-generated keys. This is only provided as a facility when creating the field (actually - when the field has no data yet).
c) Prevents keys being from removed if they are used in actual field values (both from Field UI and direct field_update_field() calls)
d) Updates the descriptions and help texts accordingly
e) Does #70 (approved by webchick on IRC) : parse the 'allowed values' string *once*, at form submit time, and store as an array, instead of currently store as a string and parse it each and every time we need to know a field's allowed values (that is, each time we display values or widgets).
Net win, much saner code, + turns out it is in fact more or less required to support b) with non-hair pulling logic : we don't have to wonder over and over how a string should be parsed and keys generated or not.
f) provides an update function to :
translate $field['settings']['allowed_values'] from strings to arrays
switch 'list' fields over to the 'list_number' field type.

yched’s picture

More details about the 'visible' effect of the patch around the 'Allowed values' input textarea :

List (number):

Help text:

The possible values this field can contain. Enter one value per line, in the format key|label.
The key is the stored value, and must be numeric. The label will be used in displayed values and edit forms.
The label is optional: if a line contains a single number, it will be used as key and label.
Lists of labels are also accepted (one label per line), only if the field does not hold any values yet. Numeric keys will be automatically generated from the positions in the list.

Sample input : (replacing linebreaks with commas just for the sake of brevity in this post)

3, 2, 1              --> array(3 => 3, 2 => 2, 1 => 1)
                         on later edits, comes out as 3|3, 2|2, 1|1
3|3, 2|2, 1|1, 0     --> array(3 => 3, 2 => 2, 1 => 1, 0 => 0)
3|foo, 2|bar, 1|baz  --> array(3 => 'foo', 2 => 'bar', 1 => 'baz')
foo, bar, baz        --> array(0 => 'foo', 1 => 'bar', 2 => 'baz') - valid at field creation time only
                         on later edits, comes out as 0|foo, 1|bar, 2|baz
foo, 2|bar, baz      --> invalid: cannot mix implicit and explicit keys, 
                         would result in array(0 => 'foo', 2 => 'baz') here, 'bar' is lost

List (text):

Help text:

The possible values this field can contain. Enter one value per line, in the format key|label.
The key is the stored value. The label will be used in displayed values and edit forms.
The label is optional: if a line contains a single string, it will be used as key and label.

Sample input:

foo, bar, baz       --> array('foo' => 'foo', 'bar' => 'bar', 'baz' => 'baz')
foo, b|bar, baz     --> array('foo' => 'foo', 'b' => 'bar', 'baz' => 'baz')

Status: Needs review » Needs work

The last submitted patch, list_field_type-932502-72.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

Tests pass fine locally.

#72: list_field_type-932502-72.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, list_field_type-932502-72.patch, failed testing.

webchick’s picture

Issue tags: +Needs tests

I'm not sure what testbot's problem with this patch is, but I tested it tonight and it seems to do what's on the tin.

I first created a "List" (no qualifier) field with a few options in it and created a node that selected one of them. Then I applied the patch and reloaded. "Kitty" turned into "e" somehow, which was a very scary moment. ;) However, after running update.php, stuff was back the way it should be. I also tried mixing/matching "0|Hello" and "Banana" and got a validation error as mentioned above. I then removed that field and created another one, this time a numerically indexed one just to confirm that that worked as well, and it did.

One nice UX improvement (other than removing the confusing and unspecific "List" type) is that List (numeric) no longer barks at you for entering a list of text values like HEAD does, implying that that list type is only for a "weight" field or similar, and not for any list at all where you want numeric keys, unless you read the fine print.

-        'allowed_values' => "1|One\n2|Two\n3|Three\n",
+        'allowed_values' => array(1 => 'One', 2 => 'Two', 3 => 'Three'),

This is a breath of fresh air, honestly, and worth breaking the handful of programmatically generated list fields out there, IMO.

Now, of course the big elephant in the room is we need tests for this. Lots and lots and lots of tests. However, because this fixes so many UX WTFs in one fell swoop, I'm willing to make an exception on the test requirement if I can get some reassurance that someone will pick this up immediately after this get committed.

yched’s picture

I welcome any hint on how to debug a 'Fatal error' (without any file name or line number) reported by a testbot while the tests pass fine on your local setup :-). Last time this happened to me, this was because I'm running PHP 5.2 and the bots seem to be running 5.3.
I have no 5.2 setup, so maybe if someone with a 5.3 can try to run the tests ?
Meanwhile, I'm using #983552: helper issue for #932502 to do in-the-dark debug throwing patch variants at the bot. Slow...

[edit: ok, found - new patch coming up]

re @webchick : tests
I have quite some post RC1 patches waiting already, and would be glad to get help with writing tests for this one. Naturally, I will do it if no-one steps up :-p

yched’s picture

Status: Needs work » Needs review
FileSize
29.86 KB

Fixes tests (tests for options.module widgets are based on list fields and needed to be updated accordingly).

Status: Needs review » Needs work
Issue tags: -Needs tests

The last submitted patch, list_field_type-932502-79.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

Big, heavy sigh. What's your problem with me today, bot ?
This very same patch file just passed in #983552-4: helper issue for #932502.

#79: list_field_type-932502-79.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, list_field_type-932502-79.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests

Can I haz anozer bot than client #32 pliz ?

#79: list_field_type-932502-79.patch queued for re-testing.

webchick’s picture

Status: Needs review » Fixed

Ok, testbot likes this patch again. I thoroughly tested it before, and it fixes a really nasty problem, coupled with some under-the-hood sanity improvements for allowed values, so I think this should go in.

Committed to HEAD. Thanks!

webchick’s picture

Priority: Major » Normal
Status: Fixed » Needs work

Oops. Not fixed. We still need tests.

sun’s picture

+++ modules/field/modules/list/list.module
@@ -142,23 +151,37 @@ function list_field_settings_form($field, $instance, $has_data) {
+    // Check that keys are valid for the field type.
+    foreach ($values as $key => $value) {
+      if ($field['type'] == 'list_number' && !is_numeric($key)) {
+        form_error($element, t('Allowed values list: each key must be a valid integer or decimal.'));
+        break;
+      }

It looks like this code also intends to handle and process decimal values, but the rest of the patch looks like the internal values are processed into array keys.

PHP only supports integers in array keys. Floats/decimals are not supported unless they are strings.

Let's add a test for decimals to ensure that this works like it should.

Powered by Dreditor.

grendzy’s picture

Priority: Normal » Major

The last commit here removed the option of storing integers, which seems like a pretty serious regression. Previously developers could choose between integer, floats, and varchar (even though the labels were confusing, all 3 options existed). The last patch on that issue removed 1 of the 3 options. If create a list(numeric) field, and look at field_data_field_FIELDNAME, you'll see it's a float.

Also, when I try to add an item to the list of allowed values, I get this error:

Notice: Undefined variable: prior_field in _list_values_in_use() (line 339 of /Users/dylan/Sites/7-drupal/drupal/modules/field/modules/list/list.module).
Notice: Undefined variable: lost_keys in _list_values_in_use() (line 339 of /Users/dylan/Sites/7-drupal/drupal/modules/field/modules/list/list.module).
EntityFieldQueryException: Field storage engine not found. in EntityFieldQuery->queryCallback() (line 1048 of /Users/dylan/Sites/7-drupal/drupal/includes/entity.inc).
webchick’s picture

Priority: Major » Critical

Dang it. :\

webchick’s picture

Priority: Critical » Major

Actually, no. We're not going to block release on code that didn't exist in HEAD 8 hours ago.

Rolled back #79.

Normally, this would also mean moving it to D8. But this really is a nasty, nasty bug. So I'm going to leave it at 7.x for now, because I feel that the potential data loss trade-off is worth breaking strings for.

However, this thing isn't getting back to RTBC without tests. Burn me once, shame on me. Burn me twice... ;)

yched’s picture

If we want to keep the ability to store ints, it means either
splitting 'List (numeric)' into 'List (integer)' and 'List (float [or decimal ?])
or introducing a new 'key_type' = 'int|float' field setting.

I'm not completely sure we really need an explicit option to store ints, though.

Will try to look at the other issues tonight.

andypost’s picture

splitting 'List (numeric)' into 'List (integer)' and 'List (float [or decimal ?])
or introducing a new 'key_type' = 'int|float' field setting.

Maybe third is merge all List(wtf) into one List and provide field setting for key type (int|float|varchar)?

I think having List(wtf) is a most confusing UX trouble

vosechu’s picture

If it helps justify working on this longer we _are_ supposed to be finding bugs during RC. We would like there to not be bugs, but I don't think we even need to justify the decision to not push this to D8. Yay first RC bug!

yched’s picture

re @sun #86 : the patch works fine with floats. The code that builds the array from the pipe/linebreak format parses numeric values as strings anyway - even for int values, for that matter.

Updated patch fixes the notices mentioned in #87.

I'll add tests once we settle on the 'store as int' question.

yched’s picture

So, about the 'store as int' topic raised in #87:

The primary intent of HEAD's current 'List' field type, which we're trying to drop, was for lists "where the stored value is meaningless" - i.e, just an index. The intent was not "use this field type if you want to store ints", it was "use this field type if you don't care about what is actually stored". It happened to store ints, but that was not really a feature and was not even explicit unless you looked up list_field_schema().

For lists where the stored value was meaningful (and numeric), e.g. "1|1 day, 7|1 week, 31|1 month", you had 'List (number)', and it stored floats no matter what. Never seemed to bug anyone so far.

So, 1st remark is that the patch doesn't really put us in a worse situation than current HEAD.
2nd remark is : is it really that bad if we store floats for numeric lists ? what's the real life impact ?

sun’s picture

IMHO, it would be most logical to have only one field type "List" along with a field setting that denotes the 'value' data storage format type.

Contrary to other "extracted" field types of the same field type (e.g., text fields), the amount of field columns and settings and things one naturally associates with the separated sub field types (e.g., textarea/textfield, summary, etc) is not different for each list field type.

Each time I had to create a list field thus far in D7, I had to bootstrap my throw-away development site in order to create the three list field types to see what the actual difference is, only to find out once more that there's not really a difference, and feeling stupid afterwards. ;)

andypost’s picture

@sun +1, Exactly what I'm talking about

yched’s picture

This doesn't really answer my question in #94 :
is it really that bad if we store floats for numeric lists ? what's the real life impact ?

fago’s picture

subscribing

webchick’s picture

This issue smells to me like we're trying to solve any and all problems with list items. This isn't the goal. D7 is UX frozen AND in RC now, so we're trying to make the minimal impact required to fix the actual bug.

I think what introduced the problems that forced a rollback of this patch in the first place was trying to do extra work to remove the plain "List" item to improve the UX. Are these three choices confusing? Certainly. But fixing that has nothing to do with auto-generating keys to prevent data corruption.

I'd like to see this patch re-focus to be just that (and, since it's easier to do that with the array form rather than the janky pipe form of the values, that change as well).

yched’s picture

#100, yay :-/

OK, agreed that keeping three separate field types is our safest option at this point :
List (integer) - former 'List'
List (float) - former 'List (number)'
List (text) - unchanged

The machine names need to change accordingly, though.

Updated patch. I'll start adding tests once this passes.

yched’s picture

Status: Needs work » Needs review
yched’s picture

beloved bot, can I haz test ?

yched’s picture

There are active bot clients and some patches get tested, while this one endlessly waits - WTF ?

Status: Needs review » Needs work

The last submitted patch, list_field_type-932502-100.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
42.37 KB

Same patch, with extensive tests.

Should be ready to review and (re-) commit ...

Status: Needs review » Needs work

The last submitted patch, list_field_type-932502-105.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
42.54 KB

did not properly update the tests for boolean (the one field type this patch doesn't touch...)

sun’s picture

Status: Needs review » Needs work

1) At least on my site, the schema version of List module is 7000 already. Had to rename the update to 7001 to execute it.

2) Even after running the update, the site WSODs with:

Fatal error: Unsupported operand types in modules\user\user.module on line 185

Call Stack:
    3.3695   21538512  18. user_field_info_alter()  called from includes\module.inc:1002
yched’s picture

1) #79 got in and was rolled back a couple hours later, just before RC1 was rolled.
if you tracked HEAD and ran the update within this time frame (or if you applied and tested #79 and kept the same db after this was rolled back), you'll miss the update.
Plus, list_update_7000() from patch #107 will fail if ran on a db where the one from patch #79 was played, so a simple func rename won't do the trick if we say we need to support this edge case.

2) is most probably caused by entity_api module :
see #986046: Check for modules to prevent altering non-existent field info arrays and my comment #19

sun’s picture

Status: Needs work » Needs review

Both counterarguments sound valid, so reverting status. Will double-check tomorrow. Perhaps someone else can also test in the meantime?

yched’s picture

Updated patch :
- renames the update to 7001
- makes sure the update runs fine on installs where the short-lived update 7000 from #79 has already played

Tested from a test HEAD setup with various list fields testing various 'allowed values' edge cases
a) applied #79, ran update 7000, applied #109, ran update 7001
b) applied #109 directly, ran update 7001
--> a) and b) lead to the same state.

yched’s picture

We should probably leave an empty placeholder update function 7000 to avoid later head scratching.
I thought we had a couple of those in HEAD already, but they must have been cleaned up before we went aplha, I can't find any.

So, not sure what PHPdoc we should use for this placeholder update (shown in the 'list of updates' UI)

yched’s picture

Issue tags: -Needs tests

#111: list_field_type-932502-110.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, list_field_type-932502-110.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests

passes at home...

#111: list_field_type-932502-110.patch queued for re-testing.

yched’s picture

bump - the patch should really be ready, and we only have a couple days to get this in...

yched’s picture

desperate bump...

seanr’s picture

I'd really like this too - ran into this on a site i'm working on and you should have heard all the cursing it generated! ;-)

webchick’s picture

How about instead of cursing, you provide a code review so this can get in?

yched’s picture

[crosspost with #119]

I bet. And this will cause much cursing if D7 ships as is...

The case of List types is a bit sad. They stayed unnoticed and mostly unattended for 1.5 year, because they look much simpler than they really are and were taken for granted, and because no one had both the time and confidence to care for them.
Now that we have a patch to push them out of the WTF zone, it's hard to find someone with both the time and confidence to review it :-) Not an unknown pattern in core, I guess. Only this time, this is part of one of the front page features for the d7 cycle, and the WTF won't go unnoticed.

This *almost* got in before RC. Webchick wants this enough to explicitly grant an exception for string freeze.
I'm 95% sure the patch is *ready*. Let's not let this sink now :-)

yched’s picture

Issue tags: -Needs tests +DrupalWTF, +DIE

tag, if that's of any use :-)

bleen’s picture

I definitly fall into the category of "not qualified to thoroughly review this" (sorry yched) but I did notice two things:

+++ modules/field/modules/list/list.installundefined
@@ -43,4 +44,75 @@ function list_field_schema($field) {
\ No newline at end of file

no new line...

+++ modules/field/modules/list/list.moduleundefined
@@ -213,45 +246,128 @@ function list_allowed_values($field) {
+  $generated_keys = $explicit_keys = FALSE;

I dont think we ever set two variables in a chain like this

Powered by Dreditor.

crashtest_’s picture

Ok, having read this issue, I proceeded to do the initial test on a clean drupal-cvs instance.

My results were exactly as described above.

Next I installed the patch from #111 and updated. This went perfectly, running update 7001.

Next I opened my original content type and observed that my "List" type had been transformed into a "List (integer)". The list items had been "1, 3, 4, 5" and were now "0|1, 1|3, 2|4, 3|5" as I expected.

What was unexpected was that I was still observing the error message "There is data for this field in the database. The field settings can no longer be changed." This was simply incorrect, and this message should be removed.

I edited that content by adding "9" to the end, and was rewarded with "0|1, 1|3, 2|4, 3|5, 9|9". After that I was able to change the entire list to "0|0, 1|1, 2|2, 3|3, 4|4, 9|9, 10|10". Note that I did change the value of key "1" to have a label of "1", it allowed this change, and allowed me to save this, still with the above error displayed, however trying to save it without the "1" key resulted in validation error "Allowed values list: some values are being removed while currently in use."

After this I created a new "List (integer)" field, and toyed with trying to force validation, which reacted correctly, for example entering "1, 2|foo, bar, 5, 6|baz" prompted a validation error "Allowed values list: invalid input.", but removing the "bar" allowed the values to be saved.

I also replicated keys that existed in the list already at the end of the list, with the result being that the last key in the list that shared a key name with previous keys was the winner, having it's key|label stored, be it a complete key|label pair or just a single key. When new values were entered in between or after duplicate keys, such as "1|foo, 2|bar, 3|baz, 2|zog, 9, 7|spaz" the result would be "1|foo, 2|zog, 3|baz, 9|9, 7|spaz", with the order of new items retained, and the order of original key items retained, but new labels applied.

I tried the same testing hooliganary on "List (float)" (with the exception that it allowed me to use notation "2e5" as a key, NICE!) and "List (text)" with the same correct results.

I can think of no other tests at this time. The one thing I think needs to be addressed is the error message "There is data for this field in the database. The field settings can no longer be changed."

If that were changed, then this gets a hardy RTBC++!! from me.

Pat

yched’s picture

Thanks a lot for the extensive test, Pat.
The misleading "can no longer be changed" message is a more general issue and will be tackled in #552604: Adding new fields leads to a confusing "Field settings" form.

grendzy’s picture

Status: Needs review » Reviewed & tested by the community

Wow, this patch is truly outstanding. A very bad situation has been made - dare I say it - nearly foolproof.

I testest all 3 list types, added and removed list options, tested invalid list options, and tested the attempted removal of a list option that was in use. I reviewed the patch in FileMerge and the diffs all seem reasonable. The are a several new tests that cover unkeyed inputs, integer keys, adding / removing keys, mixed inputs, and removing in-use keys.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

amitaibu’s picture

OG was bitten by this change in #1003516: Because of list.module change, cannot create group content types.. Maybe its worth an API change announcement.

Dave Reid’s picture

Got this warning when running the list_update_7001():
Warning: array_combine(): Both parameters should have at least 1 element in list_update_7001() (line 66 of /modules/list/list.install).

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

effulgentsia’s picture

Status: Closed (fixed) » Fixed

Moving back to fixed to keep this open another 2 weeks in case followup for #127 or #128 is still needed.

Re #128: To reproduce this, install a version of core before #111/#126 went in (e.g., RC3), then add a "List (numeric)" field to a content type, and set allowed values to an empty string. Then update to 7.0. What ends up happening is within list_update_7001():

if ($field['type'] == 'list_number') {
  $keys = array_map(create_function('$a', 'return (string) (float) $a;'), array_keys($allowed_values));
  $allowed_values = array_combine($keys, array_values($allowed_values));
}

The above array_combine() gets called with empty arrays for the arguments, which:

  1. Triggers the warning in #128.
  2. Returns FALSE rather than an empty array.

This doesn't appear, however, to cause any problems down the road. If you then edit content with the field, it appears to work, and if you go to the field configuration page and save it, the FALSE then gets converted to an empty array.

A use case for having an empty allowed_values is if allowed_values_function is used instead, which is the case within Media Gallery.

Since other than some warnings in the log for people updating from pre-release installations there aren't any problems, I'm not sure it's worth pursuing this any further, though a patch to fix it would probably be easy to write.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

yched’s picture