Finalize API for creating, overriding, and altering code-defined bundle fields

Discussed with @alexpott and @effulgentsia. This would be critical if it were considered a hard blocker for a contributed project that required code-defined bundle fields. The workaround at present would be for such a project to use a configurable field instead, which is not necessarily ideal but would still allow the contributed module to move forward. Based on this, the issue should be major; see #2350615-4: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?.

If someone has information about a specific contributed module or usecase, we can then assess its impact, disruption, etc. to see if this change can still be made during the beta phase. If it is actually a hard blocker for a particular contributed module, then we can reassess the priority.

One problematic use case is the one used by Comment:

  /**
   * {@inheritdoc}
   */
  public static function bundleFieldDefinitions(EntityTypeInterface $entity_type, $bundle, array $base_field_definitions) {
    if ($comment_type = CommentType::load($bundle)) {
      $fields['entity_id'] = clone $base_field_definitions['entity_id'];
      $fields['entity_id']->setSetting('target_type', $comment_type->getTargetEntityTypeId());
      return $fields;
    }
    return array();
  }

It allows Comment to define an entity_id reference field, but set the target_type based on the comment type setting.
This means that an entity can reference any entity type that's suitable (implements an interface, etc), and is a workaround for the fact that target_type is a storage level setting and not a field level one. Commerce uses the same trick (line items referencing purchasable entities such as product variations or product bundles).

Now, what happens if we want to have the reference field only for specific bundles (based on a setting again)? It makes sense to move the entityreference to a configurable field, but that can't work because target_type is no longer malleable. Sacrifices have to be made (keep the field, but hide it for those bundles).

This isn't an argument for changing the issue priority, just wanted to document my thoughts.

Just like the Comment module, I've run into this issue with the Group module. The use case I have is that a GroupContent bundle is created for every entity that could possibly be added to a Group.

This allows users to provide metadata (through configurable fields) about why they added a Node or any other entity to a Group. The catch is that they can add different fields to each relationship. So for Article nodes they could tell the Group why they found it interesting and for Page nodes they could add "review notes" or something.

Technically, I therefore need to provide two entityreference fields on the GroupContent entity: one to the Group, which doesn't change, and one to the groupable entity, which obviously needs to have a different target type based on the configuration. Much like the Comment example above.

So in short: I'm doing it the way Comment does now, but if this API were to change or improve, I'd definitely need to update the Group module as well.

Also, I don't know if this is a bug, but the way Comment implements it allows for a crash if copied by other modules.

In EntityReferenceItem::defaultStorageSettings() If no target type is specified and node is enabled, it will fall back to node. If node is disabled, it will fall back to user.

In EntityReferenceItem::schema(), the schema returned is based on the 'target_type' setting of the ER field:

• If the target is a content entity, the schema will reference the entities with an INT column
• If the target is a config entity, the schema will reference the entities with a VARCHAR column

The above two combined can lead to the following:

1. Comment does not mention a target_type in its base field definitions
2. It does mention them in its bundle field definitions
3. When a database table is created for an entity type, only base fields are taken into account
4. This will cause the entity_id column to be of type INT (due to lack of 'target_type' in base field definitions and the fallback to node/user)
5. This means that if you override the base field in your bundle field definitions to target a config entity, Drupal will try to store a VARCHAR inside an INT column, leading to a crash

Any updates on this issue please?

Thanks,
Sukanya

Correct me if I am wrong, but could not hook_entity_bundle_info() use the BaseFieldOverride class? The example shown in the documentation for hook_entity_bundle_info() would become the following.

function hook_entity_bundle_field_info(\Drupal\Core\Entity\EntityTypeInterface $entity_type, $bundle, array $base_field_definitions) {
  // Add a property only to nodes of the 'article' bundle.
  if ($entity_type->id() == 'node' && $bundle == 'article') {
    $fields = [];
    $base_field_definition = BaseFieldDefinition::create('string')
      ->setLabel(t('More text'))
      ->setComputed(TRUE)
      ->setClass('\Drupal\mymodule\EntityComputedMoreText');
    $fields['mymodule_text_more'] = BaseFieldOverride::createFromBaseFieldDefinition($base_field_definition, $bundle);
    return $fields;
  }
}

