Add a FieldDefinition class for defining bundle fields in code.

We should only add new things to core when they have a usage. Is there a possible usage in drupal core for this?

We should only add new things to core when they have a usage. Is there a possible usage in drupal core for this?

@borisson_

Yes, this resolves our use case. We have our internal distributions that define content types and fields, but these fields should not be changed on sites built using the profile. If the field is created using config in the install, it can be changed on the site. Using BundleFieldDefinition prevents this.

I agree with @kiamlaluno, it doesn't make sense to have a "Not a Base Field" switch on BaseFieldDefinition and the added BundleFieldDefinition is it is going to be used in hook_entity_bundle_field_info().

I think the approach in #2 looks good. I think we should frame this almost as a bugfix vs a new feature, given it's finishing off part of an API is this quite visible already (a method on ContentEntityBase).

I think this is NW for some additional documentation on the class itself and also some explicit examples in tests that expose the benefits of this class (the fact that we can install bundle specific fields and storage to get parity with config base fields, the example in entity.api.php uses a computed field).

Adding some docs and replacing instances of the test-only class with the new BundleFieldDefinition. We seem to have all the testing we need in EntityBundleFieldTest.

+++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
@@ -0,0 +1,32 @@
+class BundleFieldDefinition extends BaseFieldDefinition {

What about this comment from \Drupal\entity_test\FieldStorageDefinition?

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

What about it? I'm not aware of the original goals of the Drupal\entity_test\FieldStorageDefinition class. I think it has been abused to do something similar to what this issue is accomplishing but they are two different things. In this case we do want both a field definition and a storage definition, just like BaseFieldDefinition.

Used the class from contrib instead of core.

Some documentation erorrs:

1. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,32 @@
   + * Bundle fields can defined in code using hook_entity_bundle_field_info or by
   

   The hook name needs a () so API module will make that into a link.

2. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,32 @@
   + * implementing FieldableEntityInterface::bundleFieldDefinitions. Once a bundle
   

   Method name needs a () too, and class should be fully-qualified.

3. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,32 @@
   + * hook_entity_field_storage_info must also be implemented.
   

   Same -- ().

4. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,32 @@
   + * @see \Drupal\Core\Entity\FieldableEntityInterface::bundleFieldDefinitions
   ...
   + * @see hook_entity_bundle_field_info
   + * @see hook_entity_field_storage_info
   

   More () missing.

Thanks for the review! Fixed up the docs and slightly reworded the docbock to not require the large fully qualifier interface method in the text.

Looks good!

I think we need the change record to be written in draft before this can be RTBC?

Good point, writing one up now.

What about #2346347: Finalize API for creating, overriding, and altering code-defined bundle fields the patch there is also doing the same thing there as this patch? I think we need a review from at least one sub-system maintiner of entity/field API.

+++ b/core/modules/system/src/Tests/Entity/EntityDefinitionTestTrait.php
@@ -210,7 +210,7 @@ protected function removeBaseFieldIndex() {
-    $definitions['new_bundle_field'] = FieldStorageDefinition::create($type)

Should we remove the FieldStorageDefinition class as well here?

The parent is a meta. Looks like @amateescu is the only maintainer of field api. Assigning, hopefully he has some time to take a look at this.

I'm afraid that adding a bundle field definition class is not that simple :)

The current \Drupal\entity_test\FieldStorageDefinition is just a quick workaround, and the problem is that bundle fields should not have setters for properties that exist only at the field storage level. For example, a bundle field should *not* be able to set the cardinality, the target entity type and even the target bundle (!), because they are assigned automatically by \Drupal\Core\Entity\EntityFieldManager::buildBundleFieldDefinitions.

IMO, we need a class that extends ListDataDefinition, implements FieldDefinitionInterface and mirrors the setters from \Drupal\Core\Field\FieldConfigBase.

Thanks for looking into this @amateescu.

I assumed we would want a builder for bundle fields to be both a storage definition and field definition, with the intention that we could do stuff like #2980922: Applicable bundle field definitions defined in code should not have to manually implement hook_entity_field_storage_info. in the future.

If we introduced a builder for just the field definition part of a bundle field, I suppose we would also need #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info(). I left a similar comment to that effect in that issue.

Is there a reason we wouldn't encapsulate both of those in the one builder for DX consistency with BaseFieldDefinition?

Is there a reason we wouldn't encapsulate both of those in the one builder for DX consistency with BaseFieldDefinition?

I can't really put my finger on it, I just have the impression that it would muddy the waters on both concepts. Also, hook_entity_bundle_field_info() says that you need to override an existing base field or provide an explicit field storage, which makes perfect sense to me at least :)

Fair enough, the approach is somewhat more palatable if it's a totally new field which you'd have to write a storage definition for anyway, but it's perhaps inappropriate if you're simply doing a bundle override on an existing base field.

So this is NW to implement #22. Thanks @amateescu!

New patch since this essentially starts from scratch. Progress so far:

• The new definition builder has no storage related concepts baked in.
• EntityBundleFieldTest currently passes utilising this builder! 🎉
• BundleFieldDefinitionTest is a new test class, which essentially copies the definition related test cases from BaseFieldDefinitionTest
  • The fails in this test represent the progress on the builder so far.
  • Most methods are working, however I paused when it came to default value setters, because it will depend on #2976244: The BaseFieldOverride entity fails to normalize default values into the "array keyed by delta" format in the same way BaseFieldDefinition does when a callback is specified once that lands.

Once that blocker is in, the remaining test cases in BundleFieldDefinitionTest can be properly ported and this should be ready for a review again.

We should also update sample code in hook_entity_bundle_field_info

Good point re: docs. Uploading a combined patch with #2976244: The BaseFieldOverride entity fails to normalize default values into the "array keyed by delta" format in the same way BaseFieldDefinition does when a callback is specified.

• Removed all storage based test cases from BundleFieldDefinitionTest.
• Implemented an additional testDisplayConfigurationSettings test case that was missing from BaseFieldDefinitionTest.

Fixing last test case.

Updating the docs too as requested in #28.

Can we update hook_entity_field_storage_info docs as well instead of coping field_entity_field_storage_info? Or do you think it is out of scope? I'd nominate replacing the docs with entity_schema_test_entity_field_storage_info code.

1. +++ b/core/modules/system/tests/modules/entity_schema_test/entity_schema_test.module
   @@ -62,7 +63,9 @@ function entity_schema_test_entity_field_storage_info(EntityTypeInterface $entit
   +      ->setTargetEntityTypeId($entity_type->id());
   

   We should setup the bundle here as well using setTargetBundle.

2. +++ b/core/modules/system/tests/modules/entity_schema_test/entity_schema_test.module
   @@ -94,6 +97,6 @@ function entity_schema_test_entity_bundle_delete($entity_type_id, $bundle) {
          ->setTargetBundle($bundle);
   

   We also don't need to call

         ->setTargetEntityTypeId($entity_type_id)
         ->setTargetBundle($bundle);

   in entity_schema_test_entity_bundle_create and

   function entity_schema_test_entity_bundle_delete($entity_type_id, $bundle) {
   

Feedback from #37 and reroll after the blocker is in.

It would be great if @amateescu could have another look at this if it goes green.

Yay! blocker is in and it is green. I think @Sam152 we agreed on addressing #36 and thank you for addressing the feedback. Coding wise patch looks fine to me. Here are some docs update suggestions.

1. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -1810,6 +1811,7 @@ function hook_entity_base_field_info_alter(&$fields, \Drupal\Core\Entity\EntityT
     * @todo WARNING: This hook will be changed in
   @@ -1819,7 +1821,7 @@ function hook_entity_bundle_field_info(\Drupal\Core\Entity\EntityTypeInterface $
   

   I think we can remove this @todo from both hook_entity_bundle_field_info and hook_entity_bundle_field_info_alter.

2. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,317 @@
   + * of an entity type. A bundle field definition may also override an existing
   + * base field definition for a single bundle.
   

   We can have bundle specific overrides for base field definitions but that doesn't make them bundle fields they are still base fields. I think this should be removed.

3. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,317 @@
   + * requires storage hook_entity_field_storage_info() must also be implemented.
   

   s/requires storage/requires storage then.

4. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,317 @@
   + * @see \Drupal\Core\Field\FieldStorageDefinitionInterface
   

   Why are we adding this here this interface is not used in this class?

This is getting close :) I'll have to review again after the feedback below is addressed because these changes will have a big impact on the patch..

1. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,317 @@
   + * implementing the bundleFieldDefinitions method when defining an entity type.
   

   We should use the fully qualified method name for bundleFieldDefinitions().

2. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,317 @@
   + * requires storage hook_entity_field_storage_info() must also be implemented.
   

   Missing a comma after storage I think.

3. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,317 @@
   +  protected $type;
   ...
   +  public static function create($type) {
   

   Unlike BaseFieldDefinition, the field type is not an "important" property of a field (definition), it's just there for optimization purposes (see \Drupal\Core\Field\FieldConfigBase::$field_type) and it should be populated automatically by getting the value from the field storage.

   We should also keep the field storage in an internal fieldStorage property, just like we do in \Drupal\field\Entity\FieldConfig.

   However, the most important part missing here IMO is a createFromFieldStorageDefinition(FieldStorageDefinitionInterface $definition, $bundle), which would look very close to the implementation from BaseFieldDefinition (without cardinality and custom storage), and with one very important addition: it would also set the target bundle.

4. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,317 @@
   +  public function setTargetEntityTypeId($entity_type_id) {
   

   Bundle fields can not choose the entity type they are attached to, that's a property of the field storage, so we need a check that the entity type set here is not different than the one defined by the field storage.

5. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,317 @@
   +  public function getSetting($setting_name) {
   

   This should work like \Drupal\Core\Field\FieldConfigBase::getSetting() and also look at the storage settings if a setting is not found on the item definition.

6. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,317 @@
   +  public function getSettings() {
   

   Like above, this needs to merge in the storage settings.

7. +++ b/core/modules/system/tests/modules/entity_schema_test/entity_schema_test.module
   @@ -62,7 +63,10 @@ function entity_schema_test_entity_field_storage_info(EntityTypeInterface $entit
   +    $definitions['custom_bundle_field'] = BundleFieldDefinition::create('string')
   

   As already pointed out above, a bundle definition does make sense on its own, so we need to pass in a storage definition and use that to populate the field name automatically.

8. +++ b/core/modules/system/tests/modules/entity_schema_test/entity_schema_test.module
   @@ -62,7 +63,10 @@ function entity_schema_test_entity_field_storage_info(EntityTypeInterface $entit
   +      ->setTargetBundle('custom')
   

   We should use the $bundle variable here.

Thank you for the review. I only have these questions:

3. How would be make use of this method in the wild? Ie, what scenarios should we be testing for when introducing it?
7. Can you expand on this? Do you think we always need access to a storage definition when creating this kind of definition?

I'm not sure I understand the question for point 3..

As for 7, we already have access to the storage definitions (at least the base field ones), because both hook_entity_bundle_field_info() and FieldableEntityInterface::bundleFieldDefinitions() receive an $base_field_definitions argument.

It could be argued that the base field definitions are not enough, and that we should receive all the field storage definitions of an entity type (i.e. what EntityFieldManager::getFieldStorageDefinitions() provides), but that would mean we would be able to override in code the properties of a configurable field (which is stored in config), and I'm not sure that we want to go that far.. :)

Spoke to @amateescu on IRC. This was a summary of what we agreed:

• Bundle fields won't be initialisable without a storage definition (conceptually, two different bundles on the same entity type require that storage be 100% consistent, which is the driving motivation for this).
• All things related to storage will not be mutable (like the field type currently is and the target entity type ID), these will come from the storage definition.
• As a result 'create' will disappear and 'createFromFieldStorageDefinition' will replace it as the primary way you would initialise a bundle field.

This should make these definitions more robust, since it would be harder to create an invalid definition, given the constraints for what core supports.

Given that #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info() was raised as a potential blocker, however the \Drupal\entity_test\FieldStorageDefinition class might still be usable for testing purposes to decouple these two issues.

I think we should add a test for the computed bundle field as well to make sure that hook_entity_bundle_field_info can create the computed fields.

A few questions as I've been battling with implementing bundle specific computed fields for a couple of days now and the many issues involved are so spread out it gets quite a bit confusing.

Am I missing something or does not the code comments (after patches/suggestions here) still point to not needing to implement hook_entity_field_storage_info() for computed fields? Not needing that would makes sense to me since computed field values are not actually stored anywhere, and most of the information for that hook seems to be mostly a duplicate of what's given in hook_entity_bundle_field_info() but for the purpose of DB/querying implementations (either that or most of the example code I've seen so far is incorrect/misleading).
The discussion here seems to however indicate implementing hook_entity_field_storage_info() should be required even for computed [bundle] fields?

Somewhat related, #2852067 made it sound like \Drupal\views\Plugin\views\field\EntityField should now be possible to use with all computed fields. That would be awesome for my use as I'm computing simple numeric statistics values for one node type and I want flexibility in how they are displayed.
But, that class only works with computed base fields because storage definitions are still expected to be there to get the column types from (and base fields implicitly provide them).
Modifying that EntityField class to add support for bundle fields is possible (make it look for a 'bundle' property in the Views field definition to get bundle-specific entity field definitions if set, and use #38 to define the bundle fields), but it would need more complex logic changes to actually fully support computed bundle fields without storage, which I've been lead to believe is the expected case for computed fields, no?)

I may be completely wrong here tho, given \Drupal\Core\Field\FieldDefinitionInterface::getFieldStorageDefinition() must return something, and it can not return anything if the storage info hook was not implemented for bundle fields - as it was decided they do not implicitly also represent storage like base fields () -, which is probably the main cause of me being confused about all this.

I'll give the same example as I used earlier when I was walking to @Sam152.

If we don't require bundle fields (computed or not) to also have to define a field storage, then we can get into this situation: you define a bundle field using the string field type for the article bundle, and a different bundle field with the same name but using the text field type for the page bundle. In that case, there is no way for some generic code (e.g. Views, Rules, etc.) to know the properties available for that field.

This is the reason for @Sam152's first bullet point in #44:

Bundle fields won't be initialisable without a storage definition (conceptually, two different bundles on the same entity type require that storage be 100% consistent, which is the driving motivation for this).

Ah, so that meant two bundle fields with the same name are always required to be of the exact same type (but some of their properties may still be of different types between bundles, like target_id).

Allowing the same field be two completely different types across multiple bundles would indeed be overly complicated, and you're saying the hook_entity_field_storage_info() would be used to dictate which type it actually is, as there can be only one storage definition for each entity type and field name combination.
And that is why the $type argument in the new BundleFieldDefinition constructor is suggested to be removed (or perhaps change to instead be a field storage definition?). Field storage definition are gettable from the list of base field definition list passed to the bundle field definition hook, and for computed bundle fields you need to get them via the field type manager.
Makes more sense now, thanks.

Should not all mentions of "if it is not computed" and "unless they are computed" in relation to the bundle field definition hooks/classes be removed then?

Should not all mentions of "if it is not computed" and "unless they are computed" in relation to the bundle field definition hooks/classes be removed then?

If people agree with my explanation above, then yes, we should probably remove references to whether a field is computed or not from this area, because all of them should be treated in the same way.

Here's an example of how I'm using this. Our profile includes a set of pre-defined content types; we do not use config for these as that is a one-off install and features does not allow simple extending of content types.

So in our hero module, we define the hero field storage:

/**
 * Implements hook_entity_field_storage_info().
 *
 * {@inheritdoc}
 */
function jnjone_hero_entity_field_storage_info(EntityTypeInterface $entity_type) {
  $fields = [];

  if ($entity_type->id() == 'node') {
    $fields['hero'] = BundleFieldDefinition::create('entity_reference_revisions')
      ->setName('hero')
      ->setLabel(t('Hero'))
      ->setCardinality(1)
      ->setTranslatable(FALSE)
      ->setSetting('target_type', 'paragraph')
      ->setTargetEntityTypeId($entity_type->id());
  }

  return $fields;
}

Then in our news module we can add an instance of that field to the article:

/**
 * Implements hook_entity_bundle_field_info().
 */
function jnjone_news_entity_bundle_field_info(EntityTypeInterface $entity_type, $bundle, array $base_field_definitions) {
  $fields = array();

  if ($entity_type->id() == 'node' && $bundle == 'news_article') {

    $fields['hero'] = BundleFieldDefinition::create('entity_reference_revisions')
      ->setLabel(t('Hero'))
      ->setRevisionable(TRUE)
      ->setCardinality(1)
      ->setSetting('target_type', 'paragraph')
      ->setSetting('handler', 'default')
      ->setSetting('handler_settings', [
        'target_bundles' => [
          'hero_image' => 'hero_image'
        ]
      ])
      ->setDisplayOptions('view', [
        'label' => 'hidden',
        'weight' => -20,
      ])
      ->setDisplayConfigurable('view', TRUE)
      ->setDisplayOptions('form', [
        'weight' => -1,
        'type' => 'entity_reference_paragraphs',
        'settings' => [
          'title' => 'Hero',
          'title_plural' => 'Heros',
          'edit_mode' => 'open',
          'add_mode' => 'dropdown',
          'form_display_mode' => 'default',
          'default_paragraph_type' => '_none',
        ]
      ])
      ->setDisplayConfigurable('form', TRUE)
    ;
  }

  return $fields;
}

We use the same methodology to create all the fields on the news article that are extra to the base fields, but I haven't included them here for simplicity. We can do the same to add the same hero field to any other content type. Further, all the fields on the hero_image are created in the same way as we don't change the base fields from paragraph module.

You can see that we only have one target bundle for hero's ATM, but when we have built that we can add it to the handler settings.

I shouldn't be able to define ->setRevisionable(TRUE) & ->setCardinality(1) in the Bundle but at patch 2 that was needed.

I shouldn't be able to define ->setRevisionable(TRUE) & ->setCardinality(1) in the Bundle but at patch 2 that was needed.

Those are exactly the things that stood out while reading the code, and your conclusion is correct that you shouldn't be able to do that at the field definition level since those are storage-related properties.

I don't agree that a computed bundle field should require a field storage definition. While I get that a common field storage definition is useful for some modules, declaring it comes with a cost. First of all it will actually create the respective database tables/columns, which is certainly not what you want. You can work around that by setting hasCustomStorage() to TRUE but first of all, that really is a workaround and not the correct semantic of "custom_storage" - because there is in fact no storage at all in this case - and secondly you will still have it in the list of all storage definitions and modules may not always exclude definitions with custom storage - and depending on what they do with the storage definitions that may very well be justified. So just to make the life of a hand-picked list of very generic modules easier we would be introducing a workaround that can lead to weird and unexpected behavior elsewhere.

The only way that this would be feasible in my opinion is if we introduce the notion of "computed" on the field storage definitions themselves. But that is somewhat paradoxical in terms of semantics. So really think it is correct to not have a field storage definition for something that will not be stored and that Views, etc just have to live with the consequences of that.

Also see the linked issue for a way to let Views deal with this issue. That would in fact forces every views field to be specific to a certain bundle, so if you have computed bundle fields on different bundles with the same name but of a different type you cannot use the same views field name for it, but you can still use computed bundle fields in general.

Addressing all the comments since the last patch.

#39

1. I don’t think we're ready for that. I would say this is the first step towards that issue but doesn't resolve it.
2. I've tried to clarify this.
3. This part has been rewritten.
4. It's a a heavily related interface, why wouldn’t we add it as something people should also be aware of?

#41

1. Done.
2. This part has been rewritten.
3. Done. I think by keeping the storage definition as a property, we don’t have to copy it’s values, we can simply satisfy the relevant storage-only parts of the interface by calling off to the storage directly. What do you think to this approach? For example ::getName becomes return $this->getFieldStorageDefinition()->getName() instead of return $this->definition['field_name’]. I think this makes more sense because we don’t actually have setters for things like name anymore.
4. Setter removed.
5. Done.
6. Done.
7. Done.
8. Done.

#45
I don’t think this should be in scope.

#48/#49
Like 45, I think a different issue to properly explore computed fields would make the most sense.

Cross-posted with #52/#53.

Right now we don't have any computed fields that don't actually have an associated storage definition. Computed base fields have one because a BaseFieldDefinition satisfies both interfaces and FieldDefinitionInterface enforces that a storage definition always be available:

  /**
   * Returns the field storage definition.
   *
   * @return \Drupal\Core\Field\FieldStorageDefinitionInterface
   *   The field storage definition.
   */
  public function getFieldStorageDefinition();

I think these discussions are important but don't actually block this issue. If at some point in the future the way fields were defined allowed storage definitions to be optional, BundleFieldDefinition could be adjusted to account for definitions with and without an associated storage definition.

Can we agree to discuss storage-less base/bundle field definitions in a follow-up?

@tstoeckler, if computed bundle fields should not have a field storage, what would \Drupal\Core\Field\FieldDefinitionInterface::getFieldStorageDefinition() return?

Here's a review for #54:

1. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -1867,7 +1869,7 @@ function hook_entity_bundle_field_info(\Drupal\Core\Entity\EntityTypeInterface $
   +    $fields['mymodule_text_more'] = BundleFieldDefinition::create('string')
   

   This not correct anymore :)

2. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,302 @@
   +   * Create a new bundle field definition.
   

   Creates ;)

3. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,302 @@
   +  public static function createFromFieldStorageDefinition(FieldStorageDefinitionInterface $storageDefinition) {
   

   We need to add this parameter to the doxygen block.

4. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,302 @@
   +    $field_definition->type = $storageDefinition->getType();
   

   We don't have a $type variable anymore.

5. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,302 @@
   +   * @return string
   

   Should be {@inheritdoc}.

6. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,302 @@
   +   * @return static
   

   Should be @return $this.

7. +++ b/core/modules/system/tests/modules/entity_schema_test/entity_schema_test.module
   @@ -62,7 +63,10 @@ function entity_schema_test_entity_field_storage_info(EntityTypeInterface $entit
   +      ->setTargetBundle($bundle);
   

   We shouldn't need to set the bundle here but make EntityFieldManager::buildBundleFieldDefinitions() do it for BundleFieldDefinition just like it does for BaseFieldDefinition.

if computed bundle fields should not have a field storage, what would \Drupal\Core\Field\FieldDefinitionInterface::getFieldStorageDefinition() return?

That is a valid question! I wonder what your response to the rest of #53 is, though. I really don't think the current approach is viable as is.

if computed bundle fields should not have a field storage, what would \Drupal\Core\Field\FieldDefinitionInterface::getFieldStorageDefinition() return?

Maybe for computed fields it could return some kind of NullFieldStorageDefinition implementation? It would return NULL/[] in ::getMainPropertyName()/code>, <code>::getSchema()/code>, <code>::getColumns()/code>, <code>TRUE in ::hasCustomStorage() (and FALSE in ::isBaseField()).

I did a quick test by copying most of the \Drupal\Core\Field\BaseFieldDefinition methods into such a new class (forced it to be created using a \Drupal\Core\Field\FieldDefinitionInterface instance) and at first glance it works well with computed bundle fields, at least through Views when using the #2981047 patch.

Addressing feedback from #56.

Maybe for computed fields it could return some kind of NullFieldStorageDefinition

@amateescu had some similar ideas on slack. I've split out #2986634: Discuss the role of field storage definitions for computed fields., lets move that discussion over there so we're not blocking this issue. As per #55, we don't need all the answers to those questions here.

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -1867,12 +1869,12 @@ function hook_entity_bundle_field_info(\Drupal\Core\Entity\EntityTypeInterface $
+    $storage_definitions = hook_entity_field_storage_info($entity_type);

This is not correct. This should be a re-usable code example.

This is not correct. This should be a re-usable code example.

Can you explain this a bit further? What would you suggest the code snippet be and why is the current one incorrect?

A: I don't understand #60 and #62 either. It actually makes a lot of sense to call the hook_entity_field_storage_info() implementation explicitly so as not to duplicate the definition of the field storage definition (no pun intended).

B: Having thought about this for a while, I'm fine with the current direction of the patch. I will open a follow-up to introduce the notion of computed field storage definitions in order to properly support computed bundle fields.

C: Here's a review of the actual patch:

1. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,304 @@
   +  public static function createFromFieldStorageDefinition(FieldStorageDefinitionInterface $storageDefinition) {
   ...
   +    $field_definition->fieldStorageDefinition = $storageDefinition;
   +    $field_definition->itemDefinition = FieldItemDataDefinition::create($field_definition);
   

   While I like the idea of a createFromStorageDefinition() method, we also have create(), createFromDataType() and createFromItemType() methods, that we inherit. So it might make sense to shuffle parts of this around into create() or something, not sure.

   At least we should add test coverage for those methods.

2. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,304 @@
   +    $field_definition = new static([]);
   

   The [] is the default value, so could be omitted.

3. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,304 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setDefaultValueCallback($callback) {
   

   This is not inherited.

4. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,304 @@
   +  public function isTranslatable() {
   +    return !empty($this->definition['translatable']);
   +  }
   

   I wonder why this is not being forwarded to the field storage definition?

Opened #2986836: Support computed bundle fields by adding the notion of computed field storage definitions.

@tstoeckler, I've re-read #53 a few more times and I think that introducing the notion of "computed" at the field storage level is the proper way to go, as you seem to agree with in #63.

The concept of "computed field storage" might sound paradoxical in terms of semantics, but that's only because we haven't found a better term for "field storage" during Drupal 8's development cycle. The actual meaning of field definition and field storage definition is (IMO) something like this:

- field storage definition: the properties of an entity field that are applied at the entity type level (i.e. across all bundles). Some of these properties are simply defaults that can be overridden per-bundle (e.g. the field label, field settings, etc.) while others are "fixed" (e.g. field name, field type, target entity type, cardinality, etc.)

- field definition: the properties of an entity field that are applied only at the bundle level (e.g. bundle-specific label for the field, bundle-specific settings for the field, etc.)

About #63.4: I think content translation can be enabled per-bundle, so I don't see why would not want to expose that flexibility here. Except if you meant that we should also ask the field storage whether it is translatable or not.

Aren't #2986634: Discuss the role of field storage definitions for computed fields. and #2986836: Support computed bundle fields by adding the notion of computed field storage definitions duplicate of each other?

What I meant was after applying this patch, reading hook_entity_bundle_field_info and hook_entity_field_storage_info side by side doesn't make sense. For starter we don't define new field stroage definition in hook_entity_field_storage_info so we need to update hook_entity_field_storage_info and initiate a proper field stroage definition for node entity type.
As far as duplicating the field storage definition is concerned, we should be calling \Drupal\Core\Entity\EntityFieldManager::getFieldStorageDefinitions instead of hook_entity_field_storage_info becuase we want to give people a chance to alter the storage using hook_entity_field_storage_info_alter.

I think content translation can be enabled per-bundle, so I don't see why would not want to expose that flexibility here.

Hmm... seems a bit arbitrary to allow it for translatability, but not for e.g. the name.

Except if you meant that we should also ask the field storage whether it is translatable or not.

Good point! That should be fixed.

we should be calling \Drupal\Core\Entity\EntityFieldManager::getFieldStorageDefinitions

Not sure about that, I think that would lead to recursion. Even if not, the bundle definitions themselves can also be altered later, so having them rely on already-altered information while again being altered doesn't really sound like such a great idea to me, to be honest.

Also:

I think that introducing the notion of "computed" at the field storage level is the proper way to go

Yay, that's awesome!!!

Hmm... seems a bit arbitrary to allow it for translatability, but not for e.g. the name.

I have no idea why translatability can be enabled/disabled per-bundle, I haven't worked on that part (or with translations in general) at all, but I don't see how the machine name of a field could be different per bundle. It is a basic architectural design of the field system that this code will always return true:

$article = Node::load(1);
$page = Node::load(2);

$article->some_field->getFieldDefinition()->getFieldStorageDefinition)->getType() === $page->some_field->getFieldDefinition()->getFieldStorageDefinition)->getType();

Not sure about that, I think that would lead to recursion.

Can you elaborate?

Great discussion so far thanks for swarming on this issue! Exciting to come this far in a short amount of time :). Responding to all the comments since the last patch:

#62
I think @tstoeckler nailed this in #63.A.

#63
A: Great, I agree!
B: Woohoo! Consensus is a beautiful thing in the core queue :)
C.1: Good point. This also illustrated the only way to set the storage definition was via the new factory. I've resolved that with a setter and updated the test coverage to test every method with every factory! I think that's a big improvement.
C.2: Done, good catch.
C.3: Good catch, fixed.
C.4: I believe @plach knows the most about this, but from what I've learned in the past, translatability never affects the table layouts/storage by design. We have systems like content_translation which create BaseFieldOverride entities that allow the user to flip translatability with a checkbox, so this is indeed something which can be turned on or off at the definition level and the result is changes in the semantics of how that field is treated when new translations are created. I might be corrected on this, hopefully this can get across @plach's desk too.

#64
Nice, look forward to seeing that.

#65
Agree with these points.

#66
1. There is nothing that says all the example API hooks must form a functioning module/implementation. I believe that hook could happily be fixed in #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info().
2. Exploring this is out of scope, right now we're not enforcing the method the user takes to get a reference to the storage definition. We might want to do that in the future.

#67
@see response to #63.
Agree re: alters.

#69
@see response to #63. I believe flexibility on this is simply a feature that the content translation UIs expose.

I have updated the change record with the updated consensus.

Re #69: Sorry, I mixed up label vs. name. I now realize that e.g. label and description already use the same pattern as translatable via inheritance from ListDataDefinition, so my point about inconsistency is moot.

However, the point about asking the field storage definition for translatability still needs to be fixed. Re #71: Both FieldConfig and BaseFieldOverride (through their common ancestor FieldConfigBase) have the following:

  public function isTranslatable() {
    // A field can be enabled for translation only if translation is supported.
    return $this->translatable && $this->getFieldStorageDefinition()->isTranslatable();
  }

So I think we should implement isTranslatable() analogously here.

Re patch in #71: Nice!!! Really great idea with the setFieldStorageDefinition() class. Also really great work on the tests. Read through the patch again and couldn't find anything to complain about, but marking needs work for the translatability thing from above.

There is nothing that says all the example API hooks must form a functioning module/implementation.

That's not really helpful. Sorry, I don't understand the relationship between this issue and #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info()

1. +++ b/core/modules/system/tests/modules/entity_schema_test/entity_schema_test.module
   @@ -62,7 +63,9 @@ function entity_schema_test_entity_field_storage_info(EntityTypeInterface $entit
   +    $custom_bundle_field_storage = entity_schema_test_entity_field_storage_info($entity_type)['custom_bundle_field'];
   

   Can we call \Drupal::service('entity_field.manager')->getFieldStorageDefinitions($entity_type) instead of entity_schema_test_entity_field_storage_info($entity_type)?

Re #74: See my #67:

the bundle definitions themselves can also be altered later, so having them rely on already-altered information while again being altered doesn't really sound like such a great idea to me, to be honest.

If a module wants to alter field storage definitions I think it may very well want to alter bundle field definitions as well. So I think we explicitly shouldn't call into the EntityFieldManager in hook_entity_bundle_info(). I think the idea is literally to avoid copy-paste between hook_entity_field_storage_info() and hook_entity_bundle_info(), nothing else.

Since the bundle field definition calls out to the storage definition anyway for various things altering it will already have an effect on the bundle definition anyway. Only e.g. if a module alters the label of a field storage definition it will not automatically propagate to the bundle field definition, but it's not clear to me that that would be expected in the first place, without the module explicitly altering the bundle field definitions as well.

Re #73, good point, the interface states:

Returns whether the field supports translation.

So I think updating is correct. Attached is a patch for that.

Awesome, this looks great to me. I'll leave @amateescu to RTBC this, though. Also I guess we still need some confirmation from @jibran that they are fine with this.

Yeah, it is fine. It is a small step to mature this API which has been WIP for a long time now but we still have a lot to figure out and bikeshed. ;-) We all kinda have different ideas how it should/would work I'd like to have @plach or @Berdir's opinion on the patch before we commit.

1. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,323 @@
   +class BundleFieldDefinition extends ListDataDefinition implements FieldDefinitionInterface {
   

   I wonder whether the Bundle prefix is actually needed for this class.

   Since FieldDefinitionInterface represents the per-bundle properties of a field, shouldn't this be simply called FieldDefinition? That would also be in line with the field storage part of the API, as proposed in #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info().

2. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,323 @@
   +   *   The object itself for chaining.
   ...
   +   *   The object itself for chaining.
   

   These can be removed :)

3. +++ b/core/lib/Drupal/Core/Field/BundleFieldDefinition.php
   @@ -0,0 +1,323 @@
   +    throw new \Exception('BundleFieldDefinitions do not currently have an override config entity.');
   

   BundleFieldDefinitions is not an actual class name, so I would rewrite this with something like: "Field definitions do not currently ...".

4. +++ b/core/tests/Drupal/Tests/Core/Entity/BundleFieldDefinitionTest.php
   @@ -0,0 +1,377 @@
   + * @group Entity
   ...
   +class BundleFieldDefinitionTest extends UnitTestCase {
   

   This test looks excellent!

   Let's add @group field as well :)

Feedback from #79. I had a similar thought that this was not really bundle specific, but the only thing that gave me pause was the fact we were already covering the base field use case. I like the flexibility of keeping this naming FieldDefinition for the moment. It could always be subclassed if it needed some specific utility for bundle fields.

Whoops, patches need to be against the correct branch :)

Fixing test.

Rerunning for testbot fail and fixing cs error.

From #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info()

We should add a dedicated field storage definition class which allows modules to define field storage definitions in hook_entity_field_storage_info() without having a config-field easily.
When doing so, we might want to have the FieldDefinition class extend it, but that will have to wait for #2268049: Decouple field definitions from typed data definitions or be handled there then.

So FieldDefinition class suppose to extend FieldStorageDefinition, does this mean we are blocked on that or can it be done even after this patch?

I stumbled upon #2315237: Rename FieldDefinition to BaseFieldDefinition and FieldDefinition mentioned in IS of #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info() is now called BaseFieldDefinition so no I don't think #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info() is blocking this but it is all so confusing.

Fixing tests for the new class name.

Lots of work going on here and trying to figure out how to update my BundleFieldDefinitions to match this latest patch.

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -1867,12 +1869,12 @@ function hook_entity_bundle_field_info(\Drupal\Core\Entity\EntityTypeInterface $
+    $storage_definitions = hook_entity_field_storage_info($entity_type);

This assumes the FieldStorageDefinition comes from the same module. This might not be the case. The storage could be defined in multiple modules.

The simplest alternative I could find was

  $field_manager = \Drupal::service('entity_field.manager');
  $storage_definitions = $field_manager->getFieldStorageDefinitions($entity_type->id());

Would be good if you could just do

  $storage_definitions = $entity_type->getFieldStorageDefinitions();

but it is all so confusing

@jibran, agreed!

What happens if you try and create two fields with the same storage definition?

Having had a night to sleep on it I have been able to get my project back running using the latest patch.

I needed to have a FieldStorageDefinition class to be able to do

/**
 * Implements hook_entity_field_storage_info().
 *
 * {@inheritdoc}
 */
function jnjone_tags_entity_field_storage_info(EntityTypeInterface $entity_type) {
  $fields = [];

  if ($entity_type->id() == 'node') {
    $fields['tags'] = FieldStorageDefinition::create('entity_reference')
      ->setName('tags')
      ->setLabel(t('Tags'))
      ->setCardinality(FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED)
      ->setTranslatable(FALSE)
      ->setSetting('target_type', 'taxonomy_term')
      ->setTargetEntityTypeId($entity_type->id());
  }

  return $fields;
}

So I have created a patch over on #2280639; this makes that issue a blocker for this one.

I needed to create a new patch for Composer to be able to apply after #2280639

The issues aren't blocking each other. A field definition is still useful stand-alone when used with storage provided by a base field. There is also other ways to implement field storage without a dedicated builder class, like the one in contrib for example.

Reuploading #90 for clarity.

Agreed with @Sam152, this can go in separately from #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info() because existing code can still use their custom classes.

What we really now is another opinion from an entity maintainer for the patch in #95 , let's try @Berdir first :)

A field definition is still useful stand-alone when used with storage provided by a base field.

But if its a base field it would already be there, so you wouldn't be adding it to the bundle - unless you can at the same base field to a bundle multiple times? This was my question in #91

There is also other ways to implement field storage without a dedicated builder class, like the one in contrib for example.

What is that example, I couldn't spot it in in the history?

because existing code can still use their custom classes.

Oh I know the example now - here's a link for anyone else https://cgit.drupalcode.org/entity/tree/src/BundleFieldDefinition.php?h=...

I guess, it is not truly blocked, it's just infinitely more useful if #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info() makes it in as well.

I have added a patch with the amendment I proposed in #91 as this was what @jibran and @tstoeckler said in #66 and #67.

+++ core/lib/Drupal/Core/Entity/entity.api.php	(date 1532600073000)
@@ -1869,7 +1869,8 @@
-    $storage_definitions = hook_entity_field_storage_info($entity_type);
+    $field_manager = \Drupal::service('entity_field.manager');
+    $storage_definitions = $field_manager->getFieldStorageDefinitions($entity_type->id());

Don't know what I'm doing different, but this should be the only diif in the patches.

@dwkitchen, yes, it is more useful when we also have a field storage builder, that's why we have an issue for it. "Not blocking" meanings both can be worked on in parallel and will both hopefully be committed.

@tstoeckler explicitly disagreed with calling getFieldStorageDefinitions in #67.

unless you can at the same base field to a bundle multiple times

When you add a bundle field matching a base field storage definition, the bundle definition becomes the canonical definition

for that bundle only

. You could for example have a base field with the label "Foo" and override the label to be "Bar" for one bundle only.

Not sure why the interdiff is being messed up, right now they are really hard to read. There are steps documented here if that's helpful.

In the meantime, lets please be aware of scope. Adding this building block doesn't mean all work has to stop on this once it goes in. We have the option to continue to refine and build on the various docs and ancillary classes in the future.

Needs review for #95.

I reviewed the patch again thoroughly and I think it's ready to go. We are at the beginning of the 8.7.x cycle so we have enough time to spot any possible regression in contrib.

Another review from other entity maintainers and @bojanz would still be very helpful, but I don't think it needs to block progress in this area that has been neglected since 8.0.0 :)

Re-uploading #95 again so it's clear which patch is RTBC.

I don't think it needs to block progress in this area that has been neglected since 8.0.0 :)

It's about damn time.

Edit: It is pointed to me that my comment was a bit harsh. I was in no way criticizing people for neglecting this part of the core. I have a huge respect for all the entity/field API maintainers & contributors and I would never criticize anyone on not working on something. It was just a joyous comment.

Edit 2: It is also pointed to me that my comment can also be offensive to religious people so once again I amended my comment.

@amateescu Thanks again for the review.

I'm a bit sad that we abandoned the pattern promoted by entity_test before (having a BundleFieldDefinition that's a parallel to BaseFieldDefinition, providing both the definition and the storage). It means that Commerce and all users of Entity API's Bundle Plugins will now be on an API that's different to the core one. But it's not the end of the world, we'll just keep our \Drupal\entity\BundleFieldDefinition.

There is one DX consequence that should be addressed somewhere, the need to rewrite the docblock for FieldableEntityInterface::bundleFieldDefinitions():

   * Provides field definitions for a specific bundle.
   *
   * This function can return definitions both for bundle fields (fields that
   * are not defined in $base_field_definitions, and therefore might not exist
   * on some bundles) as well as bundle-specific overrides of base fields
   * (fields that are defined in $base_field_definitions, and therefore exist
   * for all bundles)

It says that it can be used both for overriding base fields, and for providing bundle-specific fields. After this patch, it only makes sense to use it for overriding base fields, since bundle-specific fields now require a storage that can only be provided from a hook. So it makes sense for both the storage and the definition to be provided from the appropriate hooks.

@bojanz, the class we are introducing here does not mention "bundle" in its name, it's only a builder class for FieldDefinitionInterface, so I think we can still add something like a "BundleFieldDefinition" class in core, which would work like the one provided by the Entity module in contrib. Would that be too confusing naming wise?

Maybe we should first discuss this whole problem space properly in #2346347: Finalize API for creating, overriding, and altering code-defined bundle fields, and only implement the decisions taken there in sub-issues like this one..

I think #41 to #44 touch on some of the reasoning for making the builder a definition only, I think it boiled down to: two field definitions attached to two different bundles require the same storage definition, so storage related things aren't mutable and instead reference a common item of storage.

I don't think necessarily introducing a definition builder would stop you from creating builders which are composites of both storages and definitions. Internally BundleFieldDefinition/BaseFieldDefinition could maintain their own individual definition and storage objects if we went ahead with this issue and #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info().

Doing so would allow us to continue to use patterns like in entity.module and BaseFieldDefinition where appropriate (ie, we are implementing storage hooks behind the scenes for users) or allow them to be used directly where scenarios favour individual storage/definition implementations.

It would be good to discuss further.

I think keeping that level of granularity also has some parallels to #1426804: Allow field storages to be persisted when they have no fields.:

Field API deletes fields when the last instance is deleted.

...

This makes it hard for modules to expose fields to a website, and build business-logic functionality tied to that field, without also locking at least one bundle into using (instantiating) that field.

So, maintaining the granularity (optionally) seems a bit analogous to the API version of persist_with_no_fields.

Added IS template.

There is a lot of shared code between \Drupal\Core\Field\BaseFieldDefinition and \Drupal\Core\Field\FieldDefinition. For example ::setDisplayOptions() and ::getDefaultValueCallback()

It makes me wonder if there should be an abstract parent of FieldDefinition and BaseFieldDefinition. Also with the current naming I'm left wondering why BaseFieldDefinition doesn't extend from FieldDefinition.

Leaving at RBTC as there is no actionable comment - just thoughts that might prompt some changes by the patch authors.

Thanks for the review @alexpott!

My take on that is, BaseFieldDefinition is equally a definition and a storage definition. Once we have #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info(), there will probably also be a lot of code that is duplicated there too. I think an ideal implementation would be that BaseFieldDefinition would be a composite of both, implement both interfaces and delegate its own methods to each where applicable, to definition and storage objects managed internally.

If #2943899: Moderation state field cannot be updated via REST, because special handling in ModerationStateFieldItemList lands before this, we should update the patch here to also apply the "attempt 1" patch at #2943899-44: Moderation state field cannot be updated via REST, because special handling in ModerationStateFieldItemList.

Loving the work being done here, nice job!

Re #103 Would it be possible to, somewhere down the line, make both BaseFieldDefinition and BundleFieldDefiniton extend or use FieldDefinition to share that code and share some storage logic from either a trait or by having a storageDefinition property populated in the constructor that then leads to the same class tree but for storage?

E.g.:

BaseFieldDefinition
- extends FieldDefinition
- has $fieldStorage property set to BaseFieldStorage (extends FieldStorage)
- Implements FieldStorageDefinitionInterface with all methods calling the method on $fieldStorage

BundleFieldDefinition
- extends FieldDefinition
- has $fieldStorage property set to BundleFieldStorage (extends FieldStorage)
- Implements FieldStorageDefinitionInterface with all methods calling the method on $fieldStorage

Just a reroll for now

This looks great, I read the whole discussion and I really like the solution we came up with: retrieving storage definitions is a neat way to minimize the redundancy implied by having to implement two hooks/methods. If I understand correctly the main use case here would be to override existing definitions in code, while to define a new bundle field from scratch we would implement a BundleFieldDefinition complementing BaseFieldDefinition. However it would important to preserve the ability to ensure that two bundle fields with the same machine name will always share the same storage definition. Anyway, I'm wondering whether #2980922: Applicable bundle field definitions defined in code should not have to manually implement hook_entity_field_storage_info. could be the right place to do this.

Regardless of where we actually do this, I think that would be the place to address the questions @alexpott raised in #108: on one hand I can see us implementing both BaseFD and BundleFD as wrappers around a storage + field definition pair via composition, OTOH I can also see how a base/bundle field definition is a field definition having a storage definition associated, more or less what @kristiaanvandeneynde is suggesting in #113.

Here are a few nitpicks as dressing ;)

1.       +++ b/core/lib/Drupal/Core/Entity/entity.api.php
         @@ -1867,12 +1869,12 @@ function hook_entity_bundle_field_info(\Drupal\Core\Entity\EntityTypeInterface $
         +    $storage_definitions = hook_entity_field_storage_info($entity_type);
       

   Can we change this to call mymodule_entity_field_storage_info($entity_type); for consistency with the rest of the code? This would also have the advantage of not forcing us to keep the two examples in sync.

2.       +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
         @@ -0,0 +1,321 @@
         + * bundle field definition may also override the definition of an existing base
         + * field definition on a per bundle basis.
       

   Can we add a couple of lines quickly mentioning the difference between this and BaseFieldOverride? I guess we would say that the former is for code-driven overrides, while the latter uses config for them.

3.       +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
         @@ -0,0 +1,321 @@
         + * Bundle fields can defined in code using hook_entity_bundle_field_info() or by
       

   Typo: "can be"

4.       +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
         @@ -0,0 +1,321 @@
         + * implementing the
         + * \Drupal\Core\Entity\FieldableEntityInterface::bundleFieldDefinitions() method
         + * when defining an entity type. All bundle fields require an associated storage
       

   Since all classes will implement FieldableEntityInterface::bundleFieldDefinitions() one way or another, even if they return an empty array, what about changing this to say, something like:

   Bundle fields can defined in code using hook_entity_bundle_field_info() or via the \Drupal\Core\Entity\FieldableEntityInterface::bundleFieldDefinitions() method when defining an entity type.
       

5.       +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
         @@ -0,0 +1,321 @@
         + * overriding a base field or it may be manually implemented with
       

   IMO "provided via" would read better than "implemented with" here.

6.       +++ b/core/tests/Drupal/Tests/Core/Entity/FieldDefinitionTest.php
         @@ -0,0 +1,390 @@
         +    $storage_definition->getName()->willReturn('Test field name');
       

   The "name" property holds the machine name, it would be more correct to use test_field_name here.

7.       +++ b/core/tests/Drupal/Tests/Core/Entity/FieldDefinitionTest.php
         @@ -0,0 +1,390 @@
         +   * Create a bundle field using a specified factory.
       

   "Creates" :)

It seems we also need to address #110, unless we create a dedicated follow-up for that.

If my understanding in #115 is correct it would be good to expand the IS to provide some more details around the solution.

We also need a CR for this.

Liar

However it would important to preserve the ability to ensure that two bundle fields with the same machine name will always share the same storage definition.

Do we need to document this somewhere?

Let's create follow up for #110. The changes required for that seems out of scope here. Addressed rest of the review from #115.

Do we need to document this somewhere?

Yes, we should definitely mention that in the PHP docs.

Added the follow up here: #3012944: [PP-1] Use a FieldDefinition to customise the required status of the moderation_state field conditionally per bundle..

Re: #108 and #115, I had a similar thought in #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info():

Alterations to BaseFieldDefinition should not be in scope here. Since a base field definition is both a FieldDefinitionInterface and a FieldStorageDefinitionInterface, it probably makes more sense for a base field to be composed of these two classes, not extending either one of them.

My take on this so far is that both FieldDefinition and FieldStorageDefinition are useful stand-alone components. While discussing the role of (Base|Bundle)FieldDefinition, we're not really proposing any public API changes, so really the question is around implementation details: should either make some use of these other two primitives?

Since it's purely a discussion about implementation, someone could easily evaluate and experiment with composition or inheritance based approaches, but I don't think that necessarily has to block either this issue or #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info().

[...] I don't think that necessarily has to block either this issue or #2280639: Add the FieldStorageDefinition class to define field storage definitions in hook_entity_field_storage_info().

I agree, let's fix that last bit of documentation mentioned in #119 and get this in.

Thanks for the reviews everyone. Adding some docs as requested.

Let's get this in.

This will go a long way towards a sane DX for working with bundle fields. +1

Saving credits

Committed 98d7171 and pushed to 8.7.x. Thanks!

I also published the CR, onto the follow-ups now!

Thanks for reviewing @plach! :)

You're welcome :)