#1950632: Create a FieldDefinitionInterface and use it for formatters and widgets established a new interface for field definitions which is intended shared between configurable and non-configurable entity fields. As of now, it's only implemented for configurable fields though.

#1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title started using existing entity field definition arrays and creating field definition classes from that, however I think that could become quite confusing as we'd end up with field definitions on the entity level being defined as arrays while we have an entity level field definition interface and field definition classes in most other places (widgets, formatters,..).

So, instead of having that confusing separation between two different kinds of field definitions I think we should do just one class based field definition which can suit all our needs.

As entity field definitions are just typed data definitions, consequently those must become classes as well.

So this changes how entity base fields gets defined, but I think it becomes way easier using the class based notation (todo: implement and show example here).

Advantages:
- Allows dealing with entity field definitions and configurable field definitions the same way - natively.
- Better DX when defining entity fields
- Better documentation of data/entity field definition keys and application of default values
- There is only one sort of field definition really, less confusion, better DX, better compatiblity.
- Something like his is required to do #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title without hacks.
- This is required for doing #2116363: Unified repository of field definitions (cache + API) - what should bring a nice memory usage improvement.

Changes:
Entity field definitions become objects, with a BC layer (for non-configurable fields) to support existing array structures.
Typed data definitions become objects, with a BC layer to support existing array structures.
#1928938: Improve typed data definitions of lists - the OO-interface already accounts for that and properly separates list and list-item related keys

More detailled list/summary of changes:
- Adds DataDefinition, ListDefinition classes and according interfaces for typed data definitions
- Lists are defined using separate data definitions now, as suggested by #1928938: Improve typed data definitions of lists - so list definitions and list item definitions are now clearly separated what cleans up a few things.
- Added FieldDefinition class for entity fields, based upon the list & data definition classes.
- Made classes + typed data manager work with the definition objects. For BC definition objects provide array access for now. TypedConfig still uses definitions as arrays and needs to be cleaned up in a follow-up.
- Field definitions and data definitions are still defined using arrays for now, but converted to objects once they have been picked up. That eases migration.
- Taxonomy term, string item and integer item field/data definitions has been converted as DX example.
- $this->definition in typed data objects is the data definition now, what for entity fields is the field definition. However, what's a bit confusing for field instances right now is that $this->definition will be the general non-bundle specific entity field definition, while getFieldDefinition() returns the "instance definition". This will be fixed with the follow-up of allow per-bundle field definitions in the entity field api (see todos).
- Thus, for configurable fields $this->definition is the field config entity (but should become the field instance later on).
- FielddefinitionInterface extends from DataDefinitionInterface (for now), so configurable field definition objects are data definitions as well - what removes the need to map them to yet another format. With #2002138: Use an adapter for supporting typed data on ContentEntities we can move to an adapter approach instead of using inheritance here as well.

Performance tests have been run, see comments #117 & #118.

Follow-ups:
#2112239: Convert base field and property definitions
#2114707: Allow per-bundle overrides of field definitions
#2116341: Apply defaults to definition objects
#2116347: Move FIELD_CARDINALITY_UNLIMITED constaint to the Field definition interface
#2116363: Unified repository of field definitions (cache + API)
#2120373: Optimize performance of class based field definitions
#1928868: Typed config incorrectly implements Typed Data interfaces
#2143555: Improve how field definitions deal with dependencies and testing
#2143263: Remove "Field" prefix from FieldDefinitionInterface methods

CommentFileSizeAuthor
#191 d8_field_definition_class-191.patch104.63 KBeffulgentsia
#183 d8_field_definition_class.interdiff.txt13.86 KBfago
#183 d8_field_definition_class.patch104.6 KBfago
#178 interdiff.txt813 bytesamateescu
#178 d8_field_definition_class-177.patch104.11 KBamateescu
#172 d8_field_definition_class.patch103.57 KBfago
#170 d8_field_definition_class.interdiff.txt694 bytesfago
#170 d8_field_definition_class.patch119.11 KBfago
#165 d8_field_definition_class.patch103.56 KBfago
#165 d8_field_definition_class.interdiff.txt1.15 KBfago
#158 d8_field_definition_class.interdiff.txt24.71 KBfago
#158 d8_field_definition_class.patch103.55 KBfago
#153 d8_field_definition_class.patch101.14 KBfago
#153 d8_field_definition_class.interdiff.txt34.83 KBfago
#151 d8_field_definition_class.patch107.99 KBfago
#151 d8_field_definition_class.interdiff.txt8.26 KBfago
#138 d8_field_definition_class.patch105.99 KBeffulgentsia
#131 d8_field_definition_class.patch105.74 KBfago
#131 d8_field_definition_class.interdiff.txt1.68 KBfago
#129 field_definition_class-2047229-129.patch105.72 KBsmiletrl
#129 interdiff-113-129.txt6.67 KBsmiletrl
#127 d8_field_definition_class.patch105.24 KBfago
#125 d8_field_definition_class.patch105.34 KBfago
#125 d8_field_definition_class.interdiff.txt971 bytesfago
#124 d8_field_definition_class.patch105.13 KBfago
#124 d8_field_definition_class.interdiff.txt761 bytesfago
#119 test.txt1.46 KBsmiletrl
#119 test2.txt1.51 KBsmiletrl
#119 test3.txt1.36 KBsmiletrl
#117 test.php_.txt611 bytesfago
#113 d8_field_definition_class.interdiff.txt5.33 KBfago
#113 d8_field_definition_class.patch104.96 KBfago
#108 d8_field_definition_class.patch105.13 KBfago
#108 d8_field_definition_class.interdiff.txt13.92 KBfago
#106 field_definition_class-2047229-106.patch102.48 KBBerdir
#106 field_definition_class-2047229-106-interdiff.txt1.27 KBBerdir
#104 field_definition_class-2047229-104.patch101.9 KBBerdir
#104 field_definition_class-2047229-104-interdiff.txt4.66 KBBerdir
#102 field_definition_class-2047229-102.patch101.43 KBBerdir
#101 field_definition_class-2047229-101.patch101.8 KBsmiletrl
#101 interdiff-100-101.txt652 bytessmiletrl
#100 field_definition_class-2047229-99.patch101.92 KBsmiletrl
#100 interdiff-97-99.txt2.16 KBsmiletrl
#98 field_definition_class-2047229-97.patch102.13 KBsmiletrl
#98 interdiff-95-97.txt10.76 KBsmiletrl
#95 field_definition_class-2047229-95.patch101.92 KBsmiletrl
#95 interdiff-94-95.txt5.32 KBsmiletrl
#94 d8_field_definition_class.interdiff.txt827 bytesfago
#94 d8_field_definition_class.patch102.06 KBfago
#92 d8_field_definition_class.patch102.22 KBfago
#92 d8_field_definition_class.interdiff.txt7.19 KBfago
#86 d8_field_definition_class.patch97.42 KBfago
#86 d8_field_definition_class.interdiff.txt10.86 KBfago
#81 field_definition_class-81.patch96.19 KBsmiletrl
#81 interdiff-77-81.txt11.29 KBsmiletrl
#77 field_definition_class-77.patch97.79 KBsmiletrl
#77 interdiff-74-77.txt5.46 KBsmiletrl
#75 field_definition_class-75.patch94.96 KBsmiletrl
#74 d8_field_definition_class.patch99.29 KBfago
#74 d8_field_definition_class.interdiff.txt1.02 KBfago
#72 d8_field_definition_class.patch99.22 KBfago
#72 d8_field_definition_class.interdiff.txt4.87 KBfago
#70 d8_field_definition_class.patch94.65 KBfago
#70 d8_field_definition_class.interdiff.txt2.31 KBfago
#62 d8_field_definition_class.patch93.4 KBfago
#60 d8_field_definition_class.patch94.22 KBfago
#60 d8_field_definition_class.interdiff.txt22.4 KBfago
#59 d8_field_definition_class.patch77.28 KBfago
#59 d8_field_definition_class.interdiff.txt1.53 KBfago
#57 d8_field_definition_class.interdiff.txt8.83 KBfago
#57 d8_field_definition_class.patch77.2 KBfago
#56 d8_field_definition_class.interdiff.txt1.24 KBfago
#56 d8_field_definition_class.patch72.66 KBfago
#49 d8_field_definition_class.patch71.42 KBfago
#49 d8_field_definition_class.interdiff.txt39.61 KBfago
#47 d8_field_definition_class.interdiff.txt9.89 KBfago
#47 d8_field_definition_class.patch65.66 KBfago
#44 d8_field_definition_class.patch60.95 KBfago
#44 d8_field_definition_class.interdiff-2.txt5.11 KBfago
#44 d8_field_definition_class.interdiff-1.txt3.71 KBfago
#41 d8_field_definition_class.patch57.23 KBfago
#41 d8_field_definition_class.interdiff.txt1.32 KBfago
#37 d8_field_definition_class.patch56.95 KBfago
#37 d8_field_definition_class.interdiff.txt965 bytesfago
#35 d8_field_definition_class.interdiff.txt879 bytesfago
#35 d8_field_definition_class.patch56.9 KBfago
#33 d8_field_definition_class.patch56.19 KBfago
#28 d8_field_definition_class.patch54.44 KBfago
#17 field-definition-only-2047229-17.patch9.16 KBBerdir
#13 field-definition-2047229-13.patch30.98 KBBerdir
#6 FieldDefinitionClass.jpg63.97 KBsmiletrl
#1 d8_field_definition_class.patch42.04 KBfago
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Status: Active » Needs review
Issue tags: +Entity Field API, +Typed sanity
FileSize
42.04 KB

Here is a first patch (missing some doc changes) - let's see what the bot says.

fago’s picture

Title: Make use of classes entity field and data definitions » Make use of classes for entity field and data definitions

Status: Needs review » Needs work

The last submitted patch, d8_field_definition_class.patch, failed testing.

yched’s picture

Priority: Major » Critical

+1, we really need to get this done:
- to make widgets and formatters actually work on base fields
- to be able to inject field definitions into the Field (value) classes, and thus
remove the not-great FieldConfig/FieldConfigItem::getInstance(),
and be able to write Field classes that are truly agnostic about "base field" or "configurable field"
- to be able to start figuring out how we're going to unify and optimize the APIs for "give me a list of fields" and their caching - aka "merge FieldInfo and EntityManager::getFieldDefinitions()"

I'm afraid some of this is above my head though :-)

It's not clear from the patch how this affects the UX of declaring base field definitions ?
It's not clear to me either exactly is "temporary BC layer supposed to go away" or what will stay.

+++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.phpundefined
@@ -0,0 +1,199 @@
+  public function setFieldName($name) {
+    $this->definition['field_name'] = $name;
+    return $this;

Do we really need setters ?
field name, field type, are not supposed to be mutable, are they ?
(well, generally speaking, a base field definition is supposed to be hardcoded / immutable ?)

+++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.phpundefined
@@ -0,0 +1,199 @@
+    return $this->definition['item_definition']['settings'][$setting_name];

Should be prevented against "unknown setting" (see #2050113: PHP notice on multiple items image field)
[edit: wait a sec, that's being discussed :-)]

+++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.phpundefined
@@ -0,0 +1,199 @@
+    return array_keys(\Drupal::typedData()->create($this->getItemDefinition())->getPropertyDefinitions());

statically cache maybe ?

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Field.phpundefined
@@ -0,0 +1,27 @@
+ * Note that the class only register the plugin, and is actually never used.

s/register/registers

[edit: Also, I couldn't figure what this empty class is used for]

+++ b/core/lib/Drupal/Core/TypedData/DataDefinition.phpundefined
@@ -0,0 +1,307 @@
+   * Gets

Famous last words :-)

+++ b/core/lib/Drupal/Core/TypedData/DataDefinition.phpundefined
@@ -0,0 +1,307 @@
\ No newline at end of file

That shouldn't be; right ?
(also, missing empty line after last method)

yched’s picture

Hm, also - aren't we biting our own tail here ?
field_entity_field_info() exposes configurable fields to EntityManager::getFieldDefinitions(), which then turns them into FieldDefinition objects, but those are not the (configurable) Field / FieldInstance objects (while those do implement FieldDefinitioninterface) :-)

smiletrl’s picture

FileSize
63.97 KB

- FieldDefinitionInterface should probably extend from DataDefinitionInterface, but how do configurable fields implement that best? Should we directly implement it and just cache the full config-entities with the entity-field-definitions or should we create a new definition object for that?

Hmm, should FieldDefinitionInterface extend DataDefinitionInterface? I guess no:) From field formatter and widget point, a field definition obejct/class is to replace original $field_entity and $field_instance objects, so they could apply to base fields too, not only configurable fields.

This is the current 'field defintion class' diagram, including class provided in this issue. Wish I didn't miss something:P
FieldDefinitionClass.jpg

The 'field definition class' for configurable fields now are represented by $field_entity and $field_instance. This probably answers the question -- 'should we create a new definition object for that?'?

At current diagram, This issue is to provide a new 'field definition class' -- FieldDefintion(although this name could be more specific to base fields/entity fields?) for base fields. Both non-configurable and configurable 'field defintion class' implements 'FieldDefintionInterface' now. So I guess no need to connect 'DataDefinitionInterface' and 'FieldDefinitionInterface' again? or are there other needs that $field_entity and $field_instance need to implement DataDefinitionInterface?

On the other hand, if FieldDefinitionInterface extends from DataDefinitionInterface, I guess we are unifying the field_type plugin for base fields and configurable fields in the big sense at #2052135: Determine how to deal with field types used in base fields in core entities, since all base field defintion functions are the same. And then what's the meaning of 'DataDefintion'? is this middle layer necessary here?

Just some random thoughts:)

smiletrl’s picture

So I guess my point at #6 is to imply two different field definition classes for base field and configurable fields, which could be unacceptable in this context:) Anyway, some other concerns.

1). Why should DataDefinitionInterface and DataDefinition exist here to provide info for FIELD definition?
In current diagram, since FieldDefintion implements 'FieldDefintionInterface', it should be able to getFieldType, getFieldLabel for a 'field'. So why should it also be able to getType(), getLabel() for a 'data' too? I guess we need to convert base fields from DataType to FieldType at #2023563: Convert entity field types to the field_type plugin?

2).

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

This seems 'FieldDefintion' ONLY works for base fields, instead of both cases. And it starts to imply for configurable fields, we need to use $field_entity and $field_instance as field defintion.

yched’s picture

I'd tend to agree with #7-1, although, again, I'm not really familiar with TypedData, so I have no strong opinion here.

I can't make sense of #7-2, though.

smiletrl’s picture

Regarding #7-2, I mean since 'FieldDefinition' class is supposed to work for both base field and configurable field, this method could say something like this

public function isFieldConfigurable() {
  //return FALSE;
  return $this->definition['configurable'];
}

It probably shouldn't assert that field configurability is ALWAYS false?

While this definition class always returns false configurable, defintion class Field and FieldInstance always return true. It looks like Fielddefinition only works for base field, and Field + FieldInstance work for configurable field respectively:)

yched’s picture

It looks like FieldDefinition only works for base field, and Field + FieldInstance work for configurable field respectively

Well, that's very exactly the case :-)
The "FieldDefinitioninterface object" for a configurable field is the $instance object (or the $field object in some cases where we can't narrow to a specific entity bundle).
The "FieldDefinitioninterface object" for a base field would be the FieldDefinition class this patch is introducing.

So the implementations of isFieldConfigurable() in the patch are the way they should be.

smiletrl’s picture

Ok, so I guess that would mean two(or three) different classes for field defintion. Wish this didn't break

be able to write Field classes that are truly agnostic about "base field" or "configurable field"

because in this case, we do actually know whether one defintion class is for base field or configurable field before contruct this defition class object and inject it into other places:)

The other thing for FieldDefinition is, probably it needs to implement some other entity_type specific interface, including FieldDefintionInterface?

For example, FieldDefinitionInterface::getFieldLabel perhaps needs to know which entity type it's working on. node.create, term.create, user.create? Different entity type may return different results here. They kind of look like different field instances here.

And for FieldDefinitionInterface::getFieldName it probably needs to dig into entity type more. For example, node.create and node.update may return two different 'field_names', although I'm not sure what this field_name could look like. Also, they could belong to the same 'field_type' -- date.

For FieldDefinitionInterface::getFieldPropertyNames I guess it needs to search entity type(node, taxonomy, user)'s base table's shema method, just like Field and FieldInstance search field Item's shema() method.

yched’s picture

#1994140: Unify entity field access and Field API access is blocked by this, needs a FieldDefinition object available for base fields.

Berdir’s picture

Status: Needs work » Needs review
FileSize
30.98 KB

Not sure about changing the way entity fields are defined. I'd rather keep the array structure for definition, because that's a considerable API change and (at least for a first step), just wrap it in a FieldDefinition object when requested. That might be more efficient for caching definitions and as @yched mentioned will avoid overlaps with configurable field definitions, which directly use the field/instance entities for that.

We might even be able to avoid any API changes at all, by only returning that from getFieldDefinition() as a first step (so it would just be used to provide the missing implementation for an existing API) and then discuss in smaller issues if we need API changes, like returning this structure by default?

Simple re-roll for now, something is off with the field item property definitions, they seem to be missing..

