Followup from #1735118: Convert Field API to CMI (postponed on that issue).

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -0,0 +1,463 @@
+      // Tell the storage engine to update the field. Do this before saving the
+      // new definition since it still might fail.
+      $module_handler->invoke($this->storage['module'], 'field_storage_update_field', array($this, $original, $has_data));

I don't quite understand the implications of this hunk; could you clarify?

Proposed resolution

From @swentel:

This happens before saving the updated definition of the field, because in case the storage update fails, we'd end up in a state where the database configuration of the table does not reflect the configuration of the field. See field_sql_storage_field_storage_update_field() which can throw an exception in case something goes wrong. Comment is as in D7, so I've left it for now, unless you really don't think it's clear enough :)

Let's incorporate this explanation into an improved inline comment.

Remaining tasks

  • Create a patch that improves this comment.
  • Get reviews of the proposed comment from field and documentation maintainers.
  • [#1155816]


xjm’s picture

Title: Improve inline documentation in Field::save() » Improve inline documentation in Field::save() when updating field storage information
yched’s picture

Status: Postponed » Fixed

Been fixed in the main patch meanwhile.

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

Anonymous’s picture

Issue summary: View changes