Followup for #2002138: Use an adapter for supporting typed data on ContentEntities
- The implementation Entity::getTypedData() is a bit cheating :
$class = \Drupal::typedDataManager()->getDefinition('entity')['class'];
$this->typedData = $class::createFromEntity($this);
"I pretend I don't know what is the $class for the 'entity' data type, but I assume it has a createFromEntity() static method, which is not part of any interface and only exists on TypedData\EntityAdapter.
Patch removes EntityAdapter::createFromEntity() and makes Entity::getTypedDataAdapter() use the regular TDM::create() method for instanciating the EntityAdapter
TOO LATE - EntityInterface::getTypedData() method name is not very clear on what kind of object you actually get. We settled on naming the class and concept "entity adapter", and the code typically does $entity_adapter = $entity->getTypedData(), so naming the method getTypedDataAdapter() would be more specific/accurate.
Patch does that, and renames the corresponding property in the Entity class as well.
| Comment | File | Size | Author |
|---|---|---|---|
| #31 | interdiff.txt | 900 bytes | yched |
| #31 | 2346643-typed_data_adapter-followup-31.patch | 8.64 KB | yched |
| #1 | 2346643-typed_data_adapter-followup-1.patch | 10.56 KB | yched |
| #5 | 2346643-typed_data_adapter-followup-5.patch | 10.53 KB | yched |
| #5 | interdiff.txt | 843 bytes | yched |
Comments
Comment #1
yched commentedComment #2
yched commentedComment #3
yched commentedDoh.
Comment #4
fagoIt should now use TDM::create() as usual to instantiate the object.
Comment #5
yched commentedMuch cleaner indeed.
Comment #7
yched commentedComment #9
yched commentedCrap, PhpStorm missed one rename.
Comment #10
fagoThanks, patch is good now assuming it comes back green.
Comment #11
yched commentedOK, so, too late for beta :-(, and I have no critical argument for renaming the method post-beta, so dropping the rename part, just keeping the "refactor instanciation part"
Keeping @fago's "RTBC if green" since this is a subset of the previous patch.
Comment #14
yched commentedSo, this breaks because :
- the actual 'data type' used to determine the TypedData class is not hardcoded to 'entity' anymore, TDM::create() uses the 'data type' determined by the DataDefinition it receives - in our case, EntityDataDefinition::getDataType(),
- that method always tries to be as specific as possible given the definition it holds : in that case: 'entity:[entity_type]:[bundle]'.
- that 'entity:[entity_type]:[bundle]' data type only exists if the bundle actually exists, since EntityDeriver only creates the derivatives for official bundles
This then breaks if you create an entity with a "fake" / non-existing bundles.
There seems to be quite a few places that do that atm : in tests, but also in NodeTypeForm for example:
Any thoughts ? :-)
Comment #15
yched commentedComment #16
yched commentedAgreed on a fix with @fago : in the (Entity)DataDefinition we pass to TDM::create(), do not specify the bundle if it doesn't officially exist, so that we don't try to instanciate a non-existing data type.
Comment #20
fgmSame error locally: installation with this patch does not complete, failing on property user_picture being unknown, which leaves the site incompletely installed, with user 1 still being at the "placeholde-for-uid-1" stage.
Comment #21
yched commentedHeaddesk.
Comment #23
yched commentedThe issue is then that in this case, EntityDataDefinition::getPropertyDefinitions() only return BFDs, and not the per-bundle/configurable fields, since it doesn't know its own bundle...
Ideally, the "field definitions" used by EntityDataDefinition would be injected from the Entity rather than pulled from the EntityManager. Independently of this issue here, this would be a good move towards a more injectable Entity / Field API, which is also something we discussed this week in Amsterdam.
Attached patch adds a setPropertyDefinitions() to EntityDataDefinition, and uses it in Entity::getTypedData() to inject the entity's field definitions into the EntityDataDefinition / the EntityAdapter. No docs yet, just see what breaks.
Comment #24
yched commentedComment #26
yched commentedFixes EntityAdapterUnitTest, which still used the old createFromEntity() method.
Migrate fails beat me for now :-(
Comment #28
yched commentedAlso, if anyone has a hint on how to interpret this from the testbot log :
No clue which test triggers that "Call to a member function validate() on a non-object" error.
The tests listed before and after (BaseFieldDefinitionTest, EntityViewControllerTest) are both green...
Comment #29
swentel commented@yched it goes wrong in Drupal\Tests\Core\Entity\ContentEntityBaseUnitTest
Comment #30
swentel commentedThis should fix that test - however, clueless about the migrate test as well though
Comment #31
yched commented@swentel : Thanks ! Running all unitests locally would indeed have revealed which test was failing.
Removing some unneeded code in ContentEntityBaseUnitTest::setUp(), then.
Comment #34
plachWhile working on #2394883: Language setup for entity and field based rendering in views is independent, confusing UI, lacking test coverage I found #2414721: EntityAdapter should be instantiated per language, which might be related. Feedback appreciated :)
Comment #50
smustgrave commentedThank you for creating this issue to improve Drupal.
We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.
Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!
Comment #51
smustgrave commentedWanted to bump 1 more time before closing.