Status: Needs review » Needs work

The last submitted patch, field-definition-2047229-13.patch, failed testing.

fago’s picture

It's not clear from the patch how this affects the UX of declaring base field definitions ?
It's not clear to me either exactly is "temporary BC layer supposed to go away" or what will stay.

It could be both. I think the new class-based way to define fields has better DX, but we could keep going with the array way to avoid that API change also. If we keep going with the array way we loose some improvements though, i.e. the clear separation of what would go into list settings, list constraints, .. or item settings, item constraints...

It's not clear from the patch how this affects the UX of declaring base field definitions ?
It's not clear to me either exactly is "temporary BC layer supposed to go away" or what will stay.

True, I thought of using the as part of creating them though, e.g. have

$definitions['title'] = new FieldDefinition()
     ->setFieldName('title')
     ->setFieldType('string')

I'd not bother too much about making them immutable as it might make things more difficult afterwards, currently used arrays are not either.

field_entity_field_info() exposes configurable fields to EntityManager::getFieldDefinitions(), which then turns them into FieldDefinition objects, but those are not the (configurable) Field / FieldInstance objects (while those do implement FieldDefinitioninterface) :-)

Right, we should just add in the objects already implementing the interface there. I did not do it yet, as I was thinking about the configurable field definition not being a DataDefinition yet.
So what about making FieldDefinitionI extend DataDefinitionI, i.e. implement it for configurable fields also?

Hmm, should FieldDefinitionInterface extend DataDefinitionInterface? I guess no:) From field formatter and widget point, a field definition obejct/class is to replace original $field_entity and $field_instance objects, so they could apply to base fields too, not only configurable fields.

Yes, it needs to be the same for both. So from entity-field perspective that extension would make sense, as any field is a typed data object and so described with a data definition. It just happens to define field-specific parts also, i.e. it implements both interfaces.

Hmm, should FieldDefinitionInterface extend DataDefinitionInterface? I guess no:) From field formatter and widget point, a field definition obejct/class is to replace original $field_entity and $field_instance objects, so they could apply to base fields too, not only configurable fields.

Afaik configurable field definitions are already cached as (entity) objects, but currently we even cache it another time as entity-field array. To clean this up I think we should do this and then have a single entity field definition cache which holds objects implement the field definition interface. The nice thing about the interface is that it can be conigurable field/instance objects or manually instantiated definitions for base-fields, while consuming code can work with both the same way based on the interface.

For that to work out I think we'd have to re-vamp the entity-field-info hook layout to allow placing instance objects in there also I guess.

I know it's a rather big API change, but I think it helps us to do more afterwards without changing the API again, as the field definition objects working based on methods give us more flexibility on changing internals.

yched’s picture

