Once #2277941: Allow injecting an arbitrary FieldConfig when building a FieldInstanceConfig is in, we should be able to remove the 'field_uuid' entry in field.instance.* config records.

Until recently, loading the Field for the Instance was primarily based on that field_uuid entry.
But "regular" (non-deleted) load no longer uses it now, and the 'field_uuid' entry is only ever needed to disambiguate between deleted fields with the same name.

It's also annoying when copying a field instance yaml file into config/install as "default config" : it needs to be manually edited out, because the field's uuid won't be the same across drupal installs.

--> We can simply ditch it from field.instance.* records in config, we simply need to explicitly add it before placing the instance in the "state() store of deleted instances" when an instance is deleted.

Patch coming up when #2277941: Allow injecting an arbitrary FieldConfig when building a FieldInstanceConfig is in.

CommentFileSizeAuthor
#17 interdiff.txt553 bytesyched
#17 2282627-FIC_field_uuid-17.patch6.97 KByched
#15 2282627-FIC_field_uuid-15.patch6.97 KByched
#12 interdiff.txt1.23 KByched
#12 FIC_field_uuid-2282627-12.patch6.97 KByched
#10 FIC_field_uuid-2282627-9.patch6.91 KByched
#7 FIC_field_uuid-2282627-7.patch6.88 KBswentel
#6 interdiff.txt630 bytesyched
#6 FIC_field_uuid-2282627-6.patch7.06 KByched
#3 interdiff.txt545 bytesyched
#3 FIC_field_uuid-2282627-3.patch6.44 KByched
#1 FIC_field_uuid-2282627-1.patch6.48 KByched
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

yched CreditAttribution: yched commented
Status: Active » Needs review
FileSize
FIC_field_uuid-2282627-1.patch6.48 KB

#2277941: Allow injecting an arbitrary FieldConfig when building a FieldInstanceConfig got in, here's a candidate patch.

Status: Needs review » Needs work

The last submitted patch, 1: FIC_field_uuid-2282627-1.patch, failed testing.

yched’s picture

yched CreditAttribution: yched commented
Status: Needs work » Needs review
FileSize
FIC_field_uuid-2282627-3.patch6.44 KB
interdiff.txt545 bytes

Looks like some parts of core still use FIC::loadByProperties() with a condition on the old 'field_id' property (which in fact references the field_uuid). Removing it will be for another issue...

yched’s picture

yched CreditAttribution: yched commented

3: FIC_field_uuid-2282627-3.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 3: FIC_field_uuid-2282627-3.patch, failed testing.

yched’s picture

yched CreditAttribution: yched commented
Status: Needs work » Needs review
FileSize
FIC_field_uuid-2282627-6.patch7.06 KB
interdiff.txt630 bytes

The recently added FieldInstanceConfigEntityUnitTest::testToArray() needs to be updated accordingly.

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Needs review » Reviewed & tested by the community
FileSize
FIC_field_uuid-2282627-7.patch6.88 KB

Patch didn't apply anymore for 3 files, rerolled, other than that, this is good to go.

core/modules/field/config/schema/field.schema.yml.rej
core/modules/field/src/Entity/FieldInstanceConfig.php.rej
core/modules/field/tests/src/FieldInstanceConfigEntityUnitTest.php.rej
yched’s picture

yched CreditAttribution: yched commented

Thanks @swentel :-)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: FIC_field_uuid-2282627-7.patch, failed testing.

yched’s picture

yched CreditAttribution: yched commented
Status: Needs work » Needs review
FileSize
FIC_field_uuid-2282627-9.patch6.91 KB

Reroll

Status: Needs review » Needs work

The last submitted patch, 10: FIC_field_uuid-2282627-9.patch, failed testing.

yched’s picture

yched CreditAttribution: yched commented
Status: Needs work » Needs review
FileSize
FIC_field_uuid-2282627-12.patch6.97 KB
interdiff.txt1.23 KB

s/getField()/getFieldStorageDefinition()/

yched’s picture

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

Green, back to RTBC.

yched’s picture

yched CreditAttribution: yched commented

12: FIC_field_uuid-2282627-12.patch queued for re-testing.

yched’s picture

yched CreditAttribution: yched commented
FileSize
2282627-FIC_field_uuid-15.patch6.97 KB

Reroll needed after #2286681: Make public properties on ConfigEntityBase protected

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: 2282627-FIC_field_uuid-15.patch, failed testing.

yched’s picture

yched CreditAttribution: yched commented
Status: Needs work » Needs review
FileSize
2282627-FIC_field_uuid-17.patch6.97 KB
interdiff.txt553 bytes

Forgot one ->uuid.

yched’s picture

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

2 weeks in RTBC, can we haz commit ? :-)

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented 
+++ b/core/modules/field/src/Entity/FieldInstanceConfig.php
@@ -306,10 +299,7 @@ public function toArray() {
-    $field = $this->getFieldStorageDefinition();
-
-    // Make sure the field_uuid is populated.
-    $this->field_uuid = $field->uuid();
+    $this->getFieldStorageDefinition();

Any special reason to call the method still?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

This is why dreditor has issues! Drive-by reviews where you can not change or see the context... which is...

    // Validate that we have a valid field for this instance. This throws an
    // exception if the field is invalid.
    $this->getFieldStorageDefinition();

Committed 18b755b and pushed to 8.x. Thanks!

[Edit: Originally I said dreditor sucks - it does not - but it does expose the limit of our patch review process]

yched’s picture

yched CreditAttribution: yched commented

w00t ! Thanks :-)

  • alexpott committed 18b755b on 8.x 
    Issue #2282627 by yched, swentel: Remove field_uuid from field instance...

Status: Fixed » Closed (fixed)

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