If the problem is just BaseFieldDefinition::create()returning a field that is reported to be a base field, the class resolves it.

@kiamlaluno That's close, but BaseFieldOverride assumes that there is a base field underneath it. So you get odd fatal errors, for example when anything tries to get display options. Note the usage of BaseFieldOverride::getBaseFieldDefinition()... all over the class, and who
knows where else.

If code defined "bundle fields" are going to be a thing, we actually do need a new BundleFieldDefinition class.

Meanwhile, I guess the recommended approach is to add a Base field only when it's a computed field (because you can't uninstall a module which provides a base field with data in it. See #1498720-86: [meta] Make the entity storage system handle changes in the entity and field schema definitions ). Otherwise use a config field, like this:

/**
 * Implements hook_entity_bundle_field_info().
 *
 * Add the example_field field to appropriate entity bundles.
 *
 * {@inheritdoc}
 */
function example_entity_bundle_field_info(EntityTypeInterface $entity_type,
$bundle, array $base_field_definitions) {
  $fields = [];
  // If the field doesn't otherwise exist.
  $field_config_storage = \Drupal::getContainer()->get('entity_type.manager')->getStorage('field_config');
    $existing_field = $field_config_storage->loadByProperties([
      'entity_type' => $entity_type->id(),
      'bundle' => $bundle,
      'field_name' => 'example_field',
    ]);
    if (empty($existing_field)) {
      $fields = [];

      $fields['example_field'] = FieldConfig::create([
        'entity_type' => $entity_type->id(),
        'field_name' => 'example_field',
        'bundle' => $bundle,
      ]);
    }
  }
  return $fields;
}

/**
 * Implements hook_entity_field_storage_info().
 *
 * {@inheritdoc}
 */
function example_entity_field_storage_info(EntityTypeInterface $entity_type) {
  $fields = [];
  // Make sure the field storage doesn't already exist.
  $field_storage_storage =
\Drupal::getContainer()->get('entity_type.manager')->getStorage('field_storage_config');
  $field_storage = $field_storage_storage->load('example_field');
  if (empty($field_storage)) {
    $fields['example_field'] = FieldStorageConfig::create([
      'field_name' => 'example_field',
      'entity_type' => $entity_type->id(),
      'type' => 'string',
      'locked' => TRUE,
      'settings' => [
        'max_length' => 255,
        'is_ascii' => FALSE,
        'case_sensitive' => FALSE,
      ],
    ]);
  }
  return $fields;
}

Note that there are a few things you can do with Base Fields (and the mythical "bundle fields"), which you cannot do with Config fields. My personal pain point is setting displayConfigurable=FALSE. The whole reason I'm doing this in code is because I don't want it dependent on anything in the config store, and with displayConfigurable=TRUE, every entity that receives the field gets a new entry in its displays. :|

Commerce in #3 still references this issue and implements it this way:

class BundleFieldDefinition extends BaseFieldDefinition {

  /**
   * {@inheritdoc}
   */
  public function isBaseField() {
    return FALSE;
  }

}

I have the same implementation in a few places. Core even has it in tests Drupal\entity_test\FieldStorageDefinition

Here's something to get us started. It is the most simple implementation I could think of. We'll still need to add a CR and link it into the deprecation.

Also linking in #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info(), which I also discovered while looking into this.

The FieldStorageDefinition class just defines isBaseField(), so it could be replaced by the proposed BundleFieldDefinition class: Since there is already #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info() for that class, I would rather leave to that issue the task to replace the class with a proper one, especially because the comment describing the FieldStorageDefinition class:

For convenience we extend from BaseFieldDefinition although this should not implement FieldDefinitionInterface.

I think it would be an error to first replace it with the FieldStorageDefinition class, and then with a class that doesn't implement the FieldDefinitionInterface.

