#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
Comment | File | Size | Author |
---|---|---|---|
#191 | d8_field_definition_class-191.patch | 104.63 KB | effulgentsia |
#183 | d8_field_definition_class.interdiff.txt | 13.86 KB | fago |
#183 | d8_field_definition_class.patch | 104.6 KB | fago |
#178 | interdiff.txt | 813 bytes | amateescu |
#178 | d8_field_definition_class-177.patch | 104.11 KB | amateescu |
Comments
Comment #1
fagoHere is a first patch (missing some doc changes) - let's see what the bot says.
Comment #2
fagoComment #4
yched CreditAttribution: yched commented+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.
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 ?)
Should be prevented against "unknown setting" (see #2050113: PHP notice on multiple items image field)
[edit: wait a sec, that's being discussed :-)]
statically cache maybe ?
s/register/registers
[edit: Also, I couldn't figure what this empty class is used for]
Famous last words :-)
That shouldn't be; right ?
(also, missing empty line after last method)
Comment #5
yched CreditAttribution: yched commentedHm, 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) :-)
Comment #6
smiletrl CreditAttribution: smiletrl commentedHmm, 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
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:)
Comment #7
smiletrl CreditAttribution: smiletrl commentedSo 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).
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.
Comment #8
yched CreditAttribution: yched commentedI'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.
Comment #9
smiletrl CreditAttribution: smiletrl commentedRegarding #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
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:)
Comment #10
yched CreditAttribution: yched commentedWell, 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.
Comment #11
smiletrl CreditAttribution: smiletrl commentedOk, so I guess that would mean two(or three) different classes for field defintion. Wish this didn't break
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.Comment #12
yched CreditAttribution: yched commented#1994140: Unify entity field access and Field API access is blocked by this, needs a FieldDefinition object available for base fields.
Comment #13
BerdirNot 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..
Comment #15
fagoIt 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...
True, I thought of using the as part of creating them though, e.g. have
I'd not bother too much about making them immutable as it might make things more difficult afterwards, currently used arrays are not either.
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?
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.
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.
Comment #16
yched CreditAttribution: yched commented[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 ?
Comment #17
BerdirHere's a re-roll with only the FieldDefinition() part. I guess we'd need some tests for this.
Comment #18
tstoecklerSuper nitpicky, but this would be cooler with list() IMO.
I might be completely missing something, but I don't see $this->isComputed() being defined anywhere.
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.
Comment #19
yched CreditAttribution: yched commentedYay! 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()
Nitpick: missing whiteline in between
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 ?
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.
I'm surprised we drop this ? It won't be relevant for code that needs to know "the settings of the field" ?
So, I guess external code is supposed to call createFromOldStyleDefinition() rather than new FieldDefinition() ? (I'm thinking of Field::getFieldDefinition())
Comment #20
BerdirHaven'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.
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedPer #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?
Comment #22
yched CreditAttribution: yched commented@effulgentsia: might make sense, develop against an actual use case and make actual tests turn from red to green...
@Berdir, what do you think ?
Comment #23
BerdirWorks for me, feel free to merge my latest patch into the other issue. Not sure I have time work on that..
Comment #24
fagoworking on this now.
Conclusions from previous discussions (see link)
effulgentsia, yched and me came up with the following DX suggestion for defining base fields:
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.
Comment #25
smiletrl CreditAttribution: smiletrl commentedImpressive 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?
Comment #26
BerdirNo need for a static, this is cached in the entity manager, you're not supposed to call this directly.
Comment #27
fagoDid 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:
Comment #28
fagook, 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.
Comment #29
fagoCreated #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.
Comment #31
fago#28: d8_field_definition_class.patch queued for re-testing.
Comment #33
fagoI 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.
Comment #35
fagoFixed fatal during test run due to a left-over reference on moved class.
Comment #37
fagoComment #39
smiletrl CreditAttribution: smiletrl commentedThis is quite a lot of work!
A quick review:
Maybe add \Drupal\Core\Entity\Field\FieldItemList as alias. Also, can we name different names here, instead of the same FieldItemList?
This line is hard to read, maybe
return ($this instanceof ListDefinitionInterface);
?Mixed names, ListDefinitionInterface.php Vs DataDefinitionInterface:)
I think we need return NULL/array() when $this->list is not set.
Comment #40
smiletrl CreditAttribution: smiletrl commentedThis will reflect all extended classes of TypedData, which all need to update array hint. See hierarchy.
Comment #41
fagoAddressed 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:
It returns NULL when there is no explicit return. Howsoever, this code got just moved.
Yes, in case the class override construct. The patch should already account for that.
Comment #43
smiletrl CreditAttribution: smiletrl commentedLooks like DateTimeComputed::__construct hasn't:)
Comment #44
fagoindeed - 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.
Comment #46
fagoThis 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!
Comment #47
fagoand a patch...
Comment #48
BerdirIt's temporary anyway, but why not just make it createFromOldStyleDefinition($field_name, $definition) ? It's not valid to have one without field name 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?
Naming is a bit strange, Existing doesn't say much, by the time this code runs, both exist :)
i.e. ? Can it also be something else?
Looks like it doesn't see this as a move, would be easier to review if there were any changes.
Why is this change necessary here? :)
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.
Comment #49
fagoBecause, 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.
Good catch, fixed.
Nope, but i.e. should not mean that.
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.
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 :-/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:
To solve the second TODO, should we make $definition add-in defaults for constraints and class by default? E.g. we could do
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
Comment #51
BerdirShouldn't this be addPropertyConstraint() ?
Comment #52
fago#49: d8_field_definition_class.patch queued for re-testing.
Comment #53
fagoI 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?
Comment #55
BerdirOne 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.
Comment #56
fagoIndeed - 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).
Comment #57
fagook, 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:
Comment #59
fagoComment #60
fagook, 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().
Comment #62
fagothe drop is always moving... - rerolled and fixed conflicts
Comment #64
fago#62: d8_field_definition_class.patch queued for re-testing.
Comment #66
BerdirPartial 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 ;)
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 :)
use of the annotation classes is no longer necessary, it just works
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?
Let's switch to @inheritdoc on lines that we touch anyway.
list_type vs. list_class seems to be quite inconsistent now, does that break the installer maybe?
Or should this keep the list_class/list_type mess working?
Ah, so that's why we still need the Config override of getFieldDefinition(). K, fine by me to do this in a follow-up.
Comment #67
yched CreditAttribution: yched commentedHard 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 ?)
Comment #68
fagoad #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:
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.
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?
Comment #69
yched CreditAttribution: yched commentedWorks 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.
Comment #70
fagohere 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.
Comment #72
fagook, 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.
Comment #74
fagoComment #75
smiletrl CreditAttribution: smiletrl commentedThis is a reroll for #2023563: Convert entity field types to the field_type plugin.
Comment #77
smiletrl CreditAttribution: smiletrl commentedDeal with translation stuff.
Comment #78
smiletrl CreditAttribution: smiletrl commentedOpen follow-up #2112239: Convert base field and property definitions
Comment #79
BerdirLong 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.
The new and approved standard is to use @return ..FieldDefinitionInterface[] now, which is understood by PhpStorm :) Then you don't need the @see.
Just moved, but I don't think "ids" is correct.. maybe ID's, or ID (as it's part of ...fields)
The diff is quite hard to read, but the @return needs a description.
This can can be dropped now, the _field version no longer exists. Yay :)
Same here.
bool was correct, not boolean.
same here.
That seems..,. not correct :)
Let's reference the issue that @smiletrl created in the @todo.
Instead of @inheritdoc, should way say that this is a temporary BC layer and refer to the same issue?
Also needs a @return description.
class =, on an Annotation? Isn't that set by the parser? Apparently, because that class no longer exists? :)
Can't we set the default based on configurable, like we did for the list_class?
needs description.
bool
more bool
null? Should be documented when that is used, if it can be..
Same here as in FieldDefinition...
:)
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)
Add another @todo with the issue reference, just so that we remember to update the documentation here?
Can we simplify this too, or does this happen before we convert them to objects? If so, @todo :)
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:
And a lot of basic property definitions would just be like this:
Which I think would be pretty cool :)
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.
And... this can then be removed too!
Comment #80
effulgentsia CreditAttribution: effulgentsia commentedShameless plug for a tangentially related issue: #2112759: Add ContentEntityInterface::hasField()
Comment #81
smiletrl CreditAttribution: smiletrl commented1). 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:)
Comment #83
BerdirThanks.
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.
two . ;)
Here the . is missing :)
Comment #84
amateescu CreditAttribution: amateescu commentedAbout the optional description part, I think it will be useful for when we'll generate table schemas automatically.
Comment #85
BerdirLet's ignore the description part and just convert what we have. Sorry for bringing that up here, unrelated :)
Comment #86
fagoThanks 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
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.
Comment #87
fagounassigning while not actively working on it
Comment #89
smiletrl CreditAttribution: smiletrl commented1).
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).
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?
Comment #90
smiletrl CreditAttribution: smiletrl commentedIt' pointed to #2112239: Convert base field and property definitions, not the ' base field as field_type issue':) See interdiff at #81.
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 athook_entity_field_info_alter()
phase. We need to remove this array support at this hook in #2112239: Convert base field and property definitions.Comment #91
fagolist_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
Now you'd have
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?
Comment #92
fagoFound 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.
Comment #94
fagoWhile fixing merge conflicts field_field_widget_info_alter() got added back in, what resulted in troubles. Fixed that.
Comment #95
smiletrl CreditAttribution: smiletrl commentedA few clean up:
1).
There's no need to set again:)
$field_definition = new FieldDefinition($list_definition);
has already set them.2).
It could be ListDefinition/FieldDefinition too, so it should return DataDefinitionInterface.
Other ideas:
a).
This makes a lot of sense!
b).
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
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'.
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:
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
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
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: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:)?
Comment #96
fagoRegarding #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.
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.
Right, but the example was about the 90% use case of specfiy the data type of the list item, not the list.
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.
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.
Comment #97
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #98
smiletrl CreditAttribution: smiletrl commented@fago, thanks for detailed explanation!
1).
Sorry, I mean
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).
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).
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).
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).
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:
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 becausegetDataType()
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.Comment #100
smiletrl CreditAttribution: smiletrl commentedThis should fix that error.
Also, make ListDefinition's parameter $type optional in create(), while leave it required for DataDefinition. See reason at #98 --5).
Comment #101
smiletrl CreditAttribution: smiletrl commentedreroll for #2050835: Move Widget, Formatter, and FieldType plugin types to the Core\Field system.
Also, clean code a bit.
Comment #102
BerdirTough re-roll after #2115145: Move field type plugins to Plugin/Field/FieldType.
Let's see how much I messed up.
Comment #104
BerdirQuite messed up.
Comment #106
BerdirLost a few changes due to all those file moves.
Comment #107
smiletrl CreditAttribution: smiletrl commentedThanks for reroll.
A few nitpicks:
1).
only
required.
2).
Need two spaces before 'An array..'
3).
This @todo is interesting:) Another getConstraints() comes from FieldItem class. What's the connection between these functions?
It seems
TypedData/FieldItem::getConstraints()
callTypedDataManager::getConstraints()
(From the topparent::getConstraints()
at TypedData::getConstraints()), andTypedDataManager::getConstraints()
call$definition->getConstraints()
in the following code way.Does this doc makes sense? If this function has already returned
instance of \Symfony\Component\Validator\Constraint.
, then why $typedDataManager->getConstraints() do this again?I think we need to change the doc for this part. It should return something $constraints like this
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.Comment #107.0
fagoUpdated issue summary.
Comment #107.1
fagoUpdated issue summary.
Comment #108
fagoThanks 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.
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.
Comment #108.0
fagoadded follow-up
Comment #108.1
fagoUpdated issue summary.
Comment #109
fagoI 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.
Comment #111
smiletrl CreditAttribution: smiletrl commented#108: d8_field_definition_class.patch queued for re-testing.
Comment #113
fagoLooks 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.
Comment #114
twistor CreditAttribution: twistor commentedOther patches have been using "@return self" specifically for method chaining.
Comment #115
smiletrl CreditAttribution: smiletrl commentedMaybe we are ok with this?
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.
Comment #116
Berdir"defined (or even not defined)"
Accessing undefined properties is AFAIK slower and PHP 5.5 AFAIK contains more performance improvements for accessing defined properties.
Comment #117
fagoOh great, let's use that also then!
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?
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.
Comment #118
fagoalso 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.
Comment #119
smiletrl CreditAttribution: smiletrl commentedInteresting. 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:
Test2.txt:
Test3.txt:
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.
Comment #120
smiletrl CreditAttribution: smiletrl commentedA quick note
Sure.
Comment #121
BerdirLooks 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.
"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).
Comment #122
smiletrl CreditAttribution: smiletrl commented1). --> 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?
Comment #123
fago> 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).
Comment #123.0
fagoUpdated issue summary.
Comment #124
fagoHere 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
Comment #124.0
fagoUpdated issue summary.
Comment #125
fagoI forgot to add the code example, so here it is.
Comment #127
fagoI lost the interdiff from #113 in my branch - re-added that and re-rolled it.
Comment #129
smiletrl CreditAttribution: smiletrl commentedLast patch has so many errors, so I reroll from #113 and add the interdiff since then. Hope this won't break so much~
Comment #131
fagoThe 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.
Comment #133
fago#131: d8_field_definition_class.patch queued for re-testing.
Comment #135
fago#131: d8_field_definition_class.patch queued for re-testing.
Comment #136
fago#131: d8_field_definition_class.patch queued for re-testing.
Comment #137
fago#131 is ready for review, anyone?
Comment #137.0
fagoUpdated issue summary.
Comment #138
effulgentsia CreditAttribution: effulgentsia commentedStraight reroll for HEAD changes.
Comment #139
smiletrl CreditAttribution: smiletrl commentedI think we could make this move...
Comment #140
yched CreditAttribution: yched commentedReviewing - sorry, had limited time the past two weeks, and I focused on #2019055: Switch from field-level language fallback to entity-level language fallback
Comment #141
effulgentsia CreditAttribution: effulgentsia commentedPerhaps yched will find more, but here's what I found:
I don't get what "manually" means in this comment.
"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"?
This annotation is still being used in FileItem and ImageItem, so can we keep it defined here until those are removed?
unnecessary line break
Unnecessary use statements?
The base class, TypedData, declares this as DataDefinitionInterface. This class does not yet use the more specific interface, so why declare it?
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?
I don't get what this comment is saying. Is it related to #2114707: Allow per-bundle overrides of field definitions, or something else?
Comment #142
yched CreditAttribution: yched commentedSorry, 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 :
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 ?
Comment #143
smiletrl CreditAttribution: smiletrl commentedOk, 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()
In fact, we use Field as field data definition for configurable field now. So when we need to instantiate field_image here:
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 functionremove 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.Comment #144
smiletrl CreditAttribution: smiletrl commented4. -> 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
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.
Comment #145
fagoThanks for the reviews!
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.
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.
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.
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.
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?
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.
Comment #146
fagoThinking 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.
Comment #147
webchickHm. This does not look RTBC to me.
Comment #148
smiletrl CreditAttribution: smiletrl commented1.
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.
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)
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)
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).
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).
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.
Comment #149
yched CreditAttribution: yched commented- re @fago #145 / #146:
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...
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...)
Comment #150
smiletrl CreditAttribution: smiletrl commentedHehe @yched, right, sorry for the confusing:) Also, #149 makes sense all to me:)
Comment #151
fagore #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.
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.
Yes - this is the main problem we have with list type imo.
Yes, doing that was my intention. But it misses the big DX problem of:
You have to do instead:
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:
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.
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
Yes, as discussed. I'd love to get the ball rolling on it once we've settled on this one.
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:
Comment #153
fagoAttached 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.
Comment #154
fagoComment #155
yched CreditAttribution: yched commentedYay. Didn't review #153, but big, big +1 on
- 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 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".
Comment #157
fagoClearly, 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
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.
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?
Comment #158
fagoI 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.
Comment #159
yched CreditAttribution: yched commentedFYI : #2134967: FieldDefinitionInterface should include a getTargetEntityTypeId()
Comment #160
fagoComment #161
fagoComment #162
fagoupdated summary to reflect recent changes
Comment #163
fago158: d8_field_definition_class.patch queued for re-testing.
Comment #165
fagoFixed conflicts and re-rolled.
Comment #166
fagogrml - I'm still not used to the new interface.
Comment #167
fago165: d8_field_definition_class.patch queued for re-testing.
Comment #168
fagoReviews 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!
Comment #169
smiletrl CreditAttribution: smiletrl commented$type
loses consistency with@param string $item_type
Comment #170
fagoThanks, re-rolled and fixed the $type parameter.
Comment #172
fagoInterestingly, installation worked for me. Uploading a new patch without changes, but generated with -M to detect moves.
Comment #173
fagoComment #174
fagoComment #178
amateescu CreditAttribution: amateescu commentedCould this be the problem?
Comment #179
yched CreditAttribution: yched commentedGetting silly / nitpicky remarks out of the way for starters :-)
nitpick: s/field-name/field name/
Nitpick: reads a bit tautologic ;-)
"Some basic fields cannot be translatable." ?
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 ?
Nitpick: "...a list of field items, each containing a set of properties (depending on the field type)" ?
We should probably use the FieldTypePluginManager instead.
(same for the similar code in Field/FieldInstance::getDataType() ?)
We already have an issue number for this, right ?
The phpdoc for hook_entity_field_info() / hook_entity_field_info_alter() should mention FieldDefinition / FieldDefinitionInterface somehow ?
Nitpick: extraneous whiteline at the beginning of the method :-)
Looks like there is a typo :-)
Is there an issue number already ?
Comment #181
yched CreditAttribution: yched commentedMore silly things (flushing the content of my dreditor before we're kicked out of the sprint room :-p)
Obsolete now, this is a DataDefinitionInterface
Looks like the removal of the "break" is not intentional ?
Comment #182
yched CreditAttribution: yched commentedAdditional 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
Comment #183
fago#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.
Comment #184
fagoComment #185
yched CreditAttribution: yched commentedThanks :-)
FieldInstance::getItemDefinition() could still delegate to $this->field->getItemDefinition(), also removing the need for FieldInstance::$itemDefinition ?
Comment #186
fagoSry, 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.
Comment #187
yched CreditAttribution: yched commentedHmm - true.
Yet another argument why Field implements FieldDefinitionInterface ends up being confusing.
OK, so this is ready for me :-)
Comment #188
fagoComment #189
Dries CreditAttribution: Dries commentedI 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:
Could be simplified to:
IntegerFieldDefinition
would extendBaseFieldDefinition
, andsetDefaultValue()
would be implemented inIntegerFieldDefinition
.In fact, this could be simplified further. 'Label' and 'Description' appear to be required so we could actually write the following.
Another example from the patch:
Could become:
Note that all the obscure constraint stuff would now abstracted away for people that need to create new entity definitions.
Or even simpler:
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.
Comment #190
fagoThanks! I've created the follow-up and quoted your comment there to have a discussion start: #2145115: Improve DX creating field definitions
Comment #191
effulgentsia CreditAttribution: effulgentsia commentedJust a reroll. No changes to actual code.
Comment #192
webchickOk, 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.
Comment #193
BerdirThe 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.
Comment #194
fagoThanks, yeah I think that should work. I've worked over that docu page as well to make sure it's up2date in general.
Comment #196
xjmRestoring original priority.
Comment #197
yched CreditAttribution: yched commentedReported by @chx in #2174509: Drupal\Core\Field\FieldDefinition::getSchema() is broken :
#2175017: FieldDefinition::create() doesn't populate default 'settings' for the field type
Comment #198
Berdir