SqlContentEntityStorage::getFieldStorageDefinitions() is poorly named and should not be public

The problem still persists as of 8.2.7 and the provided patch works wonderfully.
Setting to needs work as tests are still needed.

Can confirm the problem exists and that this patch cures it. But yeah: Needs tests :)

Drupal core 8.5.5: same problem, patch works.

Still needs tests.

I hope it is fine to add the additional test coverage to the existing unit test \Drupal\Tests\Core\Entity\Sql\SqlContentEntityStorageTest instead of implementing an entity kernel test to add the fields through the hook entity_field_storage_info.

The issue summary says that the problem exists when you implement hook_entity_field_storage_info to define a field storage, and then try to save the entity.

But if you implement hook_entity_field_storage_info, you are also required to implement hook_entity_bundle_field_info(), otherwise there are no field definition attached to your field storage.

Maybe it's worth continuing the explanation in #14: if you want to define a field storage which is also automatically available to all the bundles of an entity type, you need to implement hook_entity_base_field_info() instead.

+++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageTest.php
@@ -1432,6 +1442,28 @@ public function testCleanIds() {
+    $this->bundleFieldDefinitions = $this->mockFieldDefinitions(['bundle_field']);

This should be
$this->additionalFieldStorageDefinitions = $this->mockFieldDefinitions(['bundle_field']); and then the test will fail as expected.

But if you implement hook_entity_field_storage_info, you are also required to implement hook_entity_bundle_field_info(), otherwise there are no field definition attached to your field storage.

@amateescu, but even if you do so then \Drupal\Core\Entity\Sql\SqlContentEntityStorage::getFieldStorageDefinitions() will still be returning only the base field definitions. Actually this is also what the method documentation states, but it is a little bit too late to change the method name to ::getBaseFieldDefinitions().

Hm, then I think we need to deprecate it and have a new getBaseFieldDefinitions() method instead.

In any case, the reproduction steps from the current issue summary are based on a wrong assumption, so they're not really accurate..

Can we probably completely remove the method? It should not even be public. It is only used in \Drupal\Core\Entity\Sql\SqlContentEntityStorage::mapToStorageRecord().

What happens if you implement both hook_entity_field_storage_info and hook_entity_bundle_field_info for a field of the type BaseFieldDefinition, but that field is not returned by \Drupal\Core\Entity\Sql\SqlContentEntityStorage::getFieldStorageDefinitions()? In this case an exception will be thrown by \Drupal\Core\Entity\Sql\SqlContentEntityStorage::mapToStorageRecord(), right?

I am looking at the following code inside \Drupal\Core\Entity\Sql\SqlContentEntityStorage::mapToStorageRecord():

foreach ($table_mapping->getFieldNames($table_name) as $field_name) {
      if (empty($this->getFieldStorageDefinitions()[$field_name])) {
        throw new EntityStorageException("Table mapping contains invalid field $field_name.");
      }

This makes me think that we still need all the field storage definitions as returned by \Drupal\Core\Entity\EntityFieldManagerInterface::getFieldStorageDefinitions() instead only the ones as returned by \Drupal\Core\Entity\EntityFieldManagerInterface::getBaseFieldDefinitions().

Can we probably completely remove the method?

That would be a hard BC break, I think just deprecating it is fine :)

As for the second point, like I said above, I would support having a getFieldStorageDefinitions() helper in SqlContentEntityStorage.

As for the second point, like I said above, I would support having a getFieldStorageDefinitions() helper in SqlContentEntityStorage.

Well, we already have it :). The only question is what should it return.

Judging from the code I think it's obvious the problem is the documentation and implementation of getFieldStorageDefinitions() is wrong. The actual method name is correct. So simply changing the method implementation to match the name, should suffice.

What I'm missing in this patch is a test-only patch that actually fails and then goes green with the change. This could be done by implementing the field storage/bundle field info hooks in a test module and then enabling that module for the test.

Happy to RTBC when the tests actually show what's broken :)

P.S.: I've seen other places in core where the fact that bundle fields are both storage and field info causes issues. Maybe this was yet another one of those places :) One of them being #2898635-6: bundleFieldDefinitions() are not added in EntityViewsData, which made me write #2930736: EntityViewsData assumes BaseFieldDefinitions where it should use FieldDefinitionInterface