Essentially, the tasks to do are (ordering them by what I feel it's the priority):

☐ Fix the problem hook_entity_bundle_field_info_alter() and hook_entity_base_field_info_alter() have (#2346329: hook_entity_base_field_info_alter() and hook_entity_bundle_field_info_alter() are documented to get a parameter that doesn't implement an interface with setter methods)
☐ Define a new class for bundle field definitions (#2935932: Add a FieldDefinition class for defining bundle fields in code.)
☐ Correct the example given in hook_entity_bundle_field_info() (#2935932: Add a FieldDefinition class for defining bundle fields in code.)
☐ Create bundle_field_override config entities (#2935978: Create a bundle_field_override configuration entity to override a bundle field with configuration)
☐ Update the documentation where necessary
☐ Update the code and remove the comments pointing to this issue, once this marked as fixed

Re #20 as long as we can still dynamically (i.e.: through code) change a bundle field's target entity type, I'm okay with it. We need to make sure that we maintain support for the behavior as currently implemented in Comment, Commerce (#3) and Group (#4)

I think that #2346329: hook_entity_base_field_info_alter() and hook_entity_bundle_field_info_alter() are documented to get a parameter that doesn't implement an interface with setter methods should be fixed first, since that should take to the definition of a new interface that is then implemented by the BaseFieldDefinition class. In this case, the hierarchy of classes/interfaces would change.

There's also another aspect related to hook_entity_bundle_field_info() defined fields. The result of \Drupal::service('entity_field.manager')->getFieldStorageDefinitions($entity_type_id) doesn't return fields defined in this way. But it returns entity base fields together with bundle configurable fields. Is this normal?

Not sure what you mean. getFieldStorageDefinitions() returns both base fields (which implement both the field and field storage interfaces) als well as FieldStorageConfig objects. As well as any other object that modules might define that implements that interface. That is by design yes.

@Berdir, so it's by design that getFieldStorageDefinitions() returns bundle configurable fields but not bundle "code" fields?

it returns what it says, field storage definitions. I'm not sure what you mean by bundle code fields. All fields must have a field storage definitions and field storage definitions are *not* by bundle. See \field_entity_field_storage_info() and \field_entity_bundle_field_info(), you must implement both to define per-bundle fields.

I wrote a thing about computed bundle fields https://www.previousnext.com.au/blog/how-create-and-expose-computed-prop...

It would be great to see some movement on this issue. Having implemented bundle fields in code, you hit a few problems like this. My only comments below:

1. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,24 @@
   +/**
   + * A class for defining entity bundle fields.
   + */
   

   I think this would be a good place to expand on the docs for how one goes about actually using bundle field definitions outside the context of field.module config.

2. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   index 1a37d63..1cb4658 100644
   --- a/core/modules/system/tests/modules/entity_test/src/FieldStorageDefinition.php
   
   --- a/core/modules/system/tests/modules/entity_test/src/FieldStorageDefinition.php
   +++ b/core/modules/system/tests/modules/entity_test/src/FieldStorageDefinition.php
   
   +++ b/core/modules/system/tests/modules/entity_test/src/FieldStorageDefinition.php
   @@ -12,14 +12,8 @@
   + * @deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0.
   + *   Use \Drupal\Core\Field\BundleFieldDefinition instead.
   

   Hm, do we really need to deprecate things in entity_test?

Not sure how this got into "Needs work" from #16, but would be good to see if anyone has any other feedback on this.

I'm hyped to get this in, since it's such a small change that affords a lot of power.

We have bumped into this as well.

#2898635: bundleFieldDefinitions() are not added in EntityViewsData appears related.

Adding a pointer to Commerce's ConfigurableFieldManager, added since the 2015 pointers to that ecosystem.

One of the issues with Commerce's ConfigurableFieldManager is that it requires field storage. Computed fields are not possible with that approach. With bundle classes being a thing now, could bundle field definitions be handled there? I've had success with this approach inside a base entity classes bundleFieldDefinitions() method. Added custom bundle class entity handlers works pretty well.

$class = $entity_type->getHandlerClass('bundle_classes', $bundle);

if ($class && is_subclass_of($class, EntityBundleClassInterface::class)) {
  $fields = call_user_func($class . '::bundleClassFieldDefinitions', $entity_type, $bundle, $base_field_definitions) + $fields;
}

From there, each bundle class can define it's own fields.

ConfigurableFieldManager looks like some useful functions for manipulating config fields.

I don't think that's really relevant to this issue, which is about declaring bundle fields in code.

New API will have to go into 10.1.x since 9.5.0-beta1 is released.

What is next for this issue? There are no remaining tasks or even a proposed resolution. Tagging for an issue summary update so the proposed resolution and remaining tasks are clarified.

Adding credits for the triage from #2.