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
Comment | File | Size | Author |
---|---|---|---|
#111 | list_field_type-932502-110.patch | 42.72 KB | yched |
#107 | list_field_type-932502-107.patch | 42.54 KB | yched |
#105 | list_field_type-932502-105.patch | 42.37 KB | yched |
#103 | list_field_type-932502-100.patch | 32.09 KB | yched |
#102 | list_field_type-932502-100.patch | 32.09 KB | yched |
Comments
Comment #1
grendzy CreditAttribution: grendzy commentedhm. The help text says
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).
Comment #2
grendzy CreditAttribution: grendzy commentedMore 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.
Comment #3
grendzy CreditAttribution: grendzy commentedproposed solution:
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"
Comment #4
Jeff Burnz CreditAttribution: Jeff Burnz commentedFYI the borked error messages are fixed in head.
Comment #5
yched CreditAttribution: yched commentedre-scoping title + subscribe.
I thought there was some code to prevent changing allowed values once there is content for the field.
Comment #6
LaurentAjdnik CreditAttribution: LaurentAjdnik commentedTagging
Comment #7
jgraham CreditAttribution: jgraham commentedThe 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.
Comment #8
grendzy CreditAttribution: grendzy commentedyched: 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.
Comment #9
jgraham CreditAttribution: jgraham commentedAttached 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.
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()
Working on a patch described above that will replace this patch.
Comment #10
jgraham CreditAttribution: jgraham commentedUpdated 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
Comment #12
jgraham CreditAttribution: jgraham commentedre-roll of 10
Comment #14
jgraham CreditAttribution: jgraham commentedhopefully this gets it
Comment #16
jgraham CreditAttribution: jgraham commentedwrapped to check for empty allowed_values before processing.
edit: This one will fail... working on another.
Comment #17
jgraham CreditAttribution: jgraham commentedthis one should do it.
Comment #18
jgraham CreditAttribution: jgraham commentedComment #20
jgraham CreditAttribution: jgraham commentedI think there needs to be more discussion around the following;
If this is intended behavior and we keep it then I think we have to block edits entirely.
For example
and then change to
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?
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
Comment #21
grendzy CreditAttribution: grendzy commentedFrom #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.
10 | apple
20 | pear
30 | orange
10
20
30
apple
pear
orange
For comparison, here's what list(number) type shows if you try to enter string keys:
Comment #22
chx CreditAttribution: chx commentedhttp://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.
Comment #23
yched CreditAttribution: yched commentedchx : 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...)
Comment #24
yched CreditAttribution: yched commentedside 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.
Comment #25
catchThis is major, not critical.
Comment #26
catchComment #27
chx CreditAttribution: chx commentedIf 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.
Comment #28
webchickSummary 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:
to
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:
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.
Comment #29
grendzy CreditAttribution: grendzy commentedI 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:
Comment #30
bcn CreditAttribution: bcn commentedAnd the the patch for that is at: #765338: Create an 'options' FAPI element.
Comment #31
bcn CreditAttribution: bcn commentedx-post...
Comment #32
grendzy CreditAttribution: grendzy commentedHere'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.
Comment #33
yched CreditAttribution: yched commentedAgain, 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 :
is processed as
and presented that way next time the settings form is displayed.
I guess mixed entries like
need to be rejected - edge case.
and +1 for forbidding key deletion when existing data uses it.
Comment #34
yched CreditAttribution: yched commentedsorry, x-post
Comment #35
KarenS CreditAttribution: KarenS commentedSorry, 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...
Comment #36
KarenS CreditAttribution: KarenS commentedHere'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.
Comment #37
grendzy CreditAttribution: grendzy commentedJust 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).
Comment #38
grendzy CreditAttribution: grendzy commentedKarenS, thanks for the background! That helps a lot. It seems then, that this has been already been reversed more than once...
In D6 all lists have explicit keys. It's the labels that are optional.
Comment #39
grendzy CreditAttribution: grendzy commentedDiscussed 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.
Comment #40
KarenS CreditAttribution: KarenS commentedFor the suggestion in #33, I may be misunderstanding, but I would not let people input:
and have it processed as:
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.
Comment #41
grendzy CreditAttribution: grendzy commentedThe 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. :-)
Comment #42
yched CreditAttribution: yched commentedHm, automatically adding indexes on field creation also means we cannot really handle the case where people actually want no aliases, just
Comment #43
chx CreditAttribution: chx commentedrequired 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.)
Comment #44
KarenS CreditAttribution: KarenS commentedYeah, 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.
Comment #45
KarenS CreditAttribution: KarenS commentedPlus 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.
Comment #46
magnusk CreditAttribution: magnusk commentedIf 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.
Comment #47
yched CreditAttribution: yched commentedre @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 :-).
Comment #48
KarenS CreditAttribution: KarenS commentedI 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.
Comment #49
magnusk CreditAttribution: magnusk commentedYes, 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.)
Comment #50
tgeller CreditAttribution: tgeller commentedAs 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:
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.
Comment #51
webchickCatching 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?
Comment #52
webchickI 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.
Comment #53
sun.core CreditAttribution: sun.core commented#32: allowed_values.patch queued for re-testing.
Comment #54
grendzy CreditAttribution: grendzy commentedI may revisit this later, but I've gotten preoccupied lately with porting the securepages module. Sorry :-)
Comment #55
yched CreditAttribution: yched commentedLast one of the 'field' issues that could use a pre-RC push.
According to webchick, approach #33 is still on the table.
Comment #56
MXTIs this issue related in some way to http://drupal.org/node/954628#comment-3711274 ?
(Sorry if it isn't)
Comment #57
yched CreditAttribution: yched commented@MXT : no, not related
Comment #58
EvanDonovan CreditAttribution: EvanDonovan commentedI 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?
Comment #59
tstoecklerI 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).
Comment #60
tstoecklerNotes 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.
Comment #61
EvanDonovan CreditAttribution: EvanDonovan commented@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.
Comment #62
grendzy CreditAttribution: grendzy commented"List (numeric)" stores floats, which is what distinguishes it from "List".
Comment #63
tstoecklerWell '#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.
Comment #64
webchickHm. 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.
Comment #65
EvanDonovan CreditAttribution: EvanDonovan commented@webchick: So you would accept an approach like #33 but not #59?
Comment #66
yched CreditAttribution: yched commentedI don't favor #59 either. This has nothing to do with machine names, only kind of remotely similar
Comment #67
webchickYes, I said I wanted #33 like 50 replies ago. ;)
Comment #68
EvanDonovan CreditAttribution: EvanDonovan commentedI remember that; I just wanted confirmation since it's still a UI change, but I guess not as major.
Comment #69
yched CreditAttribution: yched commentedTaking a crack at this.
Comment #70
yched CreditAttribution: yched commentedSide 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
to a saner
Doing so now would break existing code creating list fields programmatically :-(.
Comment #71
yched CreditAttribution: yched commented[server hiccup, duplicate post edited out]
Comment #72
yched CreditAttribution: yched commentedHere 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.
Comment #73
yched CreditAttribution: yched commentedMore details about the 'visible' effect of the patch around the 'Allowed values' input textarea :
List (number):
Help text:
Sample input : (replacing linebreaks with commas just for the sake of brevity in this post)
List (text):
Help text:
Sample input:
Comment #75
yched CreditAttribution: yched commentedTests pass fine locally.
#72: list_field_type-932502-72.patch queued for re-testing.
Comment #77
webchickI'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.
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.
Comment #78
yched CreditAttribution: yched commentedI 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
Comment #79
yched CreditAttribution: yched commentedFixes tests (tests for options.module widgets are based on list fields and needed to be updated accordingly).
Comment #81
yched CreditAttribution: yched commentedBig, 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.
Comment #83
yched CreditAttribution: yched commentedCan I haz anozer bot than client #32 pliz ?
#79: list_field_type-932502-79.patch queued for re-testing.
Comment #84
webchickOk, 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!
Comment #85
webchickOops. Not fixed. We still need tests.
Comment #86
sunIt 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.
Comment #87
grendzy CreditAttribution: grendzy commentedThe 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:
Comment #88
webchickDang it. :\
Comment #89
webchickActually, 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... ;)
Comment #90
yched CreditAttribution: yched commentedIf 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.
Comment #91
andypostMaybe 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
Comment #92
vosechu CreditAttribution: vosechu commentedIf 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!
Comment #93
yched CreditAttribution: yched commentedre @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.
Comment #94
yched CreditAttribution: yched commentedSo, 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 ?
Comment #95
sunIMHO, 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. ;)
Comment #96
andypost@sun +1, Exactly what I'm talking about
Comment #97
yched CreditAttribution: yched commentedThis 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 ?
Comment #98
fagosubscribing
Comment #99
webchickThis 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).
Comment #100
yched CreditAttribution: yched commented#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.
Comment #101
yched CreditAttribution: yched commentedComment #102
yched CreditAttribution: yched commentedbeloved bot, can I haz test ?
Comment #103
yched CreditAttribution: yched commentedThere are active bot clients and some patches get tested, while this one endlessly waits - WTF ?
Comment #105
yched CreditAttribution: yched commentedSame patch, with extensive tests.
Should be ready to review and (re-) commit ...
Comment #107
yched CreditAttribution: yched commenteddid not properly update the tests for boolean (the one field type this patch doesn't touch...)
Comment #108
sun1) 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:
Comment #109
yched CreditAttribution: yched commented1) #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
Comment #110
sunBoth counterarguments sound valid, so reverting status. Will double-check tomorrow. Perhaps someone else can also test in the meantime?
Comment #111
yched CreditAttribution: yched commentedUpdated 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.
Comment #112
yched CreditAttribution: yched commentedWe 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)
Comment #113
yched CreditAttribution: yched commented#111: list_field_type-932502-110.patch queued for re-testing.
Comment #115
yched CreditAttribution: yched commentedpasses at home...
#111: list_field_type-932502-110.patch queued for re-testing.
Comment #116
yched CreditAttribution: yched commentedbump - the patch should really be ready, and we only have a couple days to get this in...
Comment #117
yched CreditAttribution: yched commenteddesperate bump...
Comment #118
seanrI'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! ;-)
Comment #119
webchickHow about instead of cursing, you provide a code review so this can get in?
Comment #120
yched CreditAttribution: yched commented[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 :-)
Comment #121
yched CreditAttribution: yched commentedtag, if that's of any use :-)
Comment #122
bleen CreditAttribution: bleen commentedI definitly fall into the category of "not qualified to thoroughly review this" (sorry yched) but I did notice two things:
no new line...
I dont think we ever set two variables in a chain like this
Powered by Dreditor.
Comment #123
crashtest_ CreditAttribution: crashtest_ commentedOk, 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
Comment #124
yched CreditAttribution: yched commentedThanks 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.
Comment #125
grendzy CreditAttribution: grendzy commentedWow, 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.
Comment #126
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #127
amitaibuOG was bitten by this change in #1003516: Because of list.module change, cannot create group content types.. Maybe its worth an API change announcement.
Comment #128
Dave ReidGot 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).
Comment #130
effulgentsia CreditAttribution: effulgentsia commentedMoving 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():
The above array_combine() gets called with empty arrays for the arguments, which:
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.
Comment #132
yched CreditAttribution: yched commentedre #130: Opened #1059184: warnings in list_update_7001 (edge cases)