Add protected saveNew() and saveUpdated() methods to Field and FieldInstance to improve code readability

Un-postponing.

I wasn't to sure about saveNew() because they don't actually fire the save(), but are more preparing, hence went for prepareSave() and prepareUpdate().

Suggestions welcome of course :)

Updating title

Ah yeah, agreed that those are probably better method names.

Oops... thanks @xjm! :-)

Not too fond of the various ad-hoc properties that are added at some point in the object lifecycle and are absent otherwise...
In Field :
$this->module_handler
$this->storage_controller
$this->hook
$this->hook_args
In FieldInstance :
$this->field
$this->instance_controller
$this->hook
$this->hook_args

At this point, I find the resulting code a net loss rather than a readability win :-)
Also, even though at some point we'd like to be able to receive injected services, storing module handler or storage controller in $this is a slippery slope because Field and FieldInstance get serialized. Works in this case because those properties are only populated on save(), and you usually don't generate an entity form with fields right after that.

I tried to come up with suggestions, but the code is indeed not so easy to split, and I ended up coding - sorry for the hijack :-/
Patch gets back to saveNew() / saveUpdated(), that go all the way to parent::save().

interdiff attached, but reading the patch directly might be more helpful.

No worries about the hijack, I wan't too extremely happy with the flow either, but then I had to go :)
This looks much better.

Looks like it's up for @xjm to RTBC, then ? ;-)

#1967106: FieldInstance::getField() method should allow it to be a bit nicer, but no biggie, easy reroll whichever one comes first.

Reroll.

Missed an upstream change during the merge.

Overall this looks awesome. One small thing I noticed on a quick read:

+++ b/core/modules/field/lib/Drupal/field/Plugin/Core/Entity/Field.phpundefined
@@ -275,119 +275,152 @@ public function getExportProperties() {
+   * @throws \Drupal\Core\Entity\EntityStorageException
+   *   In case of failures an exception is thrown.

Should there be an additional @throws for the FieldExceptions? Presumably the EntityStorageExceptions are coming from the parent.

I guess so. Sucks a bit to have to switch the save() phpdoc back from {@inheritdoc} to document our additional exceptions, but...

Or: should we switch all those FieldExceptions that are raised within save() to be EntityStorageException ?

Reroll after #1982984: Create Drupal::entityManager for improved DX

#15: field-save-methods-1968996-15.patch queued for re-testing.

#15: field-save-methods-1968996-15.patch queued for re-testing.

Reroll after #1875992: Add EntityFormDisplay objects for entity forms got in. Looks good imo, unless some objects.

Looks great to me. Just a couple docblock fixes.

Committed f0f2475 and pushed to 8.x. Thanks!