As discussed with @fago in Vienna:

ComplexDataInterface::getPropertyDefinitions() is currently a non-static method on Data objects, meaning it can potentially vary object by object and depend on the actual values being held. For field types, that would be very weird.
We discussed that getPropertyDefinitions() should only depend on the "item definition", and thus be a static method receiving a FieldDefinition object.

Then, there's the question of the static::$propertyDefinitions pattern currently used in there.
This is done to avoid recreating the property definitions on each call, and each field type structures its static::$properties according to the granularity by which the definitions may vary. This is slightly obscure and can get a bit complex to follow - see ConfigEntityReferenceItemBase::getPropertyDefinitions().

The way this is currently done with getSchema() is that FieldItem::getSchema() just determines the schema array and returns it; The "optimization/caching" part is not the responsibility of the FieldItem class, but is done inside the FieldDefinition object (Field::getSchema()). Meaning, the schema is "statically cached" by each FieldDefinition, and code that needs the schema for a given field asks it to the FieldDefinition rather than to a FieldItem object.
Could a similar pattern be used here ?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Title: TypedData::getPropertyDefinitions() should be static » ComplexDataInterface::getPropertyDefinitions() should be static

more accurate title

fago’s picture

yeah, given we have classed field definitions now, I've been thinking about making a Definition::getPropertyDefinitions() as primary API instead.

-> This boils into #2002134: Move TypedData metadata introspection from data objects to definition objects, posting thoughts on it there.

effulgentsia’s picture

Title: ComplexDataInterface::getPropertyDefinitions() should be static » Change field types to define their property definitions statically
Priority: Normal » Major
Issue tags: +beta target
Parent issue: » #2002134: Move TypedData metadata introspection from data objects to definition objects
FileSize
58.82 KB

How would you guys feel about doing just this much in this issue, and the rest in #2002134: Move TypedData metadata introspection from data objects to definition objects. This is just the portion of the patch there that deals with the *Item classes.

fago’s picture

In general yes, however I've been thinking that such a simple decoupling is problematic because we loose the static caching of property definition objects and thus increase memory usage quite drastically.

In the completer variant the static caching happens in the definition object, which is shared across the instances.

effulgentsia’s picture

Status: Active » Closed (duplicate)