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.Motivation
Field items and field item lists implement the typed data interface, what leads to same confusing method names.
Proposed resolution
Let's rename them:
Taken from #2002138-51: Use an adapter for supporting typed data on ContentEntities and updated:
- getDefinition() -> getDataDefinition()
- getName() + getParent() -> getDataContext()
- setContext() - either rename to e.g. setDataContext() or remove in #2268009: Make typed data object instantiation more flexible
- getPropertyPath() -> replace with a method on the manager working based on getDataContext() (it's hardly used anyway)
- getRoot() -> replace as getPropertyPath() with a method on the manager, (getEntity() can stay)
Remaining tasks
Do it.
User interface changes
-
API changes
- The method names change. Those methods are on entities, field items, field item lists and field item properties right now.
Comments
Comment #1
fagoComment #2
yched CreditAttribution: yched commented- re: getDataDefinition(), getDataContext(), setDataContext()...
I've lost track a bit on the current status of #2002138: Use an adapter for supporting typed data on ContentEntities and whether we still intend to have FieldItem[List] implements TypedDatainterface directly.
If we do, then I'm not sure whether, within the context of the business logic of FieldItem[List], "Data" is specific enough to clearly relate the methods to the TypedData API. Going with a full "getTypedDataDefinition()" might be clearer ?
- re: getName()
I think there is some existing code that uses FieldItemList::getName() to get "the field name" - with the confusing drawback that doing the same on a FieldItem gives you the delta instead of the field name... So yeah, either removing the mehtod or making it more clearly related to TypedData API rather than Field API would be good.
If we remove it, should we introduce getFieldName() on FieldItem / FieldItemList ? (merely as a handy shortcut to $this->getFieldDefinition()->getName() ?)
- Less sure about moving getPropertyPath() & getRoot() out of the the TypedData object and into the TDManager. I mean, those are rarely used, and I can see why we might want to move them out of the way from domain objects like FieldItem & stuff, but those are information held by the TypedData objects, it seems weird/artificial to have the getters for those not on the objects themeselves ?
But maybe it's just I'm not clearly seeing what it would look like ?
Comment #3
fagoTrue, but it's already getDataDefinition() now in HEAD and I must say getTypedDataDefinition() is a bit weird on a TypedDataInterface :/
Those are helper methods which generate the path based upon getParent() & getName(), so they could live outside the object and operate on that. Imo it's fine as further helpers like resolving a data selector "entity.field.0.value" shouldn't go into the objects either.
The plan would be to keep them using TypedDatainterface, so this issue would help to make sure the methods on them do not confuse people. At the current state converting them to an adapter seems to be a huge undertaking given all the property creation, computed properties, change handling etc. relies on typed data functionality - at least if we do not duplicate all that code.
Seems useful, yes. I'm wondering whether it would make sense to add it right now; it's just a little bit weird when both getName() and getFieldName() are there and do the same.
Comment #4
yched CreditAttribution: yched commentedNot really, if TypeDataInterface is kind enough to acknowledge itself as a "secondary interface", used on objects which already have their own domain interface which is what most consumers consider as the primary identity of the object :-)
path / root --> TDManager: fair enough, this is about recursing on parent / path properties of a $data, why not.
Comment #5
yched CreditAttribution: yched commentedThen again, I'd see no problem with TDI::getTypedDataRoot() & getTypedDataPath() :-)
Comment #6
fagoI think that ship has sailed for d8, or could it be 8.1.x maybe? No idea what's going to be allowed for 8.1.x but we'll see.
Comment #7
yched CreditAttribution: yched commentedAgreed, seems a bit late now...
Comment #10
fagoThis is clearly a d9 topic.
Comment #11
catch9.x may end up only dropping backwards compatibility from Drupal 8. Therefore we should explore doing this in a backwards compatible way in 8.x before we give up completely.