[edit: crosspost with @fago #15]

#13 sounds reasonable to me.
- configurable fields have their own internal formats (Field / FieldInstance config entities), it makes sense that base fields have their own internal format too (the current arrays)
- then, there is a common FieldDefinitionInterface format for both, that would then be used for APIs that list "the fields on a entity"

As mentioned in a mail to @fago, I wonder if there is a way we could tackle field definitions here without having to tackle typed data definitions too. Would probably let us move faster ?

Berdir’s picture

Status: Needs work » Needs review
FileSize
9.16 KB

Here's a re-roll with only the FieldDefinition() part. I guess we'd need some tests for this.

tstoeckler’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -0,0 +1,207 @@
    +    $parts = explode(':', $this->getItemDefinition()->getType());
    +    return $parts[1];
    

    Super nitpicky, but this would be cooler with list() IMO.

  2. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -0,0 +1,207 @@
    +    return isset($this->definition['queryable']) ? $this->definition['queryable'] : !$this->isComputed();
    

    I might be completely missing something, but I don't see $this->isComputed() being defined anywhere.

  3. +++ b/core/modules/field/lib/Drupal/field/Entity/Field.php
    @@ -775,4 +775,17 @@ public function __wakeup() {
    +  public function isFieldConfigurable() {
    +    return TRUE;
    +  }
    

    I would have thought this should return FALSE, as opposed to the FieldInstance one (where TRUE makes sense, I think).

1. is not important at all, 2. and 3. are a "needs work" but I feel like I'm missing something. So tentatively setting to "needs work" overall.

yched’s picture

Status: Needs work » Needs review

Yay! Thanks a lot for reviving this @Berdir!

It seems there are a couple pieces missing from @fago's patch - the parent class for typed data was probably providing the methods that seem to be missing here:
__construct(), getItemDefinition(), isList(), setClass(), setItemDefinition()

  1. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -0,0 +1,207 @@
    +namespace Drupal\Core\Entity\Field;
    +use Drupal\Core\Entity\EntityInterface;
    

    Nitpick: missing whiteline in between

  2. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -0,0 +1,207 @@
    +  public function setFieldType($type) {
    +    $this->definition['type'] = 'entity_field';
    +    $this->definition['item_definition']['type'] = 'field_item:' . $type;
    

    What is this used for ? I don't see this notion of 'type' = 'entity_field' anywhere other than here and in createFromOldStyleDefinition() ? So do we really need it if it's stricty internal ?
    [edit: oh, I guess that's related to the core/lib/Drupal/Core/Entity/Plugin/DataType/Field.php that's being added ?]

    + I'm a little lost between what is in $definition vs what is in $definition['item_definition'] - but I guess that's the current format of array $definitions for base fields ?

  3. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -0,0 +1,207 @@
    +  public function getFieldPropertyNames() {
    +    return array_keys(\Drupal::typedData()->create($this->getItemDefinition())->getPropertyDefinitions());
    

    this->getItemDefinition() is undefined

    Also, it's a bit unfortunate that we need to create an $item in order to get the info, but I guess there's no other choice right now.

  4. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -0,0 +1,207 @@
    +  public static function createFromOldStyleDefinition($field_name, array $definition) {
    (...)
    +    unset($list_definition['settings']);
    

    I'm surprised we drop this ? It won't be relevant for code that needs to know "the settings of the field" ?

  5. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -0,0 +1,207 @@
    +  public static function createFromOldStyleDefinition($field_name, array $definition) {
    (...)
    +    $new = new FieldDefinition($list_definition);
    

    So, I guess external code is supposed to call createFromOldStyleDefinition() rather than new FieldDefinition() ? (I'm thinking of Field::getFieldDefinition())

Berdir’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Haven't really looked at the code yet, will have to do and very obviously also have to write tests for this :)

4. Was confused by this as well, but it's the definition of the list, not the field. Not sure if it makes sense with just the field definition class, will figure that out when writing tests I guess.

5. Need to check, but the method implies that it's the old style definition, which isn't actually the case yet in my patch. So maybe move that code somewhere. As I said, haven't really read it yet.

effulgentsia’s picture

Per #1994140-20: Unify entity field access and Field API access, would it make sense to move the partial work being discussed since #17 into that issue, so that whatever is added initially is known to have a concrete use case? Then this issue can go back to the fuller API for us to evaluate what's really needed vs. not?

yched’s picture

@effulgentsia: might make sense, develop against an actual use case and make actual tests turn from red to green...

@Berdir, what do you think ?

Berdir’s picture

Works for me, feel free to merge my latest patch into the other issue. Not sure I have time work on that..

fago’s picture

Assigned: Unassigned » fago

working on this now.

Conclusions from previous discussions (see link)

  • We want to unify field definitions by using class based field definitions in the entity system also, so we can start using a common field registry and a) removed one layer of field definitions and b) only maintain the metadata once (performance++).
  • We want to support per bundly overrides of field definitions in entity fields as well, as a) it’s a pre-requesite for using entity field info as common field info service and b) use-cases popped up anyway.
  • We start improving the entity field info system and make that the system people are supposed to use. Existing configurable field info stuff can can be replaced by pointing to it and is then deprecated/will be removed.

effulgentsia, yched and me came up with the following DX suggestion for defining base fields:

class Node {

  /**
   * {@inheritdoc}
   */
  public static function baseFieldDefinitions($entity_type) {
    $info['nid'] = FieldDefinition::create()
      ->setLabel(t('Id'))
      ->setDescription(t('The node ID.')
        ->setType('integer')
        ->setReadOnly();
    $info['title'] = FieldDefinition::create()
      ->setLabel(t('Title'))
      ->setDescription(t('The node title.')
        ->setType('integer')
        ->setReadOnly();
    return $info;
  }

  public static function baseFieldDefinitionsByBundle($entity_type, $bundle = NULL, $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;
  }

Discussed with yched and dixon on how to proceed with this best. Points discussed:
- Should we have data definitions as classes then also?
- What's the relationship between data definitions and field definitions?

We figured, once we are using classes in baseFieldDefinitions it makes sense to have it in FieldItem::getPropertyDefinitions() also. Then, we need to have a typed data definition for all of the fields, what for configurable objects right now happens by converting field definition objects to arrays in field_entity_field_info(). So instead of keeping converting things between different representation formats around, we figured it makes more sense to return the required information with methods, i.e. have the field definition objects implement a (typed) DataDefinition also.

Working on a first patch for this now.

smiletrl’s picture

Impressive progress!

A bit tip, we may add the static cache inside baseFieldDefinitions($entity_type) to avoid instantiating base fields every time we call it:) Aslo, shouldn't we wait for #1994140: Unify entity field access and Field API access to bring class FieldDefinition?

Berdir’s picture

No need for a static, this is cached in the entity manager, you're not supposed to call this directly.

fago’s picture

Did not get to work on this more today :/ But we had some good discussion which are relevant to FieldDefinitions. We discussed terminology quite a bit during the entity storage discussion in Prague, see the notes here.

Instead, the idea is to clarify the providing module + who is responsible for storing the field in the field definition. Relevant part of the notes:

module’ - This key is used to tell which module provides the field defintions, base fields have a NULL value; Configurable fields will have ‘field’ - so this can be used by the Field module to identify its fields. Then, this key allows us to identify base fields (=fields provided with the entity types) as they have a NULL values. Lastly, it’s something important to have in order to be able to track dependencies, e.g. when a module is configured to rely on a field to be there (a rule configuration needs it to run, a module uses a token of that field, ..).

storage’ => CONTROLLER|CUSTOM
This indicates whether the storage controller should take care of storing the field values.
....

fago’s picture

Status: Needs work » Needs review
FileSize
54.44 KB

ok, so here is a re-rolled basic patch.

What it does:
- Add DataDefinition class for typed data definitions
- Add FieldDefinition class for entity fields, based upon the data definition class
- Made classes + typed data manager work with the definition objects. For BC definition objects provide array access for now.
- Field definitions and data definitions are still defined using arrays for now, but converted to objects once they have been picked up. That's a good approach to ease migration.

Todo:
- Make use some classes when defining an entity type and field type as example.
- Overhaul the process of fetching data definitions + hook_entity_field_info() to allow for per-bundle definitions.
- Extend FielddefinitionInterface from DataDefinitionInterface (for now) and make configurable field definition objects implement it.
- Finally - fix field_entity_field_info() to do not convert from one field definition format to another, but to just field definitions on.

fago’s picture

Created #2100549: Plugin factories needlessly restrict configuration to arrays - which would allow us to keep the data definition as second parameter for createInstance() + actually fix the method to use typed-data terminology.

Status: Needs review » Needs work
Issue tags: -Needs tests, -API change, -Entity Field API, -Typed sanity

The last submitted patch, d8_field_definition_class.patch, failed testing.

fago’s picture

Status: Needs work » Needs review

#28: d8_field_definition_class.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +API change, +Entity Field API, +Typed sanity

The last submitted patch, d8_field_definition_class.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
56.19 KB

I re-rolled the patch based on HEAD as the basic field definition class went in with #1994140: Unify entity field access and Field API access + made sure it basically works. Let's see what the bot says.

Status: Needs review » Needs work

The last submitted patch, d8_field_definition_class.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
56.9 KB
879 bytes

Fixed fatal during test run due to a left-over reference on moved class.

Status: Needs review » Needs work

The last submitted patch, d8_field_definition_class.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
965 bytes
56.95 KB

Status: Needs review » Needs work

The last submitted patch, d8_field_definition_class.patch, failed testing.

smiletrl’s picture

This is quite a lot of work!

A quick review:

+ * )
+ */
+class FieldItemList extends \Drupal\Core\Entity\Field\FieldItemList {
+
+}

Maybe add \Drupal\Core\Entity\Field\FieldItemList as alias. Also, can we name different names here, instead of the same FieldItemList?

+  public function isList() {
+    return $this instanceof ListDefinitionInterface;
+  }

This line is hard to read, maybe return ($this instanceof ListDefinitionInterface);?

+++ b/core/lib/Drupal/Core/TypedData/ListDefinitionInterface.phpundefined
@@ -0,0 +1,26 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\Core\TypedData\DataDefinitionInterface.
+ */
+

Mixed names, ListDefinitionInterface.php Vs DataDefinitionInterface:)

+  /**
+   * Overrides \Drupal\Core\TypedData\TypedData::getValue().
+   */
+  public function getValue() {
+    if (isset($this->list)) {
+      $values = array();
+      foreach ($this->list as $delta => $item) {
+        $values[$delta] = $item->getValue();
+      }
+      return $values;
+    }
+  }

I think we need return NULL/array() when $this->list is not set.

smiletrl’s picture

--- a/core/lib/Drupal/Core/TypedData/TypedData.php
+++ b/core/lib/Drupal/Core/TypedData/TypedData.phpundefined
-  public function __construct(array $definition, $name = NULL, TypedDataInterface $parent = NULL) {
+  public function __construct($definition, $name = NULL, TypedDataInterface $parent = NULL) {
     $this->definition = $definition;
     $this->parent = $parent;
     $this->name = $name;

This will reflect all extended classes of TypedData, which all need to update array hint. See hierarchy.

fago’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
57.23 KB

Addressed two problems, so let's see how many test fails are left.

smiletrl, thanks for the review. I've not addressed your points yet. However, here some comments:

I think we need return NULL/array() when $this->list is not set.

It returns NULL when there is no explicit return. Howsoever, this code got just moved.

This will reflect all extended classes of TypedData, which all need to update array hint. See hierarchy.

Yes, in case the class override construct. The patch should already account for that.

Status: Needs review » Needs work

The last submitted patch, d8_field_definition_class.patch, failed testing.

smiletrl’s picture

Looks like DateTimeComputed::__construct hasn't:)

fago’s picture

indeed - that did not exist at that time. So merged with HEAD again and fixed that.

Updated patch attached, which addresses the review, updates some docs + fixes more test fails.

Status: Needs review » Needs work

The last submitted patch, d8_field_definition_class.patch, failed testing.

fago’s picture

This should do it, again a summary:

What it does:
- Add DataDefinition class for typed data definitions
- Add FieldDefinition class for entity fields, based upon the data definition class
- Made classes + typed data manager work with the definition objects. For BC definition objects provide array access for now.
- Field definitions and data definitions are still defined using arrays for now, but converted to objects once they have been picked up. That's a good approach to ease migration.
- $this->definition in typed data objects is the data definition now, what for entity fields is the field definition. However, what's a bit confusing for field instances right now is that $this->definition will be the general non-bundle specific entity field definition, while getFieldDefinition() returns the "instance definition" (see todos).

Todo:
- Make use of classes when defining an entity type and field type as example.
- Overhaul the process of fetching data definitions + hook_entity_field_info() to allow for per-bundle definitions
- Extend FielddefinitionInterface from DataDefinitionInterface (for now) and make configurable field definition objects implement it.
- Finally - fix field_entity_field_info() to avoid converting from one field definition format to another, but just pass on the config entities as field definitions. This step will make $this->definition for configurable fields to be the field instance then!

fago’s picture

Status: Needs work » Needs review
FileSize
65.66 KB
9.89 KB

and a patch...

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -496,12 +491,18 @@ public function getFieldDefinitions($entity_type, $bundle = NULL) {
    +            $definition = FieldDefinition::createFromOldStyleDefinition($definition)
    +              ->setFieldName($field_name);
    

    It's temporary anyway, but why not just make it createFromOldStyleDefinition($field_name, $definition) ? It's not valid to have one without field name anyway..

  2. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -6,34 +6,21 @@
    +    // @todo: Should "name" be part of the data definition also?
    
    @@ -70,43 +57,60 @@ public function getFieldType() {
    +   *   The value to set.
    
    @@ -231,4 +242,68 @@ public function getFieldDefaultValue(EntityInterface $entity) {
    +    $field_definition->setItemDefinition($item_definition);
    ...
    +      // What previously was "type" is now the type of the list item.
    ...
    +      // What previously was "type" is now the type of the list item.
    +      $this->itemDefinition['type'] = $value;
    

    Shouldn't this be set using a method? Also seems like it should return, we don't want it to change the type of the field definition?

  3. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/FieldItemList.php
    @@ -0,0 +1,27 @@
    +use Drupal\Core\Entity\Field\FieldItemList as ExistingFieldItemList;
    ...
    +class FieldItemList extends ExistingFieldItemList {
    

    Naming is a bit strange, Existing doesn't say much, by the time this code runs, both exist :)

  4. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
    @@ -0,0 +1,348 @@
    +   * @param string $constraint_name
    +   *   The name of the constraint to add, i.e. its plugin id.
    

    i.e. ? Can it also be something else?

  5. +++ /dev/null
    @@ -1,224 +0,0 @@
    -class ItemList extends TypedData implements \IteratorAggregate, ListInterface {
    

    Looks like it doesn't see this as a move, would be easier to review if there were any changes.

  6. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/ValidReferenceConstraintValidator.php
    @@ -19,6 +19,9 @@ class ValidReferenceConstraintValidator extends ConstraintValidator {
       public function validate($value, Constraint $constraint) {
    +    if (!isset($value)) {
    +      return;
    +    }
    

    Why is this change necessary here? :)

  7. +++ b/core/modules/text/lib/Drupal/text/TextProcessed.php
    @@ -7,6 +7,7 @@
    +use Drupal\Core\TypedData\DataDefinitionInterface;
     use Drupal\Core\TypedData\TypedDataInterface;
     use Drupal\Core\TypedData\TypedData;
     use Drupal\Core\TypedData\ReadOnlyException;
    @@ -29,7 +30,7 @@ class TextProcessed extends TypedData {
    
    @@ -29,7 +30,7 @@ class TextProcessed extends TypedData {
       /**
        * Overrides TypedData::__construct().
        */
    -  public function __construct(array $definition, $name = NULL, TypedDataInterface $parent = NULL) {
    +  public function __construct($definition, $name = NULL, TypedDataInterface $parent = NULL) {
    

    The interface is use'd, but not used in the type hint?

Looks nice, how do we continue with this stuff. First get #2083803: Convert field type to typed data plugin for field_test module in, then re-roll this as necessary and then crowdsource the conversion of the baseFieldDefinitions() methods and using the definitions? Doing it here would make the patch quite bit I guess.

fago’s picture

It's temporary anyway, but why not just make it createFromOldStyleDefinition($field_name, $definition) ? It's not valid to have one without field name anyway..

Because, it breaks inheritance from DataDefinition - which now has the same method, but no field-name. So I changed it to go with the method - as mentioned, it's just temporary anyway.

Shouldn't this be set using a method? Also seems like it should return, we don't want it to change the type of the field definition?

Good catch, fixed.

i.e. ? Can it also be something else?

Nope, but i.e. should not mean that.

4. Why is this change necessary here? :)

Somehow it was causing test-fails, so I fixed it quite at the start. I was wondering how it worked before, as during validation empty field items or field item lists are passed on as NULL. So a lot if not most symfony constraint validators check for NULL values first and ignore them.

The interface is use'd, but not used in the type hint?

True, removed the use statement for now. We cannot type-hint on it yet, as there are still some TypedConfig classes that extend from TypedData but are still going with array based data definitions. I think it would best to work on cleaning up TypedConfig in a separate issue, as it already makes use of typed data only half way :-/

Looks nice, how do we continue with this stuff. First get #2083803: Convert field type to typed data plugin for field_test module in, then re-roll this as necessary and then crowdsource the conversion of the baseFieldDefinitions() methods and using the definitions? Doing it here would make the patch quite bit I guess.

Yes, I think we should one example conversion as part of this patch + leave the rest to follow-up issues.

Apart from that I've been wondering how far we should go with the "todo" points in this issue also. I think we should leave per-bundle field stuff and re-arranging hook_entity_field_info() to a separate issue, but make configurable FieldDefinition objects implement DataDefinitionInterface as part of this. That way this issue would successfully stop converting from one field definition format to another (except for the not yet converted BC-use), i.e. _field_generate_entity_field_definition() can go away.

More TODOs:

  • Data types need a way to specify their default list type, instead of the previous 'list class'.
  • It's confusing that we've $definition->getConstraints() and $manager->getConstraints($definition), which is not the same thing.

To solve the second TODO, should we make $definition add-in defaults for constraints and class by default? E.g. we could do

public function getClass($apply_default = TRUE, $manager = NULL) {
  // Get manager from container if not supplied.
  // Fetch type default from manager if no class is set.
}

Analogously, we could move $manager->getConstraints() directly to the definition's getConstraints(). Thoughts?

Updated patch attached, which
- addresses the review
- moves over the FieldItemList stuff to the new location
- cleans up logic around getConstraints() and isRequired()
- converts taxonomy term to use class based definitions as an example

Status: Needs review » Needs work

The last submitted patch, d8_field_definition_class.patch, failed testing.

Berdir’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php
@@ -203,67 +205,66 @@ public function postSave(EntityStorageControllerInterface $storage_controller, $
+      ->setPropertyConstraints('value', array('EntityChanged' => array()));

Shouldn't this be addPropertyConstraint() ?

fago’s picture

Status: Needs work » Needs review

#49: d8_field_definition_class.patch queued for re-testing.

fago’s picture

+ ->setPropertyConstraints('value', array('EntityChanged' => array()));

I chose to go with "set" as it actually overwrites the constraints for the given property, so it's not really adding. We could change it to be really like adding, but not sure how it should behave then when you add a constraint two times?

Status: Needs review » Needs work

The last submitted patch, d8_field_definition_class.patch, failed testing.

Berdir’s picture

One of the phpunit tests is failing with "PHP Fatal error: Call to a member function get() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal.php on line 370", at least I assume it's one of those, then the testbot can't parse the result anymore.

Ah, I somehow thought I've seen an add method in the patch that I reviewed.

fago’s picture

Status: Needs work » Needs review
FileSize
72.66 KB
1.24 KB

Indeed - thanks. It's the FieldDefinition unittestcase which fails now as I made use of the container there.. So attached is a try to mock it. Please review and give feedback - it's my first phpunit experience :-)

It's not nice nor ideal to have the FieldDefinition use the container, but it's just to make it work in a consistent manner as long as not all field types have been converted to field type plugins. Thus, once we've done that we can remove that again (see @todo).

fago’s picture

Data types need a way to specify their default list type, instead of the previous 'list class'.

ok, attached patch implements that. I.e., instead of the previous list-class we've got now "list_type" to specify the default list data type to use. This gets used neither a list type is specified nor a custom class is specified for a list definition. For BC, the old 'list class' stuff is still respected for now.

So what's left for this issue is:

make configurable FieldDefinition objects implement DataDefinitionInterface as part of this. That way this issue would successfully stop converting from one field definition format to another (except for the not yet converted BC-use), i.e. _field_generate_entity_field_definition() can go away.

Status: Needs review » Needs work

The last submitted patch, d8_field_definition_class.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
77.28 KB
fago’s picture

ok, so attached patch makes configurable field definitions be data definitions as well, what finally allows us to remove the mapping of field definitions to data definitions :-)

Again a summary:

What it does:
- Add DataDefinition, ListDefinition classes and according interfaces for typed data definitions
- Lists are defined using separate data definitions now, as suggested by #1928938: Improve typed data definitions of lists, consquently data types can now provide a "list_type", instead of a "list_class"
- Add FieldDefinition class for entity fields, based upon the data definition class
- Made classes + typed data manager work with the definition objects. For BC definition objects provide array access for now.
- Field definitions and data definitions are still defined using arrays for now, but converted to objects once they have been picked up. That's a good approach to ease migration.
- $this->definition in typed data objects is the data definition now, what for entity fields is the field definition. However, what's a bit confusing for field instances right now is that $this->definition will be the general non-bundle specific entity field definition, while getFieldDefinition() returns the "instance definition" (see todos).
- For configurable fields $this->definition is the field config entity (but should become the field instance later on).
- FielddefinitionInterface extends from DataDefinitionInterface (for now), so configurable field definition objects are data definitions as well - what removes the need to map them to yet another format.
- Taxonomy term, string item and integer item field/data definitions has been converted as DX example.

Todo:
- Performance checks (objects vs arrays) and optimization of the new classes (maybe convert to multiple properties instead of the definition array)

Left for follow-ups:
- Overhaul the process of fetching data definitions + hook_entity_field_info() to allow for per-bundle definitions
- Make field API pass on field instances as part of hook_entity_field_info().

Status: Needs review » Needs work

The last submitted patch, d8_field_definition_class.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
93.4 KB

the drop is always moving... - rerolled and fixed conflicts

Status: Needs review » Needs work
Issue tags: -Needs tests, -API change, -Entity Field API, -Typed sanity

The last submitted patch, d8_field_definition_class.patch, failed testing.

fago’s picture

Status: Needs work » Needs review

#62: d8_field_definition_class.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +API change, +Entity Field API, +Typed sanity

The last submitted patch, d8_field_definition_class.patch, failed testing.

Berdir’s picture

Partial review, like how many @todo's we can drop here (we also add a few but smaller ones I think). Also means I can close #1919466: Support for adding field settings in field_entity_field_info() and #1919468: EntityManager::getAllBundleInfo() adds default entity_type_id bundle for entity types with bundles as no longer necessary (the second only of the follow-up where we call the hook per bundle (I guess).

Make sure to read the whole review first, answering some of my own questions in later points ;)

  1. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/BooleanItem.php
    @@ -18,7 +18,7 @@
      *   label = @Translation("Boolean field item"),
      *   description = @Translation("An entity field containing a boolean value."),
    - *   list_class = "\Drupal\Core\Entity\Field\FieldItemList"
    + *   list_class = "\Drupal\Core\Entity\Plugin\DataType\FieldItemList"
      * )
    

    All those lines will conflict because this class is now added in processDefinition() of the field type plugin manager. Just a reminder to add it there instead when the base field as @FieldType issue is in :)

  2. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/FieldItemList.php
    @@ -2,28 +2,32 @@
    +use Drupal\Core\TypedData\Annotation\DataType;
    +use Drupal\Core\Annotation\Translation;
    

    use of the annotation classes is no longer necessary, it just works

  3. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/FieldItemList.php
    @@ -80,7 +83,7 @@ public function getLangcode() {
       public function getFieldDefinition() {
    -    return new FieldDefinition($this->definition);
    +    return $this->definition;
       }
    

    If I understand it correctly then $this->definition is now a FieldInstance for configurable fields, right? Does that mean we can drop the override of that method there?

  4. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/FieldItemList.php
    @@ -109,7 +112,7 @@ public function getValue($include_computed = FALSE) {
     
       /**
    -   * Overrides \Drupal\Core\TypedData\ItemList::setValue().
    +   * Overrides \Drupal\Core\TypedData\Plugin\DataType\ItemList::setValue().
        */
    

    Let's switch to @inheritdoc on lines that we touch anyway.

  5. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/LanguageItem.php
    @@ -19,7 +19,7 @@
      *   description = @Translation("An entity field referencing a language."),
    - *   list_class = "\Drupal\Core\Entity\Field\FieldItemList",
    + *   list_class = "\Drupal\Core\Entity\Plugin\DataType\FieldItemList",
      *   constraints = {
    

    list_type vs. list_class seems to be quite inconsistent now, does that break the installer maybe?

  6. +++ b/core/lib/Drupal/Core/TypedData/ListDefinition.php
    @@ -0,0 +1,78 @@
    +      // For BC check for a defined list class.
    +      // @todo: Remove once all list_class usages have been converted.
    +      if (isset($item_type_definition['list_class']) && !$this->getClass()) {
    +        $this->setClass($item_type_definition['list_class']);
    +      }
    

    Or should this keep the list_class/list_type mess working?

  7. +++ b/core/modules/field/field.module
    @@ -195,16 +195,16 @@ function field_entity_field_info($entity_type) {
    +    // @todo: Improve hook_entity_field_info() so that it can take field
    +    // instance also.
    

    Ah, so that's why we still need the Config override of getFieldDefinition(). K, fine by me to do this in a follow-up.

yched’s picture

Hard for me to do a proper review, just looked the last interdiff.
I find it really unfortunate that TypeDataDefinitionInterface methods are not namespaced while FieldDefinitionInterface methods are namespaced (getFieldXxx()). This seems backwards : a Field / FieldInstance object is pramarily a "field", and only secondarily a typed data definition. But the classes have getSettings() / getFieldSettings(), and the one most code wants is the most "hidden".
(btw, in Prague it was agreed to remove the "Field" part in FieldDefinitionInterface method names - not for this issue of course, but let's not "take" the target method names here ?)

fago’s picture

ad #66:
ad 2: I know, but then I cannot click on them any more! :D Anyway, I just moved the class - but as I touched it I removed them now.

ad 4: Yeah, I've sone so, but this was covered by my IDE ;) I fixed this and look for others, but that was the only one I found.

ad 5. Yeah, list_class should go away and is just left for BC. Installation and tests work for me though, so not sure what's the problem with the bot. :/

ad 6. yes, it still works.

ad #67:

(btw, in Prague it was agreed to remove the "Field" part in FieldDefinitionInterface method names - not for this issue of course, but let's not "take" the target method names here ?)

Indeed - so the idea is the become the same once the "Field" prefix got removed. As the patch shows they mostly already identically implemented, so it's a good match. We already figured in Prague that it would be confusing for the type, but I think that should work by keeping getDataType() vs getFieldType()/geType().
The only problem I encountered while writing the patch is getSettings(), which does not necessarily match the field settings as those apply to the list item, but not the list. There would be no harm in keeping to return the field settings for both, the list and the item though.

This seems backwards : a Field / FieldInstance object is pramarily a "field", and only secondarily a typed data definition. But the classes have getSettings() / getFieldSettings(), and the one most code wants is the most "hidden".

As said it's already implemented equivalent. Howsoever, I'm unhappy with this also - mostly because of the documentation. If you look up $field->getLabel() it really has to talk about fields, not "data". So imho we have to options to improve this:
a) Unify field definition and data definitions by remove the field* prefix from most methods + take over methods from the data definition in the field definition interface just for improving documentation.
b) Support an adapter approach for data definitions as well, once we've #2002138: Use an adapter for supporting typed data on ContentEntities. I.e., field definitions would not inherit from data definitions any more, but provide an adapter to translate them. Once we've fixed instantiation to be controlled by Field* managers, field items and item lists could be still instantiated given field definitions, while it might make re-using the typed-data optimized prototype factory a bit harder. Worst case, it should be enough to copy over that factory and adapt it to field definitions.

a) we could do already now, b) requires more work first. Maybe should we do a) already, and revisit whether b) is still needed once we've renamed field* methods?

yched’s picture

Works for me. a) is a large patch in itself though, after "remove BC arrayAccess from field / instance" moved a lot of code to the getters.
Moving to decorators sounds +1 for me later.

fago’s picture

Status: Needs work » Needs review
FileSize
2.31 KB
94.65 KB

here is an updated patch, with which installer works for me manually also. edit.module seems to have some troubles still, but I was not able to find them in a quick check :/ Let's see what fails else.

Status: Needs review » Needs work

The last submitted patch, d8_field_definition_class.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
4.87 KB
99.22 KB

ok, configurable field definitions do not have array access any more, so this needs to be used as objects now. Let's see whether I've got all of them.

Status: Needs review » Needs work
Issue tags: -Typed sanity

The last submitted patch, d8_field_definition_class.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
1.02 KB
99.29 KB
smiletrl’s picture

Status: Needs review » Needs work

The last submitted patch, field_definition_class-75.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
5.46 KB
97.79 KB

Deal with translation stuff.

smiletrl’s picture

Berdir’s picture

Status: Needs review » Needs work

Long review, mostly nitpicks, a bunch of code that can be removed now that the base fields patch landed and some ideas on how to improve definition creation.

  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -449,16 +448,9 @@ public function getAdminRouteInfo($entity_type, $bundle) {
        * @return array
    -   *   An array of field definitions of entity fields, keyed by field
    -   *   name. In addition to the typed data definition keys as described at
    -   *   \Drupal\Core\TypedData\TypedDataManager::create() the following keys are
    -   *   supported:
    -   *   - queryable: Whether the field is queryable via QueryInterface.
    -   *     Defaults to TRUE if 'computed' is FALSE or not set, to FALSE otherwise.
    -   *   - translatable: Whether the field is translatable. Defaults to FALSE.
    -   *   - configurable: A boolean indicating whether the field is configurable
    -   *     via field.module. Defaults to FALSE.
    +   * An array of entity field definitions, keyed by field name.
    ...
    +   * @see \Drupal\Core\Entity\Field\FieldDefinitionInterface
    

    The new and approved standard is to use @return ..FieldDefinitionInterface[] now, which is understood by PhpStorm :) Then you don't need the @see.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -497,19 +490,22 @@ public function getFieldDefinitions($entity_type, $bundle = NULL) {
    +            // Ensure ids and langcode fields are never made translatable.
    +            if (isset($untranslatable_fields[$field_name]) && $definition->isFieldTranslatable()) {
    +              throw new \LogicException(format_string('The @field field cannot be translatable.', array('@field' => $definition->getFieldLabel())));
                 }
    

    Just moved, but I don't think "ids" is correct.. maybe ID's, or ID (as it's part of ...fields)

  3. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -6,34 +6,31 @@
    -   * @param array $definition
    -   *   (optional) If given, a definition represented as array.
    +   * @return \Drupal\Core\Entity\Field\FieldDefinition
        */
    -  public function __construct(array $definition = array()) {
    -    $this->definition = $definition;
    +  public static function create() {
    +    $class = get_called_class();
    +    return new $class();
       }
    

    The diff is quite hard to read, but the @return needs a description.

  4. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -55,9 +52,18 @@ public function setFieldName($name) {
    -    // Cut of the leading field_item: prefix from 'field_item:FIELD_TYPE'.
    -    $parts = explode(':', $this->definition['type']);
    -    return $parts[1];
    +    // We have to support 'field_item:FIELD_TYPE' data types as well as
    +    // 'FIELD_TYPE_field' for field types that are not yet implemented as field
    +    // type plugin.
    +    $data_type = $this->getItemDefinition()->getDataType();
    +    if (strpos($data_type, ':') !== FALSE) {
    +      // Cut of the leading field_item: prefix from 'field_item:FIELD_TYPE'.
    +      $parts = explode(':', $data_type);
    +      return $parts[1];
    +    }
    +    else {
    +      return str_replace('_field', '', $data_type);
    +    }
    

    This can can be dropped now, the _field version no longer exists. Yay :)

  5. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -70,43 +76,68 @@ public function getFieldType() {
       public function setFieldType($type) {
    -    $this->definition['type'] = 'field_item:' . $type;
    +    // Determine the right data type to use.
    +    if (\Drupal::typedData()->getDefinition('field_item:' . $type)) {
    +      $this->getItemDefinition()->setDataType('field_item:' . $type);
    +    }
    +    else {
    +      // Data type has not been converted to a field type plugin yet.
    +      $this->getItemDefinition()->setDataType($type . '_field');
    +    }
         return $this;
       }
    

    Same here.

  6. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -184,21 +201,40 @@ public function getFieldCardinality() {
    -   * @param bool $required
    -   *   TRUE if the field is required, FALSE otherwise.
    +   * @param boolean $required
    +   *   Whether the field is required.
    

    bool was correct, not boolean.

  7. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -184,21 +201,40 @@ public function getFieldCardinality() {
    +   * @param boolean $queryable
    +   *   Whether the field is queryable.
    

    same here.

  8. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -184,21 +201,40 @@ public function getFieldCardinality() {
    +  public function setFieldQueryable($queryable) {
    +    return $this->setRequired($queryable);
       }
    

    That seems..,. not correct :)

  9. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -231,4 +269,76 @@ public function getFieldDefaultValue(EntityInterface $entity) {
    +   * Allows creating field definition objects from old style definition arrays.
    +   *
    +   * @todo: Remove once no old-style definition arrays need to be supported.
    +   */
    

    Let's reference the issue that @smiletrl created in the @todo.

  10. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinition.php
    @@ -231,4 +269,76 @@ public function getFieldDefaultValue(EntityInterface $entity) {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function &offsetGet($offset) {
    ...
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function offsetSet($offset, $value) {
    

    Instead of @inheritdoc, should way say that this is a temporary BC layer and refer to the same issue?

  11. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinitionInterface.php
    @@ -129,6 +130,13 @@ public function isFieldTranslatable();
    +   *
    +   * @return bool
    +   */
    +  public function isFieldQueryable();
    

    Also needs a @return description.

  12. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/FieldItemList.php
    @@ -2,28 +2,30 @@
    + * @DataType(
    + *   id = "field_item_list",
    + *   label = @Translation("Entity field"),
    + *   class = "\Drupal\Core\Entity\Field\Field"
    + * )
    

    class =, on an Annotation? Isn't that set by the parser? Apparently, because that class no longer exists? :)

  13. +++ b/core/lib/Drupal/Core/Entity/Plugin/field/field_type/IntegerItem.php
    @@ -16,6 +17,7 @@
      *   id = "integer",
      *   label = @Translation("Integer"),
      *   description = @Translation("An entity field containing an integer value."),
    + *   list_type = "field_item_list",
      *   configurable = FALSE
      * )
    

    Can't we set the default based on configurable, like we did for the list_class?

  14. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
    @@ -0,0 +1,361 @@
    +   *
    +   * @return \Drupal\Core\TypedData\DataDefinition
    +   */
    

    needs description.

  15. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
    @@ -0,0 +1,361 @@
    +   *
    +   * @param boolean $read-only
    +   *   Whether the data is read-only.
    

    bool

  16. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
    @@ -0,0 +1,361 @@
    +   * @param boolean $computed
    +   *   Whether the data is computed.
    ...
    +   * @param boolean $required
    +   *   Whether the data is required.
    

    more bool

  17. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
    @@ -0,0 +1,361 @@
    +   * @param string|null $class
    +   *   The class to use.
    

    null? Should be documented when that is used, if it can be..

  18. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
    @@ -0,0 +1,361 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function offsetExists($offset) {
    

    Same here as in FieldDefinition...

  19. +++ b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php
    @@ -0,0 +1,115 @@
    +   * @return boolean
    +   *   Whether the data is multi-valued.
    ...
    +   * @return boolean
    +   *   Whether the data is read-only.
    ...
    +   * @return boolean
    +   *   Whether the data value is computed.
    ...
    +   * @return boolean
    +   *   Whether a data value is required.
    

    :)

  20. +++ b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php
    @@ -0,0 +1,115 @@
    +   * If not specified, the default class of the data type will be used.
    +   *
    +   * @return string|null
    +   *   The class used for creating the typed data object.
    +   */
    +  public function getClass();
    

    Ah, here is the documentation for that part. Can we put that on the setter as well? (set to NULL to use the default class for the given type, or so)

  21. +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
    @@ -52,77 +52,48 @@ public function __construct(\Traversable $namespaces, CacheBackendInterface $cac
    +   * @param \Drupal\Core\TypedData\DataDefinitionInterface $definition
    +   *   The data definition of the typed data object. For backwards-compatibility
    +   *   an array representation of the data definition may be passed also.
    

    Add another @todo with the issue reference, just so that we remember to update the documentation here?

  22. +++ b/core/modules/content_translation/content_translation.module
    @@ -145,8 +145,15 @@ function content_translation_entity_field_info_alter(&$info, $entity_type) {
             if (isset($info[$key][$name])) {
    -          $info[$key][$name]['translatable'] = (bool) $translatable;
    -          break;
    +          $translatable = (bool) $translatable;
    +          // @todo: Remove array check after all base fields have been
    +          // converted to object.
    +          if (is_array($info[$key][$name])) {
    +            $info[$key][$name]['translatable'] = $translatable;
    +          }
    +          elseif (is_object($info[$key][$name])) {
    +            $info[$key][$name]->setTranslatable($translatable);
    +          }
    

    Can we simplify this too, or does this happen before we convert them to objects? If so, @todo :)

  23. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php
    @@ -203,67 +205,66 @@ public function postSave(EntityStorageControllerInterface $storage_controller, $
    +    $fields['tid'] = FieldDefinition::create()
    +      ->setLabel(t('Term ID'))
    +      ->setDescription(t('The term ID.'))
    +      ->setFieldType('integer')
    

    Wondering create should accept some arguments to improve DX.

    We can't vary the implementation, so we need to be careful, but we could do function create($type = NULL, $label = NULL) for Field and DataDefinition?

    Then this would look like this:

      $fields['tid'] = FieldDefinition::create('integer', t('Term ID')
          ->setDescription(t('The term ID.'))
          ->setReadOnly(TRUE);
    

    And a lot of basic property definitions would just be like this:

    DataDefinition::create('integer', t('Integer value'))
    

    Which I think would be pretty cool :)

  24. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php
    @@ -203,67 +205,66 @@ public function postSave(EntityStorageControllerInterface $storage_controller, $
    +    $fields['uuid'] = FieldDefinition::create()
    +      ->setLabel(t('UUID'))
    +      ->setDescription(t('The term UUID.'))
    +      ->setFieldType('uuid')
    +      ->setReadOnly(TRUE);
    

    And while we're at it and now have an interface and don't have to specify a description anymore, I'm wondering if we should really only add one when there's something useful to say.

    I mean, UUID => The term UUID. Ah yeah, you don't say?! That's stupid* :p

    Same for langcode, name, description.

    * I added those myself, so I'm allowed to say that. I therefore also now that most of the other entity types look pretty much the same.

    We're filling up the locale table/t() cache for absolutely nothing.

  25. +++ b/core/tests/Drupal/Tests/Core/Entity/FieldDefinitionTest.php
    @@ -26,6 +26,34 @@ public static function getInfo() {
       /**
    +   * {@inheritdoc}
    +   */
    +  protected function setUp() {
    +    parent::setUp();
    +
    +    // Prepare a container with a mock typed data object, that returns no
    +    // type definitions.
    +    // @todo: Remove this once all field types are converted and getFieldType()
    +    // does not need the manager any more.
    +    $typed_data = $this->getMockBuilder('Drupal\Core\TypedData\TypedDataManager')
    +      ->disableOriginalConstructor()
    +      ->getMock();
    +
    +    $typed_data
    +      ->expects($this->any())
    +      ->method('getDefinition')
    +      ->will($this->returnValue(NULL));
    +
    +    $container = $this->getMock('Drupal\Core\DependencyInjection\Container');
    +    $container
    +      ->expects($this->any())
    +      ->method('get')
    +      ->will($this->returnValue($typed_data));
    +
    +    \Drupal::setContainer($container);
    +  }
    

    And... this can then be removed too!

effulgentsia’s picture

Shameless plug for a tangentially related issue: #2112759: Add ContentEntityInterface::hasField()

smiletrl’s picture

Status: Needs work » Needs review
FileSize
11.29 KB
96.19 KB

1). fixed

2). Not sure they should be removed. Maybe someone has manually added, e.g., 'langcode' field to be TRUE in content_translation.settings.yml. In that case, we need to tell user this is not allowed.

3). fixed.

4)-9) fixed.

10). Any places we use FieldDefinition/DataDefinition as array? The only place I could see is some code like $item_type_definition['list_type'], maybe we need to add another set/get mathod for this property? And then we perhaps don't need to implement \ArrayAccess?

11)-12) fixed.

13).
'list_type' is used in ListDefinition::getDataType(), default 'list' --\Drupal\Core\TypedData\Plugin\DataType\ItemList.
and Drupal/field/Entity/Field(FieldInstance)::getDataType(), default 'field_item_list'-- \Drupal\Core\Entity\Plugin\DataType\FieldItemList.

It seems not related with 'configurable' flag. However, if we must identity a 'list type'. I would say, we should define \Drupal\Core\Entity\Plugin\DataType\FieldItemList as 'base_field_item_list' and Drupal\field\Plugin\Type\FieldType\ConfigFieldItemList(this should be derived as DataType too. I was also curious, since FieldItemList is a DataType, why not ConfigFieldItemList?) as 'Config_field_item_list'. In that case, we could connect the 'configurable' flag and this 'list_type'.

Then Drupal/field/Entity/Field(FieldInstance)::getDataType(), default 'list_type' should probably be 'Config_field_item_list' and FieldDefinition::getDataType() **(Note, not ListDefinition)**, default 'list_type' should probably be 'base_field_item_list'.

Defining \Drupal\Core\TypedData\Plugin\DataType\ItemList as a 'list', seems too generic to be used in actual context.

But I'm not sure what this 'list_type' is really for. Leave it for @fago.

14)-16) fixed.

17). See 20).

18), as 10).

19). fixed.

20). and 17) not sure about this, there's alreay a setClass() there.

21). fixed.

22). They are before the converting. Added @todo.

23). Not sure about this. Using setter seems straightforward imo. Leave it for @fago :)

