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.
Files: 
CommentFileSizeAuthor
#20 field-save-methods-1968996-20.patch18.67 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 55,903 pass(es).
[ View ]
#20 interdiff-field-save.txt2.14 KBxjm
#19 field-save-methods-1968996-19.patch18.68 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 56,714 pass(es).
[ View ]
#15 field-save-methods-1968996-15.patch19.23 KByched
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-save-methods-1968996-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 field-save-methods-1968996-14.patch19.33 KByched
PASSED: [[SimpleTest]]: [MySQL] 55,398 pass(es).
[ View ]
#14 interdiff.txt3.34 KByched
#12 field-save-methods-1968996-12.patch18.13 KByched
PASSED: [[SimpleTest]]: [MySQL] 55,999 pass(es).
[ View ]
#12 interdiff.txt755 bytesyched
#10 field-save-methods-1968996-10.patch18.18 KByched
FAILED: [[SimpleTest]]: [MySQL] 54,683 pass(es), 29 fail(s), and 129 exception(s).
[ View ]
#7 field-save-methods-1968996-7.patch19.24 KByched
PASSED: [[SimpleTest]]: [MySQL] 54,415 pass(es).
[ View ]
#7 interdiff.txt14.68 KByched
#2 1968996-2.patch17.88 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 54,434 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Postponed» Active

Un-postponing.

Status:Active» Needs review
StatusFileSize
new17.88 KB
FAILED: [[SimpleTest]]: [MySQL] 54,434 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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

Updating title

Title:Add protected prepareSave() and prepareUpdated() methods to Field and FieldInstance to improve code readabilityAdd 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.

Oops... thanks @xjm! :-)

Status:Needs work» Needs review
StatusFileSize
new14.68 KB
new19.24 KB
PASSED: [[SimpleTest]]: [MySQL] 54,415 pass(es).
[ View ]

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.

Title:Add protected prepareNew() and prepareUpdated() methods to Field and FieldInstance to improve code readabilityAdd 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.

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.

StatusFileSize
new18.18 KB
FAILED: [[SimpleTest]]: [MySQL] 54,683 pass(es), 29 fail(s), and 129 exception(s).
[ View ]

Reroll.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new755 bytes
new18.13 KB
PASSED: [[SimpleTest]]: [MySQL] 55,999 pass(es).
[ View ]

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.

StatusFileSize
new3.34 KB
new19.33 KB
PASSED: [[SimpleTest]]: [MySQL] 55,398 pass(es).
[ View ]

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 ?

StatusFileSize
new19.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-save-methods-1968996-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

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

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new18.68 KB
PASSED: [[SimpleTest]]: [MySQL] 56,714 pass(es).
[ View ]

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

StatusFileSize
new2.14 KB
new18.67 KB
PASSED: [[SimpleTest]]: [MySQL] 55,903 pass(es).
[ View ]

Looks great to me. Just a couple docblock fixes.

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.

Issue summary:View changes

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