Move save() / delete() logic in Field / FieldInstance to [pre|post]Save(), [pre|post]Delete()

Temptative patch.

This leaves out FieldInstance::delete() for now, because of the custom $field_cleanup param, this might be a bit tricky :-(.

Reroll after #2018731: Move field_has_data() to Field::hasData().

Fails were because $storage_controller->loadUnchanged($this->getOriginalID()); in FieldInstance::preSave() return FALSE when we are called from field_entity_bundle_rename().

I added a test / workaround for now, but field_entity_bundle_rename() should update the config records directly IMO, rather than re-saving the ConfigEntity. I know we went back and forth on this in the "Field / CMI" issue, but I can't remember why we stayed that way.

re @amateescu #2 / menu links - setPreventReparenting() :
Hm, smart ;-) The tricky bit might be making sure that the flag that gets set on the controller for a specific delete() operation doesn't interfere for the rest of the request. Or I guess each delete operation explicitly sets the state of the flag it needs, so the flag always has the "right" value when performing a delete ?

I probably won't have the time to look into this before I go, though :-/

Thanks ! Will look into this.

The 510 commits are probably the ones you merged from HEAD, it should be fine to push.

The instance 'id' property was not saved anymore. Caused all sorts of weirdness when loading more than two of them.
My bad :-(.

Reverted the changes in CrudTest.php

Reroll effect - the patch reintroduced the CRUD hooks that got removed in #2013939: Remove hook_field_[CRUD]_() in favor of hook_ENTITY_TYPE_[CRUD]() calls.

We might be able to remove the need for the $field_cleanup bool, actually.
It's just there to avoid an infinite loop on:
"delete last instance -> delete field -> delete all instances"
or
"delete field -> delete all instances -> delete field"

But If we move "delete all instances" to Field::postDelete(),
and "if (last instance) { delete field }" to FieldInstance::postDelete(),
then by the time those run, the actual records that might risk "re-delete ad nauseum" have been deleted already, load() will return nothing, and there is nothing to delete, no loop :-)

Oops, also, I didn't re-remove "Invoke hook_field_delete_field()" from Field::postDelete() - see #12

Thanks @pcambra!

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -380,108 +385,101 @@ public function preDelete() {
+    foreach ($fields as $field) {
+      if (!$field->deleted) {
+        $state = \Drupal::state();
+        $deleted_fields = $state->get('field.field.deleted') ?: array();
+        $config = $field->getExportProperties();
+        $config['deleted'] = TRUE;
+        $deleted_fields[$field->uuid] = $config;
+        $state->set('field.field.deleted', $deleted_fields);
+      }

This handles multiple field deletion, so for performance, it would be best to do one single $state->get() before looping, and one single $state->set() after looping ?

+ 80 chars comment wrapping needs to be adjusted after the code is moved around.

Also, this was previously done *after* the "delete all instances" part, and is now done before. Might have an impact on test fails, not sure.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -380,108 +385,101 @@ public function postDelete() {
+    foreach ($fields as $field) {
+      if (!$field->deleted) {
+        $module_handler = \Drupal::moduleHandler();
+        $instance_controller = \Drupal::entityManager()->getStorageController('field_instance');

Those two lines should move above the loop.

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -380,108 +385,101 @@ protected function saveNew() {
+        foreach ($instance_controller->loadMultiple($instance_ids) as $instance) {
+          // By default, FieldInstance::delete() will automatically try to delete
+          // a field definition when it is deleting the last instance of the
+          // field. Since the whole field is being deleted here, pass FALSE as
+          // the $field_cleanup parameter to prevent a loop.
+          $instance->delete(FALSE);
         }

That's for a second line of optimizations once this is green, but this could probably be optimized by collecting the instance ids for all the fields, and do a single loadMultiples() on all the collected ids.

Also, FieldInstance::delete(), that defined the custom $field_cleanup set to FALSE here, is now gone, so the FALSE is useless now.
Meaning the code could just do:
$instances = $instance_controller->loadMultiple(...);
$instance_controller->delete($instances);
(which is the way entity_delete_multiple does() it)

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.phpundefined
@@ -330,153 +328,105 @@ public function getExportProperties() {
+  public static function postDelete(EntityStorageControllerInterface $storage_controller, array $instances) {
+  foreach ($instances as $instance) {

indentation is broken :-)

Also note: moving our code to pre/postSave() means we can stop populating $this->original, ConfigStorageController::save() does it for us.

#1969698: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data) means we shouldn't need to check for "cannot create a *new* field/instance with the same id", ConfigEntity::save() checks that for us.
But the current code organization in HEAD forces us to have our own check, because it needs to happen before we call the EntityStorageController::on[Field|Instance]Create() - and our current call to parent::save() happens after that.
The new code organization in the patch should hopefully let us remove that.

Added that in a "remaining tasks" section in the OP, along with #24

Reroll for now
(I also updated the field-save-delete-2020895 branch in the sandbox)

#28: field-save-delete-2020895-28.patch queued for re-testing.

Let's see where this one brings us.

Some dirty parts to polish, but let's see.

Reordering a couple things.

Previous patch seems lost in testbot limbo, I'll cancel it...
Meanwhile, debugged & reordered a bit more.

Cleaned up the newly added tests.
I think this is ready for reviews now.

#37: field-save-delete-2020895-37.patch queued for re-testing.

Reroll

Should we already use getFieldType() ?
Same maybe for $field->name

I'd think not in this issue. The Field / FieldInstance classes currently have plenty of code that access their own properties directly, let's not change this here.

Fixed the typo.

Attempt to do #27

Hm, #1969698: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data) did not take care of that actually, so we need to keep those for now. Reverting the changes from #44.
We should be good here IMO.

Reroll + bump. We need to do this, the sooner the better, and it's a bit painful to merge :-/

Doh, thanks for fixing this :-)
Patch just moves the dependencies on the field type manager to standalone vars, as the current code tends to do in Field / FieldInstance pending real injection abilities in entity classes.

removed the stale "remaining tasks"

Can I haz commit ? :-p

Reroll (just offsets)

Re-reroll.

Pleaaaase ? :-)