24), same as 23). not sure

25). fixed, although I'm not pretty sure this is the right thing to do:)

The last submitted patch, field_definition_class-81.patch, failed testing.

Berdir’s picture

Status: Needs review » Needs work

Thanks.

2. Isn't about changing what it does, just the grammar of the comment.

10. No, not where we use it, where we expose it as BC, e.g. those offset methods, which will all go away. We don't have to do it, my idea was that having explicit @todo's there makes it easier for reviewers and committers too see what's temporary and what not.

13) See FieldTypePluginManager::processDefinition(). Based on configurable, we set the list_class to either the base or the configurable list class. My understanding is that list_type replaces list_class, so we should update processDefinition() to set the list_type instead of having to duplicate it everywhere. It was necessary before they were using @FieldType, it shouldn't be necessary anymore.

Feel free to leave it to @fago, ofc :)

22) Not sure I understand, the @todo is added here and points to the base field as field_type issue, if it's still necessary, then the @todo needs to be updated.

  1. +++ b/core/lib/Drupal/Core/Entity/Field/FieldDefinitionInterface.php
    @@ -133,6 +134,7 @@ public function isFieldConfigurable();
        * @return bool
    +   *   TRUE if the field is queryable..
        */
    

    two . ;)

  2. +++ b/core/modules/content_translation/content_translation.module
    @@ -147,7 +147,7 @@ function content_translation_entity_field_info_alter(&$info, $entity_type) {
    +          // converted to object in https://drupal.org/node/2112239
    

    Here the . is missing :)

amateescu’s picture

About the optional description part, I think it will be useful for when we'll generate table schemas automatically.

Berdir’s picture

Let's ignore the description part and just convert what we have. Sorry for bringing that up here, unrelated :)

fago’s picture

Status: Needs work » Needs review
FileSize
10.86 KB
97.42 KB

Thanks for the reroll, fixes and review!

ad 1) Finally, awesome! Now, we could go and use it to document our public properties better ;-) Anyway, having interfaces + methods is better. Btw, opened related #2112899: NodeInterface handles entity references inconsistently.

>10). Any places we use FieldDefinition/DataDefinition as array?

Yeah, I think there are some left that use it as typed data definition. So that's better left to remove in a follow-up. Watch out though, the $item_type_definition is the plugin definition of the data type, i.e. it stays array based.

>19) Ops, I think I should get used to bool *now* ;-)

>17,20: We've got it already on getClass() also:
* If not specified, the default class of the data type will be used.

>24: Agreed! While I don't think it relates to this issue, it's definitely something we could fix when overhauling all the field definitions. So should we just bake this in?

>23
ad

  $fields['tid'] = FieldDefinition::create('integer', t('Term ID')
      ->setDescription(t('The term ID.'))
      ->setReadOnly(TRUE);

I must say it seems strange that the label is now set as parameter while others are not, i.e. I like the explicit variant better. I think also, because it has just one value per line -> easier to read.
However, the field type is a required property and has to be always specified really - so maybe just have it as sole argument to create()? I.e., for testing you could do $fields['tid'] = FieldDefinition::create('integer') and up with an unlabelled field? Or do we effectively require a label also?

>25) Meanwhile it's still required as getDataType() now needs the typed data manager also. I've been thinking about this dependency on managers in definition objects a bit:
- We don't want to bake in dependencies on the container
- Injecting services into definition objects is not an option, as those need to serialzable very well and fast.
- Having to pass managers etc. everywhere is awkward

So what I've been thinking we could do is:
- add a static, public cache for the required services on the class + a helper method to get them. The helper method fetches the service from the container if it's not there only. So we should be able to easily inject another service for *all* definitions while keeping serialization simple. Thoughts?

I have to leave, so rerolling with phpunit fixes added back (for now) and without addressing 13 yet.

fago’s picture

Assigned: fago » Unassigned

unassigning while not actively working on it

Status: Needs review » Needs work

The last submitted patch, d8_field_definition_class.patch, failed testing.

smiletrl’s picture

1).

10. No, not where we use it, where we expose it as BC, e.g. those offset methods, which will all go away.

I mean we could remove those right now^^, as I didn't see where FieldDefinition/DataDefnition has been used as array. There's no real need to add this array support/code, so why keep those code as any 'BC' stuff:)? But as @fago pointed in #86, maybe they are still used s array somewhere. Will check those..

2).

My understanding is that list_type replaces list_class, so we should update processDefinition() to set the list_type instead of having to duplicate it everywhere.

The current 'list_type' doesn't look right to me. Only two 'list_type' have been used, 'list' and 'field_item_list'. I feel the two 'list_type' should be replaced somehow as mentioned at #81 -- 13). And then we could update the 'list_type' for all field items in a central place(e.g., processDefinition()) based upon whether it's configurable or not. The current two 'list_type', especially the 'list' type, don't make much sense to configurable field item imo.

Also, for me, it's still not very clear how 'list_type' and 'list_class' work together. As far as I see, 'list_type' is to describe the 'DataType' of the 'list_class'. Currently, there're two classes served as 'list_class', i.e., FieldItemList, and ConfigFieldItemList. FieldItemList has been converted to DataType plugin(so its DataType is 'field_item_list'), while ConfigFieldItemList has not(This is one question I've got).

Maybe FieldDefinition::getDataType() should return 'field_item_list' directly to describe list_class 'FieldItemList', and Field/FieldInstance::getType() should return 'some_type' to describe list_class 'ConfigFieldItemList'? They were about to check the itemdefinition firstly, but I guess we could simply distinguish them from the calling context. FieldDefinition is for non-configurable field while Field/FieldInstance is for configurable field. Their item's 'list_type' should be consistent with the parent defintion's context?

smiletrl’s picture

22) Not sure I understand, the @todo is added here and points to the base field as field_type issue

It' pointed to #2112239: Convert base field and property definitions, not the ' base field as field_type issue':) See interdiff at #81.

 // @todo: Remove array check after all base fields have been
-          // converted to object.
+          // converted to object in https://drupal.org/node/2112239

