SQLContentEntityStorage::saveToDedicatedTables() doesn't correctly use storage configuration

@richgerdes, I just tested the following steps manually:

- installed Drupal 8.6.0
- created a test ER field on the Tags vocabulary (configured to reference nodes)
- created a test node and then a test test term with a reference to the node
- updated the codebase to 8.8.x, then ran updates through update.php

And everything worked without errors, so the issue you encountered is not the expected behavior of the new EntityDefinitionUpdateManager::updateFieldableEntityType() API.

If you have a backup database, can you try to debug the update process again and see how or why you were getting different values from FieldStorageConfig::getUniqueStorageIdentifier()? I'm assuming that the custom field_default_access ER field is a configurable field (judging by the field_ prefix), so the only possible explanation is that the two field storage definitions, the one stored in the last installed schema repository is different than the one stored in the FieldStorageConfig configuration entity. Which make me think this is a case of "corrupted database" (specifically, the last installed schema repository), and it's not something that can be fixed in core...

@amateescu I have experienced same problem and it is exactly what you have explained. It's different configuration in FieldStorageConfig and EntityLastInstalledSchemaRepository.

Is there some way to fix that in DB? I think that FieldStorageConfig should be correct one.

@richgerdes I used this code to check it.

use Drupal\field\Entity\FieldStorageConfig;

/** @var \Drupal\Core\Entity\EntityLastInstalledSchemaRepository $last_installed_schema_repository */
$last_installed_schema_repository = Drupal::service('entity.last_installed_schema.repository');

/** @var \Drupal\Core\Field\FieldStorageDefinitionInterface[] $from_sr */
$from_repository = $last_installed_schema_repository->getLastInstalledFieldStorageDefinitions('taxonomy_term');

$field_name = '_YOUR_FIELD_NAME_HERE_';

$field_repository = $from_repository[$field_name];
echo 'From repository: ' . $field_repository->getUniqueStorageIdentifier() . PHP_EOL;

$field_storage = FieldStorageConfig::load('taxonomy_term.' . $field_name);
echo 'From storage: ' . $field_storage->getUniqueStorageIdentifier() . PHP_EOL;

You can also output $field_repository and $field_storage in files and diff them.

@richgerdes: Could you make a test that mimics the situation and that results in the given exception. And with your fix does not result in the exception. Like changing a non-revisionable entity type with some entity instances and changing it to a revisionable entity type.

@mtodor:

Is there some way to fix that in DB? I think that FieldStorageConfig should be correct one.

Yup, FieldStorageConfig holds the correct (current) values, and I think the only quick fix is to set it as the last installed definition directly by calling \Drupal\Core\Entity\EntityLastInstalledSchemaRepository::setLastInstalledFieldStorageDefinition().

I opened #3101598: Differring field storage identifiers should be reported as a missing field storage definition update for helping/guiding sites that might be in the same situation as the one reported by @richgerdes here, but I think the current issue title and scope also describe a valid bug: since the entity storage was changed to rely on the last installed definitions in #2554235: Make the content entity storage and entity query use the last installed definitions instead of the ones living in code, the two places where we generate table names based on the live (in-code) definitions could very much be a source of trouble.

Here's a start for a patch which changes the entity storage to use its internal definitions instead of the ones provided by the entity field manager.

Worked independently on the same problem in #3113306: Last installed field storage definitions not used when loading from / saving to dedicated tables, which looks like a duplicate. That means that issues for the same problem are being opened and we should better solve this faster.

Here is a similar patch that should be passing all tests.

I don't think that we need explicit test coverage beside the one that I had to adjust in \Drupal\KernelTests\Core\Field\FieldMissingTypeTest, which is implicitly asserting that the active field storage definitions are being used instead of the live ones.

A next task would be to keep track of the installed field definitions the same way as we keep track of the installed field storage definitions. I've left todos in the code for that, but this should be pursued in a follow-up issue.

Actually I am surprised that we need to make so few test adjustments.

