Update 'default field value' UI code for D7 Field API [#619230]

When a user is denied access to a field that stores a numeric value (e.g., Interger, Taxonomy, etc), on submit of the form (which does not show the denied field) Field API attempts to write an empty string to the DB, which causes a PDO error like this:

PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '' FOR COLUMN 'field_taxonomy_categories_value' at row 1: INSERT INTO {field_data_field_taxonomy_categories} (etid, entity_id, revision_id, bundle, delta, LANGUAGE, field_taxonomy_categories_value) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6); Array ( [:db_insert_placeholder_0] => 2 [:db_insert_placeholder_1] => 7 [:db_insert_placeholder_2] => 7 [:db_insert_placeholder_3] => q_a [:db_insert_placeholder_4] => 0 [:db_insert_placeholder_5] => zxx [:db_insert_placeholder_6] => )  IN field_sql_storage_field_storage_write() (line 421 of /var/www/vc/drupal-head/modules/FIELD/modules/field_sql_storage/field_sql_storage.module).

I've tried to trace this, and I know that the appropriate (module)_field_is_empty function is being called, and field_default_submit is unsetting the $item, but this is not being relayed to the ultimate database actions.

I'd love to get some guidance from a more experienced Core dev so that I can write a patch for this, but I'm stuck. Any help?

Comment	File	Size	Author
#13	field_ui_default_values-619230-13.patch	15.01 KB	yched
#13
#8	field_ui_default_values-619230-8.patch	15 KB	yched
#8
#7	619230_field_set_default_empty.01.patch	1 KB	matt2000
#7
#5	619230_field_set_default_empty.patch	837 bytes	matt2000
#5

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

matt2000 CreditAttribution: matt2000 commented 30 October 2009 at 19:05

Priority:

Normal

» Critical

Comment #2

yched CreditAttribution: yched commented 31 October 2009 at 00:16

Was not able to reproduce at first with an API-created integer field.

But then I thought 'wait a sec', and tried with a field created through the UI. Boom on node create form.

That's because the 'default value' input in 'field settings' form is submitted blank, and field_ui stores '' as the default value.
Then, on 'node add' form submit, field_default_insert() sees there's a default value and tries to insert it.

Pondering on the right fix for this.

Comment #3

matt2000 CreditAttribution: matt2000 commented 31 October 2009 at 00:28

Do we need to call [module]_field_is_empty on the default value field and use that to turn '' into NULL where appropriate?

Comment #4

yched CreditAttribution: yched commented 1 November 2009 at 10:55

Right, running field_set_empty() on $instance_values['default_value'] in field_ui_field_edit_form_submit() should do the trick.

Ideally, we should run field_default_submit() (with $obj_type and $object = NULL). That would take care of the field_set_empty() call, and more generally ensure that we go through the same processing than during a regular '[node|comment|user|...] form' submit.

Comment #5

matt2000 CreditAttribution: matt2000 commented 2 November 2009 at 23:00

Status:

Active

» Needs review

File	Size
619230_field_set_default_empty.patch	837 bytes

Yup, that seems to do the trick. Here's the patch.

Comment #6

yched CreditAttribution: yched commented 3 November 2009 at 21:17

Can we try with field_default_submit() ? I think that would be the real clean fix.

Comment #7

matt2000 CreditAttribution: matt2000 commented 4 November 2009 at 00:07

File	Size
619230_field_set_default_empty.01.patch	1 KB

Is this the right idea? It seems to work, but I'm not sure I understand exactly what happens with the normal hook_field_* stuff, so this feels out of place. (Nothing else calls field_default_submit directly.)

Comment #8

yched CreditAttribution: yched commented 20 November 2009 at 03:07

Title:	field_access causes error on submit of numeric fields	» Update 'default field value' UI code for D7 Field API
Component:	field system	» field_ui.module

File	Size
field_ui_default_values-619230-8.patch	15 KB

It's the right direction indeed.

The bug was actually larger. The validation and submission of the 'default value' widget had not been updated for from CCK to D7's field validation workflow.
Attached patch fixes this, and adds tests.

Comment #9

matt2000 CreditAttribution: matt2000 commented 20 November 2009 at 19:22

OK, I can't say that I really understand this stuff, so all I found was nit picks. However, I can confirm that the patch fixes the original issue.