Sorry for the misunderstanding. What I meant at #81 -- 22) is content_translation_entity_field_info_alter(&$info, $entity_type) is invoked before $definition = FieldDefinition::createFromOldStyleDefinition($definition);. So there're still array structures for field definitions at hook_entity_field_info_alter() phase. We need to remove this array support at this hook in #2112239: Convert base field and property definitions.

fago’s picture

Also, for me, it's still not very clear how 'list_type' and 'list_class' work together. As far as I see, 'list_type' is to describe the 'DataType' of the 'list_class'.

list_class is deprecated by list_type and separate data definitions for lists, i.e. the ListDefinition. It's still working in this patch for BC and should be removed with converting everything to data defintion objects.

Thus, previously you had:
t

array(
 'type' => 'string',
 'list' => TRUE,
 'list class' => 'CustomClass',
);

Now you'd have

$list = ListDefinition::create()
  ->setClass('CustomClass');
$list->getItemDefinition()
 ->setDataType('string');

Thus, instead of repeating some stuff with "list foo" keys and some don't (where it remains unclear whether it is for the list or not), we have an explicit data definition for the list + for its item now. I.e, list class becomes just class - and the default class is determined by the list_type. The example would go with the default list_type specified by the string data type (=item_list). You can override it by setting your own list type, or just a special list class.

For DX it might make sense to move to this, actually?

$list = ListDefinition::create('string')
  ->setClass('CustomClass');
fago’s picture

Status: Needs work » Needs review
FileSize
7.19 KB
102.22 KB

Found the problem - the annotation was not correctly modified (comma of previous line remained).

ad >13: Can't we set the default based on configurable, like we did for the list_class?

Yep, I moved the configurable field item list over to a declared list type and overhauled that.

Updated patch and interdiff attached.

Status: Needs review » Needs work

The last submitted patch, d8_field_definition_class.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
102.06 KB
827 bytes

While fixing merge conflicts field_field_widget_info_alter() got added back in, what resulted in troubles. Fixed that.

smiletrl’s picture

A few clean up:

1).

-    if (isset($definition['settings'])) {
-      $item_definition->setSettings($definition['settings']);
-    }
-    if (isset($definition['constraints'])) {
-      $item_definition->setConstraints($definition['constraints']);
-    }

There's no need to set again:) $field_definition = new FieldDefinition($list_definition); has already set them.

2).

-   * @return \Drupal\Core\TypedData\DataDefinition
+   * @return \Drupal\Core\TypedData\DataDefinitionInterface
    *   The object itself for chaining.
    */

It could be ListDefinition/FieldDefinition too, so it should return DataDefinitionInterface.

Other ideas:
a).

Yep, I moved the configurable field item list over to a declared list type and overhauled that.

This makes a lot of sense!

b).

For DX it might make sense to move to this, actually?

Well, at least that example at #91 looks a bit weird:) For ListDefinition/FieldDefinition, it needs to deal with the itemDefinition and the listself, that's perhaps why it's so weird. But for DataDefinition, usually DataDefinition::create('string') is fine, because in most cases, it doesn't need to specify another class. For ListDefintion, we may do

+    $list_definition = ListDefinition::create();
+    $list_definition->getItemDefinition()->setDataType('string');
$list_definition->setClass('CustomClass');

Perhaps, we could define DataDefinition::create($data_type == null), so we can do DataDefinition:create('integer'); directly and use above code for ListDefinition?

c). 'list_class' vs 'list_type'.

The example would go with the default list_type specified by the string data type (=item_list)

Hmm, this doesn't look right:( -- 'list_type' == ItemDefintion's DataType?

'list_type', or 'list_class' seems to be 'defined' at two possible different scearioes:
I). FieldItem annotation, where 'list_type' is something staying at the Data plugin definition array. For example, @FieldType 'integer' could has 'field_item_list' as its 'list_type'. And @FieldType 'textitem' could has 'field_configurable_item_list' as its 'list_type', because this item is configurable.
So, when we need to use the 'list_type', given an item's list_type, we could possibly do this to get this item's List Data:

$item_plugin_definition = \Drupal::typedData()->getDefinition($list->getItemDefintion());
$list_type = $item_plugin_definition['list_type'];
$list_type_plugin_definition = \Drupal::typedData()->getDefinition($list_type);
$list_data = new $list_type_plugin_definition['class'];

But in real usage cases, this is never used. So I'm questioning is it necessary to define the list_type here? What it works is only telling users that this Field Item's list_type could be something, but it's never used or validated when we actually use it IMO.

II). Field/ListDefinition itself. From my understanding, 'list_type' is the 'DataType' that this ListDefintion is trying to define, not the ListDefinition's Itemdefintion is trying to define. So the example at #91 could be

$list = ListDefinition::create()
 ->setDataType('field_item_list');

In this case, $list's 'list_type' is 'field_item_list', so when we need to instantiate the List Data, we firstly find the class which the 'list_type' is using. And then we take that class to instantiate the List Data, i.e., we do

$list_type_definition = \Drupal::typedData()->getDefinition($list->getDataType());
$class = $list->getClass() ? $list->getClass() : $list_type_definition['class'];
$new_list_data = new $class;

If 'list_type' is 'field_item_list', we get $new_list_data = new FieldList(); But, of course, we could use setClass to override its 'list_type', so we do this:

$list = ListDefinition::create()
 ->setDataType('field_item_list')
 ->setClass('CustomClass');

In this case, 'CustomClass' will be used to instantiate the List Data, instead of class 'FieldList'. But in typedDataManager::createInstance(), it uses $defintion['class'](this $defintion could be the List Data's plugin defintion) as $list_type_definition['class'] here. I feel they are the same thing. So the question becomes is concept 'list_type' necessary for Field/ListDefinition itself here:)?

fago’s picture

Regarding #95:

ad 1) There's no need to set again:) $field_definition = new FieldDefinition($list_definition); has already set them.

I don't think so. That sets list settings, not list item settings.

ad 2), a:
True. Ideally, there would be some sort of doxygen support that denotes "@return this" - but I don't think we've adopted a standard on that.

ad 2)b: I was thinking that for list definitions you mostly want to specify the data type of the item - so we pass that there. That suggestion would violate inheritance rules as ListDefinition would treat the parameter differently than DataDefinition, but given you only use that method with the specified ListDefinition/DataDefinition class anyway I think that would be ok.
In regard of making it optional, I'd prefer having it required for DataDefinition as it's actually required. If we make it optional, we could have it default to 'any'?

2)c: Sry, I don't get what you are saying here. The second part of the code example is covered by the typed data factory.

But in real usage cases, this is never used. So I'm questioning is it necessary to define the list_type here?

Sure the list_type is used. With the patch of #94 it's the list_type only that controls the class used for field item lists? It's defined by the field type and so forwarded to the field item's data type definition, which then is respected when the list of field item is created.

II). Field/ListDefinition itself. From my understanding, 'list_type' is the 'DataType' that this ListDefintion is trying to define, not the ListDefinition's Itemdefintion is trying to define. So the example at #91 could be...

Right, but the example was about the 90% use case of specfiy the data type of the list item, not the list.

But in typedDataManager::createInstance(), it uses $defintion['class'](this $defintion could be the List Data's plugin defintion) as $list_type_definition['class'] here.

Yes, now the factory can treat a list definition as any other data definition, it has a type + an optional custom class which it respects.

I feel they are the same thing. So the question becomes is concept 'list_type' necessary for Field/ListDefinition itself here:)?

Yes, in your example the list_type is not used, as you just take the example of a list definition, but don't care about the item. But in practice, you define a list of items - where you need to control the combination of list type+class and list item type+class. And what you want to define in 90% of the cases, is that you say I have a list of data type 'foo' - thus you have a list definition without a specified type + items of the 'foo' data type. Now, the 'list_type' of foo is used to determine the data type we want to use for the list by default.
-> That's how it works for fields also, we've a configurable email field item -> we default to the configurable field item list.

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -470,6 +461,7 @@ public function getFieldDefinitions($entity_type, $bundle = NULL) {
+        // @todo: Refactor to allow for per-bundle overrides.

I opened #2114707: Allow per-bundle overrides of field definitions for this. If this patch is rerolled at some point anyway, please add the link to it.

smiletrl’s picture

@fago, thanks for detailed explanation!

1).

I don't think so. That sets list settings, not list item settings.

Sorry, I mean

// Take care of the item definition now.
    $item_definition = new DataDefinition($definition);

will set those. This will do $this->definition = $definition; for ItemDefinition. Then $item_definition->setConstraints($definition['constraints']); will do $this->definition['constraints'] = $definition['constraints'];, which is a duplicate. But this will be removed soon, not important.

2).

With the patch of #94 it's the list_type only that controls the class used for field item lists?

Yes, right. Definition->getDataType() has used 'list_type' to get the plugin_id for the list definition's data type. And then, in TyepdDataManager::createInstance(), the plugin_id's type definition's class will be used to instantiate the list data, when no 'class' has been spcified for the Data Definition itself.

3).

is that you say I have a list of data type 'foo' - thus you have a list definition without a specified type + items of the 'foo' data type.

Well, I was thinking since we already have 'DataType' to describe the list data, why we need another concept 'list_type' for the list definition. But now, I could say that 'list_type' is a concept of the field item/type, and it serves as list data's DataType when its type is not set.

4).

// @todo: Should "name" be part of the data definition also?

I think it's not necessary. Data dafinition has label, id, type to identify it. We don't need another 'name' for it. 'FieldName' should be exclusively connected to FieldDefintion.

5).

In regard of making it optional, I'd prefer having it required for DataDefinition as it's actually required.

Well, @Berdir wants it too:) I did a try in this patch, making the 'type' required. Field/List/DataDefinition have different default 'type', i.e, the Data Type. It's ok for the DataDefinition, but there's a problem with FieldDefinition.

When we define a field definition for complex data, we do something similar:

+    $fields['tid'] = FieldDefinition::create()
+      ->setLabel(t('Term ID'))
+      ->setDescription(t('The term ID.'))
+      ->setFieldType('integer')
+      ->setReadOnly(TRUE);

With this patch, the new FieldDefinition will have a field type 'field_item_list' set automatically. This is ok when @FieldType 'integer's list_type is 'field_item_list'. However, if @FieldType 'integer's list_type is some type else, like 'custom_list_type', this new FieldDefintion->getDataType() will return ''field_item_list'', instead of 'custom_list_type'. This is because getDataType() will check $this->definition['type'] before check its itemDefinition's 'list_type'. And we have already set $this->definition['type'] = 'field_item_list' when we create/construct the FieldDefinition.

So, I think it's not a good thing for FieldDefinition to have a default 'type', unless we reverse the checking order in FieldDefintion->getDataType(). But that will be something unlogical then. Consequently, it's not a good thing for DataDefinition to require 'type' IMO.

Status: Needs review » Needs work

The last submitted patch, field_definition_class-2047229-97.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
2.16 KB
101.92 KB

This should fix that error.

Also, make ListDefinition's parameter $type optional in create(), while leave it required for DataDefinition. See reason at #98 --5).

smiletrl’s picture

Berdir’s picture

Tough re-roll after #2115145: Move field type plugins to Plugin/Field/FieldType.

Let's see how much I messed up.

Status: Needs review » Needs work

The last submitted patch, field_definition_class-2047229-102.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
4.66 KB
101.9 KB

Quite messed up.

Status: Needs review » Needs work

The last submitted patch, field_definition_class-2047229-104.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.27 KB
102.48 KB

Lost a few changes due to all those file moves.

smiletrl’s picture

Thanks for reroll.

A few nitpicks:
1).

+++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/ItemList.phpundefined
@@ -2,16 +2,28 @@
 
 /**
  * @file
- * Contains \Drupal\Core\TypedData\List.
+ * Contains \Drupal\Core\TypedData\Plugin\DataType\ItemList.
  */
 
-namespace Drupal\Core\TypedData;
+namespace Drupal\Core\TypedData\Plugin\DataType;
+
+use Drupal\Core\TypedData\Annotation\DataType;
+use Drupal\Core\Annotation\Translation;
+use Drupal\Core\TypedData\ComplexDataInterface;
+use Drupal\Core\TypedData\ListInterface;
+use Drupal\Core\TypedData\TypedData;
+use Drupal\Core\TypedData\TypedDataInterface;

only

+use Drupal\Core\TypedData\TypedData;
+use Drupal\Core\TypedData\ListInterface;

required.

2).

+   * @return \Drupal\Core\Field\FieldDefinitionInterface[]
+   * An array of entity field definitions, keyed by field name.
    *

Need two spaces before 'An array..'

3).

+   * @todo: Having this as well as $definition->getConstraints() is confusing.
    */
