Updated: Comment #N

Problem/Motivation

Right now, entity fields can optionally only exist on certain bundles but the same fields can not differ between bundles. Configurable field instances can have different settings per bundle and base fields can be different as well, for example the node title label can be configured per node type.

Due to that, Field API currently has a few workarounds that involve manually creating entity objects, which is very slow and a large performance problem.

Proposed resolution

Standardized terminology: base fields are all entity fields that exist independently of the bundle.

Unify Entity::baseFieldDefinitions() and hook_entity_field_info() extend them with bundle variants:

Entity class methods
Changed: $EntityClass::baseFieldDefinitions($entity_type_id) to $EntityClass::baseFieldDefinitions(EntityTypeInterface $entity_type)
New: $EntityClass::bundlefieldDefinitions(EntityTypeInterface $entity_type, $bundle, array $base_field_definitions)

Hooks
Renamed: hook_entity_field_info($entity_type) to hook_entity_base_field_info(EntityTypeInterface $entity_type)
Renamed: hook_entity_field_info_alter(&$info, $entity_type) to hook_entity_base_field_info(&$base_field_definitions, EntityTypeInterface $entity_type)

New: hook_entity_bundle_field_info(EntityTypeInterface $entity_type, $bundle, $base_field_definitions)
New: hook_entity_bundle_field_info_alter(&$field_definitions, EntityTypeInterface $entity_type, $bundle)

The bundle for EntityManager::getFieldDefinitions($entity_type_id, $bundle) is now mandatory. EntityManager::getBaseFieldDefinitions($entity_type_id) has been added to allow to to fetch the base field definitions. This makes it clear that the common use case is to specify the entity type and bundle.

Additionally, the patch implements this for the node title field, which allows to remove the currently existing overrides and various simplification and improvements in multiple places.

Remaining tasks

User interface changes

API changes

Original report by @effulgentsia

EntityManager::getFieldDefinitions() allows for optional fields per bundle, but doesn't allow for a field of the same name to have different definitions per bundle. However, we need to allow for node titles to have potentially different labels per node type, and likely other use cases as well.

https://docs.google.com/document/d/1A5BLd-KmrLnJW88SdLX5HG3Xm7AyinxwXNAG... has notes from Prague about this.

CommentFileSizeAuthor
#146 d8_field_definition_bundles-2114707-146-interdiff.txt4 KBBerdir
#146 d8_field_definition_bundles-2114707-146.patch74.91 KBBerdir
#144 d8_field_definition_bundles-2114707-144-interdiff.txt4 KBBerdir
#144 d8_field_definition_bundles-2114707-144.patch74.89 KBBerdir
#141 d8_field_definition_bundles-2114707-141-interdiff.txt2.96 KBBerdir
#141 d8_field_definition_bundles-2114707-141.patch74.75 KBBerdir
#139 d8_field_definition_bundles-2114707-139-interdiff.txt683 bytesBerdir
#139 d8_field_definition_bundles-2114707-139.patch74.59 KBBerdir
#137 d8_field_definition_bundles-2114707-137-interdiff.txt14.19 KBBerdir
#137 d8_field_definition_bundles-2114707-137.patch74.59 KBBerdir
#131 d8_field_definition_bundles-2114707-131-interdiff.txt1.9 KBBerdir
#131 d8_field_definition_bundles-2114707-131.patch74.84 KBBerdir
#128 d8_field_definition_bundles-2114707-128-interdiff.txt31.09 KBBerdir
#128 d8_field_definition_bundles-2114707-128.patch73.54 KBBerdir
#127 d8_field_definition_bundles-2114707-127-interdiff.txt4.35 KBBerdir
#127 d8_field_definition_bundles-2114707-127.patch68.64 KBBerdir
#126 interdiff.txt2.09 KByched
#126 d8_field_definition_bundles-2114707-126.patch67.11 KByched
#122 interdiff.txt12.73 KByched
#122 d8_field_definition_bundles-2114707-122.patch67.17 KByched
#120 d8_field_definition_bundles-2114707-120-interdiff.txt3.18 KBBerdir
#120 d8_field_definition_bundles-2114707-120.patch66.26 KBBerdir
#114 d8_field_definition_bundles-2114707-114-interdiff.txt17.04 KBBerdir
#114 d8_field_definition_bundles-2114707-114.patch66.05 KBBerdir
#114 d8_field_definition_bundles-2114707-108.patch54.83 KBBerdir
#111 d8_field_definition_bundles-2114707-111-interdiff.txt5.66 KBBerdir
#108 d8_field_definition_bundles-2114707-108-interdiff.txt2.24 KBBerdir
#108 d8_field_definition_bundles-2114707-108.patch54.83 KBBerdir
#104 d8_field_definition_bundles-2114707-104-interdiff.txt23.98 KBBerdir
#104 d8_field_definition_bundles-2114707-104.patch48.56 KBBerdir
#89 d8_field_definition_bundles-2114707-89-interdiff.txt6.5 KBBerdir
#89 d8_field_definition_bundles-2114707-89.patch41.75 KBBerdir
#83 interdiff.txt14.41 KBamateescu
#83 d8_field_definition_bundles-2114707-83.patch41.37 KBamateescu
#81 d8_field_definition_bundles-2114707-81.patch37.92 KBamateescu
#78 d8_field_definition_bundles-2114707-78-interdiff.txt7.3 KBBerdir
#78 d8_field_definition_bundles-2114707-78.patch37.43 KBBerdir
#74 d8_field_definition_bundles-2114707-74-interdiff.txt1.67 KBBerdir
#74 d8_field_definition_bundles-2114707-74.patch32.33 KBBerdir
#72 d8_field_definition_bundles-2114707-72.patch32.16 KBBerdir
#69 d8_field_definition_bundles-2114707-69-interdiff.txt1.42 KBBerdir
#69 d8_field_definition_bundles-2114707-69.patch32.59 KBBerdir
#67 d8_field_definition_bundles-2114707-67-mergediff.txt4.23 KBBerdir
#67 d8_field_definition_bundles-2114707-67.patch32.55 KBBerdir
#55 interdiff.txt1.4 KBeffulgentsia
#55 d8_field_definition_bundles-2114707-55.patch32.93 KBeffulgentsia
#53 d8_field_definition_bundles.interdiff.txt4.67 KBfago
#53 d8_field_definition_bundles.patch31.44 KBfago
#52 per-bundle-field-definitions-2114707-52-interdiff.txt695 bytesBerdir
#52 per-bundle-field-definitions-2114707-52.patch30.73 KBBerdir
#48 per-bundle-field-definitions-2114707-46.patch30.05 KByched
#46 per-bundle-field-definitions-2114707-46-interdiff.txt1.82 KBBerdir
#46 per-bundle-field-definitions-2114707-46.patch30.05 KBBerdir
#44 per-bundle-field-definitions-2114707-44-interdiff.txt1.42 KBBerdir
#44 per-bundle-field-definitions-2114707-44.patch29.4 KBBerdir
#42 per-bundle-field-definitions-2114707-42-interdiff.txt4.53 KBBerdir
#42 per-bundle-field-definitions-2114707-42.patch29.14 KBBerdir
#33 per-bundle-field-definitions-2114707-33-interdiff.txt2.21 KBBerdir
#33 per-bundle-field-definitions-2114707-33.patch25.49 KBBerdir
#13 per-bundle-field-definitions-2114707-13.patch24.1 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Issue summary: View changes

Fixed Google Doc link.

yched’s picture

Thought about this a bit over dinner after the last sprinting day in Vienna :-)

The original idea in Prague was this (copied from the gdoc linked in the OP)

public static function baseFieldDefinitionsByBundle($entity_type, $bundle, FieldDefinition[] $info) {
  // Assuming only page and article have a title, but for page it’s optional:

  if (!in_array($bundle, array('page', 'article'))) {
    unset($info['title']);
  }
  if ($bundle == 'page') {
    $info['title'] = clone $info['title'];
    $info['title']->setRequired(FALSE);
  }
  return $info;
}

Drawbacks:
- Convoluted DX, easy to mess up (the "clone" part especially is very fragile and DX--)
- This results in a lot of "the same but not completely" copies of the same FieldDefinition objects in cache and in the in-memory repository

I was thinking we could have FieldDefinitionOverride objects that act as shallow objects containing just the overriden properties :

public static function baseFieldDefinitionOverrides($entity_type, $bundle) {
  $overrides = array();
  // Assuming only page and article have a title, but for page it’s optional:
  if (!in_array($bundle, array('page', 'article'))) {
    $overrides['title'] = new FieldDefinitionOverride()->remove();
  }
  if ($bundle == 'page') {
    $overrides['title'] = new FieldDefinitionOverride()->setRequired(FALSE);
  }
  return $info;
}

- The methods available on FieldDefinitionOverride would only let you change what can be changed by bundle (e.g setRequired(), addConstraint(), but not setFieldType())

- The caller (EntityManager::getFieldDefinitions()) takes care of $override->setFieldName($field_name) on the results based on the array keys, just like is already done for the results of baseFieldDefinitions

- In our cache, we put the cross-bundle list of FieldDefinitions, + for each bundle the shallow list of shallow FieldDefinitionOverride objects.

- When reading out from the cache at runtime, we transform those into regular FieldDefinition objects by applying the diff. That's what we put in the in memory repository (static cache) and that's what EntityManager::getFieldDefinitions($entity_type, $bundle) returns.

- Possibly, we could be smarter than that and keep the FieldDefinitionOverride objects as wrappers around the actual FieldDefinition objects at runtime - that's as many objects, but less properties duplicated in memory

- According to @fago in Vienna, we might even consider that base fields are present on all bundles of an entity type, and then no need for the ->remove() part.

Thoughts ?

effulgentsia’s picture

+1

amateescu’s picture

The plan sounds good, but we also need to clear out the relationship between FieldDefinitionOverride and FieldInstance, right?

If we go ahead with this, we'll have Field + FieldInstance on one side and FieldDefinition + FieldDefinitionOverride on the other side, which.. I have to say is less than ideal and feels like one layer too much..

yched’s picture

Priority: Major » Critical
Issue summary: View changes

Bumping to critical.

effulgentsia’s picture

we also need to clear out the relationship between FieldDefinitionOverride and FieldInstance

Hm. Here's a crazy thought: is there ever a use case for a FieldDefinitionOverride to be code-defined rather than configuration-defined? I mean, bundles themselves are always configuration-driven (at least in core, I think), so shouldn't bundle-specific field definition overrides also be configuration? If so, then would it make sense for per-bundle overrides to always be FieldInstance objects, and to make it so that those objects can override either configurable or nonconfigurable field definitions?

amateescu’s picture

That's an interesting idea. I'm sure @yched will have a better answer but what bugs me about it is that it's quite hard to write a field_instance config object by hand, and it will eventually get down to that as I can't even imagine how we'll provide an UI for this..

effulgentsia’s picture

Issue tags: +beta target

Per http://buytaert.net/the-next-step-for-drupal-8-is-a-beta, this is a critical API and should be resolved before beta. This hasn't yet been approved as a "beta blocker" by a core committer, so just tagging as a beta target for now, but if we get close to beta without this being done, we should reevaluate whether to make it block a beta.

effulgentsia’s picture

msonnabaum informed me that the fake entity creation in EntityDisplayBase::getFieldDefinition() is currently causing a sizable performance regression and obscuring the ability to profile for other performance problems. Given that, it would be great to get this solved as quickly as possible, even if a follow up is needed to optimize DX after that.

