Right now, entity types are not exposed as proper typed data API types. There is just a type 'entity', which gets you a typed data object *wrapping* the actual entity object. But furthermore, $entity is an instance of the ComplexDataInterface of the typed data API and with #1778178: Convert comments to the new Entity Field API also an instanced of the ContextAwareInterface.
First, I'd consider this a DX is wtf, it's implementing the typed data just the half way, while all the derived fields are implementing it the full way, i.e. you can call $field->getType() or $field->getDefinition() what you cannot do for an entity.
Second, this special separation makes things more complex, e.g. it will cause problems when implementing #1696648: [META] Untie content entity validation from form validation based upon TypedData validation (as the entity itself does not implement the TypedDataInterface - ouch).
Third, the parent relationship of typed data APIs is broken for entities. E.g. when you have a referenced entity and you use the TypedData API to derive a property of the entity from the TypedData object (EntityWrapper), it's parent will be another object (the entity object) - ouch. Also, right now $typed_data->getParent() cannot assume the parent is typed data, it can just assume it's complex data or a list. We should fix that.
Once we've done, we can let the TypedData ListInterface and ComplexDataInterface extend the TypedDataInterface also - we do not need to support implementing the API half-way any more ;-)
I'm thinking about options to implement this best such that it results in no WTFs, will come back with my thoughts.
Comment | File | Size | Author |
---|---|---|---|
#86 | 1868004-improve-typeddata-entityapi-86.patch | 77.37 KB | dixon_ |
#86 | 1868004-improve-typeddata-entityapi-86-interdiff.txt | 1.46 KB | dixon_ |
#81 | d8_typed_entity-1868004-81.patch | 75.5 KB | Berdir |
#81 | d8_typed_entity-1868004-81-interdiff.txt | 6.7 KB | Berdir |
#79 | d8_typed_entity-1868004-79.patch | 82.2 KB | Berdir |
Comments
Comment #1
fagook, I think the following should work out:
DataReferenceInterface
or so. (Delegating interface methods would not result in cleanly decoupled trees. We cannot rely on the plain value being the object either as this does not make sense for other reference cases where the object do not implement the typed data API natively, e.g. language.)Comment #2
klausiCould you please post some concrete examples or code snippets in the issue summary so that we can understand the problem better? What are the implications for a node article for example?
Comment #3
fagoWhat it fixes is that the following works:
whereas the following would stop to work:
Instead, you could do something like the following (while using TypedData API only):
We need resolveReference() as we cannot rely on the value being a TypedData object of the referenced value, e.g. for language references it wouldn't.
Comment #4
fagoAdding tags.
Comment #5
fagorelated: #1909666: EntityWrapper::getValue() currently doesn't work with unsaved entities.
Comment #6
EclipseGc CreditAttribution: EclipseGc commentedThis patch allowed the EntityWrapper to support all entity types with an array('type' => 'entity:node') style invokation. type 'entity' is still supported, though internally it is known as entity:entity, and there's a little bit of a bc layer for that. I assume we want to remove that in the long term, but even if we don't it's only a couple small if statements.
The definition constraints didn't appear to be getting passed to the plugins appropriately, and since the derivatives were adding constraints for EntityType, I sort of needed it to work. It still respects whatever the user passed so array('type' => 'entity:node', 'constraints' => array('EntityType' => 'user')) should actually give you a user entity, but I didn't try it. The patch is very straight forward otherwise.
Eclipse
Comment #7
fagoNo need to do that, that's taken care of for you when you get the constraints. So you always can differentiate between type constraints and data definition constraints. Check the e-mail example - it works and has type constraints.
Comment #8
fagook, as discussed on IRC we also need away to deal with data types that cannot be instantiated properly. E.g if we do this what would we instantiate for type = entity ? It works for a user, that we could instantiate, but we cannot even instantiate node without knowing the bundle (as to the current impl.).
Comment #9
fagook, started working on this. Attached patch starts with implementing #1 and partially also #8. It's not finished yet but should show where it goes.
Todo:
* Finish abstract type implementation
* Finally make $entity implement the typed data interface
* Convert tests dealing with type => 'entity'
* Make sure all entity types and bundle combinations are registered
* Write tests for the new API
On what the value of an entity should be? I think it should be plain array representation of all the values of an entity (including translations?) That way you can create a valid entity object by using the typed data factory + passing in all necessary values and such this also serves as correct entity factory.
Comment #10
fagoComment #11
podarok#10 looks good
any progress here?
Comment #12
fagoI've worked a bit more on that, so we are getting closer. Attached patch is functionally mostly complete, but has test-fails as reference-updates are not reflected due to the stale $target property in the reference.
TODO:
* Complete the abstract entity implementation (metadata with multiple-bundles) and add tests for it.
* Finally make $entity implement the typed data interface
* Write tests for the new API
For the stale data in reference to get updated, we could do an interim hack for comparing the target-id with the source id. But for doing that properly + for #1869562: Avoid instantiating EntityNG field value objects by default + for being able to track changes to an entity we should best add support for notifying the parent data item when a property changes. Thus, we could introduce this as part of #1869562: Avoid instantiating EntityNG field value objects by default and build upon it here then.
Comment #13
fagoAs we've got a patch ready in #1869562: Avoid instantiating EntityNG field value objects by default, it's time to move on with this next.
Comment #14
moshe weitzman CreditAttribution: moshe weitzman commentedThis is a blocker for validation of REST write requests - #1696648: [META] Untie content entity validation from form validation.
Comment #15
fagoOnce #1869562: Avoid instantiating EntityNG field value objects by default gets in entities already implement the typed data interfaces but only with stubs, so we've to re-roll this and implement those stubs + add some tests for using them.
Comment #16
BerdirHave no clue yet what I'm doing, just a stupid re-roll and took care of the obvious fatal errors due to interface mismatches. Not yet sure what to do in some cases with setValue() and the $notify and some other changes.
Comment #18
BerdirThis should at least fix most of those exceptions.
$node->langcode was sometimes array(0 = NULL), which seems to be related to #1957888: Exception when a field is empty on programmatic creation.
Comment #20
Berdir#1983548: Convert contact message entities to the new Entity Field API is an example were bundles are currently optional, would be great to get your feedback there.
Still says EntityWrapper
All the implements/overrides should be replaced with {@inheritdoc}
Why is the isEmpty() check only required in EntityReference? e in isEmpty should be uppercase.
Do we need to check if we have a source here? what happens if not?
Hm, what is getString() used for example? Would something like getType() . ':' . $this->getTargetIdentifier() be helpful?
Wondering about the return value here. Any reason to not limit it to int|string or something like that?
What's the difference between those two?
Huh? Why not just let the tests fail until this is done? Also, isn't that what we are doing here?
Comment #21
fagoOps, that should not have been part of the patch.
Yeah, I think that's reasonable.
I think we should fix the code to throw a typed data MissingContextException in that case.
I'm not aware of any usage of it yet, but I'd use it whenever a string representation of the object is needed, e.g. when referring to it in an exception message. Given that use-case, I think the suggestion is good.
It depends on the target whether it supports isEmpty, e.g. language does not as we are not implementing the ComplexDataInterface for language objects yet.
Replied there!
Comment #22
fagook, I worked more on this and cleaning references up. So attached patch does the following:
Then, while $reference->getTarget() should always give you a valid typed data object, the value should be NULL when there is no data. So that's the cause of the current isEmpty() check in there, alternatively we could check for the abstract entity typed data class there. Let's revisit once we figured that part out also.
Attached patch also addresses the review.
Please review, in particular how the references work!
todo:
* Update onChange() docs.
* Make entity instantiation via typed data work and complete abstract typed data plugins.
* Fix tests and add tests.
Comment #24
das-peter CreditAttribution: das-peter commentedTried to fix some of the failures.
I'm a little bit concerned about following change I had to made:
After the change
$comment->uid->entity
always returns an entity object - even though an empty one. Thus theif
clause has to be changed to check for a proper uid.Is this the intended behaviour or should
$comment->uid->entity
return FALSE / NULL if no proper entity was found.Comment #26
das-peter CreditAttribution: das-peter commentedNext bunch of fixes:
ImageItem
TypedDataManager
-ProcessDecorator
was lost (thanks to Berdir for pointing this out)core/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Tables.php
fixed the check if it's a valid entity reference - needs review, I've no clue if this is the right way to go.LocaleContentTest
added static cache reset for the language list to ensure the newly registered languages are known.Comment #28
das-peter CreditAttribution: das-peter commentedI've really trouble to run the affected test locally (timeout). But I guess that's another case where resetting the static cache of
language_list()
before usingWebTestBase::drupalCreateNode()
helps after adding new languages using the UI.Comment #30
Berdir#28: d8-expose-entity-typeddata-1868004-28.patch queued for re-testing.
Comment #31
fagoSo we've got #2002134: Move TypedData metadata introspection from data objects to definition objects now, which solves our metadata problem for entities without bundles (nodes without a type). So we embrace that instead of the Abstract Type notion.
Together with #2002138: Use an adapter for supporting typed data on ContentEntities I guess we can re-title the issue to just improve TypedData API usage, i.e. clean entity references up and just skp the AbstractType stuff for now as #2002134: Move TypedData metadata introspection from data objects to definition objects will take care of that.
Comment #32
BerdirI *think* this means that this no longer a major, validation blocking task and needs work, right? Removing sprint tag for now then, to keep the focus on the important issues, of which we have a few right now, including quite a few that are RTBC.
Comment #33
fagoI agree, this does not block validation any more. Re-assigning sprint though as I'm focussing on getting this done now.
Comment #34
fagook, worked over it and removed that (weird) abstract typed data stuff. Instead we'll do #2002134: Move TypedData metadata introspection from data objects to definition objects - see #31.
I introduced IdentifiableInterface with this patch and make EntityInterface and the typed Language object extend it. That allows for a proper DataReferenceBase implementation and is something that scotch (and Rules) would need as well - be able to get the id of some data.
Comment #35
BerdirIs the ":" special, e.g. does the system actually understand that entity:node:article is a "subtype" of entity:node and is that documented somewhere?
Also, non-NG entities don't have bundle and to be sure, should we use the bundle() method here?
Nice to see the Abstract stuff gone because I didn't understand that from reading the patch initially (I do now, after our discussions).
Comment #36
fubhy CreditAttribution: fubhy commentedCan we assume that if a typed data object is "identifiable" it is also loadable and incorporate #1983100: Provide a LoadableInterface for Typed Data objects in the same interface?
Comment #37
fagoI'm not sure that everything we can identify is loadable for us also, so it might better be optional. Howsoever, it's not needed for this issue, while we needed the IDs. So we could deal with loadable in a follow-up I think.
This syntax stems from the usual plugin derivatives syntax, so - but it's not documented anywhere and can be seen as an implementation internal I think. It's not documented or defined to be sub-types in this patch.
I agree we'll need the possibility for sub-type checks, but it should work inline with #1867880: Bring data type plugins closer to their PHP classes and work on PHP classes/interfaces as far as possible. We cannot check relationships between classes as those have to be swappable though. However, with #1867880: Bring data type plugins closer to their PHP classes we probably need to add an optional interface key to the type definition so you can e.g. specify NodeInterface and EntityInterface. Then, we can check relationships based upon the relationships of the interfaces. Consequently, I don't think we should depend on the type syntax for that.
ok, updated that. Also re-rolled patch, fixed merge conflicts and adapted previous usages of typed_data() to use \Drupal::TypedData().
Comment #38
fagoAdded previously missing doc-updates.
Comment #39
moshe weitzman CreditAttribution: moshe weitzman commented#38: d8_expose.patch queued for re-testing.
Comment #41
yched CreditAttribution: yched commented#2012662: Constraints on 'target_id' / 'tid' properties break autocomplete if applied modified EntityWrapper, I guess that's why this doesn't apply anymore.
Comment #42
das-peter CreditAttribution: das-peter commentedHere's a re-roll. Let's see what fails ;)
Comment #44
das-peter CreditAttribution: das-peter commentedFollowing tests pass on my machine now:
Drupal\comment\Tests\CommentInterfaceTest
ExpandDrupal\rdf\Tests\CommentAttributesTest
ExpandDrupal\system\Tests\Entity\EntityFieldTest
ExpandDrupal\menu\Tests\MenuTest
always runs in a timeout, but I think that's my machine ;)I'm not sure about this adjustment:
While I'm sure we need to "sanitize" the entity instance, I'm not sure if
Drupal\Core\Entity\Field\Type\EntityReference
should sanitize to BCEntity (if applicable) or the NGEntity instead. Suggestions?Comment #46
das-peter CreditAttribution: das-peter commentedFixed caching issue in the menu test. No this should be green again *cross fingers*.
Comment #47
andypostI just care that type & bundle are displayed in correct language. Is there a test for that? probably out of scope...
Comment #48
BerdirNice, I like where this is going!
We still have the (in other places also existing problem) that bundle == entity_type does not mean that the entity isn't bundleable. We'd need the interface for that that we discussed in Portland. Not a blocker I think as we have no better way atm.
@inheritdoc.
entity_get_bundles() ensures that there's always a bundle, by default one with the same name as the entity type. So to be consistent with the above check, this should only do this if count($bundles) == 1 and $bundle != $entity_type.
Probably just moved but let's inline this while we do so.
Same here, I don't think this is consistent with the first check? Always adding the bundle would be another way to make it consistent and right now probably reflect reality?
The current coding standards define that @inheritdoc must only be used if it's the only thing that's defined, there's an issue to change it, but it might make sense to leave it out to avoid any discussions. Especially since it's missing the description.
Same. Maybe move this to an inline comment for now? Also wondering if I didn't already mention this above.
Inline as \Exception.
I'm thinking if a target identifier could be anything else... but probably not :)
Still don't completely grok what this class exactly represents :) What happens if language becomes a config entity, which there's still hope? Can we then kill the special casing of languages and just treat them as entity/entity reference?
Interesting, why is this change necessary here? My UserInterface patch adds a isAnonymous() (or was it isAuthenticated and then ! of that). Would make more sense here I guess.
The patch that makes this extend from entity reference landed, so this will need a re-roll and this is no longer necessary.
Comment #49
BerdirComment #50
fubhy CreditAttribution: fubhy commentedComment #51
fubhy CreditAttribution: fubhy commentedTried to re-roll this. Not sure if it's 100% correct ;).
Comment #53
BerdirPromoting to major as discussed with @xjm.
@fago: Can you look at this?
Comment #54
fagoyep, looking at this one.
Language as config entities got in, but I don't think this influences language_reference - as we have $node->langcode independently of language module.
Comment #55
fagoRe-rolled and fixed for latest changes.
I also had to fix another hunk in the entity-relationship-query code. I think that could need some overhaul to check for types by interfaces, but that's clearly out of scope for this issue.
Comment #56
fagoNote: Pushed code to branch typed-entity-ng of the entity sandbox.
Comment #58
BerdirRe-roll, the annotations stuff went in. Let's see how well this works.
Comment #60
BerdirI just noticed that what I did can not work, the annotation on entity won't get picked up. So we have to add that in using another way, as we don't want to move Entity into the datatype plugin directory I assume?
It also means that we are obviously missing tests on the fact that this actually works and is exposed correctly.
Comment #61
fagoWhile we discussed adding the StaticDiscoveryDecorator in IRC this poses the problem of having typed-data know/depend on the entity system - which is the wrong way round. So I looked up what we are already doing for field types: We just have an unused class for registering the plugin. While that's not very nice it technically solves everything just fine, so why not.
Thus, I implemented the same for exposing entity types and aligned both implementations, i.e. moved the field item deriver to the usual Deriver sub-path as well. Also, as those classes are never used and instantiated I documented it that way and made them abstract.
I've also added test-coverage for checking that data types get correctly exposed. References are already test as part of EntityFieldTest.
Comment #63
fagoThis should be green.
Comment #65
BerdirI had the same failures in #1939994: Complete conversion of nodes to the new Entity Field API, check if it can create the translation correctly in the step above the first fails. In my case the problem was that there was the language() cache and setting a new language didn't update it correctly. That is AFAIK the only place in core where we change the language of a node (maybe of an entity in general).
Comment #66
fagoThat's interesting as those tests were green for me with #63. Let's double-check.
Comment #67
fago#63: d8_typed_entity.patch queued for re-testing.
Comment #68
fagooh dear, there are two TranslationTest classes.. - I checked the wrong one.
So yes, it was the language issue again. I figured the problem is the BCdecorator not notifying of changes.
Comment #70
fago#68: d8_typed_entity.patch queued for re-testing.
Comment #71
BerdirDoes https://drupal.org/node/2032185 affect this?
Is it possible that this logic also addresses the problems I had in File and Entity, where I had to add special handling for uid 0?
This looks scary, should this use a hash or so instead?
Same as above, entity_load() no longer returns FALSE. Does this need to be updated (aka are there other instances that could?)
I guess this answers my question that this in fact returns an entity for id 0. Great.
Trying to understand where the cached definitions are cleared, where does that happen?
Still not completely sure that the type with type and bundle is correct, when you e.g. have an
Comment #72
Berdir#68: d8_typed_entity.patch queued for re-testing.
Comment #74
fagoI think it's the field system that clears caches when bundles change, ie. field_entity_bundle_create(). It should probably happen at the entity system level, but that's another issue.
It shuold be fine as the only two supported settings are strings, however I improved it to explicitly add in the individual settings. While being there, I noted that inheriting classes did not key correctly (as they inherit they support the same setting), so I applied the same key-ing method to all those.
I'd say so - thanks for the pointer. As there is no FALSE return statement anymore I don't think we have to differentiate between those two states here any more and can simplify the code flow + simplify things for the caller. -> Updated it to just support NULL.
Comment #76
BerdirInteresting, don't quite understand why this was not necessary before, I guess the language got loaded from the langcode field even if it was empty or something like that?
Fix makes sense to me, addTranslation() checks if the language is locked and if so, does not allow to add a translation.
Comment #77
plachYep, #76 looks definitely needed.
Comment #79
BerdirThe things we have to do to get this to green.
The number of bugs this uncovers should be enough to commit it :)
You can't translate entities with und as language, I don't really know *how* it worked, but it relied on the broken behavior in the default language definition that I fixed in the previous comment...
Comment #80
BerdirThis is an important improvement that implements the empty methods that we added earlier on to the Entity class.
The API changes are not that big, it exposes entity types property as typed data, which is more or less required for #1906810: Require type hints for automatic entity upcasting (which is critical), the handling of entity/data references makes a lot more sense than the current EntityWrapper implementation and it exposes and fixes a number of bugs that are in HEAD right now, which were made explicit here.
It has been worked and iterated on for quite a long time and I think is ready. I only did some re-rolls and fixed a few tests.
Comment #81
Berdir@effulgentsia noticed that this patch adds the EntityTranslation class again that we removed in #1810370: Entity Translation API improvements due to a merge conflict, as it was never in the interdiffs.
Removing this file again.
Comment #82
effulgentsia CreditAttribution: effulgentsia commentedThis patch looks awesome. My only concern is the following instanceof check on a non-interface:
But #2028097: Map data types to interfaces to make typed data discoverable is already open for addressing that across all of TypedData. Therefore, RTBC.
Comment #83
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #84
fagoChanges look good to me, thanks for catching those issues.
Comment #85
alexpottI know these are very minor but... if it had just been the spelling mistakes I would have fixed during commit...
registers
This logic looks interesting... how is it possible for $value to not be set? I.e. why would we pass null into $value? Talking to @berdir on irc it appears that this would to unset the reference. I think that this could do with a better comment to explain that setValue can also result in unsetting a value... plus there is a spelling mistake
Enusre
... also I think that that comment should be more in the positive likeEnsure we have the NGEntity
Comment #86
dixon_Fixed remarks from Alex's review.
Comment #87
BerdirThank you, improvements look good.
Comment #88
fagoIt's not something special that you unset it by passing NULL as all the typed data objects work that way, but howsoever having it mentioned in a comment makes sense.
Comment #89
alexpottCommitted 5fc86b0 and pushed to 8.x. Thanks!
Comment #90
BerdirUntagging.