-  public function getConstraints($definition) {
+  public function getConstraints(DataDefinitionInterface $definition) {

This @todo is interesting:) Another getConstraints() comes from FieldItem class. What's the connection between these functions?

It seems TypedData/FieldItem::getConstraints() call TypedDataManager::getConstraints()(From the top parent::getConstraints() at TypedData::getConstraints()), and TypedDataManager::getConstraints() call $definition->getConstraints() in the following code way.

+  /**
+   * Returns an array of validation constraints.
+   *
+   * See \Drupal\Core\TypedData\TypedDataManager::getConstraints() for details.
+   *
+   * @return array
+   *   Array of constraints, each being an instance of
+   *   \Symfony\Component\Validator\Constraint.
+   */
+  public function getConstraints() {
+    return isset($this->definition['constraints']) ? $this->definition['constraints'] : array();
+  }

Does this doc makes sense? If this function has already returned instance of \Symfony\Component\Validator\Constraint., then why $typedDataManager->getConstraints() do this again?

+    $defined_constraints = $definition->getConstraints();
+    foreach ($defined_constraints as $name => $options) {
+      $constraints[] = $validation_manager->create($name, $options);
     }

I think we need to change the doc for this part. It should return something $constraints like this

array( 'ComplexData' => array(
            'value' => array(
              'Length' => array(
                'max' => 255,
              )
            )
          )
)

These are added when user creates a DataDefintion, it's beyond the DataTypeself, but only for this specific Data. This array structure applys to following array $constraints too.

   public function setPropertyConstraints($name, array $constraints) {
-    $this->definition['item_definition']['constraints']['ComplexData'][$name] = $constraints;
+    $item_constraints = $this->getItemDefinition()->getConstraints();
+    $item_constraints['ComplexData'][$name] = $constraints;
+    $this->getItemDefinition()->setConstraints($item_constraints);
     return $this;
   }
fago’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

Thanks all for the improvements and re-rolls.

Attached patch addresses 1+2 from the above review. For 3, I've created a summary of the previous related discussion over at #2116341: Apply defaults to definition objects as we wanted to fix that in a follow-up. Maybe best let's continue discussing that over there.

+++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
@@ -169,7 +169,7 @@ public function isRequired() {
+   * @return \Drupal\Core\TypedData\DataDefinitionInterface

I don't think this should note the interface here as it's not a method of the interface, but just on the class only. The class knows it will return an instance of the class - not the interface, what is more specific and more concrete. So we should return that.

When, in the end the class is FieldDefinition not DataDefinition, that's unfortunate - but still DataDefintiion is more specific and thus better than DataDefinitionInterface. A setter on DataDefintion will always a return an instance of DataDefintion :-)

Also, this was inconsistent in the current patch. So I went a head and fixed it + made it consistent again. Apart from that, I went over the definition class again and made sure docs are up2date and accurate.

Attached patch also implements the suggestion related the static typed data helper and simplifies the unit test. See attached interdiff.

fago’s picture

Issue summary: View changes

added follow-up

fago’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

I also updated the summary + created some follow-ups.

If this is green again, this is already quite ready imo. One (rather critical) thing still remains though: Performance checks.

We should check (objects vs arrays) serialization speed and try whether we can optimize classes more, i.e. check whether removing $this->definition in favour of a list of defined (or even not defined) class properties consumes less memory. I think, #2116363: Unified repository of field definitions (cache + API) will make this an overall performance-gain, but we need to know the impact of this anyway.

Status: Needs review » Needs work
Issue tags: -Needs tests, -API change, -Entity Field API

The last submitted patch, d8_field_definition_class.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review

#108: d8_field_definition_class.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +API change, +Entity Field API

The last submitted patch, d8_field_definition_class.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
104.96 KB
5.33 KB

Looks like the static-cache of the typed data manager becomes stale when the container is rebuilt for some test runs :-/ As I've not seen a simple way to fix that, attached patch reverts that part.

twistor’s picture

Other patches have been using "@return self" specifically for method chaining.

smiletrl’s picture

Maybe we are ok with this?

+ // @todo: Overhaul this once field definitions to not use the container to
+ /// get the manager any more.

If we don't need the container to get manager, then FieldDefinition probably needs to inject manager in __construct(), which will make the test easier. Based on current usage of FieldDefinition, this doesn't look necessary though.

Berdir’s picture

"defined (or even not defined)"

Accessing undefined properties is AFAIK slower and PHP 5.5 AFAIK contains more performance improvements for accessing defined properties.

fago’s picture

FileSize
611 bytes

Other patches have been using "@return self" specifically for method chaining.

Oh great, let's use that also then!

If we don't need the container to get manager, then FieldDefinition probably needs to inject manager in __construct(), which will make the test easier. Based on current usage of FieldDefinition, this doesn't look necessary though.

As field definitions needs to be serialized/unserialized a lot and easily, I don't think we want to inject services as usual. Also that would make creating them more complicated.

>Maybe we are ok with this?
I think the current situation is acceptable, while we could try getting the approach reverted in #113 running in a follow-up?

Accessing undefined properties is AFAIK slower and PHP 5.5 AFAIK contains more performance improvements for accessing defined properties.

I ran some tests on that, with php 5.3.10 and php 5.5.3. In my tests I noted memory usage of 1000 definitions and measured unserialization speed by unserializing it 5000 times. You can find my test script attached to this post. So here the results:

PHP 5.3.10

unserializing 1000 array definitions, 5000 times
955.453125 kBytes
8588.65ms for 5000 runs.
1.71773ms per run

unserializing 1000 classed definitions (as the patch implements), 5000 times
1463.9921875 kBytes
15134.9ms for 5000 runs.
3.02698ms per run

same test with class without array and without declared properties
1118.7421875 kBytes
11516.14ms for 5000 runs.
2.303228ms per run

with declared properties and reasonable default values (array(), true|false,...)
1913.296875 kBytes
28181.84ms for 5000 runs.
5.636368ms per run

with declared properties without default values
1912.8828125 kBytes
24250.75ms for 5000 runs.
4.85015ms per run

PHP 5.5.3

unserializing 1000 array definitions, 5000 times
627.921875 kBytes
5498.59ms for 5000 runs.
1.099718ms per run

unserializing 1000 classed definitions, 5000 times
752.96875 kBytes
11459.82ms for 5000 runs.
2.291964ms per run

class without array and without declared properties
682.4296875 kBytes
8655.69ms for 5000 runs.
1.731138ms per run

with declared properties and reasonable defautl values (array(), booleans,...)
418.765625 kBytes
21774.24ms for 5000 runs.
4.354848ms per run

with declared properties without default values
418.578125 kBytes
20533.05ms for 5000 runs.
4.10661ms per run

Optimized variant with having only the used properties (type + label) defined
370.4765625 kBytes
9026.62ms for 5000 runs.
1.805324ms per run

Optimized variant with PHP 5.3.10
1123.1640625 kBytes
12549.25ms for 5000 runs.
2.50985ms per run

Summarizing, it can be seen that objects in php 5.5 use way less memory (which should be the case since 5.4) - even less than arrays. Afaik, it maps structures with defined properties to plain C-structs now. So having defined class properties helps to reduce memory usage here, but increases unserialization time. That makes sense as by having more properties you've a more complex data structure and variables that needs to be unserialized - the more complex the serialized objects are, the longer the unserialization takes.
I think, we should go either with the variant of not defining any properties at all, or with an optimized variant that defines properties for the properties being 90% there, so we can benefit from PHP 5.5s memory improvements.

Compared to arrays, we loose unserialization speed by moving to objects - which amounts up to 1ms per 1000 field-definitions with the suggested variants. However, given that this allows us to remove one layer of necessary field definitions, we should end up with simpler code + less data that needs to be unserialized, so I'd not be surprised if it's faster or no measurable difference in the end. Regardless, of speed, having only one layer of field definitions left should become better memory wise. But note, that for reaching that we've to do #2116363: Unified repository of field definitions (cache + API) first as with this patch, we still have 2 field-info systems in place what probably results in unnecessary duplication of the same data structures right now.

fago’s picture

also did a quick comparison of the anonymous frontpage load performance with 10 nodes for the current patch:
With patch
Page execution time was 391.01 ms. Memory used at: devel_boot()=5.62 MB, devel_shutdown()=17.49 MB, PHP peak=17.75 MB.
389.082 average with ab, 50 times
Without patch
Page execution time was 392.06 ms. Memory used at: devel_boot()=5.63 MB, devel_shutdown()=17.3 MB, PHP peak=17.5 MB.
390.534 average with ab, 50 times

There is no measurable difference (difference is below sd), while memory usage seems to be slightly higher. However tests ran with php 5.3.10 - so that should be better with 5.5.

smiletrl’s picture

FileSize
1.36 KB
1.51 KB
1.46 KB

Interesting. I also did a few test, but only for Apache/2.2.20 (Win32) PHP/5.4.13

Here's the result, each one run for 3 times:
Test.txt:

1.5137169361115ms for 100000 runs. 1.5137169361115E-5ms per run. 135128bytes used. 262144byted peaked.
1.5114908218384ms for 100000 runs. 1.5114908218384E-5ms per run. 135128bytes used. 262144byted peaked.
1.5319490432739ms for 100000 runs. 1.5319490432739E-5ms per run. 135128bytes used. 262144byted peaked.

Test2.txt:

1.4896590709686ms for 100000 runs. 1.4896590709686E-5ms per run. 135168bytes used. 262144byted peaked.
1.4907081127167ms for 100000 runs. 1.4907081127167E-5ms per run. 135168bytes used. 262144byted peaked.
1.5099201202393ms for 100000 runs. 1.5099201202393E-5ms per run. 135168bytes used. 262144byted peaked.

Test3.txt:

1.3457481861115ms for 100000 runs. 1.3457481861115E-5ms per run. 134104bytes used. 262144byted peaked.
1.3272299766541ms for 100000 runs. 1.3272299766541E-5ms per run. 134104bytes used. 262144byted peaked.
1.3222961425781ms for 100000 runs. 1.3222961425781E-5ms per run. 134104bytes used. 262144byted peaked.

While undefined properties are not recommended. It seems individual properties, i.e., Test2.txt is better than Test.txt - a central $definition array for serialization speed.
Edit: Ignore this test. I run another test this morning. It tells different results compared with above one. There's no significant difference between the first two scripts, but the third one always wins.

smiletrl’s picture

A quick note

I think the current situation is acceptable, while we could try getting the approach reverted in #113 running in a follow-up?

Sure.

Berdir’s picture

Looks great to me overall. Love the amount of workarounds and @todo's that we can remove (even though we had a few more, but those are clearly defined and approachable). I think it would be great if @yched/@effulgentsia could review/RTBC this.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
@@ -42,14 +42,15 @@ public function initTranslation($langcode);
    *
+   * Implementations may use the class \Drupal\Core\Field\FieldDefinition for
+   * creating the field definitions.
+   *

"May" seems a bit week given that it's like the 99% use case. At least ;)

"typically use"?

Would also be great if we could include a code example here. just take a nice one from Term and put it a @code/@endcode block (or whatever that syntax is again).

smiletrl’s picture

1). --> we should go either with the variant of not defining any properties at all, or with an optimized variant that defines properties for the properties being 90% there
Defined properties may make code more readable?

2). Patch has used a lot of setters, but it's not suggested by Google: Avoid writing naive setters and getters. Maybe setters' benifit is to enable chained commands here?

fago’s picture

> Defined properties may make code more readable?
Why? Code will use the methods anyway.

>Maybe setters' benifit is to enable chained commands here?
Yes, I think for sure a good reason to have them - so we have better DX when defining definitions.

#121: Agreed (with all of it).

fago’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

Here is an updated, re-rolled patch which addresses #121.

I discussed with berdir on IRC on how we should attack the performance details (on how to internally structure the class) best.

I think we should move away from the internal array variant as other variants are faster and involve less levels of nesting, thus are better to debug and feels less ugly - thus move to non-declared, public properties right now + figure out what to declare or not in a follow-up. However, berdir prefers to dealing with all in a follow-up and points out we can stay away from interesting side effects in the bc layer ($definition['itemDefinition']..). So, in order to move on with this faster, we agreed on better handling all of it in a follow-up: #2120373: Optimize performance of class based field definitions

fago’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

I forgot to add the code example, so here it is.

Status: Needs review » Needs work

The last submitted patch, d8_field_definition_class.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
105.24 KB

I lost the interdiff from #113 in my branch - re-added that and re-rolled it.

Status: Needs review » Needs work

The last submitted patch, d8_field_definition_class.patch, failed testing.

smiletrl’s picture

Status: Needs work » Needs review
FileSize
6.67 KB
105.72 KB

Last patch has so many errors, so I reroll from #113 and add the interdiff since then. Hope this won't break so much~

Status: Needs review » Needs work

The last submitted patch, field_definition_class-2047229-129.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
1.68 KB
105.74 KB

The recently added use statement for the field definition interface of the ConfigItemList got lost during the git move+merge, so re-added that + incorporated and improved "@return self" improvements of 129.

Status: Needs review » Needs work
Issue tags: -Needs tests, -API change, -Entity Field API

The last submitted patch, d8_field_definition_class.patch, failed testing.

fago’s picture

Status: Needs work » Needs review

#131: d8_field_definition_class.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, d8_field_definition_class.patch, failed testing.

fago’s picture

Status: Needs work » Needs review

#131: d8_field_definition_class.patch queued for re-testing.

fago’s picture

fago’s picture

#131 is ready for review, anyone?

fago’s picture

Issue summary: View changes

Updated issue summary.

effulgentsia’s picture

Issue summary: View changes
FileSize
105.99 KB

Straight reroll for HEAD changes.

smiletrl’s picture

Status: Needs review » Reviewed & tested by the community

I think we could make this move...

yched’s picture

Reviewing - sorry, had limited time the past two weeks, and I focused on #2019055: Switch from field-level language fallback to entity-level language fallback

effulgentsia’s picture

