Download & Extend
Issues

Failures to update a field are not properly handed in SQL Storage and Field UI

Posted by droplet on April 4, 2011 at 1:34am
Project:Drupal core
Version:8.x-dev
Component:field system
Category:bug report
Priority:normal
Assigned:Unassigned
Status:closed (fixed)
Issue tags:needs backport to D7

Issue Summary

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.

Login or register to post comments

Comments

#1

Posted by bfroehle on April 4, 2011 at 4:39am

@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;
  }
?>

#2

Posted by droplet on April 4, 2011 at 7:21am

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

#3

Posted by Damien Tournoud on April 4, 2011 at 8:00am
Status:active» postponed (maintainer needs more info)

This definitely works for me.

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

#4

Posted by Damien Tournoud on April 4, 2011 at 8:54am
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

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
AttachmentSizeStatusTest resultOperations
1115106-update-field-can-fail.patch5.53 KBIdlePASSED: [[SimpleTest]]: [MySQL] 31,573 pass(es).View details

#5

Posted by Damien Tournoud on April 4, 2011 at 9:11am
Version:7.x-dev» 8.x-dev

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

#6

Posted by droplet on October 3, 2011 at 8:15am
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.

#7

Posted by chx on October 6, 2011 at 10:12am
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' ??

#8

Posted by Damien Tournoud on October 6, 2011 at 10:22am

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.

#9

Posted by chx on October 6, 2011 at 10:28am
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!

#10

Posted by webchick on October 9, 2011 at 2:55pm
Status:reviewed & tested by the community» fixed

Looks like this needs backporting to D7 as well.

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

#11

Posted by System Message on October 23, 2011 at 3:00pm
Status:fixed» closed (fixed)

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

nobody click here