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.

CommentFileSizeAuthor
#6 entity-field-schema-initial-2925550-6.patch3.02 KBBerdir
#6 entity-field-schema-initial-2925550-6-test-only.patch2.21 KBBerdir
#2 entity-field-schema-initial-2925550-2.patch828 bytesBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Active » Needs review
FileSize
entity-field-schema-initial-2925550-2.patch828 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

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

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

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

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

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Agreed, now we just need a test for this.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
entity-field-schema-initial-2925550-6-test-only.patch2.21 KB
entity-field-schema-initial-2925550-6.patch3.02 KB

Yessir! :)

The last submitted patch, 6: entity-field-schema-initial-2925550-6-test-only.patch, failed testing. View results

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
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

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

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

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
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.

PatrickMichael’s picture

PatrickMichael CreditAttribution: PatrickMichael commented

I am not sure if the issue I am experiencing is related. I have an Entity/Fields Definition Mismatch error relating to a Paragraph field on my status report and #2914259 pointed me here.

I have run the command drush entity:updates as well as trying the following code snippet in the index.php file.

try {
  \Drupal::entityDefinitionUpdateManager()->applyUpdates();
}
catch (EntityStorageException $e) {
  print_r($e);
}

Both resulted in the following error:

TypeError: Argument 1 passed to Drupal\Core\Entity\Sql\DefaultTableMapping::requiresDedicatedTableStorage() must implement interface Drupal\Core\Field\FieldStorageDefinitionInterface, null given, called in /var/www/dev.site.com/web/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php on line 315 in Drupal\Core\Entity\Sql\DefaultTableMapping->requiresDedicatedTableStorage() (line 494 of core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php).

I am running Drupal 8.6.13 and Paragraphs 8.x-1.8 with Drush 9.6.2

Any insight greatly appreciated