Perhaps yched will find more, but here's what I found:

  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -358,19 +361,22 @@ public function getFieldDefinitions($entity_type, $bundle = NULL) {
    +            // For manually defined fields, make sure the field name is set.
    +            if ($definition instanceof FieldDefinition) {
    +              $definition->setFieldName($field_name);
    +            }
    

    I don't get what "manually" means in this comment.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -358,19 +361,22 @@ public function getFieldDefinitions($entity_type, $bundle = NULL) {
    +            // Ensure ID and langcode fields are never made translatable.
    

    "ID" is not quite right here, since it includes the bundle key as well. I think the original comment of "ids" was better in that at least it was explicitly plural. Alternatively, how about "keys"?

  3. +++ b/core/lib/Drupal/Core/Field/Annotation/FieldType.php
    @@ -117,6 +117,6 @@ class FieldType extends DataType {
    -  public $list_class;
    

    This annotation is still being used in FileItem and ImageItem, so can we keep it defined here until those are removed?

  4. +++ b/core/lib/Drupal/Core/Field/FieldDefinitionInterface.php
    @@ -8,14 +8,16 @@
    + * for a particular entity (see
    + * \Drupal\Core\Field\FieldItemListInterface). For example, $node_1->body
    

    unnecessary line break

  5. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
    @@ -8,6 +8,8 @@
    +use Drupal\Core\TypedData\DataDefinition;
    +use Drupal\Core\TypedData\DataDefinitionInterface;
    

    Unnecessary use statements?

  6. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
    @@ -23,9 +25,16 @@
       /**
    +   * The field's definition.
    +   *
    +   * @var \Drupal\Core\Field\FieldDefinitionInterface
    +   */
    +  protected $definition;
    

    The base class, TypedData, declares this as DataDefinitionInterface. This class does not yet use the more specific interface, so why declare it?

  7. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
    @@ -23,9 +25,16 @@
    -  public function __construct(array $definition, $name = NULL, TypedDataInterface $parent = NULL) {
    +  public function __construct($definition, $name = NULL, TypedDataInterface $parent = NULL) {
    

    Here and in all other similar constructors: can't we typehint $definition to the appropriate *DefinitionInterface, since TypedDataManager::create() makes the object, even if given an array?

  8. +++ b/core/modules/field/field.module
    @@ -194,16 +194,16 @@ function field_entity_field_info($entity_type) {
    +    // @todo: Improve hook_entity_field_info() so that it can take field
    +    // instance also.
    

    I don't get what this comment is saying. Is it related to #2114707: Allow per-bundle overrides of field definitions, or something else?

yched’s picture

Sorry, this is taking me a bit long, but I'm really having a hard time wrapping my head around what the patch does and evaluating the consequences.

I'm still reading through it and writing notes and remarks, but one of the things that confuses me most is the 'list_type' stuff - and reading the *very cryptic* discussion about this area in #91 / #95 / #96 leaves me quite perplexed and scared :-/.

- It's not clear to me why this is needed here, looks like the patch is doing #1928938: Improve typed data definitions of lists as well, how is it related ?

- The patch adds two "list types" to the set of data types, 'field_item_list' and 'field_configurable_item_list', by adding @DataType annotations to FieldItemList and ConfigFieldItemList.
Both classes thus get moved away to the DataType plugin discovery folder - ew, thats really unfortunate. Those classes are really important to the field system, they should have no business hidden away in a TypedData folder.
(side note, the @file phpdoc for ConfigFieldItemList is not updated accordingly)

- According to the changes in Annotation/FieldType.php, @FieldType annotations are now supposed to refer to a 'list_type' rather than a 'list_class'.
But none currently does yet, so they receive 'list_type' = 'field_item_list', which is the default. FieldTypePluginManager::processDefinition() then does :

// For configurable fields, default to 'field_configurable_item_list'.
if ($definition['configurable'] && $definition['list_type'] == 'field_item_list') {
  $definition['list_type'] = 'field_configurable_item_list';
}

I don't get this. file / image field types use a specific ItemList class ("list_class" = "FileFieldItemList") for which the patch defines no associated 'list_type'. So they come up with 'list_type' = 'field_configurable_item_list', which points to ConfigFieldItemList. So how come $node->field_image still gets created with class FileFieldItemList rather than ConfigFieldItemList ?
The list of known datatypes has one 'field_item:[field_type]' entry per field type, for data types of items, but then only 'field_item_list' and 'field_configurable_item_list' for data types of item lists. Why only those two ? Or, why do those two need to be different data types and not "file item list" or "entity ref item list" ?
Also, if this code is temporary until all @FieldType annotations use the new 'list_type' instead of the old 'list_class', shouldn't there be a @todo to remove it at some point ?

- In the end, I don't get what deserves to be exposed as a 'list' data type or not, but I would find it a huge DX-- that field types that need to use a dedicated ItemList class (e.g File, EntityRef...), now need to expose that list class as a @DataType plugin (and be moved away from their associated Item class to the DataType discovery folder).

- as far as the field definitions are involved, it seems there is non-trivial logic to allow the 'list_type' (or list class or whatever) to be defined on a field-by-field basis. I'm not sure this is something we want. You define a field of a given type (item type), and the list class/type used is stricly imposed by that (it's the one defined by the @FieldType plugin definition) ? I don't think we want to allow swapping a different list class ?

smiletrl’s picture

Ok, I'm trying to answering @yched's concern at #142:)

1. -> - It's not clear to me why this is needed here, looks like the patch is doing #1928938: Improve typed data definitions of lists as well, how is it related ?

I think work for #1928938: Improve typed data definitions of lists has been done here. In this patch, it has implemented the idea of getting rid of 'list_class', infavor of using 'list_type' with real usage, i.e., assign 'list_type' to field type, or field item's annotation.

2. -> Both classes thus get moved away to the DataType plugin discovery folder - ew, thats really unfortunate. Those classes are really important to the field system, they should have no business hidden away in a TypedData folder.

Well, the current way of instantiating the two classes is lying in TypedDataManager::createInstance(). For example, when we do $node->get('field_tag'), TypedDataManager will finally be responsible for checking the field item's "list_class", and instantiating the list_class here. So I'm thinking, why not expose "list_class" itself as typed data too? In this way, we could say we are instantiating the typed data, e.g, 'ConfigFieldItemList' directly. We will not have to check the field item's so-called 'list_class' to get the property of an content entity.

But yeah, this will add some the two common list_class, or more list classes, into typed data manager's cache. Hmm, other disadvantages of doing it this way?

3. -> So how come $node->field_image still gets created with class FileFieldItemList rather than ConfigFieldItemList ?

This is a good question:) Because we have a BC layer there to support list_class. See Field/Fieldinstance::getClass()

+  /**
+   * {@inheritdoc}
+   */
+  public function getClass() {
+    // For BC check for a defined list class.
+    // @todo: Remove once https://drupal.org/node/2112239 is in.
+    $item_type_definition = \Drupal::typedData()
+      ->getDefinition($this->getItemDefinition()->getDataType());
+
+    if (isset($item_type_definition['list_class'])) {
+      return $item_type_definition['list_class'];
+    }
+  }

In fact, we use Field as field data definition for configurable field now. So when we need to instantiate field_image here:

  public function createInstance($data_type, array $configuration) {
    $data_definition = $configuration['data_definition'];
    $type_definition = $this->getDefinition($data_type);

    if (!isset($type_definition)) {
      throw new \InvalidArgumentException(format_string('Invalid data type %plugin_id has been given.', array('%plugin_id' => $data_type)));
    }

    // Allow per-data definition overrides of the used classes, i.e. take over
    // classes specified in the type definition.
    $class = $data_definition->getClass();
    $class = isset($class) ? $class : $type_definition['class'];

    if (!isset($class)) {
      throw new PluginException(sprintf('The plugin (%s) did not specify an instance class.', $data_type));
    }
    return new $class($data_definition, $configuration['name'], $configuration['parent']);
  }

getClass() will give itemDefinition's, i.e. ImageItem's 'list_class' for instantiation. When all 'list_class' have been removed, we could (Edited) delete this function remove this list_class part inside Field/FieldInstance. P.ssorry for the confusing, we can't delete it. Therefore it will fallback to the $type_definition's class. This $type_defintion's $data_type is returned by Field::getDataType(), which will either return field item's list_type, or the default type 'field_configurable_item_list'. Then we get eveything work as expected.

smiletrl’s picture

4. -> but then only 'field_item_list' and 'field_configurable_item_list' for data types of item lists. Why only those two ? Or, why do those two need to be different data types and not "file item list" or "entity ref item list" ?

This is because we haven't been there:) We probably need to add more "list_type" for other field items. For example, ImageItem still gets a 'list_class' in its annotation. We will need to expose its list class, i.e., FileFieldItemList as typed data too, and it will serve as list data type. Anyway, this could be done #2112239: Convert base field and property definitions.

5. -> Also, if this code is temporary until all @FieldType annotations use the new 'list_type' instead of the old 'list_class', shouldn't there be a @todo to remove it at some point ?

Well, @todos are covered at Field/FieldInsance::getclass(), FieldDefinition::createFromOldStyleDefinition(), DataDefinition::createFromOldStyleDefinition() and possibliy other places.

6. -> I don't get what deserves to be exposed as a 'list' data type or not
Well, I could say, if there's an old list_class somewhere, then this 'list_class' deserves to be a list data type. However, this rule is not strictly defined. Currently, for base field definition, we could override the class used to instantiate the list, with something like

+    $fields['tid'] = FieldDefinition::create()
+      ->setLabel(t('Term ID'))
+      ->setDescription(t('The term ID.'))
+      ->setFieldType('integer')
+      ->setReadOnly(TRUE)
        ->setClass('\some_class');

In this way, when we need to instantiate list field 'tid', even as we have data type 'integer's list_type, i.e. field_item_list, to be the 'data type' for this list, we have 'class' property overrided by setClass(). And getClass() will then return '\some_class' for us to instantiate this list. Anyway, setClass() is only available for DataDefinition/Fielddefiniton. Configurable field defintion Field/FieldDefinition don't have this option.

7. -> You define a field of a given type (item type), and the list class/type used is stricly imposed by that (it's the one defined by the @FieldType plugin definition) ? I don't think we want to allow swapping a different list class ?

One way to swap another class for instantiation of list data is described as in #6 above, which only applies to base field though.

The other way is to implement hook_entity_field_info_alter(). That hook doc needs to be updated at #2112239: Convert base field and property definitions. Developer could alter the definition for specific field. For example, he/she could override field defintion's class (for base field), or override list field defintion's data type by using setDataType(), which I just found only works for base field too.

For configurable field, I think we could add setDataType() and setClass() to Field/FieldInstance accordingly, just like what FieldDefintion does. We also need to alter Field/FieldInstance::getClass() and Field/FieldInstance::getDataType(). In this way, we could define field-by-field list type for configurable field too.

fago’s picture

Thanks for the reviews!

I think work for #1928938: Improve typed data definitions of lists has been done here. In this patch, it has implemented the idea of getting rid of 'list_class', infavor of using 'list_type' with real usage, i.e., assign 'list_type' to field type, or field item's annotation.

Exactly. The list improvement has been baked in here, as it really relates to re-designing how the definitions work and helps us minimizing API changes.

So they come up with 'list_type' = 'field_configurable_item_list', which points to ConfigFieldItemList. So how come $node->field_image still gets created with class FileFieldItemList rather than ConfigFieldItemList ?

Yep, as smiletrl points out this is the BC-support for 'list class' acting. It has been left to a follow-up to move those classes over.

Or, why do those two need to be different data types and not "file item list" or "entity ref item list" ?

As of the current patch and removed BC support for 'list class' those would all be list types, i.e. field types could define their default "list type" only. Overriding with classes only would be only supported on definition level, e.g. for $node->author but not for a field type.

Imo it makes sense to have those "classes" specified as list types, i.e. it's not just a class overriding something, it's public API the field type exposes and other modules need to be able to rely upon. Having it declared as separate plugin makes that clearer imho. Interfaces might make sense also - depending on the concrete use, but that's another issue.

5. -> Also, if this code is temporary until all @FieldType annotations use the new 'list_type' instead of the old 'list_class', shouldn't there be a @todo to remove it at some point ?

Yep, as pointed out there are some, but I agree with effulgentisa that we should keep list_class on the field-type defined for now then + add the @todo there also.

- In the end, I don't get what deserves to be exposed as a 'list' data type or not, but I would find it a huge DX-- that field types that need to use a dedicated ItemList class (e.g File, EntityRef...), now need to expose that list class as a @DataType plugin (and be moved away from their associated Item class to the DataType discovery folder).

Oh, I forgot FileFieldItemList being in the same directory as FileFieldItem - I thought we generally only put plugins in there. I agree that in comparison to that moving to a separate list_type and putting the class somewhere else, this is a DX degression :/
So, we could keep supporting 'list class' for field type plugins, while the typed data stuff keeps working as in the patch with list-types?

Question is whether the list_type stuff makes sense in general then, or whether we should stay with list_class only? E.g. List definitions could have a fixed 'list' type and work with the definition-class overrides based on item-types only. Any thoughts on that?

You define a field of a given type (item type), and the list class/type used is stricly imposed by that (it's the one defined by the @FieldType plugin definition) ? I don't think we want to allow swapping a different list class ?

Well, that's already the case and stays as long as we use the typed data factory to instantiate it, as it has that flexibility built in. I'm not sure why it would be a bad thing to be able to override the class per field definition though? E.g. you could use it to provide a custom class for the node author reference and add in suiting methods, i.e. I could see it becoming useful at some point.

fago’s picture

Question is whether the list_type stuff makes sense in general then, or whether we should stay with list_class only? E.g. List definitions could have a fixed 'list' type and work with the definition-class overrides based on item-types only. Any thoughts on that?

Thinking it through, I actually think not making use of list types at all makes it more consistent, i.e. we could
- keep list_class on field types and data types for defining defaults
- have one built-in type 'list' that every list has
- apply the list class via the list definition's class property

That way, it's consistent with field types, we keep better DX and it's more clear that the list class comes from the item type. I figured the list_type as proposed by the patch has another DX problem: $list_definition->setType('string_list') would make me think the system does know about items being 'string' already, but it doesn't as the list type is determined based on the item type - not the other way round. Without having custom list types, we do not run into this situation.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Hm. This does not look RTBC to me.

smiletrl’s picture

1.

Oh, I forgot FileFieldItemList being in the same directory as FileFieldItem - I thought we generally only put plugins in there. I agree that in comparison to that moving to a separate list_type and putting the class somewhere else, this is a DX degression :/

Well, as @effulgentsia pointed at #2023563-66: Convert entity field types to the field_type plugin, point #4, "FileFieldItemList" has been put into field type plugin directory without @FieldType annotation too. And then @berdir has moved this "override class" out of the field type directory at [##2023563-69]. I think this is the chance for us to do the same thing for "FileFieldItemList" too. It's not a field type, rather a class used to override other list class. Therefore, "FileFieldItemList" shouldn't be put in the same directory with "FileFieldItem", as the current Drupal Head does.

If we continue with the "list type" concept here, we will probably need to move "FileFieldItemList" to @DataType directory. If we reserve 'list class', we should move it out of the directory too, imo.

2.

Thinking it through, I actually think not making use of list types at all makes it more consistent, i.e. we could
- keep list_class on field types and data types for defining defaults
- have one built-in type 'list' that every list has
- apply the list class via the list definition's class property

Well, I personaly don't have much opinion regarding the "list class" vs "list type" before this patch. But since we have gone so far for "list type" here, I would say "list type" isn't that bad as described here imo.

At the field item level, "list type" serves basicly the same as "list class" for a field item, i.e., provides the a class to instantiate for a field, e.g. field_image, field_tag. The difference is we have exposed "list class" as a data type here, i.e., "list type". When we need to get the old "list class", we need to go through TypedDataManager to get the type definition of 'list type' firstly, and then use the $type_definition['class'] to get the old "list class".

a)

- have one built-in type 'list' that every list has.

As said above, "list type" is mainly tight to field item to provide old 'list class'. After this concept, we get to the list itself. This "list type" of the list item will serve as the 'data type' for the list when no 'data type' is provided for this list. It could also be reset/override by setDataType(), i.e., each 'type' of the list could be far different. "list type" depends on the field item of the list. It also depends on the real usage of list-self, i.e, we could possibly override/reset it when necessary.

If we are going to dismiss "list type", I'm not sure what's the point of a built-in type for list. What does this "type" work for?

b)

- apply the list class via the list definition's class property

Well, we have done this with "list type" now. "apply the list class via list defintion's clas property" is simply a copy of "apply the list type via the list defintion's type property", which is something we have done in the patch.

Also, there's setClass() for FieldDefinition to set the class. And this 'class' property will gain higher priority before the 'data type' of the list is checked to instantiate the list. For Field/FieldInstance, we don't have such setter, as well as setDataType(). They probably need to be added as suggested at the end of #145.

c).

- keep list_class on field types and data types for defining defaults

Hmm, "list type" is not on field types? Imo, "list type" is on field types just like "list class", as a). has stated. Technically speaking, there's not much difference to instantiate this list with field item's 'list type', compared to field item's "list class" imo. Thing is the concept we have brought here. The confusing part could be 'data type' for the list.

We have tried to identify a 'data type' for list itself. And in most cases, this list "data type" comes from the "list type" of this list's field item. From the code sense, FieldDefinition is not defining some "Field", but defining some ' Data'. This 'Data' is fieldItemList, configFieldItemList, etc, and this data's type is 'field_item_list', 'field_configurable_item_list' accordingly. In this "Data"'s defintion(this defintion is "FieldDefintion"), it contains an ItemDefintion(this "defintion" is DataDefinition), which is used to define the 'field type', e.g., "FieldItem:integer" field type.

So basically, we get a "FieldDefintion" to define some "typed data", and a "DataDefintion" to define some "field type". Isn't that quite weird enough:)? In a compromised word, we could say "field type" belongs to "typed data", at least for now. And "DataDefintion" is to define some simple 'data', while "FieldDefintion" is to define some complex 'data', which contains a list of simple 'data'.

Another compromised understanding could be, FieldDefintion is not defining some "typed data", e.g., field_item_list. It is defining a list, which is neither 'typed data', nor 'field type'. This list has a property called 'type', and another property called 'class'. Property 'class' will be firstly checked to instantiate the list, and then it comes to property 'type'. Maybe we could invent something else to call the 'list', maybe "FieldList"? So Fielddefintion is defining the "FieldList":)

However, if we use 'list class', instead of 'list type', this problem doesn't exist, or at least not so confusing. This is the only benefit I could see of 'list class' over 'list type'.

3).

$list_definition->setType('string_list') would make me think the system does know about items being 'string' already, but it doesn't as the list type is determined based on the item type

Well, I think setDataType() will be rarely used in real cases. But it's been possible since we provide this function, and setClass(). When we must/want to use this setter somewhere, which means we have fully understood all the context, otherwise we don't need to call the setters. For example, I have a custom/contrib field -- "street name". Typically, this "street name" should use "FieldItem:string" as field type, and its "list type" would be "field_item_list". However, I want to extend the 'field_item_list' with some 'street_name_item_list'. This "list type" could map the street name to a region in the map, or something related.

If we use 'list class', I think this calling of setter is still possible, as we've decided to give user more flexibility. For example, we could still call setClass(), or something similar to override the 'list class' of the field type at some point, just like we are overriding the 'type' of list. If overrinding the 'list type' of field type is a problem, then I believe overriding 'list class' of field type will be a problem too.

As long as we need, this flexibility could always be achieved. This flexibility is just what we have discussed as swap the 'class' or 'type' in #145 and #144.

Anyway, if it's really bad to call such setters, we could simply remove this kind of function/flexibility. In a wrod, it's not a specific problem with 'list type' imo.

yched’s picture

- re @fago #145 / #146:

I'm not sure why it would be a bad thing to be able to override the [list] class per field definition though? E.g. you could use it to provide a custom class for the node author reference and add in suiting methods, i.e. I could see it becoming useful at some point.

Maybe - I could see this add confusion/uncertainty as well. So far I've considered "a field type" to be a fixed combination of an item class and a list class. Being able to swap in a different list class while still being considered of the same "field type" scares me a bit. Widgets and formatters work on a given "field type", and they receive an ItemList to work on. If, for a specific field, that ItemList is of a different class than the one they expect in the contract, things could get weird...

list_type as proposed by the patch has another DX problem: $list_definition->setType('string_list') would make me think the system does know about items being 'string' already, but it doesn't as the list type is determined based on the item type - not the other way round

Agreed, this is another issue I have with the current patch: list type should be derived from the item type. Currently the __construct() sequence of FieldDefinition (/ ListDefinition) objects feels backwards: the wrapped item_definition is created without an item type, the item type only (maybe) gets assigned later on by setFieldType() (which should then affect back the type of the parent list definition ? it currently doesn't in the patch).
From the business logic of "Field definitions", TypedData considerations left aside, that feels extremely unnatural - the natural DX would be that field type is a required param of FieldDefinition::create(), along with the field name. A field definition requires a field type and a field name, the rest is optional.

Not sure I fully understand the proposal in #146, but I strongly +1 anything that doesn't force us to expose FieldItemList classes as DataType plugins (or some other plugin). We should absolutely try to preserve the DX of "implementing a field type" simple: provide one @FieldType plugin class, optionally ship a specific companion "List" class, you're done.

- re @smiletrl #148
1. The examples you give (#2023563-66: Convert entity field types to the field_type plugin, #2023563-69: Convert entity field types to the field_type plugin) are unrelated. The current practice is "a field item list class sticks near to its field type class", the two work hand in hand and it's the combination of both that constitutes "the field type". I would really prefer to keep them together in the same folder, and would especially object having one in @FieldType and the other in @DataType discovery folders.

2. I don't fully get you, but in short, see above: people that write code to implement a field type should not have to deal with @DataType. We badly need to untangle Entity/Field API from TypedData API, not bake the coupling even more.

Similarly, and this is my other big worry with the patch, the DX of "writing a field definition in a baseFieldDefinitions() method" should not be dictated by the constraints of TypeData API / DataDefinition. If I understood correctly, the agreed plan is to move away from "FieldDefinition extends DataDefinition", and move to a decorator/adaptor pattern instead. Maybe we can't go there in this one single patch, but we need to design with that objective in mind. Time is runing out for later refactors, and I'm a bit worried of how much things we need to bend towards TypedData here, that we'll need to "unbend" later on (more specific examples in the rest of my review - shortly I hope)

More generally, @smiletrl, I have an extremely hard time reading and understanding your posts here :-/ The issue is complex and now long, and gets easily very confusing. Maybe make an tiny extra effort on clarity / brevity ? ;-) (I myself often refactor my posts several times before posting...)

smiletrl’s picture

Hehe @yched, right, sorry for the confusing:) Also, #149 makes sense all to me:)

fago’s picture

re #141:
Thanks, I've addressed all points in the updated patch below.

ad 7.) We can't as TypedConfigManager still passes on definitions as arrays, but I'm not keen on touching that area here also. I think it's best fixed with #1928868: Typed config incorrectly implements Typed Data interfaces. I added a respective todo though.

ad 8.) Yes, having per-bundle field definition support here allows us to have field instances in here then. I clarified the comment.

If we are going to dismiss "list type", I'm not sure what's the point of a built-in type for list. What does this "type" work for?

In an array-representation of the definition it would tell us that this is a list. Also, each data definition requires a type, so 'list' would be to logical choice to me.

However, if we use 'list class', instead of 'list type', this problem doesn't exist, or at least not so confusing. This is the only benefit I could see of 'list class' over 'list type'.

Yes - this is the main problem we have with list type imo.

However, I want to extend the 'field_item_list' with some 'street_name_item_list'. This "list type" could map the street name to a region in the map, or something related.

Yes, doing that was my intention. But it misses the big DX problem of:

$definition = ListDefinition::create('street_name_item_list');
$defintion->getItemDefinition->getDataType() != 'street_name_item';

You have to do instead:

$definition = ListDefinition::create();
$defintion->getItemDefinition->setDataType('street_name_item');

You could specify 'street_name_item_list' as the list's data type also, but that's just unnecessary repetitive. However, when we stay with 'list_class' we won't have registered 'street_name_item_list' type which kind of lets you think you can just do ListDefinition::create('street_name_item_list').

As there is no strong reason for going with list_type over list_class, I think we should stay with that then.

re yched #149:

Being able to swap in a different list class while still being considered of the same "field type" scares me a bit. Widgets and formatters work on a given "field type", and they receive an ItemList to work on. If, for a specific field, that ItemList is of a different class than the one they expect in the contract, things could get weird...

When swapping out classses/extending classes you always have to take care of not breaking the parents implementation, thus I'm not sure where you see the problem with that. However, if you think we should change that I'd suggest we open a separate issue for it and discuss it there as it's nothing this patch introduces/changes.

From the business logic of "Field definitions", TypedData considerations left aside, that feels extremely unnatural - the natural DX would be that field type is a required param of FieldDefinition::create(), along with the field name. A field definition requires a field type and a field name, the rest is optional.