+++ modules/field_ui/field_ui.admin.inc	20 Nov 2009 03:05:42 -0000
@@ -1215,106 +1206,76 @@ function field_ui_default_value_widget($
+  $items = $instance['default_value'];
   $instance['required'] = FALSE;
   $instance['description'] = '';
-  $items = $instance['default_value'];

Looks like a non-change here.

+++ modules/field_ui/field_ui.test	20 Nov 2009 03:03:52 -0000
@@ -20,8 +20,11 @@ class FieldUITestCase extends DrupalWebT
+
+    // Create test user.
     $admin_user = $this->drupalCreateUser(array('access content', 'administer content types', 'administer taxonomy'));
     $this->drupalLogin($admin_user);
+

whitespace changes?

+++ modules/field_ui/field_ui.test	20 Nov 2009 03:03:52 -0000
@@ -173,7 +180,8 @@ class FieldUITestCase extends DrupalWebT
     // Re-add body field by visiting the content type edit page.
-    $this->drupalPost('admin/structure/types/manage/' . $this->hyphen_type . '', array('body_label' => 'New body field'), t('Save content type'));
+    $edit = array('body_label' => 'New body field');
+    $this->drupalPost('admin/structure/types/manage/' . $this->hyphen_type . '', $edit, t('Save content type'));

What's this have to do with default values? I don't get it.

I'm on crack. Are you, too?

Comment #10

yched CreditAttribution: yched commented 20 November 2009 at 19:49

1) Yes, just a result of reshuffling stuff back and forth in the function, ended up in a no-op like that. That's life ;-)

2) and 3). Cleaning up while I'm in there. I try to avoid polluting patches with unrelated cleanups in actual module files, but test files get less attention, and I don't see myself rolling such minor stuff in a separate patch. So if that's OK, I'd rather keep those in.

Comment #11

yched CreditAttribution: yched commented 24 November 2009 at 12:00

Bump - would be cool to have this in to move forward on other cleanups ;-)

Comment #12

sun

German

Karlsruhe

CreditAttribution: sun commented 24 November 2009 at 16:05

Looks good. Only some very minor remarks:

+++ modules/field_ui/field_ui.admin.inc	20 Nov 2009 03:05:42 -0000
@@ -1215,106 +1206,76 @@ function field_ui_default_value_widget($
+ * Validate callback for the instance settings form.

This should be

Form validation handler for field instance settings form.

+++ modules/field_ui/field_ui.admin.inc	20 Nov 2009 03:05:42 -0000
@@ -1215,106 +1206,76 @@ function field_ui_default_value_widget($
+ * Submit callback for the instance settings form.

This should be

Form submit handler for field instance settings form.

+++ modules/field_ui/field_ui.test	20 Nov 2009 03:03:52 -0000
@@ -81,10 +84,12 @@ class FieldUITestCase extends DrupalWebT
+    $edit = array(
+      '_add_new_field[label]' =>  $this->field_label,
+      '_add_new_field[field_name]' =>  $this->field_name,
@@ -141,10 +147,11 @@ class FieldUITestCase extends DrupalWebT
+    $edit = array(
...
+      '_add_existing_field[field_name]' =>  $this->field_name,

(minor) Duplicate spaces here.

+++ modules/field_ui/field_ui.test	20 Nov 2009 03:03:52 -0000
@@ -173,7 +180,8 @@ class FieldUITestCase extends DrupalWebT
+    $this->drupalPost('admin/structure/types/manage/' . $this->hyphen_type . '', $edit, t('Save content type'));

uhm - why do we concatenate an empty string?

+++ modules/field_ui/field_ui.test	20 Nov 2009 03:03:52 -0000
@@ -207,4 +215,55 @@ class FieldUITestCase extends DrupalWebT
+    // Check that invalid default values are rejected.
...
+    // Check that the default value is saved.
...
+    // Check that the default value shows up in the form
...
+    // Check that the default value can be emptied.

In general, we use "Verify" instead of "Check" in such inline comments.

I'm on crack. Are you, too?

Comment #13

yched CreditAttribution: yched commented 24 November 2009 at 18:24

File	Size
field_ui_default_values-619230-13.patch	15.01 KB

Fixed all points except for // Check
224 occurrences of // Check ... in core .test files (that's without field and field_ui that use it all over the place), vs. 173 occurrences of // Verify ... ;-)