Reconsider whether FieldDefinition should implement FieldStorageDefinitionInterface

Whatever the outcome, I think it's a good thing to allow us to re-evaluate this at this point.

Yes, I'm not fond either of FieldDefinition implementing both interfaces. AFAIK the only (but very valid) reasons are
- DX for defining fields through code in EntityType::baseFieldDefinitions() : for most fields, providing just one definition (the FSDI, which aloso contains the info to act as a FDI, the same for all bundles) is enough. I guess we could still keep that DX sugar while still internally creating, out of those FSDI, separate FDI objects for each bundle.
- The reason then becomes performance - the current formulation in HEAD tries to minimize the # of objects we need to instantiate at runtime.

#2143291: Clarify handling of field translatability got in, we can unpostpone ?

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

I think we need this sorted out before beta, I am afraid it would be a big API breaking change...

I am wondering whether a possible solution to cope with the DX regression implied by making FD implement just FDI, could be adding a method to get a (wrapped?) FSD from a FD and keep defining it with all the properties in just one object as we do now.

I've always struggled with the notion of "base fields" as implemented by the current FieldDefinition class as a weird and confusing singularity with respect to how the rest of the ecosystem works with FieldDefinitionInterface objects.
The recent evolutions (the formalization of FieldStorageDefinitions, the "bundle overrides" being worked at #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields) only strengthen that feeling.

My current thoughts on this (and from an IRL discussion, @amateescu shares the same):

Those FieldDefinition objects are primarily handy shortcuts when writing your entity type's baseFieldDefinitions(), that lets you specify in one single definition object :
- a field storage definition, seen by the ecosystem as an FSDI
- and a "bundle field definition" on no specific bundle, seen by the ecosystem as "an FDI present on all existing bundles"

That's a very valid feature, baseFieldDefinitions() would be very painful to write otherwise.
But then it is currently painful for the rest of the system to have to deal with those weird animals at runtime.

In such a case IMO, the "shorthand" aspect that is valid at definition time should not leak out to the rest of the consuming runtime APIs.
Meaning, when building the "registry of FDI objetcs for a given $bundle", EM::buildBundleFieldDefinitions() would expand those shorthand objects into actual FDI objects, with an actual bundle.

API-wise :

- The "shorthand" class itself does not implement FDI, it's not an actual "definition o a field on a bundle".
It thus wouldn't be named FieldDefinition, but BaseFieldDefinition.
--> In MyEntityType::baseFieldDefinitions(), I create and return BaseFieldDefinition objects - makes sense.

- A BaseFieldDefinition is "the definition of one FSDI + a shorthand for definition FDIs on all bundles". It could still implement FSDI directly, I guess. No need to create a separate object for "the FSDI of this BaseFieldDefinition".

- The FieldDefinition class, implementing FDI, is the one used for the actual FDIs expanded for each bundle at runtime.

- FDI objects, manipulated by all the runtime ecosystem, are "field definitions on a bundle". No more "FDIs with no bundle because all bundles" singularity.

- If you need a list of "fields that are constituent of the Entity Type and appear across all bundles no matter what" (that's a need for Rules, if I understand correctly), you still have EM::getBaseFieldDefinitions(), that returns BaseFieldDefinition objects.

- #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields would then mean that all FDIs are in fact ConfigEntities. The ones expanded out of BaseFieldDefinition objects simply happen not to be ->save()d and written into config by default, but they *can* be (and you then have an override).
--> might go as far as FDI extends ConfigEntityInterface ?

The above IMO is an API design that faithfully represents what our actual concepts are.

I do realize that there's some perf overhead to be watched for :
- more objects creation in EM::buildBundleFieldDefinitions()
- more objects serialization on cache write, more unserialization on read, and after that more objects in memory at runtime (when no overrides though, those additional objects would be just a $bundle string and a pointer to their source BaseFieldDefinition)

But I would rather try to actually measure the perf cost and find ways to mitigate it on top of a sound & consistent API structure, rather than having confusing optimization shortcuts baked in the API design.

I totally agree with the main points of #7 and the idea that

- #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields would then mean that all FDIs are in fact ConfigEntities. The ones expanded out of BaseFieldDefinition objects simply happen not to be ->save()d and written into config by default, but they *can* be (and you then have an override).
--> might go as far as FDI extends ConfigEntityInterface?

is pretty exciting.

If we get agreement on this - then I would suggest postponing #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields on this and making this beta blocking.

Actually I think that we could do #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields first as long as we agree the class names and config file names. And do this as part of the beta - yes we'd want to get this in early but the only modules we'd break as ones providing new Entity types or extending ContentEntityDatabaseStorage. I don't think this change falls under "stable data model and stable critical APIs" criteria for beta blockers. (Of course Berdir or yched might prove me wrong :) )

However I do think the concept of FieldDefinition and FieldDefinitionStorage and their separation is critical and it will be great when both FieldAPi and EntityFieldAPI both enforce this distinct through the implementations - rather than having just FieldAPI do it.

And do this as part of the beta - yes we'd want to get this in early but the only modules we'd break as ones providing new Entity types or extending ContentEntityDatabaseStorage.

Isn't this pretty much exactly the definition of the content storage model though?

Yes this will change how you define the content storage model ie...

  /**
   * {@inheritdoc}
   */
  public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    $fields['nid'] = FieldDefinition::create('integer')
      ->setLabel(t('Node ID'))
      ->setDescription(t('The node ID.'))
      ->setReadOnly(TRUE)
      ->setSetting('unsigned', TRUE);

    $fields['uuid'] = FieldDefinition::create('uuid')
      ->setLabel(t('UUID'))
      ->setDescription(t('The node UUID.'))
      ->setReadOnly(TRUE);
 //....

There will be a new class here - it won't be FieldDefinition - but the model itself show not change. But the methods should all be the same. And there will be internal change in the EntityManager but I think that should be about it.

So, I think that is actually pretty much the definition of a beta blocker, if it's critical. The question for me is whether it's actually critical (I struggle to make a case), and how much time it would take.

I think we should proceed with #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields since this creates a BundleFieldDefinition object and goes a lot of the way there. This issue could then discuss if we want to change getFieldDefinitions() to always return objects that implement BundleFieldDefintitionInterface objects and whether we want to merge FieldDefintionInterface and BundleFieldDefinitionInterface together and rename the FieldDefinition object to something like BaseFieldDefinitionBuilder.

As to whether this is a critical beta blocker - the more I think about it I think that with #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields we're in a situation we could live for D8. I think if we do the simple renames and merges this won't be a lot of work. But actually trying to remove FieldDefinition and replace it with a builder that implements both FSDI and FDI and then decomposing that into 2 run time objects feels like a lot of work.

My point is, I'd really like to avoid introducing BundleFieldDefinitionInterface in addition to FieldDefinitionInterface, IMO that is adding awkward complexity to work around a flawed model, adds more confusion on top of the current confusion :-/

Fine if BFDI is added as an intermediate step, but it would be sad if we shipped with both.
FWIW, I'm willing to work on this once "rename FieldConfigl is in.

The main reason we went with the current approach is to try and limit the amount of definition objects we need at runtime because this sums up very very fast. Take a bigger site which displays nodes of 10 different types on a page, with terms, users, comments and that's just core. You're quickly way past 100 field definitions that you're carrying around.

Right now, we need both the definition and the storage definition to do anything useful.

I can see that this might actually be an improvement *if* we manage to avoid loading the storage definitions on a normal, cached node view, for example. Then this could actually be an improvement compared to now as we only have to do 1 cache get instead of 2. (only applies to the first bundle of an entity type), although we trade it with needing more memory as soon as multiple bundles are involved.

My point is, I'd really like to avoid introducing BundleFieldDefinitionInterface in addition to FieldDefinitionInterface, IMO that is adding awkward complexity to work around a flawed model, adds more confusion on top of the current confusion :-/

I don't think our current model is a confusing shortcut - it models the reality of having a single field definition for all the base fields that have no per-bundle variations. There is just one thing - but if the API makes two out of the single-defined thing, this is something our (simplified) model enforces on it.

Anyway, if the simplified model works sufficiently there is nothing against it to say. However, I must say that performance concerns me here as well. I don't think it makes sense to change this when we end up having more stuff being loaded into memory just for keeping our simplified model intact.

That said, I'm not sure I fully understand the reasoning on why the current model is seen as flawed / bad. Is it the aim to have the simplified storage+instance model everywhere, or is it more related to having #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields and per-bundle variations pretty much everywhere anyway?

My point is, I'd really like to avoid introducing BundleFieldDefinitionInterface in addition to FieldDefinitionInterface, IMO that is adding awkward complexity to work around a flawed model, adds more confusion on top of the current confusion :-/

BundleFieldDefinitionInterface sounds like making it confusing, yep - not sure where the need for this would come from. I guess I head over and read up on #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields now, which is probably related.

#17 looks accurate to me (@effulgentsia++), just a couple of minor remarks that do not affect in any way the validity of this writeup:

However, code that acts on one entity at a time, shouldn't need to care whether a field is a base or bundle field; as far is it's concerned, it's just working on "all the fields of the entity", and at least with respect to that entity, a field is a field regardless of whether it also exists or not on other bundles.

This is a bit misleading: it's correct that you don't need to worry whether a particular field on an $entity object is base or bundle, BUT the main reason to have base fields is exactly being always able to rely on them. If a module defines (or targets) a particular entity type, the business logic it implements should be built on base fields, which are guaranteed to be there for any $entity object. A good example of this would be the price of a commerce product (aside from the fact I am pretty sure it will be implemented as configurable field as in D7, that is as a bundle field).

- BaseFieldBundleOverride implements FieldConfigurationInterface

Shouldn't it implement also ConfigEntityInterface? @yched brought up the idea of having a single config entity shared between Field[Instance]Config and BaseFieldBundleOverride, which would have lots of advantages, however @alexpott in #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields brought up good reasons for not doing that. I guess this discussion should be continued over there.

Shouldn't it implement also ConfigEntityInterface?

Scratch that, sorry. I missed this due to the new line:

-  FieldConfigurationInterface extends FieldDefinitionInterface, ConfigEntityInterface

Thanks for the comprehensive summary, pure @effulgentsia magic :)

I still stand that having both FieldDefinitionInterface and FieldConfigInterface, meaning a 3rd "orthogonal distinction" in your explanation, is an unfortunate cognitive overload, only justified by performance impact that we never actually evaluated, and thus qualifies as a preemptive, possibly premature optimization sadly baked in the shape of the API.

But I agree that this is our best way forward with #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields atm.

5. We need to remove setters from FDI and move them to FCI

Then BaseFieldDefinition needs to duplicate those setters' doc (which would also be the case in my scenario where BFD *not* implements FDI).

Also, if the setters present on BFD are not part of any interface, what type-hint do we use for methods/functions that:
- return such objects specifically:
EntityType::baseFieldDefinitions(), hook_entity_base_field_info[_alter](), EM::getbaseFieldDefinitions()...
- receive such objects with the explicit intent to use those setters :
EntityType::bundleFieldDefinitions(), hook_entity_bundle_field_info[_alter]()...
?

It seems we either type-hint on BFD directly, or need a BaseFieldDefinitionInterface with setters duplicated from FCI ?

FieldConfig implements FieldConfigurationInterface

Hm - maybe FieldConfigInterface for consistency ?
(I know I suggested FieldConfigurationInterface in IRC, re-reading it tickles now)

BaseFieldBundleOverride: base_field_bundle_override, core.field.*
FieldConfig: field_config, field.field.*

entity_type id and config prefixes: that's were I was not really sure, that's merely a proposal to be discussed in #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields.

Further thinking on my comment #20 above :

With #2283977: Create a new ConfigEntity type for storing bundle-specific customizations of base fields, EntityType::bundleFieldDefinitions(), hook_entity_bundle_field_info[_alter]() should be made to return FCIs rather than clones of BFDs ? And thus, renamed to bundleFieldConfig() ?

Before :

public static function bundleFieldDefinitions(EntityTypeInterface $entity_type, $bundle, array $base_field_definitions) {
  $fields['some_field'] = clone $base_field_definitions['some_field'];
  $fields['some_field']->setXxx('foobar');
  return $fields;
}

After :

public static function bundleFieldConfig(EntityTypeInterface $entity_type, $bundle, array $base_field_definitions) {
  $fields['some_field'] = BaseFieldBundleOverride::createFromFieldDefinition($base_field_definitions['entity_id'], $bundle);
  // Or, easier:
  $fields['some_field'] = $base_field_definitions['some_field']->getConfig($bundle);
  $fields['some_field']->setXxx('foobar');
  return $fields;
}

A by-bundle clone of a BFD makes little sense, that's what FCIs are here for. Plus the current DX is horrible :-)

What's great about #21 is that FD's could lose all their setters! +1

re #22 ignore me I'm wrong.

Trying to translate #17 into actionable issues:

1. We should rename FieldDefinition (the implementation class, not the interface) to BaseFieldDefinition, since it's an implementation specific to base fields

Opened #2315237: Rename FieldDefinition to BaseFieldDefinition (major, beta-target)

4. remove the getBundle() method from FDI. There are few callers of FDI::getBundle() and they can/should be replaced with either passing $bundle as a separate param (if the need for the bundle is unrelated to changing the definition) or using FCI instead (if the need for the bundle is to override the definition for that bundle).

5. remove setters from FDI and move them to FCI

Less sure whether those should be split to dedicated issues or adressed as a whole. The latter still has unresolved consequences mentioned in #20.

Thanks for the great write-up effulgentsia.

I've read through all the IRC log as well, but #17 is a great summary to me. I think this is a solid plan, so overall +1, but here some additional comments:

1. I like the way you point the distinction between base fields vs. bundle fields and FieldStorageDefinitionInterface vs FieldDefinitionInterface is ortogonal. Base field / bundle field determines for *whom* the fields have been defined, while FSD vs FD determines *what* has been defined.
2. As yched pointed out in #21 we need a way to alter/override field definitions in alter hooks. I don't think we should or have to rename the hook, as the system in the end only cares to get field definitions back - that's the interface which matters for the system here. However, I agree that the provided DX should be around using field configurations. Maybe this would be a reasonable DX?
   $override = $base_field_definitions['some_field']->getBundleOverride($bundle);
   If getBundleOverride() finds an pre-existing bundle field which is not a bundle override, i.e. someone hand-crafted another implementation, it can still take over its values based on the interface getters + replace the object with an suiting override object. It's a litte bit of extra work for us to do, but that way we end up with a consistent way to override while the field definitions returned to and from the EM stay read-only again, i.e. if you ask the EM about the field definitions of a bundle you are not supposed to change anything and you can't.

3. 1. We should rename FieldDefinition (the implementation class, not the interface) to BaseFieldDefinition, since it's an implementation specific to base fields

   Yep, that seems reasonable!

@alexpott, @effulgentsia, and I (well mostly @alexpott and @effulgentsia) discussed this issue at TCDrupal. We had the following @todo for @effulgentsia:

"Make sure all feedback is addressed and close works as designed"

I still think this is a major beta deadline, not a critical beta target, though I guess it's a moot point if we're closing it works as designed. As I stated above, I'm not comfortable with the idea of changing this after beta; it seems like it would impact a lot of code. Tagging to look at during the pre-AMS beta sprint.