Field update (field_update_field)

There 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. :-)

Bump. 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.

Well, 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...

subscribing

What is the status of this?

This 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.

I'm working on this this weekend and have a new patch in the works.

New 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.

forgot to change status

Small 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.

New 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.

The bug mentioned in #17 is #582754: Undefined index errors viewing nodes with fields, unrelated to field_update_field.

New 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.

subscribe

Great ! No time for a real review right now, but +1 on the approach.

New patch, adds hook_field_update_field() to field.api.php and fixes the PHPdoc on field_update_field().

Dries called this the most important Field API patch and asked for reviews, I figure that counts as a favorite.

In general, this patch is much needed because we can't afford to screw up people's data. Some comments:

+++ modules/field/field.api.php	21 Sep 2009 13:10:12 -0000
@@ -1274,6 +1274,49 @@ function hook_field_create_instance($ins
+ * Forbid a field update from occuring.

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.

+++ modules/field/field.crud.inc	21 Sep 2009 13:10:12 -0000
@@ -302,6 +302,87 @@ function field_create_field($field) {
+    throw new FieldException("Attempt to update a nonexistent field.");

Is is non-existent instead of nonexistent? Also, we could use single quotes here.

+++ modules/field/field.crud.inc	21 Sep 2009 13:10:12 -0000
@@ -302,6 +302,87 @@ function field_create_field($field) {
+  // Use the prior field values for anything not specifically set by the new 
+  // field to be sure that all values are set.

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?

+++ modules/field/field.test	21 Sep 2009 13:10:12 -0000
@@ -1697,6 +1698,103 @@ class FieldCrudTestCase extends FieldTes
+  function testUpdateField() {

This function could use line breaks (and a code comment) between the different "logical blocks". Minor again.

+++ modules/field/modules/list/list.module	21 Sep 2009 13:10:12 -0000
@@ -85,8 +85,14 @@ function list_field_schema($field) {
+ * 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 databae (use field_attach_query()) to find out.
+ * Implement the validate function via hook_field_update_forbid() so
+ * list.module does not depend on form submission.

We should probably use @todo instead of TODO.

+++ modules/field/modules/text/text.module	21 Sep 2009 13:10:12 -0000
@@ -116,7 +116,7 @@ function text_field_schema($field) {
+function text_field_settings_form($field, $instance, $has_data) {

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 ...

+++ modules/field_ui/field_ui.api.php	21 Sep 2009 13:10:12 -0000
@@ -14,14 +14,29 @@
+ * TODO: Only the field type module knows which settings will affect

Use @todo instead.

+++ modules/field_ui/field_ui.api.php	21 Sep 2009 13:10:12 -0000
@@ -14,14 +14,29 @@
+ * schema changes are permitted once a field already has
+ * data. Probably we need an easy way for a field type module to ask

Wrapping could be better. Some sentences wrap too early.

Great patch, I think this is really close.

Made all requested changes from #25, and fixed a few other double-space issues along the way. No functional code changes.

Thanks @bjaspan. I think this is RTBC.

Committed to CVS HEAD. Thanks!

Awesome ! A couple late remarks, sorry for not chiming in earlier. Minor stuff.

+function field_attach_field_has_data($field) {

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 ?

+    // Create a decimal 5.2 field.
+    $field = array('field_name' => 'decimal53', 'type' => 'number_decimal', 'cardinality' => 3, 'settings' => array('precision' => 5, 'scale' => 2));

'decimal53' or 'decimal52' ?

+  /**
+   * Test updating a field with data.
+   */

That behavior is implememted by field_sql_storage's hook_field_update_forbid() ? So should the test be in field_sql_storage.test ?

+  // Add additional field settings from the field module. The field module is
+  // responsible for not returning settings that cannot be changed if
+  // the field already has data.

"The field-type module is responsible..." would be more accurate (was incorrect before the patch)

All 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.

New patch for changes from #28. Untested.

Looks good, except the testUpdateFieldSchemaWithData() test moved into field_sql_storage.test might require we enable number.module in the corresponding setUp ?

Actually, number.module is required, so it never needed to be enabled explicitly.

Ah, right. RTBC, then.

Committed. Thanks.

I'm pinging to try and get some feedback.

This patch added the following to the file field:

  $form['uri_scheme'] = array(
    '#disabled' => $has_data,
  );

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.