Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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 CreditAttribution: yched commentedComment #2
yched CreditAttribution: yched commentedComment #3
yched CreditAttribution: yched commentedDoh.
Comment #4
fagoIt should now use TDM::create() as usual to instantiate the object.
Comment #5
yched CreditAttribution: yched commentedMuch cleaner indeed.
Comment #7
yched CreditAttribution: yched commentedComment #9
yched CreditAttribution: yched commentedCrap, PhpStorm missed one rename.
Comment #10
fagoThanks, patch is good now assuming it comes back green.
Comment #11
yched CreditAttribution: 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 CreditAttribution: 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 CreditAttribution: yched commentedComment #16
yched CreditAttribution: 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 CreditAttribution: yched commentedHeaddesk.
Comment #23
yched CreditAttribution: 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 CreditAttribution: yched commentedComment #26
yched CreditAttribution: yched commentedFixes EntityAdapterUnitTest, which still used the old createFromEntity() method.
Migrate fails beat me for now :-(
Comment #28
yched CreditAttribution: 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 CreditAttribution: swentel commented@yched it goes wrong in Drupal\Tests\Core\Entity\ContentEntityBaseUnitTest
Comment #30
swentel CreditAttribution: swentel commentedThis should fix that test - however, clueless about the migrate test as well though
Comment #31
yched CreditAttribution: 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 :)