I agree in general, however the field name is already defined via the array-key and gets assigned to the definition in the processing step of the manager. I'd prefer to avoid duplicating the field-name at the definition.
Having the list item type as parameter to ListDefinition/FieldDefinition create() makes lots of sense to me also. We've been discussinga a better optimized create() methods above with berdir, but I have been hesitant to adding multiple required parameters as it's not really consistent with having setters for the remaining. Moreover we were worried by inheritance violations, but I just figured that should not be a problem with static methods - it should be safe to override parameters as desired here as you'll never end up calling static methods on something you don't know the class of anyway.

Thus, I'd vote for staying with one parameter for DataDefinition and FieldDefinition, and none for ListDefinition, i.e. do

    $fields['name'] = FieldDefinition::create('text')
      ->setLabel(t('Name'));
If I understood correctly, the agreed plan is to move away from "FieldDefinition extends DataDefinition", and move to a decorator/adaptor pattern instead.

Yes, as discussed. I'd love to get the ball rolling on it once we've settled on this one.

Time is runing out for later refactors, and I'm a bit worried of how much things we need to bend towards TypedData here, that we'll need to "unbend" later on (more specific examples in the rest of my review - shortly I hope)

Yeah, I don't think it's a big deal in this case (and there should nothing else be bending it more together). For unbending it should be enough to remove inheritance, copy stuff over and improve docs and write an adapter class or method for translating. But sure, we'll have yet to see how this works out in detail.

---------------

Updated patch attached. It also contains some new fixes for the entity language changes - let's make sure things are working correctly before doing the list_class/list_type updates.

Oh and, a really nice DX improvement I just noted while working on this with Phpstorm. This now has autocompletion support:

$definitions = $entity_manager->getFieldDefinitions($entity_type, $bundle);
foreach ($definitions as $definition) {
  $definition->isFieldT...
}

Status: Needs review » Needs work

The last submitted patch, 151: d8_field_definition_class.patch, failed testing.

fago’s picture

Attached patch moves back to $list_class mechanism and improves definition creation DX as suggested in #151.

I've left FieldItemList in the plugin folder, besides the FieldItem class. When we do that for field items, we should do that generally for data types imo - i.e. always have the list class besides the type class.

fago’s picture

Status: Needs work » Needs review
yched’s picture

Yay. Didn't review #153, but big, big +1 on

  $fields['name'] = FieldDefinition::create('text')
    ->setLabel(t('Name'));

- field name "required in the definition" or "present in the array keys":
Yes, that bugged me as well, no good answer for that, so yes, let's keep the current DX sugar: field name is the array key, setFieldName() gets called automatically based on that. But then we need to make sure this happens before the FieldDefinitions reach the alter step or anything else. No code, other than the one that creates the definition objects, should ever have a chance to hit an "incomplete" FieldDefinition without a field name.

- DataDefinition as decorators:
OK, then only thing that's unclear to me is if you plan this to be done in this patch here, or in a later followup ?
My worry is the name of the methods on FieldDefinition: the ones that pertain to fields, the ones that pertain to typed data... As long as they have to live in the same class, we need to differenciate them with Field or Data prefixes, and this bends the API.

I've left FieldItemList in the plugin folder, besides the FieldItem class. When we do that for field items, we should do that generally for data types imo - i.e. always have the list class besides the type class

I see the logic and I won't fight a fierce fight on this, but still sad panda... FieldItemList is a base class containing buisness logic about the Field system, and works along with Core\Field\FieldItemBase more than with Core\Field\Plugin\DataType\FieldItem, which is just an empty marker class that has always confused the heck out of me ;-). This move is consistent with regard to the TypedData system, but IMO really confusing when you look at things "just trying to understand the Field system".
Well, that's another occurrence of "objects are primarily a TypeData, and secondarily also their own business class" vs "they are just their own business class, and can produce a separate TypeData decorator when needed".

Status: Needs review » Needs work

The last submitted patch, 153: d8_field_definition_class.patch, failed testing.

fago’s picture

OK, then only thing that's unclear to me is if you plan this to be done in this patch here, or in a later followup ?

Clearly, in a separate issue which I plan to tackle asap we've been able to move on with this one: #2002138: Use an adapter for supporting typed data on ContentEntities

But then we need to make sure this happens before the FieldDefinitions reach the alter step or anything else.

true, we've been worried about applying the field_name for fields added during alterations, but 1. you can add/should fields in the initial hook, 2. once we restructure the hook to separate hooks for getting entity type and bundle specific fields the array structure will be flat, i.e. people would really use the primary hook for adding stuff. Consequently, I agree it's a good idea to move it before invoking alterations.

FieldItemList is a base class containing buisness logic about the Field system, and works along with Core\Field\FieldItemBase more than with Core\Field\Plugin\DataType\FieldItem, which is just an empty marker class that has always confused the heck out of me ;-)

True. To me, having both classes side by side was appealing, but having the actual FieldItemBase code not in the FieldItem class makes it less appealing. We could solve that by moving FieldItemBase into the empty FieldItem class also, but I assume having Drupal\Core\Field\Plugin\DataType\FieldItemBase as parent would be bad DX as it lets you think about data types where you shouldn't have to?

fago’s picture

Status: Needs work » Needs review
FileSize
103.55 KB
24.71 KB

I had to restore FieldItem::getDefaultValue() for fixing test fails, however I must say it makes sense to me to have it anyway, as it provides a simple way for field item classes to provide a "default value function". Thus I removed the respective todo. (we might be able to support setting uuid values via computed property also, but that's another story/issue)

Attached patch also
- does the alteration changes as discussed, i.e. it makes sure field names are set first + it makes sure definitions are objects before alteration now also
- updates hook_entity_field_info() docs
- moves back the FieldItemList class - whatever we decide here moving around this class has not be in this issue at all, so let's not postpone it on it.

Updated patch attached.

yched’s picture

fago’s picture

Issue tags: -Needs tests +sprint
fago’s picture

Issue summary: View changes
fago’s picture

Issue summary: View changes

updated summary to reflect recent changes

fago’s picture

Status: Needs review » Needs work

The last submitted patch, 158: d8_field_definition_class.patch, failed testing.

fago’s picture

Fixed conflicts and re-rolled.

fago’s picture

Status: Needs work » Needs review

grml - I'm still not used to the new interface.

fago’s picture

fago’s picture

Reviews anyone?

I'd love to get this in asap so we can move on with follow-up issues and the typed data fixes. Then, I people could help on the following conversion issues during the Drupalcamp Vienna sprints this weeking!

smiletrl’s picture

+  /**
+   * The data definition of a list item.
+   *
+   * @var \Drupal\Core\TypedData\DataDefinitionInterface
+   */
+  protected $itemDefinition;
+
+  /**
+   * Creates a new list definition.
+   *
+   * @param string $item_type
+   *   The data type of the list items; e.g., 'string', 'integer' or 'any'.
+   *
+   * @return \Drupal\Core\TypedData\ListDefinition
+   *   A new List Data Definition object.
+   */
+  public static function create($type) {
+    return new static(array(), DataDefinition::create($type));
+  }

$type loses consistency with @param string $item_type

fago’s picture

Thanks, re-rolled and fixed the $type parameter.

The last submitted patch, 170: d8_field_definition_class.patch, failed testing.

fago’s picture

Interestingly, installation worked for me. Uploading a new patch without changes, but generated with -M to detect moves.

fago’s picture

Status: Needs review » Needs work
fago’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 172: d8_field_definition_class.patch, failed testing.

The last submitted patch, 172: d8_field_definition_class.patch, failed testing.

The last submitted patch, 172: d8_field_definition_class.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
104.11 KB
813 bytes

Could this be the problem?

yched’s picture

Getting silly / nitpicky remarks out of the way for starters :-)

  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -357,22 +360,31 @@ public function getFieldDefinitions($entity_type, $bundle = NULL) {
    +            // Automatically set the field-name for non-configurable fields.
    

    nitpick: s/field-name/field name/

  2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -357,22 +360,31 @@ public function getFieldDefinitions($entity_type, $bundle = NULL) {
    +        // Ensure all untranslatable fields are not defined as translatable.
    

    Nitpick: reads a bit tautologic ;-)
    "Some basic fields cannot be translatable." ?

  3. +++ b/core/lib/Drupal/Core/Field/FieldDefinition.php
    @@ -67,47 +66,65 @@ public function getFieldType() {
       public function setFieldType($type) {
    -    $this->definition['type'] = 'field_item:' . $type;
    +    $this->getItemDefinition()->setDataType('field_item:' . $type);
         return $this;
       }
    

    Do we still want this method ? It shouldn't be possible to create a FieldDefinition without a field type, and it shouldn't be possible to change the field type of an existing FieldDefinition ?

  4. +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
    @@ -9,20 +9,16 @@
    + * An entity field is a list of field items, containing various field item
    + * properties. Note that even single-valued entity fields are represented as
    

    Nitpick: "...a list of field items, each containing a set of properties (depending on the field type)" ?

  5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/LegacyConfigFieldItemList.php
    @@ -102,8 +102,8 @@ public function deleteRevision() {
    +    $type_definition = \Drupal::typedData()->getDefinition($this->getItemDefinition()->getDataType());
    

    We should probably use the FieldTypePluginManager instead.
    (same for the similar code in Field/FieldInstance::getDataType() ?)

  6. +++ b/core/modules/field/field.module
    @@ -194,16 +194,17 @@ function field_entity_field_info($entity_type) {
    +    // @todo: Improve hook_entity_field_info() to allow per-bundle field
    +    // definitions, such that we can pass on field instances as field
    +    // definitions here.
    

    We already have an issue number for this, right ?

  7. +++ b/core/modules/system/entity.api.php
    @@ -627,9 +628,7 @@ function hook_entity_form_display_alter(\Drupal\entity\Entity\EntityFormDisplay
      *   - definitions: An array of field definitions to add all entities of this
    - *     type, keyed by field name. See
    - *     \Drupal\Core\Entity\EntityManagerInterface::getFieldDefinitions() for a list of
    - *     supported keys in field definitions.
    + *     type, keyed by field name.
    

    The phpdoc for hook_entity_field_info() / hook_entity_field_info_alter() should mention FieldDefinition / FieldDefinitionInterface somehow ?

  8. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Entity/Term.php
    @@ -200,67 +202,56 @@ public function postSave(EntityStorageControllerInterface $storage_controller, $
    +
    +    $fields['tid'] = FieldDefinition::create('integer')
    

    Nitpick: extraneous whiteline at the beginning of the method :-)

  9. +++ b/core/tests/Drupal/Tests/Core/Entity/FieldDefinitionTest.php
    @@ -26,6 +26,34 @@ public static function getInfo() {
    +    // @todo: Overhaul this once field definitions to not use the container to
    +    /// get the manager any more.
    

    Looks like there is a typo :-)
    Is there an issue number already ?

The last submitted patch, 178: d8_field_definition_class-177.patch, failed testing.

yched’s picture

More silly things (flushing the content of my dreditor before we're kicked out of the sprint room :-p)

  1. +++ b/core/lib/Drupal/Core/TypedData/TypedDataInterface.php
    @@ -18,7 +18,9 @@
        * Gets the data definition.
        *
        * @return array
    -   *   The data definition array.
    +   *   The data definition represented as array.
    +   *
    +   * @see \Drupal\Core\TypedData\DataDefinition
    

    Obsolete now, this is a DataDefinitionInterface

  2. +++ b/core/modules/content_translation/content_translation.module
    @@ -134,8 +134,7 @@ function content_translation_entity_field_info_alter(&$info, $entity_type) {
    -          $info[$key][$name]['translatable'] = (bool) $translatable;
    -          break;
    +          $info[$key][$name]->setTranslatable((bool) $translatable);
    

    Looks like the removal of the "break" is not intentional ?

yched’s picture

Additional remarks (some discussed with @fago here in Vienna):

- we can remove Field/FieldInstance::setDataType()
- FieldInstance::getDataType() could do return $this->field->getDataType() instead of duplicating the logic.
- same with getItemDefinition() and getClass() ?
- the DataDefinitionInterface / ListDefinitionInterface methods that get added to field.module's Field/FieldInstance config entities make me die inside, but well, that's an expected consequence of FieldDefinitionInterface extends DataDefinitionInterface. This would go away when we decouple TypedData to decorators, but meanwhile, well... I can only try not to weep too loud :-)
At least please let's group them together at the bottom of the classes ?

More specifically:

- the getFoo() + getFieldFoo() methods are an awful WTF :-/ Happy head-scratching when looking at IDE method autocomplete suggestions...
The current plan is to remove the "Field" prefix from FieldDefinitionInterface, and thus have getFoo() methods that implement two interfaces (and hope that the same code will satisfy what each interface expects). We'd really need to make this happen soon after this lands - opened #2143263: Remove "Field" prefix from FieldDefinitionInterface methods.
- getClass(): looks like we could more simply return FieldTypePluginManager()->getDefinition($this->type)['list_class'] ?
- getItemDefinition(): sad panda, but no better suggestion

- Then there's the question of FieldDefinitionInterface specifying no setters, even though hook_entity_field_info_alter() implementations currently call setter methods.
We agreed to discuss that in a separate issue, opened #2143297: setters on FieldDefinitionInterface

fago’s picture

#179,7: As discussed, it should be fine with the reference on the interface.

Created #2143555: Improve how field definitions deal with dependencies and testing

>At least please let's group them together at the bottom of the classes ?
done so

Addressed all other points and re-rolled the patch, see attached interdiff.

fago’s picture

Issue summary: View changes
yched’s picture

Thanks :-)

FieldInstance::getItemDefinition() could still delegate to $this->field->getItemDefinition(), also removing the need for FieldInstance::$itemDefinition ?

fago’s picture

FieldInstance::getItemDefinition() could still delegate to $this->field->getItemDefinition(), also removing the need for FieldInstance::$itemDefinition ?

Sry, I forgot to mention this. No, I don't think we should forward this one as it makes use of the field settings, which will be different for the instance - i.e. so $this->definition->getSettings() reflects the instance settings.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Hmm - true.
Yet another argument why Field implements FieldDefinitionInterface ends up being confusing.

OK, so this is ready for me :-)

fago’s picture

Issue summary: View changes
Dries’s picture

I looked at this patch and it is a step in the right direction, and I plan to commit it to unblock the other issues that depend on it. However, I'd argue that as it stands it doesn't really move the needle in developer experience. I believe there is a lot more we could do.

Example taken from the patch:

+    $fields['weight'] = FieldDefinition::create('integer')
+      ->setLabel(t('Weight'))
+      ->setDescription(t('The weight of this term in relation to other terms.'))
+      ->setFieldSetting('default_value', 0);

Could be simplified to:

+    $fields['weight'] = new IntegerFieldDefinition()
+      ->setLabel(t('Weight'))
+      ->setDescription(t('The weight of this term in relation to other terms.'))
+      ->setDefaultValue(0);

IntegerFieldDefinition would extend BaseFieldDefinition, and setDefaultValue() would be implemented in IntegerFieldDefinition.

In fact, this could be simplified further. 'Label' and 'Description' appear to be required so we could actually write the following.

+    $fields['weight'] = new IntegerFieldDefinition(t('Weight'), t('The weight of this term in relation to other terms.'))
+      ->setDefaultValue(0);

Another example from the patch:

+    $fields['changed'] = FieldDefinition::create('integer')
+      ->setLabel(t('Changed'))
+      ->setDescription(t('The time that the term was last edited.'))
+      ->setPropertyConstraints('value', array('EntityChanged' => array()));

Could become:

+    $fields['changed'] = new EntityChangedFieldDefinition();
+      ->setLabel(t('Changed'))
+      ->setDescription(t('The time that the term was last edited.'));

Note that all the obscure constraint stuff would now abstracted away for people that need to create new entity definitions.

Or even simpler:

+    $fields['changed'] = new EntityChangedFieldDefinition(t('Changed'), t('The time that the term was last edited.'));

Thoughts?

Could we please get a follow-up issue to discuss this type of further refinement/simplification of? Once the follow-up issue is created, then we can can commit this patch to make progress.

fago’s picture

Thanks! I've created the follow-up and quoted your comment there to have a discussion start: #2145115: Improve DX creating field definitions

effulgentsia’s picture

Just a reroll. No changes to actual code.

webchick’s picture

Title: Make use of classes for entity field and data definitions » Change notice: Make use of classes for entity field and data definitions
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Approved API change, +Needs change record

Ok, looks like Dries's feedback was addressed. Also marking this as an approved API change, since other than the follow-up there didn't seem to be an issue doing this. Not sure why it's critical, though.

Committed and pushed to 8.x. Thanks!

Will need a change notice.

Berdir’s picture

Title: Change notice: Make use of classes for entity field and data definitions » Make use of classes for entity field and data definitions
Status: Active » Fixed
Issue tags: -Needs change record

The related change notice is https://drupal.org/node/1806650, which doesn't go into details of what to do in baseFieldDefinitions() but points to https://drupal.org/node/2078241, which I now updated.

fago’s picture

Thanks, yeah I think that should work. I've worked over that docu page as well to make sure it's up2date in general.

Status: Fixed » Closed (fixed)

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

xjm’s picture

Priority: Major » Critical

Restoring original priority.

Berdir’s picture

Issue tags: -sprint