Posted by xjm

Problem/Motivation

  • This is a followup for #1735118: Convert Field API to CMI and postponed on that issue. Field::save() and FieldInstance::save() include long if/else statements that execute different logic based on whether a field or instance is new or updated.

Proposed resolution

  • Factor this logic out into two methods.

Remaining tasks

  • Create a patch that refactors save() in this way.
  • Propose how the hooks should be invoked with the refactor.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Postponed » Active

Un-postponing.

swentel’s picture

Status: Active » Needs review
FileSize
17.88 KB

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 :)

Status: Needs review » Needs work

The last submitted patch, 1968996-2.patch, failed testing.

tstoeckler’s picture

Title: Add protected saveNew() and saveUpdated() methods to Field and FieldInstance to improve code readability » Add protected prepareSave() and prepareUpdated() methods to Field and FieldInstance to improve code readability

Updating title

xjm’s picture

Title: Add protected prepareSave() and prepareUpdated() methods to Field and FieldInstance to improve code readability » Add protected prepareNew() and prepareUpdated() methods to Field and FieldInstance to improve code readability
Issue tags: -Novice

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

tstoeckler’s picture

Oops... thanks @xjm! :-)

yched’s picture

Status: Needs work » Needs review
FileSize
14.68 KB
19.24 KB

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.

swentel’s picture

Title: Add protected prepareNew() and prepareUpdated() methods to Field and FieldInstance to improve code readability » Add protected saveNew() and saveUpdated() methods to Field and FieldInstance to improve code readability

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.

yched’s picture

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.

yched’s picture

Reroll.

Status: Needs review » Needs work

The last submitted patch, field-save-methods-1968996-10.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
755 bytes
18.13 KB

Missed an upstream change during the merge.

xjm’s picture

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.

yched’s picture

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 ?

yched’s picture

swentel’s picture

swentel’s picture

Status: Needs review » Needs work
Issue tags: +DX (Developer Experience), +Field API

The last submitted patch, field-save-methods-1968996-15.patch, failed testing.

swentel’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
18.68 KB

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

xjm’s picture

Looks great to me. Just a couple docblock fixes.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f0f2475 and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Removing myself from the author field so that I can unfollow the issue. --xjm