fago’s picture

Issue tags: +Entity Field API
Berdir’s picture

Yep, I can confirm that that function (or actually _field_create_entity_from_ids()) is extremely slow, EntityFieldTest calls it 32 times, taking 4.5s which is 20% of the whole test run. The entity factory will help, but this really needs to happen :)

Berdir’s picture

As discussed in the meeting today, we will try to get started here by changing the data structures/cache/hooks to be per bundle, without touching the override part. That will hopefully allow us to put field instances in there and unblock most things, then we can introduce overrides later with minimal API changes. At least that's the plan.

Will work on this asap.

Berdir’s picture

Assigned: Unassigned » Berdir
Berdir’s picture

Status: Active » Needs review
FileSize
24.1 KB

This basically works without the per bundle override method as mentioned above.

Something seems to be not quite right yet, getting a fail in EntityTranslationTest. Let's see how the other things are looking.

Note that there's a slight behavior change that's actually due to a bug in the old field_entity_field_info() implementation, see #1919468: EntityManager::getAllBundleInfo() adds default entity_type_id bundle for entity types with bundles, it incorrectly assumed that if $bundle == $entity_type, the entity type then doesn't support bundles and all fields are for all bundles. So it didn't pass in the bundle to getFieldDefinitions() in that case but then got it back in that function. While we could restore something similar to the previous behavior if an entity type only has a single bundle, I think this is the correct behavior as long as we don't have an explicit information if an entity type supports bundles or not.

Berdir’s picture

Issue tags: +sprint

Status: Needs review » Needs work

The last submitted patch, 13: per-bundle-field-definitions-2114707-13.patch, failed testing.

yched’s picture

Berdir++

