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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bfroehle’s picture

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

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;
  }
droplet’s picture

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

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

    module_invoke($storage_type['module'], 'field_storage_create_field', $field);

hook_field_storage_create_field()

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:

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';
Damien Tournoud’s picture

Status: Active » Postponed (maintainer needs more info)

This definitely works for me.

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

Damien Tournoud’s picture

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
FileSize
5.53 KB

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
Damien Tournoud’s picture

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

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

droplet’s picture

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.

chx’s picture

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' ??

Damien Tournoud’s picture

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.

chx’s picture

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!

webchick’s picture

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.