The patch for this issue Mismatched entity and/or field definitions is updating the field schema for 'owner_id' to be not null, but the change is not actually applied to the db.

This causes the table column 'owner_id' to be different depending on whether you're updating (Null => YES) or installing (Null => NO).

In the patch code, $has_original variable is always going to be false, because is always based on a new base field instead of trying to get the current. ($field_storage_definition->getName() is null and $field_storage_definition->getTargetEntityTypeId() is null)

  $field_definition = BaseFieldDefinition::create('entity_reference')
    ->setLabel(new TranslatableMarkup('User ID'))
    ->setSetting('target_type', 'user')
    ->setTranslatable($entity_type->isTranslatable())
    ->setDefaultValueCallback(Consumer::class . '::getDefaultEntityOwner');
  $field_storage_definition = $field_definition
    ->getFieldStorageDefinition();
  $has_original = (bool) $edum->getFieldStorageDefinition(
    $field_storage_definition->getName(),
    $field_storage_definition->getTargetEntityTypeId()
  );
  $has_original
    ? $edum->updateFieldStorageDefinition($field_storage_definition)
    : $edum->installFieldStorageDefinition('owner_id', 'consumer', 'consumers', $field_definition);

The update is always going to run $edum->installFieldStorageDefinition. In this case, if the 'owner_id' field exists, the storage class only recreates the related index and updates the schema, leaving the existing field untouched.

Because the schema is updated the error disappears, but the database is left in an inconsistent state.

Comments

samu711 created an issue. See original summary.

sam711’s picture

This is my update function. It works if you haven't applied the 1.9 update yet.

sam711’s picture

This fixes consumers_update_8106 (consumers v1.9).

sam711’s picture

Status: Active » Needs review
eojthebrave’s picture

Hey @sam711 if I'm reading this correctly it looks like maybe we can get by with just the patch in #3. Which would then work for both people who have not updated to 1.9 yet, because it'll run all the updates in order, 8106 will run and result in a schema mismatch, but then 8107 will run and fix it. And it'll also work for anyone who has already 8106 and currently has the schema mismatch. Does that sound right to you?

And for testing purposes it sounds like I could install an older version of the module without 8106. Create a consumer entity. Then install the latest version + this patch and run update.php and verify it all worked. Sound right? Thanks!

sam711’s picture

Hi @eojthebrave, you're right. I was reviewing the code in 8106 and wanted to propose #2, but it was committed too fast. I thought to leave it there, but changed my mind and created #3 for a potential 1.10 release.

For testing there is no need to create consumer entities, as the error comes from a difference between the code and the stored schema and later (1.9) from a difference between the stored schema and the database when updating from older versions.

I'd go two ways to make sure this is fixed:

This causes the table column 'owner_id' to be different depending on whether you're updating (Null => YES) or installing (Null => NO).

First test:
1) Install some version < 1.7
2) update to 1.7 or 1.8 and verify the mismatched entity error
3) Update to 1.9 + #3 patch
4) Verify that the mismatched error disappears and the 'owner_id' column has the not null constraint.

Second test:
1) Install 1.9 + #3
2) Verify that the mismatched error disappears and the 'owner_id' column has the not null constraint.

Thanks for looking at this!

sam711’s picture

StatusFileSize
new957 bytes

I'm updating this patch according to the code committed in #3052959: Add an update path test and fix any bugs it finds.

It needs to add a default value to the owner_id before applying the update $edum->updateFieldStorageDefinition($field_storage_definition). If not, the database schema won't be updated correctly.

sam711’s picture

Version: 8.x-1.9 » 8.x-1.x-dev
berdir’s picture

Can't you use $field_definition->setInitialValue(0)?

Also, no need to get the anonymous user like that, that is definitely going to be 0 :)

sam711’s picture

Can't you use $field_definition->setInitialValue(0)?

Sure, according to https://www.drupal.org/node/2877964 it seems that it can only be used when installing a new base field to a content entity type. We still need the update query, right? (I haven't found any examples of setInitialValue with updateFieldStorageDefinition.

Also, no need to get the anonymous user like that, that is definitely going to be 0 :)

I guess I tried not to make assumptions, lol.

Thanks @berdir, patch updated according to your comments.

eojthebrave’s picture

Thanks for working on this. Given that there's already a release with consumers_update_8106 in it, and therefore people who have already executed that code. Do we need to put this into a new 8107 function? Or is this sufficient. Sorry for being a bit slow on this one, apparently I'm just struggling to wrap my head around it.

sam711’s picture

We can probably leave consumers_update_8106 empty and move the fixed code to consumers_update_8107 so we cover all cases. I don't see any problem in running the code twice for the ones updating from 1.9 or 1.10.

What do you think @berdir?

berdir’s picture

I'm still somewhat confused about what exactly happens here and why our new test doesn't cover it? Does it only happen when updating from a certain version?

But yeah, moving it to a new method should be OK.

berdir’s picture

We could also keep the existing method and then simplify the new one and only cover update?

sam711’s picture

It's only for certain versions, yes.

owner_id was introduced as an entity_key when EntityOwnerTrait replaced the previous owner_id base field in v1.5, (here: Commit cb6abd75).

Until the current version (1.10), it wasn't possible to update the field storage, just install it.

Users coming from versions < 1.5 with empty consumer_field_data.owner_id fields, who have run consumers_update_8106 before the current 1.10, have their owner_id field allowing null values which is inconsistent with the drupal stored schema.

Not a strong feeling, but I'd say that the first option looks easier to understand when looking at the update.

sam711’s picture

Correcting myself:

Users coming from versions < 1.5 with empty consumer_field_data.owner_id fields, who have run consumers_update_8106 before the current 1.10, have their owner_id field allowing null values which is inconsistent with the drupal stored schema.

Even if all the consumers had the owner_id not empty, the database table alter making owner_id not null is not executed by installFieldStorageDefinition when the field exists.

benstallings’s picture

Status: Needs review » Closed (outdated)

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.