FieldInstanceConfig::preSave() doesn't call the parent implementation (that was probably empty by the time FIC::preSave() was written).

- It thus duplicates code about config dependencies that's also present in the parent
- The parent is the one that adds $this->original, so it looks like nothing populates this then ?
Strike that, the parent *doesn't* populate $this->original...

CommentFileSizeAuthor
#5 interdiff.txt1.36 KByched
#4 2320253-FIC_preSave_parent-4.patch1.24 KByched
#2 2320253-FIC_preSave_parent-2.patch1.83 KByched

Comments

yched’s picture

yched commented
Issue summary: View changes
yched’s picture

yched commented
Title: FIC::preSave() doesn't call the parent implementation » FIC/FSC::preSave() do not call the parent implementation
Status: Active » Needs review
StatusFileSize
new 2320253-FIC_preSave_parent-2.patch1.83 KB

Same for FieldStorageConfig, actually.

Patch.

Status: Needs review » Needs work

The last submitted patch, 2: 2320253-FIC_preSave_parent-2.patch, failed testing.

yched’s picture

yched commented
Status: Needs work » Needs review
StatusFileSize
new 2320253-FIC_preSave_parent-4.patch1.24 KB

Our code needs to run before the parent's

yched’s picture

yched commented
StatusFileSize
new interdiff.txt1.36 KB

Forgot the interdiff

Status: Needs review » Needs work

The last submitted patch, 4: 2320253-FIC_preSave_parent-4.patch, failed testing.

swentel queued 4: 2320253-FIC_preSave_parent-4.patch for re-testing.

swentel’s picture

swentel commented

Probably bot fluke.

Related question though: most other config entities call the parent function at the top of the code, not the beginning. Don't think it will make much difference I guess ?

swentel’s picture

swentel commented
Status: Needs work » Needs review
yched’s picture

yched commented

@swentel:
Patch #2 called the parent at the top and got a test fail.

I didn't really investigate, but in #2283977-133: Create a new ConfigEntity type for storing bundle-specific customizations of base fields I did a similar remark about the location of the parent call in BaseFieldOverride::preSave(), and in the next patch @alexpott apparently could not do better than place it after the line that does $this->settings += [default settings]".

Probably something like "all settings need to be present before we try to calculate dependencies" - which seem be reasonable ?

So given the shape of FieldInstanceConfig::preSave() and FieldStorageConfig::preSave(), I chose to move the call straight to the bottom rather than in an awkard middle position.

This being said, not sure why we initialize default settings in preSave(), this leaves freshly created entities broken for runtime use (incomplete settings) until they are actually saved. We should do this as early as postCreate().

yched queued 2: 2320253-FIC_preSave_parent-2.patch for re-testing.

yched’s picture

yched commented

Asking for a re-test of #2 though.

swentel’s picture

swentel commented
Status: Needs review » Reviewed & tested by the community

Oh right, ok, nevermind then, RTBC for #4.

Damn, next time I should really investigate two patches in such a small issue :)

The last submitted patch, 2: 2320253-FIC_preSave_parent-2.patch, failed testing.

  • webchick committed 0d86fa9 on 8.0.x 
    Issue #2320253 by yched: Fixed FIC/FSC::preSave() do not call the parent...
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Good catch.

Committed and pushed to 8.x. Thanks!

yched’s picture

yched commented

not sure why we initialize default settings in preSave(), this leaves freshly created entities broken for runtime use (incomplete settings) until they are actually saved. We should do this as early as postCreate().

Patch in #2327883: Field [storage] config have incomplete settings until they are saved

Status: Fixed » Closed (fixed)

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