Problem/Motivation
Right now the TypedDataInterface sits on all the objects and adds confusing methods there, e.g. $entity->getDefinition(), $entity->getValue().
Proposed resolution
Avoid typed data enforcing generic methods on your objects by implementing adapters instead, that translate generic typed data interface to use-case specific methods. E.g. $field->getEntity() could be translated to getRoot().
Patch summary :
- ContentEntityInterface no more extends ComplexDataInterface (i.e is no more a TypedData object)
- A first set of methods are removed from ContentEntityBase:
getDataDefinition()
getValue()
setValue()
getString()
applyDefaultValue()
isEmpty()
getConstraints()
getName()
getRoot()
getPropertyPath()
getParent()
setContext()
- Most of those go into a new TypedData\EntityAdapter class, that extends TypedData (or we just rely on the default implementation in TypedData)
- A second set of methods remain in ContentEntityBase, and are added to ContentEntityInterface directly, in some cases with more specific docs / param names :
get($field_name)
set($field_name, $value, $notify = TRUE)
getFields($include_computed = FALSE)
onChange()
validate()
- EntityInterface::getTypedData() is added to obtain the TypedData\EntityAdapter for an entity
- Adds TypedDataInterface::createInstance() static factory, and a default implementation in TypedData
TypedDataManager::createInstance() now uses that instead of new $class();
- moves TypedData\ReadOnlyException to TypedData\Exception\ReadOnlyException
renames/move TypedData\MissingContextException to TypedData\Exception\MissingDataException
removes DataDefinitionException (was actually not used anywhere)
Remaining tasks
None
User interface changes
None
API changes
Since TypedDataInterface is not on ContentEntityInterface any more, most of its methods got removed from entities, while some got replacements - e.g. getProperties() got replaced by getFields(). For any usage of a removed method, one now has to fetch the typed data adapter object for the entity and invoke the method on it like the following:
$definition = $entity->getDataDefinition();
// becomes
$definition = $entity->getTypedData()->getDataDefinition();
This is for example needed by the FieldItemList class which has to work with the parent typed data object - however those usages are mostly obsolete once/if we'd fix #2227227: FieldTypePluginManager cannot instantiate FieldType plugins, good thing TypedDataManager can instantiate just about anything and improve the field item list's implementation to be field-related primarily.
In detail the API changes are:
- ContentEntityInterface no more extends ComplexDataInterface (i.e is no more a TypedData object)
- A first set of methods are removed from ContentEntityBase:
Removed:
- getDataDefinition()
- getValue()
- setValue()
- getString()
- applyDefaultValue()
- isEmpty()
- getConstraints()
- getName()
- getRoot()
- getPropertyPath()
- getParent()
- setContext()
Re-moved but re-added to ContentEntityInterface methods with a new name or docs are:
- get($field_name)
- set($field_name, $value, $notify = TRUE)
- getFields($include_computed = FALSE)
- onChange()
- validate()
Further TypedData-related smaller API changes are:
- TypedDataInterface::createInstance() is added, however most implementations extend TypedData and do not need to be changed.
- The TypedData exceptions change a bit:
- TypedData\ReadOnlyException -> TypedData\Exception\ReadOnlyException (moves into new sub-namespace Exception)
- TypedData\Exception\MissingDataException gets added
- The unused DataDefinitionException is removed
Related Issues
#2047229: Make use of classes for entity field and data definitions moves typed data definitions from arrays to objects based upon a defined interface
Original report by @fago
Let's remove the typed data interface and rely checking interfaces instead of doing $data->getType(). We'll probably need to separate out a ValidatableInterface also
This depends on doing #2002102: Move TypedData primitive types to interfaces first.
Comment | File | Size | Author |
---|---|---|---|
#137 | d8_typed_adapter.interdiff.txt | 5.26 KB | fago |
#137 | d8_typed_adapter.patch | 71.7 KB | fago |
#134 | interdiff.txt | 4.29 KB | yched |
#134 | 2002138-typed_data_adapters-134.patch | 72.16 KB | yched |
#132 | interdiff.txt | 2.31 KB | yched |
Comments
Comment #1
fagotagging
Comment #2
BerdirPromoting to major as discussed with @xjm.
Comment #3
dixon_Should we really remove
TypedDataInterface
completely? I agree that we should break out things intoValidatableInterface
and maybe something likeParentAwareInterface
and start checking more on interfaces. But doesn't it make sense to have a common interface for typed data?Comment #4
dixon_Ok, I've done some more reading on this, and I see that it probably makes sense to remove
TypedDataInterface
completely.I've started breaking up
TypedDataInterface
intoValidatableInterface
andParentAwareInterface
. The interface is obviosuly not removed yet, but I'm attaching it here to get some feedback on wether is is the right approach or not.I'm not sure how/if we should remove the class
TypedData
, or if we should keep it as a base class extending the interfaces we break out fromTypedDataInterface
.The patch is based on #2002102-45: Move TypedData primitive types to interfaces, so the tests will break unless that patch is applied first.
Comment #5
fagoYep, I still think we want to remove typed data interface if possible, in particular from EntityInterface.. ad ParentAwareInterface - we had ContextAwareInterface for that thing earlier, is that a better name?
We'll probably have to fix getPropertyInstance() in the typed data manager though, as it relies on being able to get the $type from the $root object. Instead, I think it's fine to going with it's class - as this maps to the type now and is used for a static-cache-key only anyway.
Comment #6
dixon_I went with
ParentAwareInterface
since that describes more explicitly what it's mean for, but also to avoid confusion withContextAwarePlugin*
classes (although those are of course another namespace). I'm fine with either really.Comment #7
Berdir+1 to ContextAware, as there's more than just the parent, things like root and name.
Comment #8
fagoAs it's also about the name and has setContext() I think ContextAware would be better - I guess namespaces should be enough to differentiate as we do that quite frequently now (check the "field" class :D).
Comment #9
dixon_I have a patch coming up soon.
Comment #10
dixon_TypedDataInterface
is almost gone. What's still to figure out is what to do with (or where to move)TypedDataInterface::getDefinition()
andTypedDataInterface::applyDefaultValue()
.Some doxygen documentation also needs to be revisited, e.g.
Drupal\Core\TypedData\Annotation\DataType
.With this change,
TypedData
has now become an even more abstract base class that data types can extend to save some code. But to complete the implementation of a data type you have to implement one of the three typed data interfaces;PrimitiveInterface
,ComplexDataInterface
orListInterface
.There's a @todo in
TypedDataManager::getPropertyInstance()
that will be removed/simplified once #1868004: Improve the TypedData API usage of EntityNG lands.Comment #11
dixon_I also had to make
Drupal\Core\TypedData\Plugin\DataType\Any
implementPrimitiveInterface
, because all data types needs to be an implementation of one of the three typed data interfaces (as mentioned above) to not lose track of things.Comment #13
dixon_Hmm, that seems a bit random... Giving it another try.
Comment #14
BerdirThat looks like you have fatal errors on those page callbacks.
Comment #15
fagohm, any as primitive data type does not make much sense to me. Why do we need to be it some of those? Any is more useful on a metadata basis so you can specify that something can be anything, but then we do not care whether it implements some of those interfaces as well it can be anything. So why care?
getDefinition() I think should just go - do we use it somewhere except the already mentioned gePropertyInstance() ? Or maybe better, I think it would fit into ContextAwareInterface, i.e. something context aware can know about itself as well ;-)
applyDefaultValue() probably would need it's own interface? Not sure what a good name would be here, DefaultableInterface ?
Comment #16
dixon_@Berdir Thanks, I'll look into that!
@fago
The reason why I extended
PrimitiveBase
was becauseAny
wouldn't have anyget/setValue()
otherwise since those are removed fromTypedData
that it previously extended. So why doesn't theTypedData
base class haveget/setValue()
? Because sinceTypedDataInterface
is going away theTypedData
base class turned into an even more abstract class that only implemented things that most typed data objects would need (e.g. validation, context aware, defaultable etc.) without making any assumtions about the data itself and how it's fetched. For example; it doesn't make sense to haveget/setValue()
for complex data that instead hasget/set($property_name)
.And although we have #2028097: Map data types to interfaces to make typed data discoverable, I really think that all unique interfaces should extend from a few common interfaces that we can make generic checks or generic type hinting on. My suggestion is that those generic interfaces should be:
PrimitiveInterface
,ComplexDataInterface
andListInterface
.When I think about "primitive" I'm thinking about a data object with a holistic/stand-alone value without much meta data. So if you look at any php variable holisticly, they're primitive ;-) But maybe that's the wrong way to look at it... :-)
But, if we don't see a use case for a few common interfaces I'd be happy to make a completely unique interface for
Any
.Ok, I'll see if I can remove it completely.
Makes sense.
Comment #17
fagoI'd agree with that.
For "any" I don't see the use-case as it just tells me it may be anything, so it could be a primitive or a list or complex data, I don't know. Likewise I don't think an interface is needed here. I guess the class has no much use anyway, it's more the data type that's useful in data definitions to refer to anything.
Also, from wikipedia the definition of primitive does not sound as being a match for "any":
Comment #18
dixon_TypedDataInterface
still not completely gone, but I found more instances ofgetType()
so I'll first see if I can get this to pass...Comment #19
BerdirThis is an overlap with the action config entity, there ->type()/getType() is a thing
Comment #21
dixon_Now I've got up to the point where only
getDefinition()
is left inTypedDataInterface
, but all tests should pass so far. So it's close! :)I've done the patch on-top of #1868004-81: Improve the TypedData API usage of EntityNG, so the attached patch includes that patch as well. Reason being that there are many overlaps between these two issues and re-rolling after that goes in would have been unecessary work. However, the attached interdiff is only what this issue is changing, relative to that.
Here are some random notes:
TypedDataInterface
to their own interfaces, e.g.ValidatableInterface
,ContextAwareInterface
andStringableInterface
.Any
type and added a simple interface for it, since its not implementing any of the three main interfaces anymore. The helper method in typed data manager still recognizes it as valid typed data so it can be used to mock with, etc. SoAnyInterface
is almost like our fourth valid "main" typed data interface, which I think is ok.AnyInterface
is not stringable anymore. I don't think that makese sense, because the value can be anything (i.e. complex) that is impossible to string. And the intent is forAnyInterface
to be very dumb, so I removed that (and the previous tests for it)Map
type, it implementsComplexDataInterface
and should not be stringable, imo. Partly because the properties of a map can be anything (i.e. complex), so I removed that as well.Map
type implementation needs some more clean-up, because it currently hasget/setValue()
which a complex data type should not have. We haveget/setPropertyValues()
in theComplexDataInterface
for that. Either we should open up a new issue, or do this clean-up in: #1928938: Improve typed data definitions of listsget/setValue()
to the config schema types for backward-compatibility and not touched that further. The implementation for config schemas needs some serious love, but that's another issue: #1928868: Typed config incorrectly implements Typed Data interfacesThat's it for now. Now I need to figure out how to remove
getDefinition()
and finally killTypedDataInterface
.Comment #22
jibran#21: 2002138-remove-typeddatainterface-test4.patch queued for re-testing.
Comment #24
BerdirNot sure if all of this is still possible, despite it being a major task. Also, it doesn't yet tackle some of the reasons why this issue exists.. e.g., there's still an Entity::getType() :)
Trying to start doing this in separate steps, here's step #1: #2056721: Remove TypedDataInterface::getType()
Comment #25
webchickI think the overall sanity that this brings to the API is important enough to justify the BC break here. However, I would appreciate a second thumbs-up from another core maintainer, so tentatively assigning to alexpott.
Incidentally, this patch is already mighty big enough as it is; should we move the getDefinitions() stuff to a separate issue(s)?
Comment #26
webchickOh, effulgentsia pointed out #2002134: Move TypedData metadata introspection from data objects to definition objects which is where that probably makes more sense to tackle.
Comment #27
yched CreditAttribution: yched commentedProbably related: #2061331: Add getEntity() helper in FieldInterface / FieldItemInterface
The Field::getEntity() method added here would just duplicate Field::getParent()
Comment #28
alexpottYep this is change brings alot of sense to an Entity's API. TypedDataInterface should not pollute it.
Comment #29
BerdirRe-roll.
Still not quite sure what to do with this, need to think about it.
Comment #30
BerdirPatch would help.
Comment #32
BerdirOk, this should fix the test fail, but...
This summarizes what my problem with this is :)
All those things have something in common. They're typed data. So what's wrong with TypedDataInterface?
Yes, we should remove all those methods that are in the way for most typed data classes. But we obviously have use case where we need to check if an object is typed data. Then let's do that using an interface?
I'm not yet sure if getDefinition() should be renamed/moved/removed, but there are valid use cases for what it does, see #2056721-45: Remove TypedDataInterface::getType() for one example.
Comment #33
BerdirComment #34
smiletrl CreditAttribution: smiletrl commentedinherit from who?:) TypedDataInterface::getName() has been deleted in this patch. And some other similar methods.
Comment #35
smiletrl CreditAttribution: smiletrl commentedNot sure shout this
getDefinition()
., but maybeconvert to something like:
Comment #36
msonnabaum CreditAttribution: msonnabaum commentedcore/lib/Drupal/Core/TypedData/TypedDataManager.php:
core/lib/Drupal/Core/TypedData/Validation/Metadata.php:
This worries me.
If we still have code that has to check if something is typed data, then there's no point in removing the interface. This code is a regression from what we had. It's a bit hard to grok why isTypedData is still needed, but if it is and we can't get away from it, a TypedDataInterface with no methods would be a better option. That said, I wonder if we can't have a more polymorphic way of handling this.
Comment #37
jibran#33: 2002138-remove-typeddatainterface-32.patch queued for re-testing.
Comment #39
BerdirThe more I look at this the less sense it makes. I'm getting close to none at all :)
This patch does many different things and touches various different places, and it's really hard to grasp and review/work on this. It does a lot of ugly things to forcefully remove the interface (like simply removing it as a type int, leaving us with non-type hinted object arguments, crazy methods like isTypedData() to re-introduce a way to identify typed data) but we don't actually gain much in the end. ComplexData is still ContextAware data, Entities are still ComplexData and therefore ContextAware data, which means they still have stupid methods like getRoot(), getName(), setContext() that they do not and can not implement in a useful way.
There would IMHO only be one way to remove TypedDataInterface. And that would be to remove the *concept* of typed data (TypedDataManager, the plugin type, interfaces, base classes, ...) and have entities, fields/fieldlists, primitives and references and so on that are all completely their own thing, in a fixed structure/hierarchy. That might have advantages for the known structure of entity -> field -> primitives/references, but there's also stuff that I don't enough about, like context stuff, config schema and future uses of typed data, like integrating it with the token system). I don't think that is possible at this point.
My suggestion would be to identify smaller, actionable points and deal with them in separate issues.
* I don't know what exactly that would be, but I think it should be possible to deal with getDefinition(), While not the interface, but the TypedData base class implements PluginInspectionInterface, which defines getPluginDefinition() and I have no clue what the difference between the two really is. and how one could possibly return something different from the other.
* Other methods like getString(), getName(), the context/parent stuff.
* validate(), which could also be useful to have outside of typed data? Also, looking at the patch context some config schema classes, they do seeem to implement validate() in a completely different way. Not sure if it's used like that or just incorrectly implemented and unused. Should possibly be used to more specific interfaces too.
I'd love to get some more feedback from @yched and @effulgentsia, who worked extensively with/around typed data stuff in the field context and from context/config schema people.
Comment #40
BerdirTagging for rocketship focus
Comment #41
yched CreditAttribution: yched commentedI can't say I have an overall vision of TypedData API, its scope and current uses, the generic features it provides (validation, context...)
so unfortunately I can't provide a fully informed opinion... But:
I would be very very +1 on that. The current situation where everything is, first and foremost, a piece of "typed data", and then also "something more specific" (an entity, a field within an entity, a property within a field...), leads to problematic limitations and absurd situations.
Currently all field types need to be exposed as FieldItem classes that are @FieldType plugins. But the Field/FieldItem objects themselves are only ever instantiated as TypedData objects, through the TypedData plugin manager. So @FieldType plugins need to exist as @TypedData plugins as well, this is done by using a plugin derivative that automatically adds all @FieldType plugins as @TypedData plugins.
Result:
- the plugin definitions are duplicated in two separate plugin managers, both with their own separate persistent + static cache. Memory clutter, cache invalidation fun.
- FieldItem classes are exposed as @FieldType plugins with some plugin id, but are always instantiated as a different plugin type, with a different id. This is a confusing WTF for things like getPluginId() - "no, at runtime, the plugin id won't be the one defined in the annotation at the top of the file - and the pluginDefinition will be slightly different too".
- Meanwhile, the list of @TypedData plugin definitions is an unstructured soup of things of very different nature and interfaces. Since real life use cases often need to tell them apart (Field UI needs only "the list of field types"...), plugin definitions need to add custom metadata entries for "I'm a 'field' kind of typed data" or "I'm a 'property' kind of typed data".
There are several cases where we find Field / FieldItem classes have special needs though:
- The $definition that gets injected should be a FieldDefinitionInterface object, not an array
-
Field / FieldItem need to be aware of their own language, and although I didn't really investigate how to pass that information yet, I kind of fear the typed data factory will make that a pain - might be FUD, not sure yet.[edit: patch for this at #2078625: Field/FieldItem value objects should carry their language - the constraints there mainly come from the "prototypes" optimization trick in TypedDataManager, and we'd probably want to keep that anyway, so no actual impact here]
On a high-level, no-implementation-details-so-talking-out-of-my-*ss architectural point of view, it would seem the generic features provided by TypedData would be better off split in different interfaces, that the different object types (entity, field, properties) would implement. The difficulty would probably be providing base implementations in base classes that can be inherited... Another place where we'd need traits...
Comment #42
twistor CreditAttribution: twistor commentedHaving spent quite a bit of time with TypedData lately I thought I would share my experiences as someone who's trying to consume the API.
This is sort of related to yched's comment above, but trying to introspect an entity using the appropriate plugin managers is a nightmare. The only sane way I could find to do it was to create an empty entity, and then iterate. I could never figure out how to get from a Field definition to its FieldItem definition. Nobody cares about the Field. It's a list, why is it impermeable? (I'm sure I'm screwing something up.) Trying to figure out the plugin managers is a PITA. I'm pretty sure I saw an issue that wants to remove the the metadata from the instances, which will kill my current approach. The thing is, that I understand the idea well enough, it's just very difficult to grok the implementation. Entity -> Field -> FieldItem -> Primitive seems simple enough (I'm sure that's not exactly right). But Entity, Field, and FieldItem all have ancestors to basic types(TypedData) on their own. What I'm thinking, or hoping, this issue is getting at is that there's not a real reason for each of the major types to descend all the way down. In practice, you're very much interested in knowing the difference between the types, more so than the similarities.
Part of me wants to like it, I remember the type system in entity api for 7. That was very easy to grok. It wasn't that much different, but there was a single point of discovery. An actual hierarchy rather than a net.
Comment #43
smiletrl CreditAttribution: smiletrl commentedYeah, I agree it's a good idea to split the whole typed data concept.
Not only "field type", but also 'entity type' are derivative typed data for TypedDataManager. But the two "types" do have their own Managers, i.e., FieldTypePluginManager and EntityManager. Personnaly speaking, TypedDataManager looks wierd to take any responsibilities here.
Hmm, "typed data" Entity is instantiated via its own storageController I think, i.e.,
Drupal::entityManager()->getStorageController($entity_type)->create($values)
. This is a special case, while other 'typed data' are instantiated via TypedDataManager.Totally agree:)
Maybe move validating/constraint method, like 'getValidationConstraintManager' of TypedDataManager into some base "data plugin manager" for generic data managers(TypedDataManager, FieldTypePluginManager, EntityManager, and maybe other "data plugin managers") to inherit. TypedDataManger remains to only takes care of premitive data/ list Data, perhaps.
And Method 'getPropertyInstance' of TypedDataManger have different context to deal with when it comes to specific Type's property. If this method is spilt into different manager, I guess it will make code easy to read too:)
Comment #44
yched CreditAttribution: yched commentedEdited my #41 above - the point about "Field / FieldItem need to be aware of their own language" cannot be counted against TypedDataManager's factory.
Comment #45
fagoExcept from the validation stuff (which we could probably deal with) I think the main reason we need something like typed data is that we keep a place for registering "data types" to which we can refer in data definitions. So we can make use of them when we describe condition parameters or block contexts.
True - there are quite some confusing points. So maybe, we can improve that situation with the folllowing:
Yeah, I had that idea in mind as well. Could we inherit from FieldTypeManager from the TypedDataManager + make adjustments by overrides as necessary? That way, field items could be created via the field type manager? However then, what if someone tries to create a field type via the typed manager plugin manager? I guess we could re-route that to the appropriate manager via the derivative class.
That would help improving situation in regard to instantiation, but not really in regard to plugin identity. For that I think it would make most sense for that to be the highest-level plugin, i.e. the field type (or entity type). Once we've instantiation run through the field type manager this should be doable.
Then, I think the remaining PITA is:
We could solve this PITA we need to fix up this generic wording by making use of use-case specific wordings, i.e. replace getRoot() with getEntity() ... Again, this should be doable when we have per-data-type instantiation overrides. I don't think the generic ones are so problematic here, I think it's more ComplexDataInterface.
ComplexDataI is the harder one though. First off it's used by the validator to navigate through the tree of data. We could solved that by writing custom metadata adaptors for each use-case, but we need be able to generically drill down the tree in other situations also, e.g. for deriving token values or applying data selector of any block context UI system or Rules. So could we
- leave ComplexDataI in place for checking its capabilities, but strip it down and remove generically named stuff (e.g. everything imposing "property" terminology)
- have simple adapters, e.g. let derivative classes translate from generic typed data terminology to use-case specific terminology
Comment #46
fagoWe discussed this and #2002134: Move TypedData metadata introspection from data objects to definition objects during a hard-problems after-meeting in Prague. The whole notes can be found here, I just re-post the conclusions here:
Furthermore related, the conversion to class-based field definitions over at #2047229: Make use of classes for entity field and data definitions moves data definitions to be classed based as well. This opens the questions whether we should support an adapter approach here as well.
For discovering typed data objects, we discussed having a helper method on the object that looks for known interfaces or classes (related: #2028097: Map data types to interfaces to make typed data discoverable). Alternatively, we could keep an empty TypedDataInterface for discovery purposes, but we should not require that as it makes it more difficult to expose e.g. php objects of other libraries as typed data.
Comment #47
fagoSo, based on #46 here is a suggested plan of attack:
1. Start moving to adapters for ComplexDataInterface first, i.e. #2002134: Move TypedData metadata introspection from data objects to definition objects.
2. Start with #2028097: Map data types to interfaces to make typed data discoverable and add support for discovering typed data objects based on interfaces/classes.
3. Support adapters for TypedDataInterface + ListInterfaces also
As following, the TypedDataInterface probably won't go away - but e.g. move from your objects to adapters - I'm retitling this issue and updated the issue summary accordingly.
I'll start working on this as soon as the necessary ground-work for #2047229: Make use of classes for entity field and data definitions has been put in place.
Comment #47.0
fagoupdated issue summary
Comment #48
fagoLet's move on with this, once we've decoupled the metadata related methods from our objects #2002134: Move TypedData metadata introspection from data objects to definition objects.
Comment #49
effulgentsia CreditAttribution: effulgentsia commentedI think this will involve an API change to Entity Field API that will affect quite a few modules, so adding the "beta target" tag. Maybe I'm wrong though?
Comment #50
fagoIt's not necessarily affecting them, but given that we want to do this in order to remove methods from our primary data objects that don't suit there - yes those methods would go away.
So I think we need at least go though the list of methods and mark any methods that we want to go away later as deprecated before beta. But of course, it would be better to get this done before beta so I'm agreeing with at least a 'beta target'.
Comment #51
fagoBased on the recent improvements done over at #2002134: Move TypedData metadata introspection from data objects to definition objects I've given this issue quite a bit of thought recently.
I figured, typed data can already work with the suggested "adapter approach", where a class bridges between the typed-data view of the world and the domain-specific objects. The wrapping typed data objects act already as that "adapter class" and translate between the general and domain-specific APIs. However, we do not make use of this adapter approach for entities and fields right now, but decided to natively implement the typed data interfaces here what leads to the perceived problems.
So, let's re-iterate on what the problems are:
Let's see how a solution could look like:
S1: Use wrapping typed data objects for implementing typed data for entities.
For 1. and 2. the natural solution would be to start doing a separate typed data entity object, which just wraps the real entity object and so acts as "adapter" between the generic typed data interfaces and the entity interface. This frees us from having typed data interfaces and methods on the entity object and solves typed data issues around the 'entity:*' data types.
#1868004: Improve the TypedData API usage of EntityNG completed typed-data integration for entities by doing it analogously to fields - by making entities being typed data itself. A good reason for doing that was also to fulfill the typed data API of fields and field items, i.e. their implementation of getParent() has to return the parent typed data object. By having the entity object being typed data we avoided having a confusing getParent() implementation on fields. What leads us to the problem of having confusing methods on our domain objects.
From #46:
I think this conclusion is fine, as it makes sense to build upon the existing API to implement field item properties, field items and field item lists instead of re-inventing the wheel. Doing otherwise would require us to duplicate a lots of the logic/code for fields now. But what exactly are the problems related to terminology that exist here?
- getName() is not necessarily the field name.
- Given S1, getParent() is not necessarily what one would expect
- getDefinition() on field items is confusing as one does not get the field definition, but the field item definition
- ..
So it boils down on typed data method names being too generically named, so that they are confused with something else in the domain context. Therefore, in order to make natively implementing typed data interfaces (as fields do) feasible I'd suggest doing:
S2: Name typed data interface methods unambiguously
For example:
TypedDataInterface
Yes, that's in particular important for field items being able to act as field item primarily, and as typed data secondly. For keeping a way to instantiate the typed data objects we'll need to do:
S3: Make typed data object instantiation more flexible
Right now, the typed data manager expects a certain typed-data object constructor to be on the data type class. For allowing objects like fields to implement their own constructor and for allowing DI I'd suggest to move to an instantiation method similar to the ContainerFactory, but by going with a method that is specifically named related to typed data - e.g. public static createTypedData($container, $definition, $name, $parent). That way we can use the same constructor as previously by default, while we implement custom construction via the field type manager for field item lists and field items.
So what would be downsides of doing S1-S3? One issue I can of think of right now is that
$violation->getRoot()
of entity validation violation would point to the wrapping typed data object, instead of poing to the entity itself. This relates to us/typed-data not knowing on whether an object implements the typed data API natively (as fields) or in the wrapping manner (as primitives, or with S1 entities). We could solve that by e.g. adding a flag to the data type annotation that tells us that, e.g. "wrapper: true|false". This would help us on other places where we have the same requirement (know in which form to pass the data on best) as well.Then, there is the question on whether we should implement an adapter approach for field definitions. Right now, field definition objects implement the data definition interface, and are so native typed data definitions. The API as proposed at #2002134: Move TypedData metadata introspection from data objects to definition objects would allow us already to separate field definitions from the typed data definitions by implementing it as a separate FieldItemListDefinition class that wraps a regular field definition object to implement the typed data interface. However doing so does not require this issue, so I think it should be treated separately. It probably makes sense to do S3 + the instantiation changes for fields first though.
Generally speaking, I think S1, S2 and S3 can be implemented independently, thus could receive their own issues. We might want to do S2 before S1 though to ensure we do not run into confusing issues with $field->getParent() meanwhile.
I think S1-S3 would solve our problems and present a good way forward that's doable, as it works without re-writing the whole entity field implementation (what won't be feasible that late in the cycle anyway). Thoughts?
Comment #52
yched CreditAttribution: yched commentedThe list of "problems" in #51 only deals with value objects; But the same drawbacks are duplicated for "definition" objects. So I'd add:
4. the "definition" objects are primarily TypedData definition objects, instead of "just" being domain definition objects. That greatly complexifies FieldDefinition / Field / FieldInstance since:
- they have to extend DataDefinition, some methods of which make little sense for the domain
- they cannot be a self-contained collection of domain properties (field type, name, cardinality, settings...), but need to be split between a "definition" and an internal "item definition".
[edit: sorry, I posted that before getting to the end of #51. Basically you're saying :
- we could move to a FieldItemListDataDefinition + FieldItemDataDefinition, generated from the (domain) FieldDefinition object
- this would be made easier by #2002134: Move TypedData metadata introspection from data objects to definition objects
- this can be done independently of what is discussed above about data objects
Did I get that right ?]
Comment #53
BerdirOk, trying to get this started, here's an initial patch that removes the ComplexDataInterface from ContentEntityInterface, which is the first and possibly easier part of S1, if I understood that right.
Added back whatever methods seemed useful directly to ContentEntityInterface, renamed property to field. That part looks a lot saner now I think :)
Everything else is obviously still very hackish, I extend the existing abstract typed data entity plugin class to implement ComplexDataInterface and wrap a content entity, not everything in there is implemented yet. Also a bunch of hacks at random places, but EntityFieldTest is passing with those changes now. There will obviously be lots of failures but that's ok ;)
I'm also fine with doing S2 first, but I wanted to see how the content entity API will look like when doing this and starting to understand the problems. For example, EntityFieldTest already nicely shows some of the drawbacks that @fago pointed out, like getRoot() returning the wrapper object and not the actual entity.
I also think that part of the problem of the definition objects was already addressed in the other issue, it's getDataDefinition() now I think and no longer getDefinition. No idea about the current state of the definition objects, though.
Comment #54
BerdirComment #56
Berdir53: typed-data-wrappers-53.patch queued for re-testing.
Comment #58
BerdirFixed a few more things, maybe this will be able to install?
Comment #60
BerdirFixing unit tests to get some real results. The crazy stuff that's going on in some unit tests is.. crazy ;)
Also removed a weird and wrong check in EntityNormalizer, there's no reason to ignore id fields? o.0
Comment #62
BerdirWow, hal entity serialization is fully of crazy stuff and hacks.
Found out why it had the !is_object() check in there... EntityReferenceItemNormalizer also passed in config entities, which only worked because it also used the included_fields option to only return the uuid. That then called something like $node_type->get('uuid'), which returned the UUID as a string, which then was filtered out again. So that somehow worked but resulted in incomplete definitions without UUID's for config entities I think.
For now, made that normalizer only apply to content entity references. Should probably be renamed as well, just like I renamed EntityNormalizer? There's an issue to add config entity serialization support, but we don't support them as they don't have validate() for example anyway...
Comment #64
yched CreditAttribution: yched commentedStill cannot look at code, but saw #2216569: Move Entity/TypedData normalization inline pass in my twitter feed. Might want to synchronize ?
Comment #65
tim.plunkettAccording to this patch, setFieldValues() is only called in core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.php
In #2216569: Move Entity/TypedData normalization inline we discussed having EntityInterface::toArray(), which would basically be the same as getFieldValues().
If we rename getFieldValues here, we can rename ConfigEntityInterface::getExportProperties in another issue, and we'll finally have a unified way to get the values of an entity as an array!
Comment #66
BerdirGrml, spent way too much time debugging the Symfony Serializer because their normalizer cache is inconsistently used, so I incorrectly assumed my change could work. Not sure why there even are supports*() methods when the results is cached by class anyway, it would be much easier if it would just have methods to get the class and supported formats and check that.
Opened up https://github.com/symfony/symfony/pull/10457 to at least unify the use of the normalizer cache there.
Need a break now, will look into renaming the method tomorrow.
Comment #67
BerdirComment #69
BerdirOk, hopefully fixed the remaining tests.
Removed setFieldValues() and renamed getFieldValues() to toArray().
Need some feedback on how to continue here... The patch proves that the adapter approach works, but does so by manually wrapping it every time it's necessary. should we have $entity->getDataWrapper() ? Should we maybe have an interface for that ?
To understand the serializer changes, see the attached patch between the output of entity/node/1 before and after the patch.
- Before, a broken node_type was in there, with an empty href and no uuid
- nid is now no longer in there, because we now check for the actual entity ID not just a field named "id".
- Looking at that, having the revision id in there is just as problematic, but it's not our problem to solve.
Related to config entity references: I think we should support to reference those by ID and provide a target id entity resolver. See what I did for to support that in the default_content module in https://github.com/md-systems/default_content/commit/e180dda7945be1b99a6....
Comment #70
klausiThat rename is excellent, I very much like toArray() as it is immediately clear what it does.
@param daty type is missing, also elsewhere.
The class is never used, but it still contains methods? What's the point then? Please clarify.
This reminds me of entity metadata wrappers in D7. Is that really what we want again? I guess I'm not up to speed with recent meta data improvements in core, but introducing wrappers again feels wrong. I assume that most entity API use cases now can be done very well without metadata inspection on the typed data level? Then special casing the typed data use cases with this wrapper might be the right thing.
The changes to the HAL serializer look fine, the original code is quite old and therefore might look weird.
Comment #71
Berdir3. & 4. The documentation and class name is bogus, I just did something to get started. It should very likely be called ...Adapter. Entity API introspection is equally capable as Typed Data, this just provides an Adapter for different API's/method names, which allows us to move methods on entities that make sense (getFields() instead of getProperties() and so on.
getProperties() -> toArray() is something that would make sense for ComplexDataInterface as well I think...
Comment #72
tim.plunkettPostponed #2216569: Move Entity/TypedData normalization inline on this. Loving the toArray() name change!
Comment #73
BerdirMoved the hal cleanup to #2219795: Config entity references broken and other bugs, added tests and found even more bugs. Please review and help to get that in to keep the size of this down...
Comment #74
tim.plunkettFiled #2219925: Rename ConfigEntityInterface::getExportProperties() to toArray(), whichever one of these gets in second should move toArray to EntityInterface
Comment #75
tim.plunkettThat issue went in, and @Berdir opened #2223361: Rename ComplexDataInterface::getPropertyValues() to toArray(), remove setPropertyValues()
Comment #76
BerdirRe-rolled the patch now that the two issues went in.
Comment #78
BerdirEvery time I'm too lazy to even to a basic check of the patch, this happens.
Comment #80
BerdirFixing the unit tests.
Comment #81
BerdirFeedback plz!
Comment #82
msonnabaum CreditAttribution: msonnabaum commentedReroll to handle the FieldableEntityStorageControllerBase -> ContentEntityStorageBase rename.
Comment #83
effulgentsia CreditAttribution: effulgentsia commentedIt's hard to tell from the patch which of these wrappings are actually needed vs. temporary artifacts. For example:
Is this only needed because the patch doesn't yet remove TypedData stuff from FieldItemList?
Why is this needed?
Comment #84
effulgentsia CreditAttribution: effulgentsia commentedIn my opinion, no, at least not a public one. A protected
toTypedData()
would be fine, if ContentEntityBase needs it as its implementation detail for how it chooses to implement validate(), but the whole point of adapters is that it's not up to the adaptee's interface to have any knowledge of them. Instead, what about adding anadapt(mixed $value)
method to TypedDataManager, and then have a tagged service for each possible type of $value, with TypedDataManager finding the appropriate one to call (or calling all of them until the first one returns non-NULL), similar to how we implement normalizers and cache contexts.Comment #85
fagoShould we start wrapping this logic in a place that makes sense already, e.g. on the field type manager?
I think we should not hardcode the EntityTypedDataWrapper class, but use the 'entity' data type classes registered in the typed data manager instead.
So you use the typed data manager to instantiate a typed data wrapper of an entity given its typed data type and $entity.
It would be good to describe what kind of $values are supported here.
It sucks to have to instantiate the parent wrapper just for being able to have a reference on it :/
We should be able to improve this when constructing fields via the field manager though. I could see us doing $data_type_class::createFromDataDefinition() for defining how you can construct your class via typed data, which then can re-route to the usual, data type dependent construction logic, e.g. in the field type manager.
Yes, it's great to see that!
Deprecated :)
Maybe go with EntityWrapper again?
Having TypedData in there doesn't read very well, and the interfaces should tell you.
debug
We'll need to document acceptable values for entities. In d7 we support entity objects and ids, what was very useful. I guess we should do that as well what allows us to clean up the "entity_reference" data type class then to work like the language_reference.
Deprecated now since that got solved?
I agree with effulgentsia, we shouldn't. You shouldn't have much business in doing so, and if, you are still able to instantiate it via the TDM factory. This means, you have to know the data type to use at least, which we could supported detecting based upon interfaces, e.g. you declare a matching interface in the data type annotation which then the TDM can use to lookup the type / definition.
What about doing an optional interface for providing a data definition for an object, e.g.
DataDefinitionProviderInterface
or so? We won't be able to rely on it being there, so not sure it helps a lot. So maybe better, we could have equivalent functionality on the data type class, i.e. you have to know or be able to derive the data type again (what should be ok I think). Then, the TDM can let the data type class provide a decent data definition for your object + provide an public API for that:TDM->getDataDefintion($type, $value)
?So, instantiation of typed data wrappers via the TDM could work like that:
TDM->createInstance($type, $value)
, which usesTDM->getDataDefintion($type, $value)
and then$class::createFromDataTypeDefinition()
. Alternatively, we can keep having the public APITDM::create($definition, $value)
when you already have the definition.$class::getDataDefintion($value) I see mostly useful for cases like entities, where we can come up with a definition that contains more than just the basic 'entity' type for some $value. Not sure there are many uses besides that, but having it only do a DataDefinition::create($type) in most cases should be fine.
Interesting idea. So instead of denoting the interface in the annotation, you'd implement a method for checking it yourself? Sounds like this could be more efficient and/or at least simpler to write than doing that logic generically based on the metadata. Not sure whether having the interface in the metadata would have other use cases besides that also? (Cannot think of one right now).
Comment #86
fagoI've created issues based on the plan layed out in #51, whereas I think
is the job of this issue to establish.
Created issues:
#2268031: Name typed data interface methods unambiguously
#2268009: Make typed data object instantiation more flexible
#2268049: Decouple field definitions from typed data definitions
In regards to beta, I think we should have:
- Either fixed this one or mark to-be-removed methods as deprecated, such that are not confusing any more.
- #2268031: Name typed data interface methods unambiguously
- #2268009: Make typed data object instantiation more flexible should be able to go in after beta I think?
- However, #2268049: Decouple field definitions from typed data definitions needs it and is an API change visible to people. Not sure whether it would be applicable to mark to-be-removed methods as deprecated and mark them for removal + remove them after beta?
Comment #87
fagoThis actually needs work.
Comment #88
fagoQuickly re-rolled the patch, it seems to be broken now though.
Also, unassigning myself while not actively working on it.
Comment #90
xjmDiscussed with @effulgentsia, @fago, @alexpott, @berdir. It's a potential that parts of this could go in after the beta, but we need to figure out which parts those are before it. ;)
Comment #91
andypostre-roll
Comment #93
xjmAnother reroll. It's probably still broken.
Comment #95
xjmSigh.
Comment #97
xjmThere we go. :) The full exception is:
Comment #98
BerdirThis should fix that, we added that check a while ago while fixing a bug when calling that for config entities, will need a better check...
Didn't check anything else.
Comment #100
Jose Reyero CreditAttribution: Jose Reyero commentedSorry to interrupt, but this issue just caught my attention.
Really, though I've read very good ideas on the first posts of the issue, like simplifying and breaking down the interfaces, it seems that those are long forgotten and we have ended up with some wrapper as the last resource. Well, I'd rather have interfaces fixed (and IMO it's not TypedDataInterface that needs to be dropped -simplified yes-, but ComplexDataInterface whose same says it all.)
Or maybe just the idea that the container of typed data structures (Entity) must be a typed data itself?
Anyway this is better than nothing, so down to the patch, which is at least some improvement.
Why does the wrapper need to be a (never used) plugin?.
I think 'entity' could very well be a valid data type and we could save the 'Wrapper' name. Otherwise, if you just want it to be some 'wrapper' (though TypedData objects are all wrappers, aren't they?), then it doesn't need to be a new plugin.
Understandable, many TODOs in the patch yet, but I hope we are not committing any code with more TODOs...
I know it was already there, but since we are changing that line, can't we use 'instanceof' instead?
What kind of logic is this? Is this a wrapper or is this an entity? We should know I mean..
Or maybe the method doesn't belong here?, just wondering...
Comment #101
BerdirThat is *exactly* what this issue aims to do. But then we need a way to interact with those non-longer-typed-data-objects as typed data, which is what the wrapper approach is for.
The initial ideas around "just remove the interfaces" never made sense to me, we do need a generic way to interact with this data. Context is making more and more use of this, as one example, as will rules, generic tokens, search api and many other places like those.
The actual patch is just a proof of concept that needs more work, it's far from complete/working and I'm aware it's full of hacks. No, it will not be committed like that ;)
- The docblock on the entity class is outdated and was there before where it really wasn't used. Now it is.
- instanceof only works on class instances, what we have there is a class name as a string,
- the instanceof check is exactly the kind of stuff where I got stuck ;)
Comment #102
Jose Reyero CreditAttribution: Jose Reyero commentedPosting an incomplete patch just in case it is any use, no time to finish it any time soon -also not familiar enough with Entity/Field APIs.
Based on @Berdir's 98, the idea is as follows:
- TypedData itself is a wrapper or an adapter so no need for an extra 'adapter' concept.
- Just as Integer wraps an int value, this wraps a value that is an entity. Thus the class/plugin name should be 'Entity' (or maybe ContentEntity)
These are the relevant bits of the code:
What I find really confusing is the 'EntityReference' plug-in. That's not a data type at all and it should be better renamed (Maybe to EntityId if the value is an entity id) or dropped because DataType/Entity could very well implement the DataReferenceInterface. But really the thing is as confusing as having a 'String' and a 'StringReference' data types to hold the same kind of value.
Then there's EntityDeriver, I have no idea what we need it for if we already have an 'Entity' data type.
Anyway, seeing it again, I think @Berdir's patch in #98 is very well aimed, just needs to clarify when it uses TypedData/Entity and when it is a Drupal/Core/Entity instead. But it really cleans up a lot of the entity interfaces.
(Not marking this patch 'for review' because it's just a few pieces of example code, won't pass any test)
Comment #103
fagoAs discussed with berdir, I've been working on this and finally came back again to it today. I've no green patch yet but it's a start and should be enough for architecturial reviews.
I'd agree with Jose that we should not call it EntityTypedDataWrapper but follow the typed data naming scheme. So I'd go with an "Entity" class as well, which is the "Entity" data type - it's below the data type namespace anyway. That matches what we do in other places already, e.g. we have a NodeType entity class and a NodeType condition class.
The instantiation of that "Entity" data type has to work via the regular TDM factory - so I changed the Entity data type class accordingly. For creating the class from a given entity object, I've just added another static create method: createFromEntity() what creates the right data definition and instantiates the typed object as usual.
Then, I think we should add a getTypedData() method to the entity interface what makes it easy to get a typed data object from an entity and work from that. For not that's mostly needed by context and validator stuff, but it makes the conversion way easier.
Coming back to naming, I think we should to refer to typed data objects wrapping entities as $typed_entity as this goes along the naming of the Typed Data API, where "data" is the generic concept. Thus, in case of the data being an entity it makes sense to use the name $typed_entity to me. I agree it's not very nice, but that is as the name "Typed data" is not very nice - still it's the name so imo we should work with that and avoid inventing new names.
Attached patch seems to basically work, tests are not green yet. I had some troubles with entity serialization in form workflows when the static cache in Entity::getTypedData is enabled. Not sure what's going wrong here, so I quickly disabled the static cache for now.
What's left is:
- fix the static cache and tests
- update the entity_reference implementation
- reviews and get it in
Comment #104
fagoWell, the point of the data reference is that it can give you the reference data and/or its definition, while it's not defined / hard-coded how the reference works. Yes, often the reference is id-based but that's not necessarily the case, it might be some combination or a backward reference via a computed field.
StringReference does not make much sense anyway, however yeah - at a first glance I agree that "entity_reference" does not seem to be a separate type but could be just "entity". I've been thinking quite a bit about this though and I came to no good solution that handles both the entity and a referenced entity in a single plugin - separating it into two plugins just makes things way more stream-lined code-wise. Moreover, it's not only that as there is quite a big semantic difference between changing a referenced entity and a (stand-a-lone) entity variable - so the reference cannot be the same object as the regular typed data entity object.
Comment #106
fagoComment #107
fagoI've been working on it at #2330525: Test helper issue for use adapters for supporting typed data to get it green - so here's a first green patch, without a static typed entity property.
todo:
- reviews :)
- Typed data related parts of ContentEntityBaseUnitTest need to be moved to a new unit test though.
- fix the static typed entity property to work
Notes:
- Validation constraints DX of the entity type and bundle constraint became worse as it's working on both the
entity_reference
andentity
data types, but latter is passed wrap while the former one isn't. Not sure this is an issue as it's quite special to the EntityType and Bundle constraints to be used with both.Comment #108
BerdirHappy to see this green again :) Feedback/Questions below.
What's the difference between getTarget() and getValue()? Don't remember if this was already here before (added by me) or not :)
Looks like the static cache is currently disabled? Ah yes, you said that.
What exactly do you get when you do this for a config entity?
I see, this happens ;)
This also needs a check then on the interface, otherwise we get a fatal error?
Instaed of checks everywhere, we could also have two classes, a base class and one for content entities, the base class would just unconditionally throw a NotImplementedException or something?
This is different from how isEmpty() on the entity itself worked but I guess it makes sense, an entity is never really going to be empty as soon as you have a single default value.
Ah, getTarget() returns the typed data entity, getValue() the actual entity object.
That @todo sense makes not. :)
Not sure if "default bundle" is the right term here. Maybe fallback bundle? nobundle bundle? :p
@throws should be below @return I think? Same for others in this interface.
Oh, so now we're also allowing to inject stuff here? There's an option issue for this somewhere, which we can then close? Then we should add the DependencySerializationTrait to TypedData.
I think I added this, if you look up in this mehtod, there's now an interface check for this and it falls back to the parent, so you should be able to remove this.
Comment #109
fagogetValue() contains the value of the data reference, while target gives you a typed data object of the referenced data. That's the pre-existing API of typed data references, but before this patch the value of an entity reference was typed data just as getTarget(), after this patch the value is the entity object while the target is the typed entity.
exactly :)
A typed data object wrapping the entity as usual. But given the current lack of config entity typed data support, the typed data object won't be able to provide any metadata about properties. Still, we can easily bridge the existing typed-config metadata though (and make sure it's proper typed data if it isn't yet) in a follow-up and finally have proper typed data support for config entities.
Yep. The error message is not fully correct though, fixed that anlong with 6.
Yep, I think it would be unexpected to have it be "empty" if there is an entity without any values - so that should be better.
Corrected it is now.
Right - problem here is that we have neither words for it or even standardized/documented the behaviour. Reminds me of: #1919468: EntityManager::getAllBundleInfo() adds default entity_type_id bundle for entity types with bundles. Do you agree with my conclusions there?
Right, fixed.
Yep, I think so. It was required to instantiate Entity in a previous version of the patch, but meanwhile it works with the standard constructor. Thus, we could extract that part into another issue? -> #2268009: Make typed data object instantiation more flexible
ok, let's try without it.
Comment #110
chx CreditAttribution: chx commentedI would recommend the word proxy . As in AccountProxy or http://en.wikipedia.org/wiki/Proxy_pattern
Comment #111
effulgentsia CreditAttribution: effulgentsia commentedComment #112
BerdirRe-rolled. Only conflicted in UserUniqueValidator with a git rebase, looks like the change there is no longer needed.
Comment #116
Berdir@effulgentsia: This is the most problematic part about this issue fago and I think. This adds complexity to validators.
Comment #117
Wim LeersBerdir asked me to review this. Please know that I don't know Typed Data at all, so I reviewed this "from the surface", i.e. mostly the Entity API changes.
Lots of minor/nitpicky things, hope that's okay.
I understand where these terms come from, but it's confusing that they don't match.
string
s/property/field/
mixed|null
s/React/Reacts/
Child field? We don't call it a "child" field anywhere else?
I guess "child" was added here to stress that this is not called when a field in a referenced entity is changed. But I think that's self-evident, and if it's not, I suggest including a line about that in the docblock, but not calling this "child field".
Missing newline.
Blast from the past :) This is no more! Must be a remnant :)
Wow, never seen this syntax before!
Dead code that can be removed? Or…?
Missing newline.
s/the/this/?
Why all the plural "objects"?
s/typed data API/Typed Data API/?
And these provide… what exactly?
Follow-up or still in this issue?
string|null
Typehint should be updated to allow null.
Comment #118
Berdir1. They don't match because they are different. get() returns FieldItemList objects, set() supports any possible value, from NULL over scalars, a single field item array to multiple field item arrays and even FIeldItemList objects.
2. Fixed.
3. Fixed.
4. mixed already includes null, so the only reason would be to explicitly expose that, and I'd rather not because that behavior is weird and only needed for serialization crazyness.
5. Hm, I'm actually not sure it is Reacts. In a way, this is similar to a hook, where we afaik do not use the third person. But other onSomething() methods use Reacts, so Reacts it is. Removed child.
6. Fixed.
7. Yeah, that totally blasted in ;) Removed.
9. This is supposed to be enabled I think (statically cache the typed data object) but apparently broke things. Let's see what's still breaking.
10. Fixed.
11/12. Cleaned that up.
13. Yep.
14. These exist as typed data types. This is for example used for context, see https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Annotatio... for examples.
15. What you can see here is a living example of a "wishful thinking" @todo :) No, not in here.
16. Fixed.
17. Fixed.
Thanks!
Comment #120
fagoThanks for the review and the fixes, changes look good to me.
Yep, still breakage. We'll have to fix that I guess, because else it will needlessly create a lot of wrapper objects when creating fields I think.
Comment #121
xjmTagging as a priority for a pre-AMS beta sprint. Also removing redundant beta target tag; it's beta deadline. :)
Comment #122
chx CreditAttribution: chx commentedI would recommend the word proxy at least in the issue title . As in AccountProxy or http://en.wikipedia.org/wiki/Proxy_pattern (neither the word proxy nor adaper appears in the patch, though)
Comment #123
andyceo CreditAttribution: andyceo commented+1 for Proxy word.
Comment #124
BerdirRe-roll, this fixes some tests, let's see how many.
Comment #126
BerdirWow, all of them, apparently :) That one is new and should be fixed now too.
Comment #127
fagoGood catch! berdir++
Comment #128
fagoI do no think the term proxy fits. Proxies implement the interfaces of the proxied objects such that the caller does not have to know - this is not the case here.
Attached patch fixes some small quirks and added unit test coverage for the new class / moved the old coverage from previous methods on the entity.
Comment #129
yched CreditAttribution: yched commentedMinor edits seen with @fago
Comment #130
yched CreditAttribution: yched commentedSeen with @fago as well : renamed TypedData/Entity to TypedData/EntityAdapter, and rename a couple variables accordingly.
Comment #132
yched CreditAttribution: yched commentedAdded issue links to the dangling @todos ("support Config entities as TypedData") - note: those are about something that doesn't work in HEAD either. The patch here just adds explicit @todos about that.
Comment #134
yched CreditAttribution: yched commentedFixes :
- TypedDate\Entity -> TypedDate\EntityAdapter in strings that PhpStorm didn't find
- TaxonomyFormatter using $items->getParent() instead of getEntity()
Comment #137
fagoThanks. I've undone the two sneaked in changes. This contains improved comments to the validation issue, pointing to #2346373: Data reference validation constraints are applied wrong for the fix.
Comment #138
yched CreditAttribution: yched commentedComment #139
fagoComment #140
yched CreditAttribution: yched commentedFollowup (not introduced by this patch, but needs this patch to get in first) : #2346433: $entity iterators omit computed fields by default
Comment #141
fagoComment #142
fagoAdded change record draft: https://www.drupal.org/node/2346441
Comment #143
effulgentsia CreditAttribution: effulgentsia commentedLooks good!
Comment #144
webchickWELL! That certainly is MUCH nicer DX! :D
I had a question here, which is why this fails silently when the function is supposed to validate something. Alex said that this makes it so it doesn't flag an error on a blank field.
Apart from that I can't see anything to complain about. Let's do it!
Committed and pushed to 8.x. Thanks!
Comment #146
fagoWOHOO! :-)
Comment #147
yched CreditAttribution: yched commentedRenaming to reflect what the final approach actually was :-)
Comment #148
fagoFigured we did not re-add Traversable on the interface: #2346635: Content entities are iteratable but do not implement Traversable
Comment #149
yched CreditAttribution: yched commentedAlso, discussed with @fago a bit and posted a couple adjustements as a followup issue in #2346643: Adjust Entity::getTypedData()
Comment #150
yched CreditAttribution: yched commentedKind of related: #2354485: Harmonize toArray() / getValue() on Entity / FieldItemList / FieldItem - debugging WTF :-)