Patch looks good. It has enough testing for me. Some minor points left:

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1235,9 +1235,14 @@ protected function loadFromDedicatedTables(array &$values, $load_from_revision)
   +      // @todo add support for last installed field definitions.
   
   @@ -1323,13 +1328,18 @@ protected function saveToDedicatedTables(ContentEntityInterface $entity, $update
   +    // @todo add support for last installed field definitions.
   

   Is there for this todo a followup? If not, should we create one?

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1235,9 +1235,14 @@ protected function loadFromDedicatedTables(array &$values, $load_from_revision)
          foreach ($definitions[$bundle] as $field_name => $field_definition) {
   
   @@ -1323,13 +1328,18 @@ protected function saveToDedicatedTables(ContentEntityInterface $entity, $update
        foreach ($definitions as $field_name => $field_definition) {
   

   Nitpick: Could we change the variable $field_definition to $d as we are not using it anymore.

3. +++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
   @@ -343,6 +343,14 @@ protected function setUpFields(EntityInterface $entity, UserInterface $account)
   +    // Ensure that a new instance of the entity storage is created, so that it
   +    // contains the new field storage definitions as part of the active field
   +    // storage definitions.
   +    /** @var \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager */
   +    $entity_type_manager = $this->container->get('entity_type.manager');
   +    $entity_type_manager->clearCachedDefinitions();
   +    $this->entityStorage = $entity_type_manager->getStorage(static::$entityTypeId);
   

   Can we move this piece of code to the method setup() as we are there setting the value of $this->entityStorage. It becomes a bit confusing/double now.

Addressed 18.2 and 18.3.

for 18.2

@@ -1323,13 +1328,18 @@ protected function saveToDedicatedTables(ContentEntityInterface $entity, $update
     foreach ($definitions as $field_name => $field_definition) {

For this case,$field_definition is being used in the followup code.

Fixed CS issue.

@anmolgoyal74: Thank you for working on this patch.

1. The change I asked for in #18.3 needs to be reverted. The value of $this->entityStorage must be set before and needs to be updated afterwards.
2. You are right that the second instance of the variable $field_definition cannot be renamed.
3. For the todo's: Just leave them. I have looked in the ussue queue if there is an issue for the todo's, but I cannot find it.

After these things are done it is for me RTBC.

OCD-fueled typo fix in issue title

The patch implements the proposed solution from the IS.
All changes in the patch look good to me.
Some tests needed to be changed for the solution.
There is no API change, therefore no CR needed.
I could not find any outstanding issues for the added @todo's.
For me the patch is RTBC.

@anmolgoyal74: My apologies for making you do the by my requested changes and then having you reverting them.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1196,9 +1196,14 @@ protected function loadFromDedicatedTables(array &$values, $load_from_revision)
   +      // @todo add support for last installed field definitions.
   ...
   +        // Consider only the active field storage definitions.
   +        if (!isset($this->fieldStorageDefinitions[$field_name])) {
   
   @@ -1284,13 +1289,18 @@ protected function saveToDedicatedTables(ContentEntityInterface $entity, $update
   +    // @todo add support for last installed field definitions.
   ...
   +      // Consider only the active field storage definitions.
   +      if (!isset($this->fieldStorageDefinitions[$field_name])) {
   +        continue;
   +      }
   
   +++ b/core/tests/Drupal/KernelTests/Core/Field/FieldMissingTypeTest.php
   @@ -56,13 +56,15 @@ protected function setUp(): void {
   +   * As the storage is using the installed instead the live field storage
   +   * definitions the manipulated field storage entity should not be loaded and
   +   * therefore the exception should not be thrown.
   

   The comment in the test about only using the installed definitions and not the live definitions seems to contradict what is commented in the code.

2. +++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
   @@ -343,6 +343,14 @@ protected function setUpFields(EntityInterface $entity, UserInterface $account)
   +    // Ensure that a new instance of the entity storage is created, so that it
   +    // contains the new field storage definitions as part of the active field
   +    // storage definitions.
   +    /** @var \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager */
   +    $entity_type_manager = $this->container->get('entity_type.manager');
   +    $entity_type_manager->clearCachedDefinitions();
   +    $this->entityStorage = $entity_type_manager->getStorage(static::$entityTypeId);
   

   This test class should not be adding services like entityStorage to the test. It's a recipe for unexpected side effects. That said this change is a concern for unintended impacts on contrib and custom tests.

Ah I see where things have got confused... see \Drupal\Core\Entity\EntityFieldManager::getActiveFieldStorageDefinitions()

  public function getActiveFieldStorageDefinitions($entity_type_id) {
    if (!isset($this->activeFieldStorageDefinitions[$entity_type_id])) {
      $this->activeFieldStorageDefinitions[$entity_type_id] = $this->entityLastInstalledSchemaRepository->getLastInstalledFieldStorageDefinitions($entity_type_id);
    }
    return $this->activeFieldStorageDefinitions[$entity_type_id] ?: $this->getFieldStorageDefinitions($entity_type_id);
  }

getActiveFieldStorageDefinitions() doesn't do what you think it does... the change we're making here is changing things to only use last installed definitions! So that makes the comments incorrect.

Stumbled on this issue while trying to update a BaseField cardinality from single to unlimited in an updateFieldableEntityType call ; loadFromDedicatedTable reading the current base field instead of the last installed one leads to issues in that case. I can confirm the patch fixes that case.

Attaching a re-rolled patch (patch applied with fuzz), with comments fix for 26.1. That being said, I wonder if simply removing the @todo and keeping the active in the comment might be better, given that those fields are retrieved with getActiveFieldStorageDefinitions.

Not sure of what to do for 26.2 ; what's happenning here is that ResourceTestBase already got entityStorage injected. After the new fields creation, that storage initially injected is outdated, because it doesn't contain those new fields definitions ; that's why this patch reloads a fresh storage in $this->entityStorage. Not storing it at all on the object and fetching a fresh entityStorage when needed would solve that issue, but that seems like a far more invasive change to the tests, compared to "If your test is adding new fields, you-ll need to fetch a fresh entity storage".

Patch updated for the new fail in #29.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -1205,8 +1205,12 @@ protected function loadFromDedicatedTables(array &$values, $load_from_revision)
-      foreach ($definitions[$bundle] as $field_name => $field_definition) {
-        $storage_definition = $field_definition->getFieldStorageDefinition();
+      foreach ($definitions[$bundle] as $field_name => $d) {
...
+        $storage_definition = $this->fieldStorageDefinitions[$field_name];

any reason to rename variable? looks out of scope

I'm not the one who introduced that change so I can't tell for sure, but I guess that as $field_definition is now longer used, the variable name was changed to match the style of the outer foreach loop (first line of the context of the patch, foreach ($bundles as $bundle => $v) { ), so it's more consistent with the style of the surrounding code.

That being said, I can change that back to the original variable name if needed.

I think it'd be better to do foreach (array_keys($definitions[$bundle] then instead of having a dead variable. That way we only iterate over the field names

Needs work for #34

This also needs an issue summary update, the remaining task states that the solution is not yet decided. Is it?

This patch restores the changes to the comments, which is what I understand #27 to mean. I also reverted the change to the foreach to keep this in scope.

The change made to ManageFieldFunctionalTest in #31 is unexpected. If the functional test is testing what a user would do through the UI, then clearing the cache in the middle of working is not an expected action. Am I wrong and this is the correct fix or should that be at the beginning of the test or is there something still to work on in the patch?

I have updated the proposed resolution and remaining tasks in the IS, removing tag.

Forgot to upload the patch.

Seems that I didn't make the changes to the Issue Summary I intended to. I have do so now.

I put this issue in #bugsmash and lendude replied is agreement with #37, middle paragraph, that the change of storage in the middle of a functional test doesn't seem right. And that relates back to #26.2 and the reply in #29, third paragraph. So what is the direction this should take?

Just chiming in to say that the patch from #38 works perfectly to solve my issues - I added a custom number field with unlimited cardinality to an existing entity via an update hook + base field info alter hook, which was previously causing the "table not found" error to be thrown on the entity's forms. Now the field loads and values can be properly stored. I know that's not the same as what the issue description is for, but searching through various related issues brought me here.

So what's the decision regarding 26.2

Moving back to NW until an answer for #26.2 is added. Or change code.

EDIT:
Patch #15 applies to 10.2.7. It does not apply to 10.3.0.

Patch from #38 doesn't apply to Drupal 10.3.0.
Something must have changed in FieldMissingTypeTest.

patching file core/tests/Drupal/KernelTests/Core/Field/FieldMissingTypeTest.php
Hunk #1 FAILED at 56.

Recreated the patch from #38 to successfully apply to Drupal 10.3.1.