Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Current no field settings can be changed but we've already identified use cases (like allowed values) where they must be changeable. We need to create a method to differentiate between values that can and can't be changed so CCK knows what to allow.
Comment | File | Size | Author |
---|---|---|---|
#30 | field-update-field-367013-30.patch | 9.79 KB | bjaspan |
#25 | field-update-field-367013-25.patch | 38.42 KB | bjaspan |
#22 | field-update-field-367013-21.patch | 35.33 KB | bjaspan |
#19 | field-update-field-367013-19.patch | 34.95 KB | bjaspan |
#17 | field-update-field-367013-17.patch | 30.48 KB | bjaspan |
Comments
Comment #1
bjaspan CreditAttribution: bjaspan commentedThere is a lot of background discussion of this issue at http://groups.drupal.org/node/17758.
I am still thinking that we do not need a separate "scheme settings" array.
Right now, cck_field_settings_form_submit() assumes that $form_state['values']['field'] contains a complete field structure that can be passed to field_update_field() (when that function exists). This is not an assumption we should be making. First of all, the field structure is only an array temporarily; we are probably going to switch it to an object representation at some point. This means we'll have to construct the object structure piece by piece out of the field data, which we should be doing anyway (see http://groups.drupal.org/node/18019 where we discuss converting form data to object properties).
Given that the $field structure should be assembled manually from the form data, it follows that cck_field_settings_form_submit() cannot assemble the per-field-type settings from the form structure itself because it does not know what the per-field-type settings are. Thus, it has to call something like hook_field_settings_form_submit($form, $form_state, &$field), which corresponds to the hook_field_settings_form() that built the per-field-type settings form in the first place and which fills in $field based on what it finds in the form. This also frees the form not to have to correspond directly to the settings layout.
Well, cck_field_settings_form() can pass $has_data to hook_field_settings_form(), which will then know not to return any form elements that hook_field_settings_form_submit() will be unable to handle if $has_data is true.
Anyway, that's what I'm thinking. I'm sure Karen will set me straight. :-)
Comment #2
KarenS CreditAttribution: KarenS commentedCCK has to present the non-updatable form elements as well as the updatable ones -- we need to present them when the field is first created and I continue to present them until data is created to make it easier to undo simple mistakes or changing your mind before actually building data. After data is created I no longer want to allow those particular items to be altered.
So I need a method where I can get both updatable and nonupdatable form elements and know the difference between them.
Maybe what you're describing would work, but I'm not really tracking you. Exactly what changes would you make to hook_field_settings_form()?
Comment #3
bjaspan CreditAttribution: bjaspan commentedComment #4
bjaspan CreditAttribution: bjaspan commentedComment #5
bjaspan CreditAttribution: bjaspan commentedBump. Maybe the thinking is that this will be easier to do once #516138: Move CCK HEAD in core is one, but I really think we to resolve how field_update_field() will work for the non-schema-changing parts at least before D7.
Comment #6
yched CreditAttribution: yched commentedWell, my thinking was "we need to sort this out at some point (which now means 'pretty damn soon'), but right now I don't really know how" :-/
It's not only 'non-schema-changing' bits that are critical. At the sprint, we agreed that forbidding a change of, say, varchar length or *TEXT type was deemed OK, but not being able to change the set of indexed columns once the field is created is kind of embarrassing. It's hard to know in advance the exact set of Views you'll build on a field, for instance...
Comment #7
KarenS CreditAttribution: KarenS commentedSee #567042: Field settings are never applied to schema. One of the problems created in the UI is that we have to create a field before we ask for the settings for a decimal position and scale attributes. The field gets created with the default values and we cannot change them by the time we get to the screen where we ask the user to select their values. This is a schema change that should be allowed, so it's time to figure out how to allow changes while they are still 'safe', like while the field is being configured.
Comment #8
mfbsubscribing
Comment #9
KarenS CreditAttribution: KarenS commentedAfter discussing with bjaspan and yched, lambic and I are working on a patch for this. It will probably not get finished today tho.
We are creating a method where the API invokes a hook to check to see if the field module will allow the proposed change or not, and a helper function to see if there is data for this field or not, and go ahead and make it if it is not destructive or there is no data.
Comment #10
bjaspan CreditAttribution: bjaspan commentedWhat is the status of this?
Comment #11
KarenS CreditAttribution: KarenS commentedAh, I got slammed after getting back from Paris and will be buried through this weekend. I was hoping to get back to this next week. I am posting what I have so far, which is broken and not ready. I picked up some conflicts from cvs changes since I started on this that need to be cleaned up too, but am uploading the file with the conflicts because I don't have time to clean them up right now.
The process for signaling whether a change should be allowed or not needs more thought, and once that is figured out we also need to edit field_ui to show the unchangable data on the first field settings screen and changable data on the second screen, along with other changable instance and widget data. Currently you see all field settings on both screens, which is not right, it was waiting for a way for the UI to know which settings were which.
Comment #12
bjaspan CreditAttribution: bjaspan commentedThis is a good approach, but already it reveals serious complexities. You are calling Schema API functions from field.crud.inc. That's no good, because we can't assume the field storage is in SQL; only the field storage module knows that. So actually it may not really be up to the field type module to decide if an update is possible, or only up to the field type module, because the field storage module needs a say too. And in general a field type module will have no API to do the equivalent for Schema API for non-local-SQL storage, so it will just have to expose its columns/indexes information and hope the storage module can figure out how to change from prior to current schema (as you've current written for SQL in crud.inc).
This is a major TODO before D7, I hope to find time to work on it.
Comment #13
bjaspan CreditAttribution: bjaspan commentedI'm working on this this weekend and have a new patch in the works.
Comment #14
bjaspan CreditAttribution: bjaspan commentedNew patch attached. This patch is inspired by Karen's approach but is somewhat different.
First, I'll define two forms of "field update":
* Weak Field Update: The storage columns for a field with data can never be changed. Indexes can be added or removed.
* Strong Field Update: The storage columns for a field with data cannot be changed unless doing so is guaranteed not to lose any data. Indexes can be added or removed.
Karen's approach attempts to implement Strong Field Update but isn't done. My approach implements Weak Field Update for now, works, and the remaining TODOs are just details and cleanup. I believe my approach is also capable of implementing Strong Field Update if we want to go to the effort of teaching Field SQL Storage module how, but I think we should commit Weak Field Update before embarking on that journey.
The idea behind my patch is that a field type module only cares that the Field API gives it and accepts data in the format the field type defines. The field storage engine is responsible for storing it and deciding whether the storage layout can be changed safely from an old form to a new form. When a field is about to be updated, field_update_field() asks all modules whether they want to FORBID the update (note that Karen's patch asks if any module wants to allow it). There are a variety of reasons a module might forbid an update:
1. A field storage module can forbid it if it cannot change the storage to the new layout. field_sql_storage currently forbids all changes if data exists (weak field update). If changing to strong field update is possible, there is where we would do it.
2. A field type module can forbid it if it knows the semantics of stored data is changing in some way that it will not be able to interpret later. For example, what should a List field do when it is updated so that the allowed values no longer include keys that it used to include? When the field reads in such data, it will not know what to do with it. So it forbids the update if data exists. (It could instead allow the update and handle the problem in hook_field_update_field(), but that leads to a bulk update problem. Or it could just decide that it is okay for it to get old key data that is no longer in the allowed values, in which case it does not need to do anything.)
If no module forbids the update, field_update_field() tells the field storage engine to perform the update, then invokes hook_field_update_field() to tell everyone the field changed.
TODOs:
* Implement hook_field_update_forbid() for all field types that need one. We have to decide on a strategy. For example, should text.module refuse updates that make the text length shorter, losing data? (While right now field_sql_storage will refuse such an update if there is any data, it might not in the future.) I think field types should NOT do that. If someone calls field_update_field() in a way that will reduce data fidelity, I think the API has to assume they meant it. However, in the List case above, I think it is reasonable for List to refuse updates if the set of keys changes, unless it can safely delete all the about-to-be-lost keys. However, we could go the other way and have field types refuse any data-losing change. Not sure what is best.
* Add tests for adding/removing indexes. I wrote code to implement this but have not tried it at all yet.
* Update Field UI to use this function.
* Teach Field UI how to decide what form elements to show on which pages. I actually think this is a separate issue from the field_update_field() API itself.
Setting to CNR for testbot and for a review of the approach.
Comment #15
bjaspan CreditAttribution: bjaspan commentedforgot to change status
Comment #16
bjaspan CreditAttribution: bjaspan commentedSmall update: Change field_ui.module to use field_update_field() instead of field_ui_update_field(). This fixes #567042: Field settings are never applied to schema.
Comment #17
bjaspan CreditAttribution: bjaspan commentedNew patch: Add $has_data argument to hook_field_settings_form() so field types can decide which settings to allow to be changed based on that. There is more we can do in this department, but I think this is enough for the initial field_update_field() commit.
I think there is a bug somewhere b/c I saw a bunch of drupal_set_message() errors after creating a node with a field type that had been updated. It looked like perhaps $field['settings'] was lost, except it is still there in the db. Leaving CNR anyway so someone will look at the whole patch.
Comment #18
bjaspan CreditAttribution: bjaspan commentedThe bug mentioned in #17 is #582754: Undefined index errors viewing nodes with fields, unrelated to field_update_field.
Comment #19
bjaspan CreditAttribution: bjaspan commentedNew patch, adds tests for adding/removing indexes, and fixes that code to actually work. All the todos listed in #14 are now at least done to the first approximation.
Comment #20
seutje CreditAttribution: seutje commentedsubscribe
Comment #21
yched CreditAttribution: yched commentedGreat ! No time for a real review right now, but +1 on the approach.
Comment #22
bjaspan CreditAttribution: bjaspan commentedNew patch, adds hook_field_update_field() to field.api.php and fixes the PHPdoc on field_update_field().
Comment #23
bjaspan CreditAttribution: bjaspan commentedDries called this the most important Field API patch and asked for reviews, I figure that counts as a favorite.
Comment #24
Dries CreditAttribution: Dries commentedIn general, this patch is much needed because we can't afford to screw up people's data. Some comments:
Typo in 'occuring', I think. The phpDoc didn't explain why forbidding a field update might be useful, but the code example made it clear. Read: the phpDoc wasn't very useful for me.
Is is non-existent instead of nonexistent? Also, we could use single quotes here.
This might be information that is useful to specify in the phpDoc rather than in the inline comments? Furthermore, the inline code comment is slightly confusing because the @param description tells you to specify a 'complete field structure'. Either way, this is minor because I could figure it out pretty easily but you might want to refine the wording a bit?
This function could use line breaks (and a code comment) between the different "logical blocks". Minor again.
We should probably use @todo instead of TODO.
In general, the $has_data handling got me wondering. It is great that we pass it in, but some field types might not care about $has_data so one could argue that we could leave it up to the field types to determine $has_data. I'm not suggesting you change it, just want to bring it up for discussion. In general, I prefer to pass it in because it encourages proper behavior and usage, but still ...
Use @todo instead.
Wrapping could be better. Some sentences wrap too early.
Great patch, I think this is really close.
Comment #25
bjaspan CreditAttribution: bjaspan commentedMade all requested changes from #25, and fixed a few other double-space issues along the way. No functional code changes.
Comment #26
Dries CreditAttribution: Dries commentedThanks @bjaspan. I think this is RTBC.
Comment #27
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #28
yched CreditAttribution: yched commentedAwesome ! A couple late remarks, sorry for not chiming in earlier. Minor stuff.
As per #569072: Clean field_attach_* namespace, this function does not belong to the field_attach_*() namespace:
"functions that fieldable entities need to call at the right time in their own workflow in order to call themselves fieldable:
f_a_load(), f_a_presave(), f_a_insert()...". They are listed on http://api.drupal.org/api/group/field_attach/7, which lets developpers have a cleaner view of " what do I need to do to be fieldable ? is there any f_a_*() function I have forgotten ?".
Could we rename to field_has_data() ?
If raising an exception is the regular way for hook_field_update_field_forbid() to say 'no sir', should this use a dedicated FieldUpdateException class, to differentiate from other exceptions ?
'decimal53' or 'decimal52' ?
That behavior is implememted by field_sql_storage's hook_field_update_forbid() ? So should the test be in field_sql_storage.test ?
"The field-type module is responsible..." would be more accurate (was incorrect before the patch)
Comment #29
bjaspan CreditAttribution: bjaspan commentedAll of yched's comments are valid, except for the second. I create the field 'decimal53' with scale 2 because the point of that function is to update to have scale 3, and then test it with scale 3, and I can't change the name of the field along with the update.
I'll whip up a new patch soon, unless someone beats me to it; these are now pretty easy changes to make.
Comment #30
bjaspan CreditAttribution: bjaspan commentedNew patch for changes from #28. Untested.
Comment #31
yched CreditAttribution: yched commentedLooks good, except the testUpdateFieldSchemaWithData() test moved into field_sql_storage.test might require we enable number.module in the corresponding setUp ?
Comment #32
bjaspan CreditAttribution: bjaspan commentedActually, number.module is required, so it never needed to be enabled explicitly.
Comment #33
yched CreditAttribution: yched commentedAh, right. RTBC, then.
Comment #34
Dries CreditAttribution: Dries commentedCommitted. Thanks.
Comment #36
Alan D. CreditAttribution: Alan D. commentedI'm pinging to try and get some feedback.
This patch added the following to the file field:
But image was not updated, I'm not even sure if this was created at this point. So now, we can dynamically update the image file schema while we are locked into the selected file file schema. I thought that the underlying file handling was the same, so is there any reason for this? And if not, should we relax this for the file field? If so, should we add the $has_data condition to the image field now?
Initial support request before pouring though the git logs to find this commit. #1433844: Remove file field uri_schema $has_data lock.