Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Posted by xjm
Problem/Motivation
- This is a followup for #1735118: Convert Field API to CMI and postponed on that issue.
Field::save()
andFieldInstance::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.
Comment | File | Size | Author |
---|---|---|---|
#20 | field-save-methods-1968996-20.patch | 18.67 KB | xjm |
#20 | interdiff-field-save.txt | 2.14 KB | xjm |
#19 | field-save-methods-1968996-19.patch | 18.68 KB | swentel |
#15 | field-save-methods-1968996-15.patch | 19.23 KB | yched |
#14 | field-save-methods-1968996-14.patch | 19.33 KB | yched |
Comments
Comment #1
yched CreditAttribution: yched commentedUn-postponing.
Comment #2
swentel CreditAttribution: swentel commentedI 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 :)
Comment #4
tstoecklerUpdating title
Comment #5
xjmAh yeah, agreed that those are probably better method names.
Comment #6
tstoecklerOops... thanks @xjm! :-)
Comment #7
yched CreditAttribution: yched commentedNot 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.
Comment #8
swentel CreditAttribution: swentel commentedNo worries about the hijack, I wan't too extremely happy with the flow either, but then I had to go :)
This looks much better.
Comment #9
yched CreditAttribution: yched commentedLooks 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.
Comment #10
yched CreditAttribution: yched commentedReroll.
Comment #12
yched CreditAttribution: yched commentedMissed an upstream change during the merge.
Comment #13
xjmOverall this looks awesome. One small thing I noticed on a quick read:
Should there be an additional @throws for the FieldExceptions? Presumably the EntityStorageExceptions are coming from the parent.
Comment #14
yched CreditAttribution: yched commentedI 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 ?
Comment #15
yched CreditAttribution: yched commentedReroll after #1982984: Create Drupal::entityManager for improved DX
Comment #16
swentel CreditAttribution: swentel commented#15: field-save-methods-1968996-15.patch queued for re-testing.
Comment #17
swentel CreditAttribution: swentel commented#15: field-save-methods-1968996-15.patch queued for re-testing.
Comment #19
swentel CreditAttribution: swentel commentedReroll after #1875992: Add EntityFormDisplay objects for entity forms got in. Looks good imo, unless some objects.
Comment #20
xjmLooks great to me. Just a couple docblock fixes.
Comment #21
alexpottCommitted f0f2475 and pushed to 8.x. Thanks!
Comment #22.0
(not verified) CreditAttribution: commentedRemoving myself from the author field so that I can unfollow the issue. --xjm