Minor remarks for starters

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -480,8 +480,7 @@ public function getPropertyDefinition($name) {
         if (!isset($this->fieldDefinitions)) {
    -      $bundle = $this->bundle != $this->entityType ? $this->bundle : NULL;
    -      $this->fieldDefinitions = \Drupal::entityManager()->getFieldDefinitions($this->entityType, $bundle);
    +      $this->fieldDefinitions = \Drupal::entityManager()->getFieldDefinitions($this->entityType, $this->bundle);
         }
         return $this->fieldDefinitions;
    

    If the list of fields by bundle is statically cached in the entity manager, do we need to cache it in the Entity as well ?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -327,90 +332,108 @@ public function getAdminRouteInfo($entity_type, $bundle) {
    +          $this->fieldDefinitions[$entity_type] = $this->buildFieldDefinitions($entity_type, $bundle);
    

    We're in the if (!$bundle) case, so passing the $bundle param is a bit misleading.

  3. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -327,90 +332,108 @@ public function getAdminRouteInfo($entity_type, $bundle) {
    +          // Rebuild the definitions and put it into the cache.
    

    s/it/them/ ;-) (same below)

Berdir’s picture

2. I could alternatively pass NULL through explicitly, but we do need to pass that value through somehow :)

yched’s picture

More substantially:

  • +++ b/core/modules/content_translation/content_translation.module
    @@ -126,13 +126,10 @@ function content_translation_entity_field_info_alter(&$info, $entity_type) {
    +      if (isset($fields[$name])) {
    +        $fields[$name]->setTranslatable((bool) $translatable);
    

    Yeah, this now runs on FieldInstances, and calling setTranslatable() breaks, setTranslatable() is not part of FieldDefinitionInterface (FieldDefinition has no setters - #2143297: setters on FieldDefinitionInterface)

    Not sure how we easily fix that. I think we currently misuse isTranslatable() for two different things:
    - "does it support translatability" - that's always true for configurable fields, and is defined by the code that provides the field for base fields
    - "is it actually translatable" - that's configuration, held by e_t.module, and it's that one that e_t tries to set with setTranslatable() here, and that the whole Entity translation API runtime code then checks with isTranslatable(). This one can be per $instance.

    So it looks like the setTranslatable() / isTranslatable() pair we have right currently is the second one, it could be part of FieldDefinitionInterface, but then we need separate methods for the first one.

  • yched’s picture

    related to that dual isTranslatable() meaning : #2143291: Clarify handling of field translatability

    yched’s picture

    re #17

    I could alternatively pass NULL through explicitly, but we do need to pass that value through somehow :)

    Why can't the $bundle param in buildFieldDefinitions() be marked optional ? De facto, it is, no ?

    yched’s picture

    Looked into the OptionsFieldTest fails:
    in OptionsFieldTest::testUpdateAllowedValues()

    // Removed options do not appear.
    $this->field->settings['allowed_values'] = array(2 => 'Two');
    $this->field->save();
    $entity = entity_create('entity_test', array());

    The $field->save() triggers field_info_cache_clear() / entity_info_cache_clear(), yet the entity_create() then creates FieldItem objects whith a wrong, stale field definition.

    ContentEntityBase::getTranslatedField() calls \Drupal::typedData()->getPropertyInstance() which seems to simply reuse an existing prototype.

    fago’s picture

    If the list of fields by bundle is statically cached in the entity manager, do we need to cache it in the Entity as well ?

    Getting the field definition is on the critical path when accessing any field value, so bailing out to the EM + another method is probably too much here. At least this would need benchmarks.

    as #20: Yeah, I'd vote for both - passing an explicit NULL + make it optional - that puts the important fact that $bundle can be NULL into developers' face.

    So it looks like the setTranslatable() / isTranslatable() pair we have right currently is the second one, it could be part of FieldDefinitionInterface, but then we need separate methods for the first one.

    yeah, this bugs us repeatedly - I think we finally need to do a proper solution here. I'd prefer to get away without yet another method that will confuse people. So maybe it's enough to differentiate between FALSE and NULL ? Aka, NULL means there is translation support but it's not enabled and FALSE | TRUE that there is translation support and its enabled/disabled ?

    yched’s picture

    I'd prefer to get away without yet another method that will confuse people

    Really unsure about that. Using the same getter / setter for different information at different moments in the lifecycle of a field definition is inherently broken IMO.
    See #2018685-26: Remove field_is_translatable(). Right now, if I call $field_definition->isTranslatable(), I get a different result depending on where I got $field_definition from and whether e_t has kicked in and made its alters or not. That's utterly confusing.

    We shouldn't try to be confusingly smart and magic IMO. If those are two separate informations provided by different sources (the field definition / e_t configuration), they should be accessed and set separately, not stacked onto each other.

    fago’s picture

    I'm not suggesting to do anything magic, I'm suggesting to go with assigning semantics to each of NULL, FALSE and TRUE instead of just having FALSE and TRUE but in two methods. But whether that's feasible is how well the semantics suit their values, i.e. the meaning makes sense for a value?

    plach’s picture

    The main reason for assigning two different semantics to the 'translatable' key was that we had no bundle-level support at the time. Since we are going to introduce it here, I'd suggest to follow the same pattern we introduced for entity type/bundle info: in that case the 'translatable' key at entity-type level means that the entity type supports translation, while the bundle-level 'translatable' key means that the specific bundle it refers to is enabled for translation. ET alters the latter based on its configuration data, and would do the same with bundle-level field info.

    This would be very consistent and shouldn't impose an additional DX burden, since people will already need to understand this for entity-type/bundle info.

    If we go this way, in #2018685: Remove field_is_translatable() we will have to use bundle-level field info.

    fago’s picture

    One thing that came to my mind was the question of whether config fields should be still implementing the field definition interface once this is done. I mean we've been implementing it to make it possible to put the config fields under the entity field umbrella, but conceptually it's the field instance which is a complete field definition only.
    So once we can put field instances as the field definitions into entity field info repository I guess there is no need left for config fields implementing this interface. Instead, we might want to come up with a separate interface for the notion of a "incomplete field definition that is shared across all different field definitions across bundles". yched and me have been using the term "FieldStorageDefinition" while discussing this in Vienna.

    Berdir’s picture

    FieldStorageDefinition would then be used for the field stuff on the storage controller once that becomes not-config/custom-field specific, schema and so on?

    fago’s picture

    yes, that interface would be used to replace FieldInterface at onFieldCreate()/update/delete then

    effulgentsia’s picture

    Instead, we might want to come up with a separate interface for the notion of a "incomplete field definition that is shared across all different field definitions across bundles"

    Drupal\field\Plugin\views\field\Field::buildOptionsForm() and possibly other places needs to get a formatter for a $field rather than a $instance. Those kinds of use cases was the initial thinking for having $field implement the same FieldDefinitionInterface as $instance. How would you need to adjust formatters and/or Views if you want to split the interface?

    yched’s picture

    The discussion of FieldStorageDefinition for cross-bundle definitions came from the fact that the "unified FieldDefinitionInterface that works for both $field & $instance" is basically a lie (or "convenient abstraction"), that was introduced because back then, unlike "D7 Field API" fields, ENtityNG fields didn't intend to support the notion of by-bundle variants.

    Now they do have this need, and we're trying to find a way to make that happen on top the lie we introduced under the opposite assumptions. Which feels particularly backwards :-)
    Everything has by bundle variants, yet we work on a definition format that claims variants don't exist.

    That abstraction / lie bites us in more and more places:
    - Here with setTranslatable()
    - More generally when when it comes to defining setters for FieldDefinitionInterface - we currentlly don't have any, even thouh we do have a hook were modules are supposed to alter FieldDefinitionInterface objects :-).
    - #2144327: Make all field types provide a schema() has FieldItem::schema() receive a FieldDefinitionInterace. This means field types authors can write storage schemas that vary by bundle for the same field, which is inherently wrong. This is exactly what the $field / $instance dichotomy in D7 was there to avoid.

    Something that works on a FieldDefinitionInterface alone doesn't know in which context it is (cross bundle or bundle specific ?), and thus doesn't know what is legit todo with it, or to what extent the informations it gets from it are workable (is the the real value, or just a default while the actual values will vary on each actual bundle ?)

    So yeah, the @fago / @yched discussion in Vienna was about reintroducing a split for "cross-bundle" vs bundle-specific". We fried our brains before reaching actual conclusions or an actionable plan, though :-/.

    yched’s picture

    Configurable fields inherently have that "cross-bundle" vs "bundle-specific" split.

    What we stumbled upon in Vienna was that we need to preserve the following critical DX point for base fields:
    You shouldn't be forced to declare both
    a) in one method a list of bundle-agnostic FieldStorageDefinitions
    b) then in another method a list of actual FieldDefinitions for each bundle.

    But I'm thinking that maybe the override model described in #1 might adress that: (actual class names to be enhanced, of course)
    - you specify a list of definitions that are "full definitions", but only actually implement FieldStorageDefinitionInterface, and only get passed around as such.
    - then you optionally specify a "by-bundle" list of bundle-specific Override objects, that are wrappers with just the overriden properties (possibly none), and that *do* implement FieldDefinitionInterface. As mentioned above, the "empty override" case can be made implicit and automatic for less verbosity.

    --> FieldStorageDefinitionInterface is only about what is common across bundles,
    FieldDefinitionInterface is only about what is true for a given bundle,
    which stays true to our actual conceptual model (i.e "not a lie").

    effulgentsia’s picture

    #30 and #31 make sense, and I see that as we expand FieldDefinitionInterface, there are some things that really have no logical meaning in $field. But I'm still unclear on what formatters and widgets are supposed to act on. Because of Views, they need to be able to act on something regardless of whether it's per-bundle or cross-bundle. Currently, they act on FieldDefinitionInterface. I don't think it makes sense to change that to FieldStorageDefinitionInterface. Do we need yet some other new interface that both $field and $instance can implement, but that's neither FieldDefinitionInterface nor FieldStorageDefinitionInterface? Especially, since if we're adding setters to these interfaces, and formatters and widgets have no business invoking setters.

    [EDIT: and by "act on", I mean methods that don't receive a $items, like settingsForm() and settingsSummary(), because for methods that do receive a $items, those are always bound to a bundle.]

    Berdir’s picture

    Fixing a lot of test fails with two changes:

    - Allow to clear the prototype cache in the typed data manager so that injected definitions there are updated. We might want to move this to a separate method, as clearCachedDefinitions() clears way more than we're interested in I think.
    - Stupid name clash in content_translation_entity_field_info() altered away all field definitions if content_translation.module was enabled. That caused most of the crazy test fails there, leaving us only with the actual problem: Fatal error: Call to undefined method Drupal\field\Entity\FieldInstance::setTranslatable(). That function certainly needs more work, we need to be specific about the bundle we're handling now, although I'm not sure how to deal with the no-bundle case. Maybe ignore? No idea.

    yched’s picture

    lol @ the content_translation_entity_field_info_alter() interdiff :-)

    +++ b/core/modules/field/field.info.inc
    @@ -39,6 +39,7 @@ function field_info_cache_clear() {
       \Drupal::typedData()->clearCachedDefinitions();
       \Drupal::service('plugin.manager.field.field_type')->clearCachedDefinitions();
       \Drupal::service('config.factory')->reset();
    +  \Drupal::typedData()->clearCachedDefinitions();
    

    It's already present 3 lines above ? Is this about the ordering then ?

    Berdir’s picture

    Move on, there's nothing to see in that interdiff ;)

    Oh, didn't even see that existing call. No the reason it works now is that I added the override in TypedDataManager and reset $this->prototypes. Obviously not necessary to call it twice.

    The last submitted patch, 33: per-bundle-field-definitions-2114707-33.patch, failed testing.

    Berdir’s picture

    Not a big fan of having a NULL/TRUE/FALSE tristate. While we could manage it like this internally (although I'd prefer constants or two separate properties), I think it would be much better DX to have to separate methods, NULL/FALSE having a different meaning means that we have to type safe comparisons?

    fago’s picture

    Summarizing, I see the following requirements:

    • We've got configurable fields, which are per bundle, in which case they do not have a complete, working field definition defined per entity type (if the bundle is unknown).
    • We've got base fields, which typically are per entity-type, i.e. are the same across all bundles.
    • We've got some base fields, which need some bundle-specific overrides, e.g. node title needs to change it's required flag per bundle. The difference to the first point is though, that the shared per entity-type field is a working, complete field definition which acts as default for every bundle.
    • Finally, we've got the need to work with incomplete field definitions (=Field config, FieldStorageInterface) as they were complete in some situations, i.e. the Views widget use-case.

    To address all that requirements I think an override approach as in #1 works, however we should make sure that our objects honestly implement interfaces and do not lie to us. Thus, if an object says its complete, it really should be a proper and working field definition *plus* the other way round.

    - you specify a list of definitions that are "full definitions", but only actually implement FieldStorageDefinitionInterface, and only get passed around as such.

    I think that would be problematic for base fields as it would make them lie - if it's a complete, working field definition it should implement the respective interface.

    So here is how I could see it working:

    1. Have two interfaces, FieldDefinitionInterface for complete definitions as we have it, plus FieldStorageDefinitionInterface for the incomplete, shared storage definition.
    2. Implement an override approach as in #1, where the bundle-specific definition wins over the generic entity-type level definition.
    3. Define base fields as fully working field definitions as we do now. If an override is necessary, also define an override field definition for which we can provide a override-class helper that proxies through not overridden stuff.
    4. Define configurable fields as they really are, e.g. per bundle. Full definitions go to the bundle, field storage definitions to the entity type level.
    5. Do some sanity checking in the manager, e.g. if there is a bundle-specific field definition at least a fields storage definition must be present.
    6. Finally, imo the Views use case should be solved analogously to what it really is: We've got an incomplete field definition, so we need to come up with the stuff that's missing for being able to render the widget. Thus, for Views being able to use the storage field with a formatter, it needs to create an intermediate (fake) field instance and provide values for missing defaults. We could also provide a helper class for that if we want, but just creating a field instance and not saving it should do it as well.

    I'm not so sure yet about the DX of providing field storage definitions, but it's probably best to just define them along side with full field definitions on the per entity type level. This shows nicely that they share the same namespace, and we can easily differentiate afterwards by checking the interface and so implement two different repository methods that return a) only fully field definitions and b) all storage definitions. Full field definitions should extend the field storage definition Interface such that they are returned in case b) also, i.e. you'd get a complete list of storage fields, where some are complete field definitions and some aren't.

    effulgentsia’s picture

    Sorry guys. I've been on vacation, so have only been able to intermittently follow / comment on this.

    Finally, imo the Views use case should be solved analogously to what it really is: We've got an incomplete field definition, so we need to come up with the stuff that's missing for being able to render the widget.

    What we have is not incomplete at all from the perspective of what Views needs to do: interact with formatters/widgets. FieldDefinitionInterface was introduced initially as the stuff that formatters and widgets need to know about a field (see #1950632-75: Create a FieldDefinitionInterface and use it for formatters and widgets). Now #30 mentions setters that are needed in this issue, and storage-related getters needed in #2144327: Make all field types provide a schema(). It looks like #38 is proposing a FieldStorageDefinitionInterface to hold things like those storage getters. If we do that, then what about reconsidering something along the lines of #1950632-75: Create a FieldDefinitionInterface and use it for formatters and widgets and partitioning the full FieldDefinitionInterface into smaller interfaces, so as to allow $field to implement the ones that make sense without needing to implement the ones that would be a lie, but that way, formatters and widgets could receive the appropriate smaller interface that's implemented by both?

    yched’s picture

    Widgets / Formatters' 90% use case is to be used in a bundle-specific context, and thus have a full-fledged $instance / FieldDefinitionInterface to work on.

    Crippling them to work only on a "more restricted field definition" (e.g. by contract/interface, cannot access instance settings) to account for the 10% use case where they are used in a non-bundle-specific context seems like a bad tradeoff.

    FWIW:
    - Although it would be nice, since 4.7 we never got to the point where a widget could be rendered as an exposed view filter. Because it's hard :-)
    - So, the only cases where widgets / formatters methods are called in a non-bundle specific context is "configuration form for a formatter used in a View" (even the actual rendering is made on an actual field in an actual entity, with a defined bundle)
    - For the case above, Views D7 was using a function called ctools_fields_fake_field_instance() :-) - which is actully very close to what #30 is proposing...
    Not claiming that this is super clean, but at least it's not worse than what it was in D7 ?

    The big D8 win is "widgets / formatters on base fields" more than "widgets / formatters on $field structures across bundles" ?

    yched’s picture

    Regarding the isTranslatable() / setTranslatable() blocker:

    I do find the "same property / different meaning at different points in the code flow" thing to be confusing, in an area that's already fairly convoluted (base field, bundle override, $field, $instance...) :-).

    What particularly baffles me currently is this:
    - baseFieldDefinition() sets $field_definition->setTranslatable(TRUE/FALSE) - here, this means "supports translation or not"
    - e_t_field_info_alter() does setTranslatable() to override with what it has in its own configuration - then, it means "actually translatable".
    - To build its configuration UI, e_t admin form does

    if ($field_defintion->isTranslatable()) {
      - Show a checkbox;
      - if (my own config) {show it checked} else {show it unchecked}
    }

    But the $field_defintion->isTranslatable() it uses here is the one it has *already altered*, since $field_defintion comes from EntityManager::getFieldDefinitions(), where hook_field_info_alter() has already ran.

    Maybe this works currently, but frankly this looks a bit incestuous :-)

    For configurable fields, this is further mixed up by the fact that $fields still save a useless 'translatable' flag in their YML (useless because e_t holds the real config). We should really stop doing that - #2143291: Clarify handling of field translatability

    So yeah, I'd really favor a two flags approach where it's clear who states what and meaning what :-)
    - isTranslatable() / setTranslatable($bool) is handy for "translatability actually enabled" :
    isTranslatable() is called by all the Entity/Field runtime code,
    setTranslatable() would be only called by e_t (or similar).
    - supportsTranslation() / setSupportsTranslation() would be for "supports translation" :
    supportsTranslation() is called by e_t to build its UI,
    setSupportsTranslation() is set by baseFieldDefinitions(), and not needed for config fields (always TRUE).

    setTranslatable(TRUE) throws an exception if !supportsTranslation() ?

    The only problem is finding a decent name for "setSupportsTranslation()" :-)
    Maybe just supportsTranslation($bool = NULL), that acts as both a getter and setter ?

    Berdir’s picture

    While we discuss this, here's a patch that implements the baseFieldDefinitionsByBundle() part, based on the somewhat ugly but easy to implement and working clone approach. Updated the node title definition to use this.

    The last submitted patch, 42: per-bundle-field-definitions-2114707-42.patch, failed testing.

    Berdir’s picture

    The last submitted patch, 44: per-bundle-field-definitions-2114707-44.patch, failed testing.

    Berdir’s picture

    Looking into those fails.

    As discussed in @yched in IRC, the FieldInstance's already have the correct translation setting through their field, we only need to care about base fields there, apparently? Might not be the final fix, but works.

    The other one was tricky, the problem is that ConfigFieldItemList didn't call out to the parent to get constraints on the field item list level. Which I think is #2070429: Configurable fields aren't validated against the "required" constraint except in forms. So just changing that will probably cause test fails? We didn't have that problem before becaue the NodeTitle list class extend from FieldItemList.

    yched’s picture

    ConfigFieldItemList::getConstraints() not calling its parent might have been originally related to #2070429: Configurable fields aren't validated against the "required" constraint except in forms - avoid "required" constraints being reported by both FAPI and field validation.

    But meanwhile the approach that was agreed to fix #2070429: Configurable fields aren't validated against the "required" constraint except in forms is actually already in place in HEAD, just not at the best place currently. - see my comment #21 over there.

    So my guess would be that ConfigFieldItemList::getConstraints() might be able to call its parent without triggering bugs/fails now. Let's see.

    yched’s picture

    Testbot seems stuck on #46 :-/
    Just reuploading...

    The last submitted patch, 46: per-bundle-field-definitions-2114707-46.patch, failed testing.

    Status: Needs review » Needs work

    The last submitted patch, 48: per-bundle-field-definitions-2114707-46.patch, failed testing.

    The last submitted patch, 48: per-bundle-field-definitions-2114707-46.patch, failed testing.

    Berdir’s picture

    Awesome, you're right :)

    This should then fix that test failure. I love it when things start to work as expected and tests point out weird bugs :)

    fago’s picture

    setTranslatable(TRUE) throws an exception if !supportsTranslation() ?

    The only problem is finding a decent name for "setSupportsTranslation()" :-)
    Maybe just supportsTranslation($bool = NULL), that acts as both a getter and setter ?

    Yeah, imo a tristate would work well as we don't have to manually exclude the not possible forth state then. Howsoever, if you think separate methods are better anyway that's fine with me. But I've the feeling this shouldn't be discussed/solved here - couldn't we do a simple class check or so to move on here and discuss on the best DX in its own issue?

    Widgets / Formatters' 90% use case is to be used in a bundle-specific context, and thus have a full-fledged $instance / FieldDefinitionInterface to work on.

    Yes, I think that's the regular case as well, so that should be what they receive as interface. As said, for the other case I think it's totally fine to have some sort of helper to provides us with a complete field definition with sane defaults for the incomplete storage definition. It shouldn't be the widget/formatters job to figure out the differences, but the reasonable defaults should be passed to it. But as those defaults are not generally valid configuration, they shouldn't be part of the general object - so an interim helper object/instance seems to be the logical fit to me.

    The patch looks already quite good, however baseFieldDefinitionsByBundle() parameter ordering has not been matching the docs. Also, I think we should not alter the definitions by reference but explicitly return the fields by bundle - for two reasons:
    1. It makes us easily aware of the overrides. This might be helpful if we want to optimize caching strategy later on and it turns out that we want to cache per entity-type base fields separately, instead of caching them by bundle (what will lead to duplicate definitions in memory).
    2. We should aim for consistency between hook_entity_field_info() and the methods on the entity class - modules defining an entity type should use the methods on the entity class instead of the hook.

    Regarding that consistency the method and the hook also differ in that the method gets the non-bundle specific fields passed in, but the hook not. Maybe we should split the hook into two as we do for the methods, such that we have
    - one hook for getting non-bundle specific fields, mapping to the method
    - one hook for getting bundle specific fields, mapping to the method
    - one final alter hook
    ?

    Attached patch changes the byBundle method to return the fields instead and tries to improve the related docs a bit, but does not change the hook yet (let's discuss this first). Please check the interdiff.

    What seems to weird about the current patch is that I think it never invokes hook_entity_field_info() with $bundle == NULL, does it? It seems to go with the old behaviour of using the entity type instead. But as NULL does make more sense for the "this applies to all bundles case", I guess it should call it with NULL as specified by the interface.

    yched’s picture

    (setTranslatable() & friends)
    I've the feeling this shouldn't be discussed/solved here - couldn't we do a simple class check or so to move on here and discuss on the best DX in its own issue?

    Yes, that's actually what @berdir's latest patch does, and it's totally good enough here.
    But it's only after wrapping my head around what e_t does with 'translatable' in #2143291: Clarify handling of field translatability that I realized the $instance->setTranslatable() blocker could in fact be worked around that easily - the fact that it took us 30 comments here is IMO a sign that this area is currently really confusing :-/.

    #2143291: Clarify handling of field translatability could probably be repurposed for this "clean up $field->translatable / $instance->translatable / isTranslatable() / setTranslatable() flow".

    effulgentsia’s picture

    Fixing one more @todo in HEAD that was waiting on this issue.

    effulgentsia’s picture

    1. +++ b/core/modules/content_translation/content_translation.module
      @@ -114,25 +115,22 @@ function content_translation_entity_bundle_info_alter(&$bundles) {
           // Currently field translatability is defined per-field but we may want to
           // make it per-instance instead, so leaving the possibility open for further
           // easier refactoring.
      

      Is this comment obsolete now?

    2. +++ b/core/modules/content_translation/content_translation.module
      @@ -114,25 +115,22 @@ function content_translation_entity_bundle_info_alter(&$bundles) {
           foreach ($translation_settings as $bundle => $settings) {
      

      Seems like this local variable now conflicts with the new $bundle function parameter? My naive assumption is that we should remove the loop, and just use the $bundle passed to the function, but I don't know if we still need this loop for when $bundle is NULL.

    effulgentsia’s picture

    What seems to weird about the current patch is that I think it never invokes hook_entity_field_info() with $bundle == NULL, does it? It seems to go with the old behaviour of using the entity type instead.

    I don't see where it uses entity type as bundle, do you? Instead, looks to me like the hook gets called with $bundle = NULL when EntityManager::getFieldDefinitions() itself gets called with $bundle = NULL, and it gets called with a non-null $bundle when EntityManager::getFieldDefinitions() is called with a non-null $bundle. But what that means is that the hook, unlike the baseFieldDefinitionsByBundle() method, must return the complete set of fields that module is responsible for for that bundle, even if they're not bundle-specific. E.g., path_entity_field_info() is called once for each bundle, and returns a new FieldDefinition object for the path field each time, even though the contents of that definition doesn't vary by bundle.

    Maybe we should split the hook into two as we do for the methods

    That would be one way to achieve consistency. Another would be to remove the baseFieldDefinitionsByBundle() method and instead add a $bundle parameter to baseFieldDefinitions() to make it match the hook. When we discussed this in Prague, the argument against the latter was that it would be memory inefficient to call baseFieldDefinitions() repeatedly for different $bundle and keep getting back new objects containing identical content as ones we already have. But, with the current patch, we have a @todo to figure out better DX for baseFieldDefinitionsByBundle() and an inconsistency between the methods and the hooks. Would it be better instead to have this patch go the memory inefficient route, but not introduce bad DX and an inconsistency, and then have a follow up for optimizing memory and DX?

    effulgentsia’s picture

    Re #40: seems like the changes you and fago are proposing aren't needed by this issue any more, so I'll wait till we have a new issue created for it to continue that discussion.

    The last submitted patch, 55: d8_field_definition_bundles-2114707-55.patch, failed testing.

    effulgentsia’s picture

    yched’s picture

    content_translation_entity_bundle_info_alter(&$bundles) {
         // Currently field translatability is defined per-field but we may want to
         // make it per-instance instead, so leaving the possibility open for further
         // easier refactoring.

    Is this comment obsolete now?

    Not yet, but that would be the scope of #2143291: Clarify handling of field translatability, which I just repurposed as discussed in #54 above

    fago’s picture

    Re #40: seems like the changes you and fago are proposing aren't needed by this issue any more, so I'll wait till we have a new issue created for it to continue that discussion.

    Yep, seems reasonable.

    But, with the current patch, we have a @todo to figure out better DX for baseFieldDefinitionsByBundle() and an inconsistency between the methods and the hooks.

    The @todo is now limited to how we do the field overrides, the general approach of baseFieldDefinitionsByBundle() would work for me.

    Would it be better instead to have this patch go the memory inefficient route, but not introduce bad DX and an inconsistency, and then have a follow up for optimizing memory and DX?

    The problem with the memory inefficient route is that it would require a rather major API change - or quite some work-a-rounds - in order to make it work memory efficiently. But given how late we are in the cycle we really should avoid 2 iterative API changes in favour of one that cuts it.

    Imo, the bad DX is of baseFieldDefinitionsByBundle() is limited to the necessary clone operation. So fixing that would mean replacing the clone operation with e.g. a helper object/class, i.e. it can be improved without changing how baseFieldDefinitionsByBundle() overally works.

    So if we can agree on the overall DX of baseFieldDefinitionsByBundle() as it is now, I think aligning the hooks to that would be the better way to move on.

    Berdir’s picture

    Agreed on the byBundle() pattern being the better way forward.

    About the clone, now that we expect to get altered fields back from the method/hook, would it help to add an alter() method that would essentially hide away the clone? Might be a bit easier to understand, although it's still easy to forget, but I doubt we find a solution that solves that completely.

    $fields['title'] = $fields['title']->alter()
      ->setTitle($node_type->title_label);
    

    One idea that I had about enforcing to use that would be to have an internal frozen flag and then the flow in the build method on the entity manager would be:

    1. Call base field method
    2. Call for all bundles hook
    3. Freeze definitions
    4. Call by Bundle method
    5. Call by bundle

    That would allow us to throw an exception if you try to change anything directly in the by bundle methods, stating that you need to call alter() first. It's quite an overhead (in lines of code) though, as we would need to check this in every setter. And certainly something we could discuss/do in a follow-up.

    fago’s picture

    From our IRC discussion the proposal would be to have:
    1. base field definitions
    2. base field definitions by bundle
    3. hook
    4. hook by bundle
    5. alter hook

    As discussed, it would make more sense to have two separate alter hooks, i.e. one for 3 and one for 4.

    Berdir’s picture

    The last submitted patch, 55: d8_field_definition_bundles-2114707-55.patch, failed testing.

    Berdir’s picture

    The last submitted patch, 67: d8_field_definition_bundles-2114707-67.patch, failed testing.

    Berdir’s picture

    And now a version that might install ;)

    effulgentsia’s picture

    Status: Needs review » Needs work

    per #64.

    andypost’s picture

    1. +++ b/core/modules/field/field.attach.inc
      @@ -221,31 +221,18 @@ function field_invoke_method_multiple($method, $target_function, array $entities
      -  $field_definitions = array();
      +  $definitions = \Drupal::entityManager()->getFieldDefinitions($entity_type, $bundle);
      ...
      -      $field_definitions[] = $entity->get($options['field_name'])->getFieldDefinition();
      ...
      +    $definitions = array_intersect_key($definitions, array($options['field_name'] => TRUE));
      ...
      -      $field_definitions[] = $items->getFieldDefinition();
      ...
      -  else {
      -    foreach ($entity as $items) {
      

      Now result is keyed array...

    2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
      @@ -325,91 +330,113 @@ public function getAdminRouteInfo($entity_type, $bundle) {
         public function getFieldDefinitions($entity_type, $bundle = NULL) {
      
      +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
      @@ -321,14 +310,7 @@ public function getHighestWeight() {
      +    $definitions = \Drupal::entityManager()->getFieldDefinitions($this->targetEntityType, $this->bundle);
      +    return isset($definitions[$field_name]) ? $definitions[$field_name] : NULL;
      ...
         protected function getFieldDefinition($field_name) {
      

      Would be great to have getFielddefinition($entity_type, $bundle, $field_name)
      Probably follow-up

    Berdir’s picture

    Status: Needs work » Needs review
    FileSize
    32.16 KB

    Another re-roll.

    Status: Needs review » Needs work

    The last submitted patch, 72: d8_field_definition_bundles-2114707-72.patch, failed testing.

    Berdir’s picture

    Another one, fixing those tests. Want to have a green patch before I try to implement what we discussed.

    Status: Needs review » Needs work

    The last submitted patch, 74: d8_field_definition_bundles-2114707-74.patch, failed testing.

    The last submitted patch, 74: d8_field_definition_bundles-2114707-74.patch, failed testing.

    plach’s picture

    +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -312,90 +317,112 @@ public function getAdminRouteInfo($entity_type, $bundle) {
    +  protected function buildFieldDefinitions($entity_type, $bundle) {
    

    According to the PHP docs $bundle should be optional.

    Berdir’s picture

    Another re-roll and fixing EntityManagerTest.

    That one will need more work when we're done, I just dropped two methods for now as the bundle map no longer exists and the constraints stuff will be removed in the typed data issue.

    moshe weitzman’s picture

    Green! Anyone available to review? The perf team has identified this issue as causing slowness so it would be great if we could move quickly. Our notes say "Get rid of creating fake entities (expensive) because base field definition cannot vary by bundle". Hope this issue is the one to fix that.

    effulgentsia’s picture

    Status: Needs review » Needs work

    From what I can tell, this still doesn't address #64, so back to needs work.

    amateescu’s picture

    Assigned: Berdir » amateescu
    Status: Needs work » Needs review
    FileSize
    37.92 KB

    Our notes say "Get rid of creating fake entities (expensive) because base field definition cannot vary by bundle". Hope this issue is the one to fix that.

    Yep, this is the one. Here's where we remove the need for that fake entity:

    +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -326,20 +326,13 @@ protected function getFieldDefinition($field_name) {
    -      if ($entity_info->isSubclassOf('\Drupal\Core\Entity\ContentEntityInterface')) {
    -        $entity = _field_create_entity_from_ids((object) array('entity_type' => $this->targetEntityType, 'bundle' => $this->bundle, 'entity_id' => NULL));
    -        foreach ($entity as $field_name => $items) {
    -          $definitions[$field_name] = $items->getFieldDefinition();
    -        }
    -      }
    +      $definitions = \Drupal::entityManager()->getFieldDefinitions($this->targetEntityType, $this->bundle);
    

    I'm going to take a crack at #64 today, but let's get a painful reroll out of the way first. (NR is just for the bot)

    Status: Needs review » Needs work

    The last submitted patch, 81: d8_field_definition_bundles-2114707-81.patch, failed testing.

    amateescu’s picture

    Assigned: amateescu » Unassigned
    Status: Needs work » Needs review
    FileSize
    41.37 KB
    14.41 KB

    The patch attached implements #64, with the following order:

    1. base field definitions
    2. hook
    3. alter hook
    4. base field definitions by bundle
    5. hook by bundle
    6. alter hook by bundle

    I'm not sure if 5. should receive an additional $base_field_definitions parameter like ContentEntityInterface::baseFieldDefinitionsByBundle does.

    Berdir’s picture

    Nice work, thanks so much!

    1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
      @@ -375,33 +389,27 @@ protected function buildFieldDefinitions($entity_type_id, $bundle = NULL) {
       
      -    // Automatically set the field name for non-configurable fields.
      -    foreach ($field_definitions as $field_name => $definition) {
      -      if ($definition instanceof FieldDefinition) {
      -        $definition->setName($field_name);
      -      }
      

      Are we sure that we don't need this for by bundle fields?

    2. +++ b/core/modules/field/field.module
      @@ -184,13 +184,11 @@ function field_system_info_alter(&$info, $file, $type) {
      +function field_entity_field_info_by_bundle($entity_type_id, $bundle) {
      +  // Define all configurable fields, which are always set for a specific bundle.
      +  return Field::fieldInfo()->getBundleInstances($entity_type_id, $bundle);
       }
      

      I think the discussed ideas about a field storage definition interface that is returned in hook_entity_field_info() won't work, because it's still not the same behavior. That would return fields that might exist on any of the bundles (1-n), not all of them, no matter what bundle it is, which is what hook_entity_field_info() is about.

    3. +++ b/core/modules/node/tests/modules/node_access_test/node_access_test.module
      @@ -80,11 +80,11 @@ function node_access_test_permission() {
        */
      -function node_access_test_entity_field_info($entity_type, $bundle) {
      -  if ($entity_type === 'node') {
      -      $fields['private'] = FieldDefinition::create('boolean')
      -        ->setLabel(t('Private'))
      -        ->setComputed(TRUE);
      +function node_access_test_entity_field_info($entity_type_id) {
      +  if ($entity_type_id === 'node') {
      +    $fields['private'] = FieldDefinition::create('boolean')
      +      ->setLabel(t('Private'))
      +      ->setComputed(TRUE);
       
           return $fields;
         }
      

      Based on the changed code, we actually have a ENTITY_TYPE_field_info(), so we could simplify this a bit as we touch it. Don't care much, though.

    Apart from those minor issues and the asked question about passing the base fields to the by_bundle() hook, the changes look great and I think should address the concerns that were raised earlier. Needs more reviews! :)

    fago’s picture

    This looks already pretty good. I found #84.4 as well, imho we should fix this to promote the existence of this hooks, but that's no biggie, yes.

    The only thing in the patch that still worries me is that we use the default bundle of entity types without bundles (user -> user). Those fields configured for all users won't apply to all users any more once a module introduces different kind of user bundles. I think it would be better to move the configurable fields that the user assigns to the entity type (+default bundle) to $bundle = NULL, so it's properly mapped to be a cross-bundle field?

    +++ b/core/modules/system/entity.api.php
    @@ -664,57 +664,98 @@ function hook_entity_form_display_alter(\Drupal\Core\Entity\Display\EntityFormDi
    + * Define custom entity fields which are specific to a bundle.
    + *
    + * @param string $entity_type_id
    ...
    + * @see \Drupal\Core\Entity\EntityManagerInterface::getFieldDefinitions()
    

    We should clarify here, that this can be used to add in overrides as you can do in baseFieldDefinitionsByBundle().

    yched’s picture

    Needs reroll :-)

    yched’s picture

    re @Berdir #84:

    1. Agreed, we should probably do that check for bundle fields too - the sample code in hook_entity_field_info_by_bundle() does not do a ->setName() on the field it adds :-p.
    This could be moved to a prepareFieldDefinitions() method along with the code about "Ensure all basic fields are not defined as translatable", which is done on base & bundle fields too.

    2. Well, that notion of "fields that exist on all bundles" is shaky IMO, since a configurable field might very well be present on all bundles. Why should such a field not be returned by buildFieldDefinitions($entity_type) ? What's the difference between node title and node body if "body" is present on all bundles ?

    We had an unresolved discussion with @fago about this in Vienna - when you call buildFieldDefinitions($entity_type), what is the exact question you're asking ? "give me all the *what* ?" And for which kind of use cases is the answer we're currently giving relevant ?

    i.e : a good API is clear about what "question" each method method lets you ask and what answer you get. I'd be incapable of explaining what buildFieldDefinitions($entity_type) means as opposed to buildFieldDefinitions($entity_type, $bundle)

    But well, leaving that alone here :-)

    3. True - also, those ENTITY_TYPE hook variants are not documented in entity.api.php :
    hook_ENTITY_TYPE_field_info()
    hook_ENTITY_TYPE_field_info_alter()
    hook_ENTITY_TYPE_field_info_by_bundle()
    hook_ENTITY_TYPE_field_info_by_bundle_alter()
    More generally - all of those are used to build cached stuff. Do we really need the ENTITY_TYPE hook variants ?

    Review coming next.

    yched’s picture

    1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
      @@ -62,6 +65,30 @@ public function initTranslation($langcode);
      +   * @todo Provide a better DX for field overrides.
      

      Yeah, the current clone approach remains a bit painful :-)
      Should we open a followup and link it here ?

    2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
      @@ -297,94 +302,124 @@ public function getAdminRouteInfo($entity_type_id, $bundle) {
         public function getFieldDefinitions($entity_type_id, $bundle = NULL) {
      ...
      +  protected function buildFieldDefinitions($entity_type_id, $bundle = NULL) {
      

      Looks like in practice EM::$baseFieldDefinitions & EM::$fieldDefinitions are duplicate ?

      On cold caches:
      - getFieldDefinitions($entity_type) calls buildFieldDefinitions($entity_type),
      - buildFieldDefinitions($entity_type) computes the "list of base fields" , puts it in $this->baseFieldDefinitions[$entity_type] and returns it
      - getFieldDefinitions($entity_type) then puts that same result in $this->fieldDefinitions[$entity_type]
      But it's the same data :-)

      It doesn't help IMO that both getFieldDefinitions() & buildFieldDefinitions() operate in two largely different "modes" ($bundle NULL or not NULL). It would be clearer to split buildFieldDefinitions() in two separate methods :

      • getFieldDefinitions($entity_type) :
        calls buildFieldDefinitions($entity_type),
        and caches in $this->baseFieldDefinitions
      • getFieldDefinitions($entity_type, $bundle) :
        calls buildFieldDefinitionsByBundle($entity_type, $bundle),
        and caches in $this->fieldDefinitionsByBundle
        • buildFieldDefinitionsByBundle($entity_type, $bundle)
          calls getFieldDefinitions($entity_type) as a 1st step (leveraging static & db cache),
          and then does the "by bundle" steps on top of that

      ?

      I'd probably even advocate splitting getFieldDefinitions() itself :

      • getBaseFieldDefinitions($entity_type)
        calls buildBaseFieldDefinitions($entity_type)
        caches in $this->baseFieldDefinitions
      • getFieldDefinitions($entity_type, $bundle)
        calls buildFieldDefinitions($entity_type, $bundle)
        caches in $this->fieldDefinitions
    3. +++ b/core/modules/content_translation/content_translation.module
      @@ -145,25 +146,22 @@ function content_translation_entity_bundle_info_alter(&$bundles) {
      -    foreach ($fields as $name => $translatable) {
      -      foreach ($keys as $key) {
      -        if (isset($info[$key][$name])) {
      -          $info[$key][$name]->setTranslatable((bool) $translatable);
      -          break;
      -        }
      +    foreach ($field_settings as $name => $translatable) {
      +      if (isset($fields[$name]) && $fields[$name] instanceof FieldDefinition) {
      +        $fields[$name]->setTranslatable((bool) $translatable);
      +        break;
             }
      

      The break statement looks wrong. Now that there's only one loop left instead of two nested loops, it exits the foreach ($field_settings), which is not what we want.

    4. --- a/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
      +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
      
      

      Leftover "use Drupal\Core\Field\FieldDefinition;" at the top.

    5. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
      @@ -453,4 +452,17 @@ public static function baseFieldDefinitions($entity_type) {
      +  public static function baseFieldDefinitionsByBundle($entity_type, $bundle, array $field_definitions) {
      +    $node_type = node_type_load($bundle);
      +    $fields = array();
      +    if (isset($node_type->title_label)) {
      +      $fields['title'] = clone $field_definitions['title'];
      +      $fields['title']->setLabel($node_type->title_label);
      +    }
      +    return $fields;
      +  }
      

      Yay !
      The existing @todo about node title in Node::baseFieldDefinitions() can be removed :-)

    6. +++ b/core/modules/system/entity.api.php
      @@ -664,57 +664,98 @@ function hook_entity_form_display_alter(\Drupal\Core\Entity\Display\EntityFormDi
      + * @see \Drupal\Core\TypedData\TypedDataManager::create()
      + */
      +function hook_entity_field_info_by_bundle($entity_type_id, $bundle) {
      

      Minor : do we really need that @see TypedDataManager here and in hook_entity_field_info() ?
      Not really related, and lots of @sees already :-)

    7. +++ b/core/modules/system/entity.api.php
      @@ -664,57 +664,98 @@ function hook_entity_form_display_alter(\Drupal\Core\Entity\Display\EntityFormDi
      +function hook_entity_field_info_by_bundle($entity_type_id, $bundle) {
      +  if (mymodule_uses_entity_type_and_bundle($entity_type_id, $bundle)) {
      +    $fields = array();
      +    // Add a property only to nodes of the 'article' bundle.
      +    if ($entity_type_id == 'node' && $bundle == 'article') {
      

      Minor: The code sample is a bit contradictory : does it do generic checks on $entity_type / $bundle, or is it hardcoded on 'node' / 'article' ?

      Maybe we should remove the mymodule_uses_*() metaphor and go with simpler hardcoded logic on specific $entity_type & $bundle in these examples for clarity ?

    Berdir’s picture

    Re-roll and started to work on the reviews, thanks!

    #85/@fago: I'm aware that you think that, but has nothing to do with this issue. Right now, entity types must have a bundle, we can not change this here. The reason I had to change that call there was that it was wrong and only worked because of an incorrect implementation of field_entity_field_info() as discussed in #1919468: EntityManager::getAllBundleInfo() adds default entity_type_id bundle for entity types with bundles. That had a hardcoded check to make fields base fields when bundle == entity_type. This was wrong and already caused other hidden bugs, for example code in the entity query join stuff that forgot to pass the bundle along and then was missing all configurable fields and it only worked with entity_test due to that wrong implementation. Happy to discuss that in a separate issue, but please please not here .)

    #87:
    - Moved the setName() call down.
    - Yeah, that is still a bit fragile, but a) that's not what the storage interface idea was about as far as I understood it and it wouldn't help with that, and b) implying that a field that exists on every existing bundle then exists on all bundles is problemematic, because adding a new bundle could change that. I've discussed that before with fago in the context of single/no-bundle entities. For example a rule condition that uses such a field would suddenly be invalid if you add one more bundle as the field would "vanish".
    - Yeah, I'm considering to drop those hooks too, as they add 4 additional hook invocations per entity type/bundle and need to be documented/handled in unit tests and so on. The advantage they bring is minimal.

    #88:
    1. Yes, follow-up for that would be nice IMHO, we already have an improve DX issues based on feedback from Dries, which was related to definition classes by type, we could use that to discuss this?

    2. Good points. Need to think a bit about this but you're right that they are the same. The current code is based on a step by step refactoring from current HEAD. So leaving this as it is to see if it is still green before making larger changes.

    Also slightly changes the meaning of base field, right now, it's IMHO only things defined by the entity class, not fields added by other modules, like the path alias stuff. Not saying that is wrong, just that it would be a change..

    3. Removed the break, I think that's what you are saying?

    4. Removed.

    5. Removed todo

    6. Yes, removed. I think the reason it is there is that various documentation on available types and possible definitions lives there, but it shouldn't and we have definition classes now which are self-documenting.

    7. Agreed, simplified and removed those example function calls everywhere.

    => ·@todo: removing ENTITY_TYPE hook versions and refactor the get/build field definition methods

    Status: Needs review » Needs work

    The last submitted patch, 89: d8_field_definition_bundles-2114707-89.patch, failed testing.

    fago’s picture

    ad #85/@fago & bundles:

    Happy to discuss that in a separate issue, but please please not here.

    I'd be happy to, but by bringing the change in here it brings the discussion with it ;) Howsoever, I give this more thought and posted a comment for further discussion to #1919468: EntityManager::getAllBundleInfo() adds default entity_type_id bundle for entity types with bundles. Summarizing, I agree this is the right thing to do.
    Howsoever, we need clarification on how we properly deal with the no-bundle case, so please read my comment at #1919468: EntityManager::getAllBundleInfo() adds default entity_type_id bundle for entity types with bundles and respond there.

    3. True - also, those ENTITY_TYPE hook variants are not documented in entity.api.php :
    hook_ENTITY_TYPE_field_info()
    hook_ENTITY_TYPE_field_info_alter()
    hook_ENTITY_TYPE_field_info_by_bundle()
    hook_ENTITY_TYPE_field_info_by_bundle_alter()
    More generally - all of those are used to build cached stuff. Do we really need the ENTITY_TYPE hook variants ?

    Yeah, I think the entity-type specific hooks are useful and should stay. When doing d8 projects adding custom (computed) fields per entity type will be something often needed imo.

    The entity-type specific documentation is another topic. It's mostly missing anyway, e.g. consider all crud hook documentation for all config entities (hook_block_update(), etc.) - it's non-existing. I guess we should open an issue for discussing that. I could see as dropping it in favour of mentioning it at the general hooks only - as we do it with per form-id hooks.

    +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -401,6 +395,12 @@ protected function buildFieldDefinitions($entity_type_id, $bundle = NULL) {
    +    // Automatically set the field name for non-configurable fields.
    +    foreach ($base_definitions as $field_name => &$base_definition) {
    

    This needs to run before the alter hook.

    yched’s picture

    Also slightly changes the meaning of base field, right now, it's IMHO only things defined by the entity class, not fields added by other modules, like the path alias stuff. Not saying that is wrong, just that it would be a change

    Yeah, this is precisely what I find shaky currently :-).

    - "Base fields":
    According to the current code = fields defined in [EntityClass]::baseFieldDefinitions() + baseFieldDefinitionsByBundle()
    It's not a real notion you can build on, though, since there's no API that returns a list of "base fields", current API always merges with fields added by 3rd party hooks. So even though we refer to it in a lot of discussions, code comments or docs, API wise, the notion of "base fields" doesn't exist.

    - fields returned by EM::getFieldDefinitions($entity_type, NULL):
    It's still not clear to me what that represents, or what you should use that list for. It's not the list of "fields potentially available on an entity type across bundles", which would be the actual use case for e.g Views or Panels, since those need to include configurable fields (so this API need is unadressed for now, other than by field_info_fields(), which is *only* configurable fields)
    It's there to adress Rules use cases, but I've still to hear a clear definition of the concept in generic, non-Rules terms.

    - fields returned by EM::getFieldDefinitions($entity_type, $bundle):
    The list of fields you'll find in an actual $entity. It's the only clear & solid concept at the moment IMO.

    The win of the patch as it stands is to give us that last one, which is currently missing. It's very valid in itself, and lets us clean lots of nasty workarounds throughout current core. So yeah, we probably want to get it in asap & push the conceptual clarifications on the stack above that to #2116363: Unified repository of field definitions (cache + API)...

    fago’s picture

    @base-fields:

    So even though we refer to it in a lot of discussions, code comments or docs, API wise, the notion of "base fields" doesn't exist.

    berdir and me just discussed on IRC before. So far we've used base fields as "the fields which come with an entity type, i.e. across bundle and bundle-specific - but as you say that has no API implication. But furthermore we have across-bundle entity fields which can be added in by other modules. So to avoid confusion with yet another concept/term we've been thinking about defining "base fields" as "fields across bundles" vs bundle-specific fields. With that definition of base fields the path alias field would be a base field as well, defined by path module.

    I like this idea as it avoids us having to differentiate between yet another sort of field, we'd have just base-fields and by bundle fields.

    - fields returned by EM::getFieldDefinitions($entity_type, NULL):

    It's the list of fields available on entities of this type, regardless of which bundle they are. Going with the meaning of "base fields" as "fields across bundles", it'd give you the base fields of an entity type. :-)

    Rules would use it yes, but any other module could use it as well. It's up to the module to decide whether it wants to provide configuration options only for fields that are there for sure or for all possibly there fields. But, yeah adding other options is definitly something for #2116363: Unified repository of field definitions (cache + API).

    andypost’s picture

    At the same time base fields should be used in storage controller for schema-less saving, so they still needed, not sure are the need to alter them

    yched’s picture

    @fago' #93 works for me - then the method names and code flow suggested in #88.2 would make sense IMO.

    @andypost makes a good point though: does this definition of "base fields" map to storage layout ?

    fago’s picture

    I think for that we should do as discussed in Prague and add a module key to the field definitions; see #2143069: Add getProvider() and hasCustomStorage to FieldDefinitionInterface.

    Regarding #88.2, do you mean?

        getBaseFieldDefinitions($entity_type)
        calls buildBaseFieldDefinitions($entity_type)
        caches in $this->baseFieldDefinitions
        getFieldDefinitions($entity_type, $bundle)
        calls buildFieldDefinitions($entity_type, $bundle)
        caches in $this->fieldDefinitions

    getBaseFieldDefinitions() would be fine to me, I'm not so sure about getFieldDefinitions(). Strictly speaking a base field definition is a field definition also, but you'd not be able to define it Entity::fieldDefinitions()?

    Maybe getBaseFieldDefinitions() and getBundleFieldDefinitions() would be an option that makes the relationship clearer?
    Along with the hooks hook_entity_base_field_info($entity_type), hook_entity_bundle_field_info($entity_type, $bundle) ?

    Not sure, if you think getBaseFieldDefinitions() and getFieldDefinitions() is be clear enough, I'd be ok with that.

    yched’s picture

    a) getBaseFieldDefinitions($entity_type), getFieldDefinitions($entity_type, $bundle)
    b) getBaseFieldDefinitions($entity_type), getBundleFieldDefinitions($entity_type, $bundle)

    Yeah, not 100% sure either.

    I guess my reasons for favoring a) would be:
    - I'd rather have "get the list of fields for this $e_t and $bundle" method be the most intuitive / easiest to find, since AFAIK it's going to be by far the most common use case.
    - The fact that getFieldDefinitions() has a non-optional $bundle argument makes it clear that this is about "a specific bundle", without needing to get overly verbose in the method name.
    - I feel b) gives a wrong impression that there's a notion of "bundle field" next to the notion of "base field".

    Also, true, the hooks should probably be named in a way that relates to the method they are tied to, which might be part of the naming equation...

    Berdir’s picture

    Need to read the whole discussion, but a) sounds good to me as well. And yes, the methods on the entity classes and hooks should then probably be re(-named) as possible.

    Yeah, I think the entity-type specific hooks are useful and should stay. When doing d8 projects adding custom (computed) fields per entity type will be something often needed imo.

    Yeah. And all you save there is a simple if ($entity_type_id == 'node'). Often you will do it for a specific bundle ony anyway, so you will need an if () anyway, and just two instead of one condition. That's not that complicated. This is not the same as e.g. hook_node_presave(), where you can type hint as NodeInterface.

    As mentioned, we'd save *4* hook invocations per entity_type/bundle for that additional line. That's imho worth to think about. It was fine when we had 2 + 2 alters including per entity_type, but now we suddenly have 8.

    Will try to update the patch tomorrow.

    fago’s picture

    - The fact that getFieldDefinitions() has a non-optional $bundle argument makes it clear that this is about "a specific bundle", without needing to get overly verbose in the method name.

    True. And yeah, we don't want to create a new notion of "bundle field" so this is a good point for a)

    However, it came to my mind that this might lead people to do

    function fieldDefinititions($entity_type, $bundle) {
      $fields['langcode'] = FieldDefinition::create('language')
          ->setLabel(t('Language code'))
          ->setDescription(t('The node language code.'));
      return $fields;
    }
    

    But that would result in a per-bundle copy of the field instead of a single field across all bundles?

    Often you will do it for a specific bundle ony anyway, so you will need an if () anyway, and just two instead of one condition.

    Not if you are adding a base field ;-) I must say that I'm not concerned by the number of hook invocations, they should be pretty fast, and the result is cached anyway. But yeah, we could still add per entity type hook if it turns out they would be useful later on.

    yched’s picture

    Hm, yeah.
    - EM::getFieldDefinitions($entity_type, $bundle) is what we want to promote to the outside world.
    - [Entity]::fieldDefinitions($entity_type, $bundle) is *not* what we want to promote for entity type providers
    Which kind of collides when deciding which one to put forward...

    Then maybe we do disconnect naming for the "by bundle" variants between the "outside facing API" and the "internal builders & collectors" ?

    • EM::getBaseFieldDefinitions($entity_type) / EM::buildBaseFieldDefinitions($entity_type)
      [Entity]::baseFieldDefinitions($entity_type)
      hook_entity_base_field_info($entity_type) + _alter()
    • EM::getFieldDefinitions($entity_type, $bundle) / EM::buildFieldDefinitionsByBundle($entity_type, $bundle)
      [Entity]::fieldDefinitionsByBundle($entity_type, $bundle)
      hook_entity_field_by_bundle_info($entity_type, $bundle) + _alter()

    Regarding hook_ENTITY_TYPE_*() hooks:
    I'm not too concerned about performance either, which is why I'm not sure the ENTITY_TYPE variants are useful :-)
    AFAIK, those more specific variants are used to avoid too many calls to a single generic hook on mission-critical code paths, that would result in a lot of no-ops after "if ($trivial_condition)" checks in lots of implementations.
    I don't think that it's a concern here ?

    fago’s picture

    The naming suggestion of #100 seems good to me!

    I'm not too concerned about performance either, which is why I'm not sure the ENTITY_TYPE variants are useful :-)

    So, we've the per-type variants for performance and type-hinting, neither is a reason here. Still, I'd say hook_node_base_field_info() has a better DX though, e.g.
    I'd see

    function mymodule_node_base_field_info() {
      $fields['myfield'] = FieldDefinition::create('string')
          ->setComputed(TRUE)
          ->setLabel(t('Custom'));
      return $fields;
    }
    

    nicer than

    function mymodule_entity_base_field_info($entity_type) {
      if ($entity_type == 'node') {
        $fields['myfield'] = FieldDefinition::create('string')
            ->setComputed(TRUE)
            ->setLabel(t('Custom'));
      }
      elseif ($entity_type == 'foo') {
        //..
      }
      return $fields;
    }
    

    No biggie though.

    Berdir’s picture

    I completely agree that it would be nicer. The only question is, is it worth the effort (additional calls, code, documentation, test coverage and unit test mocking).

    Especially the last part is uglier than you might think, EntityManagerTest is currently quite ugly to work with, as you need to set up tons of mocked calls (so that it returns an empty array and not NULL, otherwise it results in array_merge($array, NULL) or similar. I had to mess with that here and in #2039435: Convert EntityManager to extend DefaultPluginManager for example, and they're going to conflict quite a bit. And we'd kind of need test coverage for every single of those hooks :)

    If it's OK, I'm going to roll a patch tonight without per-entity type hooks and if @fago is OK with it, we can still open a follow-up issue to consider to add them again? I don't want to block this issue on discussing them and the documentation and test coverage gates that we're supposed to have :)

    fago’s picture

    Works for me - I guess we should rethink how we test and document all of this hooks, but yeah - this really shouldn't postpone this one.

    Berdir’s picture

    #100 it is!

    Implemented that, dropped the entity_type hooks for now (will open a follow-up asap), updated calls to it. Also had to make setting the bundle in ContentEntityBase::getDataDefinition() non-conditional and EntityDataDefinition call the correct method, to be consistent with the changed behavior.

    yched’s picture

    Yay, looks much cleaner IMO.

    Can't do a full review atm, just :

    +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
    @@ -804,7 +773,9 @@ public function setDiscovery(DiscoveryInterface $discovery) {
       public function testClearEntityFieldInfo() {
    +    $this->baseFieldDefinitions = array();
    +    $this->baseFieldDefinitions = array();
    

    It should be empty after that :-)

    Status: Needs review » Needs work

    The last submitted patch, 104: d8_field_definition_bundles-2114707-104.patch, failed testing.

    The last submitted patch, 104: d8_field_definition_bundles-2114707-104.patch, failed testing.

    Berdir’s picture

    Ah, but you can never be sure! We should probably do something like this:

    // Safe delete all base fields.
    for ($i = 0; $i < 7; $i++) {
      $this->baseFieldDefinitions = array();
    }
    

    :)

    Had to grep for the test fails, let's see if I found all of them. Stupid phpunit integration :(

    This somehow had an interesting side effect on the Comment Validator Test, previously, the comment_body field was ignored but it is required, I guess it's related to the bundle handling?

    fago’s picture

    1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
      @@ -296,87 +294,129 @@ public function getAdminRouteInfo($entity_type_id, $bundle) {
      -        // Ensure all basic fields are not defined as translatable.
      ...
      -        $untranslatable_fields = array_flip(array('langcode') + $keys);
      

      This is missing from getBaseFieldDefinitions().

    2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
      @@ -296,87 +294,129 @@ public function getAdminRouteInfo($entity_type_id, $bundle) {
      +    $field_definitions = $class::baseFieldDefinitionsByBundle($entity_type_id, $bundle, $base_field_definitions) + $base_field_definitions;
      

      This shouldn't include the $base_field_definitions in the result. It should only build overrides / per bundle fields and merge upon retrieval, after loading from cache.

    3. +++ b/core/lib/Drupal/Core/Entity/TypedData/EntityDataDefinition.php
      @@ -60,8 +60,12 @@ public function getPropertyDefinitions() {
      -        $bundle = is_array($bundles) && count($bundles) == 1 ? reset($bundles) : NULL;
      ...
      +        if (is_array($bundles) && count($bundles)) {
      +          $this->propertyDefinitions = \Drupal::entityManager()->getFieldDefinitions($entity_type_id, reset($bundles));
      

      If there is more than 1 bundle, adding in the bundle fields of the first bundle is not valid. That's why it has check for 1 bundle.

    4. +++ b/core/modules/field/field.attach.inc
      @@ -124,31 +124,18 @@ function field_invoke_method($method, $target_function, EntityInterface $entity,
      - *   - deleted
        *   - field_name
      - *   - field_id
      

      Unrelated?

    5. +++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.php
      @@ -357,7 +357,7 @@ public function testIntrospection() {
      +    $definitions = \Drupal::entityManager()->getFieldDefinitions($entity_type, $entity_type);
      

      That looks weird. Maybe add a comment to explain it.

    6. +++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Entity/EntityTestRev.php
      @@ -30,7 +30,8 @@
      + *     "bundle" = "type",
      + *     "label" = "name",
      

      Unrelated?

    yched’s picture

    re: doc change in field.attach.inc : yeah not strictly related, this just updates a stale phpdoc to reflect the actual current state of the code.
    Don't sweat too much on it, field.attach.inc is removed in #2095195: Remove deprecated field_attach_form_*() anyway.

    Berdir’s picture

    Re #109:

    1. Is it possibe that you reviewed the previous patch? I moved it from byBundle to Base in the last one.
    2. Good point, let's see if I got that right.
    3. Agreed, I did not mean to remove the == 1. That said, multiple bundles are broken either way ;)
    5. Comment added. Not that different from a ('user', 'user').
    6. I don't remember the exact reason but I think it was required to fix something.

    yched’s picture

    +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -392,7 +394,7 @@ protected function buildFieldDefinitionsByBundle($entity_type_id, $bundle) {
         // Allow the entity class to override the base fields.
    -    $field_definitions = $class::baseFieldDefinitionsByBundle($entity_type_id, $bundle, $base_field_definitions) + $base_field_definitions;
    +    $field_definitions = $class::baseFieldDefinitionsByBundle($entity_type_id, $bundle, $base_field_definitions);
    

    The comment is no longer relevant here, then.

    Could maybe move in getFieldDefinitions() :

    if ($cache) {
      $field_definitions_by_bundle = $cache->data;
    }
    else {
      $field_definitions_by_bundle = buildFieldDefinitionsByBundle();
      cache_set($field_definitions_by_bundle);
    }
    // Allow the per-bundle fields to override the base fields.
    $this->fieldDefinitionsByBundle[$entity_type_id][$bundle] = $field_definitions_by_bundle + getBaseFieldDefinitions();
    
    yched’s picture

    Bleh, never mind #112, I guess the comment still kind of applies - invoking $class::baseFieldDefinitionsByBundle() & actually merging the results happen in separate places, so where the "override" actually happens is debatable :-)

    Berdir’s picture

    Issue summary: View changes
    FileSize
    54.83 KB
    66.05 KB
    17.04 KB

    Either way, looks like we definitely arrived in the nitpick phase, so we're hopefully close :)

    Left the comment and added a new one to getFieldDefinitions().

    Also renamed the baseFieldDefinitionsByBundle() to fieldDefinitionsByBundle() now, missed that one. And renamed $entity_type to $entity_type_id for baseFieldDefinitions(), so everything should be consistent now.

    Updating the issue summary, a change notice is next.

    The last submitted patch, 114: d8_field_definition_bundles-2114707-108.patch, failed testing.

    Status: Needs review » Needs work

    The last submitted patch, 114: d8_field_definition_bundles-2114707-114.patch, failed testing.

    tstoeckler’s picture

    Couldn't find anything standing out on a cursory review. The node example does in fact show that the DX could be improved, IMO, but there's a @todo for that, and the current way of doing it is documented well, so I think that's fine.

    Found one nitpick:

    +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -296,87 +294,135 @@ public function getAdminRouteInfo($entity_type_id, $bundle) {
    +  }
    +  /**
    

    Missing newline.

    sun’s picture

    I can't really comment on the patch, because I'm still not very familiar with the internals yet. (Looks good! ;))

    Just one question: Can we rename the hooks to the following?

    hook_entity_field_base_info()
    hook_entity_field_bundle_info()

    For consistency, simplicity, namespacing, and clarity. If the latter clashes with an existing hook, then simply make "field" plural ("fields").

    tstoeckler’s picture

    Re #118: I'm all for consistency and namespacing, but I'm not sure whether that's a good pattern here. Right next to each other the two look good, but given that I wouldn't know the purpose already and see "hook_field_bundle_info()" I'd think it was used to declare bundles of fields or something like that.

    I'm not necessarily opposed to the suggestion, just wanted to bring that point up, as well.

    Berdir’s picture

    Hm, not sure about that @stoeckler has a good point.

    Also, PhpStorm told me that the LogicException wasn't documented.

    Edit: The reason for those test fails were that the changes resulted in a different order of the fields (by bundle fields being first), found the neat little function array_replace() that allows us to keep the order, even if base fields are overridden.

    Berdir’s picture

    Change notice prepared: [#2205235]

    I think we're pretty much ready to go, apart from the hook name suggestion by @sun. @yched, @fago?

    yched’s picture

    Started to nitpick on docs, but it turned out to be faster (and friendlier) to do a pass on this myself. Notably tried to unify the formulations between the various methods and hooks - and cleanup "EntityNG vs Field API"-era formulations like "entity field" ;-)

    Other than that, my only remaining code remark:

    +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -296,87 +294,140 @@ public function getAdminRouteInfo($entity_type_id, $bundle) {
    +    // Invoke 'per bundle' alter hook.
    +    $this->moduleHandler->alter('entity_field_info_by_bundle', $field_definitions, $entity_type_id, $bundle);
    +
    +    return $field_definitions;
    

    Shouldn't we do as in buildBaseFieldDefinitions() and ensure that the "basic fields" are not made translatable in their "by bundle" version either ? Earlier versions of the patch did that IIRC.
    The code could be moved to a helper checkFieldDefinitions() method called from both build*() methods ?

    Similarly, the code that adds ->setName() and ->setTargetEntityTypeId() on non-configurable fields could be moved to a prepareFieldDefinitions() method ?

    It would be simpler if all of those could happen in one single helper. The translatability check needs to happen after the alter hook, and striclty speaking it's safer to do the setName()/setTargetEntityTypeId() before the alter.
    alter implementations receive the $entity_type_id as an arg, and the field names are the array keys, so maybe it's no biggie if they are not always present in the definition object themselves at this stage ?

    tstoeckler’s picture

    +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -296,87 +294,138 @@ public function getAdminRouteInfo($entity_type_id, $bundle) {
    +    $keys = array_intersect_key(array_filter($entity_type->getKeys()), array_flip(array('id', 'revision', 'uuid', 'bundle')));
    +    $untranslatable_fields = array_flip(array('langcode') + $keys);
    

    Re @yched: If we want to do that, we probably want to fix this code, as well. I didn't bring this up earlier because it's not strictly related, but #2143729: Entity definitions miss a language entity key showed that it doesn't work as expected. $keys has *keys* of 'id', 'revision', etc. and then an array with an integer key is +'ed onto that. That means that currently the 'langcode' field of an entity can be marked as translatable without the exception being thrown, but a theoretical field with name 0 cannot.

    fago’s picture

    Yes, #109 was based on #104. Too much progress ;)

    I had the same thinking as #119 on the rename - it suggests me something else. Therefore, I'd prefer the existing variant.

          // Allow the entity class to override the base fields.
          $base_field_definitions = $this->getBaseFieldDefinitions($entity_type_id);
          $field_definitions = $class::fieldDefinitionsByBundle($entity_type_id, $bundle, $base_field_definitions);
      
          // Invoke 'per bundle' hook.
          $result = $this->moduleHandler->invokeAll('entity_field_info_by_bundle', array($entity_type_id, $bundle));
          $field_definitions = NestedArray::mergeDeep($field_definitions, $result);

      Great to see #119.1 resolved already. However, the hooks should receive the same parameters as the method, i.e. $base_field_definitions is missing from them. You might want to add an override in the hook - just as in the method.

          $base_field_definitions = $class::baseFieldDefinitions($entity_type_id);
      
          // Invoke hook.
          $result = $this->moduleHandler->invokeAll('entity_base_field_info', array($entity_type_id));

      Generally, I wonder whether we should pass on the $entity_type domain object now? Seems preferable compared to passing around ids.

      Nitpicks:

    1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
      @@ -50,41 +50,45 @@ public function initTranslation($langcode);
      -   * Defines or alters base field definitions for the given bundle.
      +   * Provides or alters field definitions for a specific bundle within an entity type.
          *
      

      Too long now?

    2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
      @@ -50,41 +50,45 @@ public function initTranslation($langcode);
      +   * The field definitions returned here take precedence, for the bundle, on
      +   * the base field definitions specified by baseFieldDefinitions() for the
      

      confusing commas?

    3. +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
      @@ -42,16 +43,16 @@ public function getEntityTypeLabels();
      -   * Gets an array of content entity field definitions.
      +   * Gets the field definitions for a specific bundle within a content entity type.
      

      Comment is too long now.

    4. +++ b/core/modules/system/entity.api.php
      @@ -735,14 +735,14 @@ function hook_entity_field_info_by_bundle($entity_type_id, $bundle) {
      - * Alter bundle-specific entity fields.
      + * Alters field definitions for a specific bundle within an entity type.
      

      Alters bundle-specific field definitions. ?

      It does not contain the base-fields for the bundle, so you cannot alter them here. The comment would me suggest I can though.

    fago’s picture

    The issue summary needs an update as it misses the API changes of the hooks.

    yched’s picture

    Adresses :
    #124 1. 3. Oops, IIRC at some point function descriptions were allowed to exceed 80 chars if needed, but it seems not anymore.
    #124 2. Reworded

    Left 4 out as it relates to the points at the beginning of #174, about what the hook receives.

    Berdir’s picture

    - Added $base_field_definitions to by_bundle(), alter hooks can only have two context arguments, if we want to pass the base fields to that one as well we need to make an array out of entity_type_id/bundle :(
    - Either way, you can only alter the bundle specific fields, changing base fields will have no effect, so going with fag's suggestion for 4.
    - Also found another place that we can simplify now. BTW, I did some quick tests on a relatively simple 8.x site that we're experimenting with and we're spending 22% of the whole request in _field_create_from_ids() when displaying a few different nodes (of different types), this definitely has a huge performance impact on real sites.
    - In regards to checking key fields (or however we want to call them), I thought about it too, but my argument for not doing it was that it is probably not allowed to do *any* by-bundle changes on those, so not sure we need to revalidate?

    Changing to passing $entity_type around sounds interesting, we're changing all affected code already anyway, anyone opposed to doing that? Not sure it really is that useful, as there shouldn't be that many definitions where the fields would depend on the entity type definition? That said, field could possibly check if an entity type is fieldable, would save some calls on entity types which aren't?

    Uploading a patch without that, will follow up with one that does the rename.

    Berdir’s picture

    Well, let's try it. It does make the patch a bit bigger, due to the use's.

    The last submitted patch, 127: d8_field_definition_bundles-2114707-127.patch, failed testing.

    Status: Needs review » Needs work

    The last submitted patch, 128: d8_field_definition_bundles-2114707-128.patch, failed testing.

    Berdir’s picture

    Nice try EntityReferenceAutocompleteTest, attaching a field to an entity that is not fieldable :p

    Berdir’s picture

    Issue summary: View changes

    Updated issue summary, added the alter hooks.

    yched’s picture

    Regarding names: I agree with @sun #118 that _by_bundle() / ByBundle() is a bit ugly, but I agree with the others that the suggested replacements are not great either.

    Contrary to what I wrote in #97 :

    xxxBundleFieldDefinitions() / hook_entity_bundle_field*() gives a wrong impression that there's a notion of "bundle field" next to the notion of "base field"

    Well, that impression is not wrong, there is a notion of "bundle fields", they are the ones who are not "base fields" :), i.e those who are specific to a bundle.

    So we have :
    - methods/hooks that expose/collect/alter base fields only
    - methods/hooks that expose/collect/alter bundle fields only
    - EM::getFieldDefinitions() that uses the above to return all (base + bundle) fields merged together, for a given entity type & bundle.

    So maybe after all we could go with:

    • EM::getBaseFieldDefinitions($entity_type)
      EM::buildBaseFieldDefinitions($entity_type)
      [Entity]::baseFieldDefinitions($entity_type)
      hook_entity_base_field_info($entity_type)
      hook_entity_base_field_info_alter($entity_type)
    • EM::buildBundleFieldDefinitions($entity_type, $bundle)
      [Entity]::BundleFieldDefinitions($entity_type, $bundle)
      hook_entity_bundle_field_info($entity_type, $bundle)
      hook_entity_bundle_field_info_alter($entity_type, $bundle)
    • EM::getFieldDefinitions($entity_type, $bundle) - all the fields relevant for most runtime work, "base" or "bundle"

    ?

    The above shows that we miss a method on EntityManager that lets outside code get "just the bundle fields for $entity_type + $bundle" (cached), but I'm not sure there's an actual use case, can probably be added in #2116363: Unified repository of field definitions (cache + API) if needed.

    Berdir’s picture

    Warning: You have only one free rename left :p

    I'm fine with either, however, I'd really love to get this in ASAP and stop renaming things :) So can we agree on something, then I'll roll a patch and then we get it committed ;)

    yched’s picture

    @berdir: I know, sorry :-/. Thanks for the nice and humorous phrasing :-)

    As an element of excuse for my back and forth dance here, the naming suggested in #133 also builds on recent changes in the code flow (like buildBundleFieldDefinitions() only returning bundle fields, without merging them with base fields yet), previous iterations were not that clear cut. I feel #133 has a good mix of accurately representing what the current code is doing, and keeping naming that is clear, (relatively) friendly, and consistent with the underlying concepts ?

    fago’s picture

    Thanks for the updates - improvements look good.

    ad #133:
    yeah, the way you put it makes sense to me. It also helps to have the term "bundle fields" when talking about non-base fields. Thus, the suggestion in #133 works for me assuming the parameter $base_field_definitions for the bundle fields stays?

    Regarding renaming things - yep, I think we should be done here. But we should get started on another front:

    Nice try EntityReferenceAutocompleteTest, attaching a field to an entity that is not fieldable :p

    #2100343: Remove 'fieldable' key in entity definitions in favour of 'field_ui_base_route'

    Berdir’s picture

    Ok, here we go :)

    Also updated a few variables now that we have the naming part done: base field definitions + bundle field definitions => field definitions of an entity/bundle.

    Updated issue summary and change record.

    Status: Needs review » Needs work

    The last submitted patch, 137: d8_field_definition_bundles-2114707-137.patch, failed testing.

    Berdir’s picture

    Aw unit tests ;)

    yched’s picture

    Status: Needs review » Reviewed & tested by the community

    Thanks a lot @Berdir !

    Minor code flow nitpick:
    Since the whole "collect bundle fields" stack ($class::bundleFieldDefinitions(), hook_entity_bundle_field_info() + alter()) now receives $base_field_definitions as a param, it would make sense that EM::buildBundleFieldDefinitions() does too :
    EM::getFieldDefinitions() fetches getBaseFieldDefinitions() and passes it down the stack to buildBundleFieldDefinitions()
    rather than currently both getFieldDefinitions() & its child buildBundleFieldDefinitions() calling getBaseFieldDefinitions() separately.

    Not a blocker & can be done in a followup. RTBC if green.
    Thanks all !

    [edit : fixed messed up method names in the paragraph above]

    yched’s picture

    Awesome :-)

    RTBC++

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, 141: d8_field_definition_bundles-2114707-141.patch, failed testing.

    Berdir’s picture

    Updated the tests and also updated the cache keys.

    Status: Needs review » Needs work

    The last submitted patch, 144: d8_field_definition_bundles-2114707-144.patch, failed testing.

    Berdir’s picture

    And a re-roll, conflicted in content_translation_entity_base_field_info_alter() due to config() -> Drupal::config().

    swentel’s picture

    Status: Needs review » Reviewed & tested by the community

    Go go !

    fago’s picture

    Yep, good changes - let's get in :-)

    Berdir’s picture

    Issue summary: View changes
    alexpott’s picture

    Status: Reviewed & tested by the community » Fixed

    Committed 7a6fb33 and pushed to 8.x. Thanks!

    Noticed that fieldDefinitionsByBundle() was still mentioned in comments. Fixed the following on commit.

    diff --git a/core/lib/Drupal/Core/Entity/ContentEntityInterface.php b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
    index d6c1e0f..dee43a0 100644
    --- a/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
    +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
    @@ -51,7 +51,7 @@ public function initTranslation($langcode);
        * @endcode
        *
        * If some elements in a field definition need to vary by bundle, use
    -   * fieldDefinitionsByBundle().
    +   * \Drupal\Core\Entity\ContentEntityInterface::bundleFieldDefinitions().
        *
        * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
        *   The entity type definition. Useful when a single class is used for multiple,
    diff --git a/core/lib/Drupal/Core/Field/ConfigFieldItemList.php b/core/lib/Drupal/Core/Field/ConfigFieldItemList.php
    index 32cd440..35b7098 100644
    --- a/core/lib/Drupal/Core/Field/ConfigFieldItemList.php
    +++ b/core/lib/Drupal/Core/Field/ConfigFieldItemList.php
    @@ -7,10 +7,6 @@
     
     namespace Drupal\Core\Field;
     
    -use Drupal\field\FieldInstanceConfigInterface;
    -use Drupal\Core\TypedData\TypedDataInterface;
    -use Drupal\field\Field;
    -
     /**
      * Represents a configurable entity field item list.
      */
    
    andypost’s picture

    Status: Fixed » Closed (fixed)

    Automatically closed - issue fixed for 2 weeks with no activity.

    Berdir’s picture

    Issue tags: -sprint