Here's a patch from #12 updated according to #16, just to make sure new test works as expected.

Yup seems good. As explained in #22, the method name was correct but the documentation and implementation wasn't. This fixes that.

I can see just changing what the method returns, but would like a second opinion from an entity maintainer on this one - while the method might not be doing what it was supposed to, there could be code relying on the current behaviour out there.

I'm not sure there's anything actually broken here, aside from the inconsistency between the method name and its implementation/documentation: ::mapToStorageRecord is meant to work with shared field tables (e.g. node, node_field_data, node_revision, node_field_revision), so retrieving only the base field definitions is the right thing to do, because only (single-value) base fields are stored in shared field tables. In a way it also makes sense to call the method ::getFieldStorageDefinitions(), because those are the storage definitions the storage handler needs to deal with in this particular context.

As pointed out by @amateescu in #14/#15, a storage definition alone makes no sense from the Entity Field API point of view: it either needs to be "part of" a base field definition or complement a set of bundle field definitions.

Therefore, any storage definition being also a base field definition will be correctly handled by ::mapToStorageRecord, while any other one should be ignored because bundle fields are stored in dedicated tables, and so they should not be returned when dealing with shared field tables. The error currently reported in the IS should be fixable by implementing hook_entity_bundle_field_info() besides hook_entity_field_storage_info(), see the (recently updated) docs.

If I'm missing something and things are actually broken, it would be good to update the IS with correct STR and provide a test-only patch that actually shows what's wrong. The current one is just asserting a different expected return value for ::getFieldStorageDefinitions(), which in itself tells us nothing about what's actually broken.

Otherwise, I'd just rescope this to deprecate ::getFieldStorageDefinitions(), we should make it a protected helper in D9.

Actually, the fact that the patch in #3 is not failing tests makes me think that we are missing some test coverage.

From #16:

But if you implement hook_entity_field_storage_info, you are also required to implement hook_entity_bundle_field_info(), otherwise there are no field definition attached to your field storage.

@amateescu, but even if you do so then \Drupal\Core\Entity\Sql\SqlContentEntityStorage::getFieldStorageDefinitions() will still be returning only the base field definitions. Actually this is also what the method documentation states, but it is a little bit too late to change the method name to ::getBaseFieldDefinitions().

SqlContentEntityStorage::getFieldStorageDefinitions() is public. So even if there is some undocumented logic you need to be aware of as shown by #28, the method name is still very much misleading for what it's actually doing.

Also, could you please answer #18, @plach?

What happens if you implement both hook_entity_field_storage_info and hook_entity_bundle_field_info for a field of the type BaseFieldDefinition, but that field is not returned by \Drupal\Core\Entity\Sql\SqlContentEntityStorage::getFieldStorageDefinitions()? In this case an exception will be thrown by \Drupal\Core\Entity\Sql\SqlContentEntityStorage::mapToStorageRecord(), right?

Also, a test was added in #24 that fails on the current code base and passes with the changes. But that test does not go as far as to implement both hooks to demonstrate that saving an entity with a bundle field currently causes an exception to be thrown.

---

Finally, at a glance it really seems like SqlContentEntityStorage tries to save all fields on an entity, including bundle fields. So if we don't load their storage records, we will get an exception. This seems to match my findings from a couple months ago where I implemented both a storage and bundle field hook and still got errors when trying to save my entities.

EntityBundleFieldTest seems to cover everything just fine, though. But I do recall having serious issues with custom defined bundle fields/storages a while ago (early 2018). That said, the test's module seems to do more than simply implement those two hooks, maybe that's the key?

I'll see if I can cook up something to confirm it's actually still broken.

The following was enough for it to work properly:

/**
 * Implements hook_entity_field_storage_info().
 */
function group_test_entity_field_storage_info(EntityTypeInterface $entity_type) {
  if ($entity_type->id() == 'group') {
    $definitions['cadabra'] = \Drupal\entity_test\FieldStorageDefinition::create('string')
      ->setName('cadabra')
      ->setLabel(t('A custom bundle field'))
      ->setTargetEntityTypeId($entity_type->id());
    return $definitions;
  }
}

/**
 * Implements hook_entity_bundle_field_info().
 */
function group_test_entity_bundle_field_info(EntityTypeInterface $entity_type, $bundle) {
  if ($entity_type->id() == 'group' && $bundle == 'abra') {
    $definitions['cadabra'] = \Drupal::entityManager()->getFieldStorageDefinitions($entity_type->id())['cadabra'];
    return $definitions;
  }
}

