field_create_field invoked field_storage_create_field and then called db_create_table to create a new table. when it's failed, it will stop and throw error immediately. There's no chance to remove field_config record on failed creation.

Files: 
CommentFileSizeAuthor
#4 1115106-update-field-can-fail.patch5.53 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 31,573 pass(es).
[ View ]

Comments

@droplet: There is a try/catch block.. what am I missing?

<?php
function field_create_field($field) {
 
// ...
  // Store the field and get the id back.
 
drupal_write_record('field_config', $record);
 
$field['id'] = $record['id'];
 
// Invoke hook_field_storage_create_field after the field is
  // complete (e.g. it has its id).
 
try {
   
// Invoke hook_field_storage_create_field after
    // drupal_write_record() sets the field id.
   
module_invoke($storage_type['module'], 'field_storage_create_field', $field);
  }
  catch (
Exception $e) {
   
// If storage creation failed, remove the field_config record before
    // rethrowing the exception.
   
db_delete('field_config')
      ->
condition('id', $field['id'])
      ->
execute();
    throw
$e;
  }
?>

@bfroehle,
I though like you before but it isn't.

it's issue split from:
http://drupal.org/node/1052248#comment-4270018

<?php
    module_invoke
($storage_type['module'], 'field_storage_create_field', $field);
?>

hook_field_storage_create_field()

<?php
function hook_field_storage_create_field($field) {
 
$schema = _field_sql_storage_schema($field);
  foreach (
$schema as $name => $table) {
   
db_create_table($name, $table); // ERROR OCCURRED and throw error HERE !!!
 
}
 
drupal_get_schema(NULL, TRUE);
}
?>

simple php test script:

<?php
function test($x) {
   
func();
return
$x;
}
try {
    echo
test(0);
} catch (
Exception $e) {
    echo
'Hello World';
    echo
'Caught exception: ' . $e->getMessage();
}
// Continue execution
echo 'Hello World 2';
?>

Status:Active» Postponed (maintainer needs more info)

This definitely works for me.

By any chance, would you have xdebug with weird configuration options enabled?

Title:failed field creation will broken the site (field_create_field)Failures to update a field are not properly handed in SQL Storage and Field UI
Status:Postponed (maintainer needs more info)» Needs review
StatusFileSize
new5.53 KB
PASSED: [[SimpleTest]]: [MySQL] 31,573 pass(es).
[ View ]

Ok, there are a couple of issues here:

  • The core issue is that the Field SQL Storage doesn't handle failure to update a field schema properly: it first drops the tables, then try to recreate them with the new schema and doesn't properly rollback when something fails
  • The Field UI was incorrectly trying to catch FieldException instead of a general exception in a couple of places, and was calling field_update_field() without catching the possible exceptions in one place

Version:7.x-dev» 8.x-dev

Actually, this needs to go in 8.x first.

Status:Needs review» Reviewed & tested by the community

@#3,
Oh, right, I enabled Xdebug, and now i see the error.

#4 patch work for me.

Status:Reviewed & tested by the community» Needs work

+++ b/modules/field_ui/field_ui.admin.incundefined
@@ -1650,7 +1648,7 @@ function field_ui_widget_type_form_submit($form, &$form_state) {
     field_update_instance($instance);
     drupal_set_message(t('Changed the widget for field %label.', array('%label' => $instance['label'])));
   }
-  catch (FieldException $e) {
+  catch (Exception $e) {
     drupal_set_message(t('There was a problem changing the widget for field %label.', array('%label' => $instance['label'])));

That's weird. If you are updating an instance, why would the patch change that? And while at it, why is that message not 'error' ??

That's weird. If you are updating an instance, why would the patch change that? And while at it, why is that message not 'error' ??

PDOException can currently be triggered by field_sql_storage_field_storage_update_field() (see #4 for the summary). Either we catch them or we re-wrap them into a FieldException, but we need to do something.

The message is not error, but it is not touched by this patch.

Status:Needs work» Reviewed & tested by the community

While #8 is bogus, for consistency reasons I can accept this. Please file a followup for the missing 'error' thanks!

Status:Reviewed & tested by the community» Fixed
Issue tags:+needs backport to D7

Looks like this needs backporting to D7 as well.

Committed and pushed to 8.x and 7.x. Thanks!

Status:Fixed» Closed (fixed)
Issue tags:-needs backport to D7

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