Follow-up of #2346019: Handle initial values when creating a new field storage definition

Quoting myself from there:

So this causes an interesting problem, specifically, it results in an entity schema definition mismatch on all fields that had previously manually set an 'initial' value through a custom StorageSchema handler.

Because what happens now is that we remove those two keys from the *generated* schema but it might still exist in the stored installed schema.

I've seen this so far in paragraphs (status, behavior_settings) as well as in file_entity (type). I think the solution is to process both the stored and the generated schema equally, to consistently ignore that value, or explicitly process the installed schema and remove it from that? I'll open an issue for that.

As written, there are two possible solutions:

a) Clean/Process both schemas, generated and installed
b) Provide an update function that explicitly cleans up/resaves installed schemas.

I'll provide a patch for a) because that's a lot easier to implement although a bit unclean maybe.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Status: Active » Needs review
FileSize
828 bytes

Testing this is a bit tricky, as we would need an example entity type that has such a schema in an old dump.

amateescu’s picture

Fixing it like this makes sense because we can not provide an update function for contrib/custom code, they have to manually update their schema handlers and use the new setInitialValue() method on their base field definitions.

Testing this is a bit tricky, as we would need an example entity type that has such a schema in an old dump.

We can manually alter a dump to include an 'initial' definition for a base field, with something similar to what we do in core/modules/rest/tests/fixtures/update/drupal-8.rest-rest_post_update_resource_granularity.php ;)

Berdir’s picture

I think it would technically be possible to have an update function, we basically just would have to reproduce what \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::saveFieldSchemaData() does on all entity types and fields.

Also setInitialValue() is backwards compatible in that it only sets a value and doesn't replace/remove a value that was set in a different way. So contrib isn't *forced* to do it, but it of course makes it simpler for them and requires one less custom class (if only used for that)

But I think this is easier :)

amateescu’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Agreed, now we just need a test for this.

Berdir’s picture

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @Berdir!

+++ b/core/modules/system/tests/src/Functional/Update/EntityUpdateInitialTest.php
@@ -0,0 +1,33 @@
+  public function testInitialIsIgnored() {
+    $this->runUpdates();
+  }

I'm usually not a fan of "empty" test methods like this for update path tests but in this case we don't really have anything else to assert, we only want to know that we don't have mismatched entity definitions after the update.

In other words, let's do this! :)

Berdir’s picture

Yeah, I previously had an explicit check for that myself, but when running the test noticed that it didn't even get that far because it already fails inside doUpdates(). (Unrelated: The logic there doesn't make sense anymore with the PHPUnit conversion as it always stops on the first assert and never displays the mismatched definitions anymore ;))

  • catch committed 3516185 on 8.5.x
    Issue #2925550 by Berdir, amateescu: Fields with a manually defined...

  • catch committed 63cf12b on 8.4.x
    Issue #2925550 by Berdir, amateescu: Fields with a manually defined...
Berdir’s picture

Status: Reviewed & tested by the community » Fixed

Looks like this was committed, closing. Thanks! :)

Status: Fixed » Closed (fixed)

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