Why in entity_data_type_info() do we have 'class' and 'list class'? I wonder if it would be easier to instead have 'FOO_item' and 'FOO_property'.

Comments

fago’s picture

@list-class:
We have the class which represents a list of items (= the whole property), which by default is EntityPropertyList (as it's called as of now). Then we have the regular entity property item class. Usually, the list class can stay but we want to be able to change it, so you can do computed properties or to add methods that ease multiple loading for entity reference.

@ 'FOO_item' and 'FOO_property':
Interesting idea. It would make things a bit cleaner as we'd have to separate definition arrays for two things that really a separate. That would mean you'd have to create two types when you provide a new field type though. But also, we'd need developers adding properties or the field API to create both definitions - but where to put the definition of the items?

So what about doing a generic 'item_list' type that also has an 'item type' key?

$property_definition = array(
  'label' => 'My text field foo',
  'type' => 'item_list',
  'item type' => 'text_item',
);

But then, where do we add more metadata about the item? E.g. if we need validation constraints on the item vs on the list? So maybe better:

$property_definition = array(
  'label' => 'My text field foo',
  'type' => 'item_list',
  'items' => array(
    'type' => 'text_item',
   ),
);

So you could specify your custom class already here then:

$property_definition = array(
  'label' => 'My text field foo',
  'type' => 'item_list',
  'class' => 'CustomListClass',
  'items' => array(
    'type' => 'text_item',
    'class' => 'CustomItemClass',
   ),
);

Or you could introduce your "custom_item_list" type, i.e. we can have item_list and field_item_list as data types that we could go with usually for property/field definitions.

fago’s picture

This relates to being able to use plugins: #1732724: Implement data types as plugins..

I tend to think that property definitions should stay outside of plugins though. Similarly, property definitions could stay as they are now and leave it to the factory function (drupal_wrap_data()) to instantiate the right plugin.

fago’s picture

Revisiting this, I think we could do this without changing property definitions if we have an association between the list type (=property) and the item type somehow. Maybe we could just hardcode a naming pattern into the factor?

E.g.:

The FOO property type would expose the following typed data wrappers:

FOO_item
FOO_item_list

So a FOO property is of type "FOO_item_list" - yes, it's a list of FOO_items. Also, this naming convention would match the interface names.

fago’s picture

Title: Decouple properties from property items » Decouple property item lists from property items
fmizzell’s picture

Irrelevant

fago’s picture

I don't think that helps, as the structure may not be nested arbitrarily anyway. The structure defines it's contained properties in a method on its class already, see the StructureInterface or the EntityReferenceItem implementation.

fmizzell’s picture

@fago sorry about that, after reading the comments I see that #5 is not useful at all as you said.

From what I have read (I have not looked at the code). All properties, whether they are multivalued or not, will be wrapped on a list. If that is the case why do we even need that to be part of the definitions? shouldn't it just be implied?
But at the same time I noticed that you mentioned the possibility of having an item_list, a field_item_list, and possibly other custom_item_list classes. I think that having that flexibility is great, but I have not been able to think of the use case in which a general class (item_list or property_list) would not be enough.

so in the end our definition would look like this:

$property_definition = array(
  'label' => 'My text field foo',
  'type' => 'text_item',
  'list_class' => 'Blah' //optional, default is item_list class
  ... any other info relevant to validation, etc ...
);

if no list_class is given, it can be assumed that item_list class will be the wrapper by default. otherwise the given list class will be used.

I purposefully left out the "class" key. Isn't each type associated with a class already in the data_type definition? If this is the case we should be able to get the class from the 'type' given.

Hopefully this comment is more in line with the discussion in this issue :)

fago’s picture

If that is the case why do we even need that to be part of the definitions? shouldn't it just be implied?

No, because (typed) data definitions are used on each level, e.g. also for the values contained in an entity property item. There we do not have lists.

I think that having that flexibility is great, but I have not been able to think of the use case in which a general class (item_list or property_list) would not be enough.

It might be useful for adding related methods that are specific to a property on list level, e.g. do multiple handling (load, view whatever). Then, for computed properties you'll need to specify your class that does loading on that level as well.

I purposefully left out the "class" key. Isn't each type associated with a class already in the data_type definition? If this is the case we should be able to get the class from the 'type' given.

Hopefully this comment is more in line with the discussion in this issue :)

Yes. What you described is exactly how it works now - I'm not sure whether we need or want to change it.

fago’s picture

Status: Active » Closed (duplicate)

We have a new issue for that now: #1928938: Improve typed data definitions of lists