So I guess whatever was broken a while back seems to work fine now. There are still other woes with bundle fields (see #23), but this no longer seems to be one of them.

Which leaves us at method name vs method implementation and how we might fix that? Maybe the IS and title need a change?

@kristiaanvandeneynde:

#30:

SqlContentEntityStorage::getFieldStorageDefinitions() is public. So even if there is some undocumented logic you need to be aware of as shown by #28, the method name is still very much misleading for what it's actually doing.

Agreed. I think we should just deprecate it since it offers no real value over retrieving base field definitions from the entity field manager. Ideally it would have a been a protected method named ::getBaseFieldStorageDefinitions(), or something along those lines.

Also, could you please answer #18, @plach?

What happens if you implement both hook_entity_field_storage_info and hook_entity_bundle_field_info for a field of the type BaseFieldDefinition, but that field is not returned by \Drupal\Core\Entity\Sql\SqlContentEntityStorage::getFieldStorageDefinitions()? In this case an exception will be thrown by \Drupal\Core\Entity\Sql\SqlContentEntityStorage::mapToStorageRecord(), right?

If you define a field via the hook_entity_field_storage_info()/hook_entity_bundle_field_info() hooks, you are defining a bundle field, regardless of the class you are using to provide the definition. ::getFieldStorageDefinitions() is not meant to deal with bundle fields, because it was introduced and is used only to deal with shared-field table field records, which store only (single-value) base fields. I don't expect any exception to be thrown because the new field won't be listed among base fields.

Finally, at a glance it really seems like SqlContentEntityStorage tries to save all fields on an entity, including bundle fields. [...]

Of course it does, but it treats (single-value) base fields and all the other fields in different ways: the former are stored in shared-field tables, while all the other fields get dedicated tables.

[...] So if we don't load their storage records, we will get an exception.

Nope, sorry :)

As long as ::getFieldStorageDefinitions() is only used by the ::map* methods, it's perfectly fine to return only base fields, actually it's the right thing to do. The only problem here is that the method was introduced as public and with a misleading name, so people might actually be using it in unintended contexts and with expectations not matching the original intents. However that's not the official API to retrieve field storage definitions, so there's little we can do for them, besides not breaking their code by changing the method's behavior.

#32

Which leaves us at method name vs method implementation and how we might fix that? Maybe the IS and title need a change?

As mentioned above, I think we should rescope the issue to introduce a protected ::getBaseFieldStorageDefinitions() method and deprecate ::getFieldStorageDefinitions() in favor of the official API :)

It would also be good to add a test making sure that if in the future we attempt to do something like #3, we actually get some test failures. That is, we should assert that a code-defined bundle field is a) stored in a dedicated table b) loaded from a dedicated table.

We might also try to leverage this issue to throw an exception in the entity field manager, if we have an orphan storage definition with no corresponding bundle definition or viceversa. @amateescu, what do you think?

We might also try to leverage this issue to throw an exception in the entity field manager, if we have an orphan storage definition with no corresponding bundle definition or viceversa. @amateescu, what do you think?

The reason why I'm asking confirmation is that I think that, for configurable fields, it might be ok to have only a field storage definition without bundle field definitions, at least in very specific parts of their life-cyle, that is while purging the data of the last occurrence of a bundle field. Not 100% sure about this, though.

Great feedback, thanks @plach! I agree with what you suggested at the bottom of #33 (in reply of #32).

The exception might need more input from others, though. I'm thinking of a module shipping with an exported field storage definition that may be used by the module further down the line. For instance, Group ships with a field.storage.group_content.group_roles.yml that is only instantiated whenever a group type is created. I didn't use the hooks because Views is still choking on bundle fields. When using exported field config, it does work.

I think the same reasoning from #1426804: Allow field storages to be persisted when they have no fields. applies to in-code field storage definitions as well, so it should be valid to have a storage definition without any bundle definitions. However, the other way around is not valid, so we should throw an exception if we encounter any bundle definitions without a storage definition.

This is as far as I got tonight, still would like to add a test that fails on #3

This method is being deprecated in #2554235: Make the content entity storage and entity query use the last installed definitions instead of the ones living in code, so I think we can close this one.