Overview

This is the first issue for implementing an entity property API as discussed during the WSCCI Web Services Format Sprint (report) and first proposed at the core conversations in London. In huge parts its about decoupling Field API systems and taking them to entity properties - so one could call it also “re-factor Field API”.

The goal is to provide a unified and simple API for working with entity properties - any non-property data on an entity is not supported in any way (except for IDs). For improved DX, properties should become classed objects and provide a sane default handling for language.

For more details look at the according meta issue: #1346214: [meta] Unified Entity Field API. For a short overview of the system, please read my blog post. The description there applies to this patch.

Task

Develop the Entity Property API and implement it for a first entity type and a test entity type. Further entity types will be ported to the new system in follow-up issues.

In detail:

  • Properties become classed objects and include what is currently fields plus what is currently raw entity data (e.g., {node}.uid).
  • Properties can define custom classes for improving DX by adding further suiting methods.
  • Introspection capabilities are built in the Entity and Property objects, i.e. you call a method to get back the definitions of the contained properties. Make introspection available without any concrete data as well (i.e. look up the properties of a certain entity type, bundle, field, ..)
  • Handle entity references in a meaningful way.
  • Support computed properties, which store no data but provide a mechanism to lookup the property value. I.e. add support for entity back-references that can be looked up that way.
  • Support property based validation of values and access checks.

Implementation plan

  • Initially, work with field-based and non-field based properties and provide a uniform interface for both. (Once everything is done all properties (fields and non-fields) should be just the same, see #1637642: Start implementing the Property API).
  • Introspection works based upon general interfaces and a way to define properties based upon data types, which both can be used for describing non-entity data as well (implements #1346220: Add entity property metadata).
  • Aim for compliance with PHPCR to ease future support or integration, e.g. consider following relevant terminology or interfaces.

Implementation overview

  • Entity properties are defined using data definitions, which are accessible via EntityStorageControllerInterface::getPropertyDefinitions(). The base properties of an entity type get defined by its storage controller, whereas modules may add their own via hook_entity_property_info() what field.module implements.
  • Property definitions work the same way on every level, i.e. the individual values of a property item are described the same way as entity properties are described.
  • Property objects are created based upon those definitions and the plain data values retrieved from storage. The creation of property objects happens in getter methods which usually are invoked by magic methods.
  • Property definitions and the creation of property objects are just special cases of an underlying lower-level API: The typed data API. This API mostly consists of a set of defined interfaces for dealing with data (data lists and complex data) and a set of pre-defined data primitives. That is what then is leveraged by the entity system for describing entity properties in a re-usable fashion.

All entity properties have to follow the same structure (as already known from fields) what makes it easy to predict the structure of any entity property:
drupal 8 entity data model(3).png

So on the first level the Property\EntityPropertyInterface must be implemented by all entity properties and essentially makes sure it is a list of property items (thus an instance of the TypedData\ListInterface) of property items. Then, the Property\EntityPropertyItemInterface defines that a property item holds multiple further "value properties" thus extends TypedData\ComplexDataInterface (just as an entity does too). Then the individual "value properties" are just objects implementing the TypedDataInterface, potentially being computed e.g. a loaded entity object or processed text.

An overview of classes and interfaces involved can be seen in the diagram here.

Relationship to fields

  • As discussed with yched, entity property items should become the basis for field types. Initially, field API can extend entity property items to add whatever functionality field API needs to provide a new field type. In the longer term it would be ideal if we could save all the current per-field presave/insert/update hooks by moving logic to computed properties, what should boost performance and allow field types to do not differ from an entity property any more.

Relationship to Schema API

Data/property definitions do not replace Schema definitions, right now both are in place. First off, the Schema API applies only to SQL-centric entities, but not for entities using other custom storage mechanisms. Then, Schema API is a (SQL-centric) description of data storage, whereas data definitions describe the data we work with, i.e. the PHP objects. That's sometimes quite similar, but it's not necessarily the same as data can be stored in various optimized ways while the data object stays the same.
That said, it makes sense to have both, but we could work on auto-generating a schema based upon property definitions as a follow-up, such that developers would have to specify only the property definitions.

Typed data API

The TypedData API data model consists of data lists, complex data and data primitives, whereas a list is a sequential list of data and complex data contains any number of data properties. Data definitions describe data based upon defined data types, which may be primitives or any module defined data type (as e.g. a field type or an entity). Added data types may be mapped to the built-in primitives which are predefined, e.g. an e-mail type would be mapped to the string primitive (but could add further validation logic). This allows modules to operate with any kind of typed data just by supporting complex data, data lists and the built-in primitives, such as needed when serializing entities to certain formats. In this case, it saves us from having to do n*m integrations between module-added data types and serialization formats or other "data consumers".

The list of pre-defined primitives has been created by looking at other systems like PHPCR, XML-Schema, the d7 entity property api and SDL.

Built-in primitive data types

Data type Machine name Plain value Further value formats (may be used when setting)
String string PHP string any PHP variable that casts well to a string
Boolean boolean PHP boolean any PHP variable that casts well to a boolean
Integer integer PHP integer any PHP variable that casts well to an integer
Float float PHP float any PHP variable that casts well to a float
Date date DateTime object any string supported by DateTime::__construct(), or a timestamp as integer
Duration duration DateInterval object a ISO8601 string as supported by DateInterval::__construct, or an integer in seconds
URI uri PHP string (absolute URI) -
Binary binary PHP file resource absolute stream resource URI as string

Further data types may be declared via hook_data_type_info(), which is built upon the plugin system so we can easily move to annotations in a follow-up (for details see #1732724: Implement data types as plugins.). Data definitions relate to a class via the specified data type or directly specify a certain class to be used instead. Then, a typed data object wrapping a plain data value may be created from the class, which has to be based upon the TypedDataInterface. Further typed data objects can be derived from data objects implementing the ComplexDataInterface or the ListInterface.

Why another API??

While introducing a separate lower-level API for the sake of describing entity properties might look like overkill, it turns out to have many more uses than that. The following use-cases have been identified:

  • Entity Property API: Describe entity properties and field types, so we know how to serialize them or whether they are translatable (d8mi!)
  • Blocks&Layouts: Describe required block parameters (e.g. a node), available data sources (e.g. data provided by the routing system) and map between both.
  • Blocks&Layouts+Rules: Describe condition parameters based upon the defined data types.
  • Rules+ActionsAPI: Describe actions parameters based upon the defined data types (should be the same as for conditions). For Rules, also describe available event variables and map between between both (similar to blocks&layouts!).
  • Tokens: Work based upon existing entity property definitions and moreover based upon pre-defined data types and interfaces. By doing so we don't have to annotate data in hook_token_info() again, but can focus on providing various textual representations ("token formatters") of the defined data types.
  • CMI metadata: CMI metadata needs to refer to data types - have them centrally declared and documented is certainly a plus. Also this should ease implementing the property API for configuration entities. See #1648930: Introduce configuration schema and use for translation
  • Semantic mappings: It turned out that the d7 core rdf mappings are insufficient to handle mappings of complex properties/fields, such as an addressfield. By having all components of an addressfield defined we can extend/fix our API to cover that as well.
  • Template variables?: effulgentisa mentioned that it could potentially useful there as well.

Then, this typed data API is not something totally new in Drupal. We kind of already have two of those in d7 contrib: CTools contexts and the entity API module's property info system / Rules data types. So in d7 some module had to map back and forth between both worlds (+ to the types the token system uses). Also both, CTools and Rules solved a lot of similar if not the same problems in d7 - by having a common foundation for describing data both worlds can start working together now!

What's done

The patch contains a working Property API with the "entity_test" entity type converted to make use of it. Field support is implemented, whereas this currently just makes field values to live in property classes as properties do. The only field types for which property classes have been implemented so far are those of the text module. Thanks to that, you can access the processed text value via:

// Access the processed value.
echo $entity->field_text->processed
// Access the plain value.
echo $entity->field_text->value

Then, the patch already implements full multi-lingual support for properties (as this was already added to the test-entity). The property-translation handling of the test-entity has been aligned with field translation handling, such that a uniform implementation was possible. It allows access to translated values via a EntityTranslation helper object, what allows to re-use the same API for translations and to consistently work with translation objects as with the original untranslated entity, e.g.:

    // Output the translated name property in all available languages (including the original language):
    foreach ($entity->getTranslationLanguages() as $langcode => $language) {
      $translation = $entity->getTranslation($langcode);
      echo $translation->name->value:
    }

Implementation notes

The recent field API attacher fixes of #1374030: Remove entity_extract_ids() now that we have proper classed entity objects made it a requirement to pass full entity objects to field API. This is good, however it complicates this conversion as we've two different ways to read/write entity values now and field attachers support only one. So, this patch solves this by adding all Property API changes to the Entity class into its own temporary class EntityNG, which has a "compatibility mode". If the compatibility mode is enabled, it's possible to read/write data values as previously, so we can use field API attachers in compatibility mode while having the new API in place else. Further derivations of the DatabaseStorageController and the EntityFormController classes with NG suffixes have been created for entity types that are converted to the new property API.

Code

Work is done in the entity api improvements sandbox, see the entity-property-* branches and the related issues.


For more details please have a look at the specific issues in the sandbox - thanks!

TODO:
Write some first documentation
Run performance tests and add maybe add caching to getting entity definitions.

Then, as follow-ups:
Implement it for a first core entity type, i.e. comments: #1778178: Convert comments to the new Entity Field API
#1732734: Implement per-type validation
#1732730: [meta] Implement the new entity field API for all field types
Implement it for all entity types
Fix field API to use the new property API and then
- remove the compatibility mode
- add in improvements from *NG classes to the regular ones and remove the NG classes
- add magic getters/setters to the EntityInterface

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Issue tags: +Entity system, +API addition
fago’s picture

ok, I've made good progress recently so I think it's time to post a first patch for review here.

Overview

For a short overview of the system, please read my blog post. The description there applies to this patch.

The patch implements properties as classed object, based upon a PropertyInterface, a PropertyContainerInterface for a data structure containing properties and a PropertyListInterface for a list of properites. Properties are based upon property definitions, an array defining the property and holding information like the property's data type, it's label and more.

This all is not tied to entities at all and can be used for other data as well, e.g. Rules or a future action system can use it for defining action parameter, data sources, ..

Then, the entity.module builds upon that to define interfaces that enforce the data model for all entities:
drupal 8 entity data model(3).png

First off, the EntityPropertyInterface must be implement by all entity properties and essentially makes sure it is a list (thus an instance of PropertyListInterface) of property items. Then, the PropertyItemInterface defines that a property item holds multiple further "value properties", thus is an instance of a PropertyContainerInterface (just as an entity is too). Then the "value properties" are just properties implementing the PropertyInterface.

Thus, as outlined in the blog post it generally can be used like that:

echo $node->field_image[0]->alt;
// or just leave [0] out to access the first item
echo $node->field_image->alt;
$node->field_image->alt = 'update';

What's done

The patch contains a working Property API with the "entity_test" entity type converted to make use of it. Field support is implemented, whereas this currently just makes field values to live in property classes as properties do. The only field types for which property classes have been implemented are those of text module as of now. Thanks to that, you can access the processed text value via

// Access the processed value.
echo $entity->field_text->processed
// Access the plain value.
echo $entity->field_text->value

Then, the patch already implements full multi-lingual support for properties as this was added to the test-entity recently. The property-translation handling of the test-entity has been aligned with field translation handling, such that a uniform implementation was possible.

Implementation notes

The recent field API attacher fixes of #1374030: Remove entity_extract_ids() now that we have proper classed entity objects made it a requirement to pass full entity objects to field API. This is good, however it complicates this conversion as we've two different ways to read/write entity values now and field attachers support only one. So, this patch solves this by adding all Property API changes to the Entity class into its own temporary class EntityNG, which has a "compatibility mode". If the compatibility mode is enabled, it's possible to read/write data values as previously, so we can use field API attachers in compatibility mode while having the new API in place else.

What's left

Agree upon the overall architecture
#1732754: Revisit and document definition keys
#1525958: revisit primitive data types
#1732726: Create property classes for all primitive data types
Implement it for a first core entity type, e.g. comments.
Run performance tests and add maybe add caching to getting entity definitions.

Then, as follow-ups:
#1732734: Implement per-type validation
#1732724: Implement data types as plugins.
#1732730: [meta] Implement the new entity field API for all field types
Implement it for all entity types
Fix field API storage to use it and remove the compatibility mode.

Reviews
As this is not commit-ready yet, it's too early to start nitpicking here. If you want do so, please open an issue in the sandbox and roll a patch. What this needs now are architecturial reviews!

fago’s picture

FileSize
128.73 KB

and a patch...

fago’s picture

Status: Active » Needs review
nevergone’s picture

Something idea for the general entity access layer? http://drupal.org/node/777578
#2 is very good work! :)

Status: Needs review » Needs work

The last submitted patch, d8_entity_property.patch, failed testing.

sun’s picture

Tagging.

fago’s picture

Status: Needs work » Needs review
Issue tags: -Needs architectural review
FileSize
128.72 KB

Looks like I broke field API default handling - NULL vs property_exists() what is problematic as there is no magic for property_exists(). Patch updated.

fago’s picture

cross post

Status: Needs review » Needs work

The last submitted patch, d8_entity_property.patch, failed testing.

tim.plunkett’s picture

FileSize
128.77 KB

Needed a reroll due to #1717678: Entity::createDuplicate() does not account for uuid property

Obviously the branch can figure this out, I just want to see it pass the bot :)

tim.plunkett’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-1696640-11.patch, failed testing.

fago’s picture

FileSize
130.36 KB

Adapted recent UUID patch additions and fixed entity properties to do a deep clone. Patch attached.

fago’s picture

Status: Needs work » Needs review
pounard’s picture

I would use Traversable instead of IteratorAggregate in the interface PropertyListInterface definition. This avoids exposing the getIterator() method as public and allow more flexibility in the implementation.

pounard’s picture

Why did you create the getString() method instead of using __toString() ?

EDIT: Sorry those two last comments are not architectural...

effulgentsia’s picture

FileSize
130.2 KB

This just removes an empty line removal (in other words, a no op) from EntityInterface.php in order to apply to HEAD.

dixon_’s picture

Assigned: Unassigned » dixon_
Status: Needs review » Needs work

I'm going work on renaming the interfaces, to what we sort of concluded yesterday.

PropertyContainerInterface > DataStructureInterface
PropertyListInterface > DataListInterface
PropertyInterface > DataItemInterface

Basically, I guess we will "get rid of" the whole Drupal\Core\Property namespace and replace it with Drupal\Core\Data. Then, the Entity module will extend those and define EntityProperty* classes.

moshe weitzman’s picture

That rename is a big improvement. Thanks. I'm wondering though if the 'Data' abstraction layer is really helpful. Would it be so bad if Rules had to implement this on its own? I have not heard use cases of others who want this abstraction. Less abstraction would be very helpful here, IMO.

dixon_’s picture

A generic Metadata API will be needed to build many UI components that requires arguments of some kind. It's needed so we can figure out what data (of what type etc.) is available and to be able to mock it as well.

The other use cases we talked about during the core conversation this morning was:

  • Block NG data arguments (think of it like the D7 equivalent of the CTools context data types)
  • Action/condition arguments in core
  • Maybe to add proper data type handling to Tokens?

So, all in all we might have 4 use cases in core (including Entity Property API). We might have more. So I think it makes sense.

dixon_’s picture

However, I'm also of the feeling that there's room for more simplification... Don't know exactly how atm, that will require some more thinking :)

pounard’s picture

Simple question, but isn't the word "data" too generic?

dixon_’s picture

Assigned: dixon_ » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs architectural review, -Entity system, -API addition

Here's a re-rolled patch with the new naming.

@pounard Yes, "Data" is very generic. We had this discussion yesterday here at DrupalCon and some people said what you just said. However, I think it's a very generic API we are building here.

dixon_’s picture

DrupalCon WiFi FTW! I'm currently unable to upload the patch. I'll do it as soon as I get to my hotel...

dixon_’s picture

Status: Needs review » Needs work
dixon_’s picture

Status: Needs work » Needs review
FileSize
129.96 KB

Here's a re-rolled patch with the new naming.

dixon_’s picture

Just talked to @effulgentsia and he suggested TypedData*, must say I like that a lot. What do you other people think about that?

Status: Needs review » Needs work

The last submitted patch, 1696640-entity-property-24.patch, failed testing.

pounard’s picture

Why are you extending IteratorAggregate in your interfaces and not just Traversable?

fago’s picture

Why are you extending IteratorAggregate in your interfaces and not just Traversable?

It should be Traversable, yep.

That rename is a big improvement. Thanks. I'm wondering though if the 'Data' abstraction layer is really helpful. Would it be so bad if Rules had to implement this on its own?

It wouldn't be bad if Rules had to implement its own, but it would be bad if we'd end up with multiple variants of the API in different systems which we then have to map back and forth. This happend in Drupal 7 with CTools and Rules/Entity module, both invented a "drupal data model". So people already have to map back and forth here between both worlds :/

I have not heard use cases of others who want this abstraction. Less abstraction would be very helpful here, IMO.

Conditions is not only Rules, scotch needs them as well. We want it to match with what we do. Next use case is blocks (for Drupal 8) and potentially tokens - so that makes 4 different use cases.

Also, I don't think we are inventing a big abstracting layer. We are just separating out of the entity property api what is re-usable, such that it is. We need to come up with a way for describing properties in property definitions anyway, what invents a way to describe data. So let's better use the same way for describing data in the other use-cases as well instead of doing yet another one...

fago’s picture

After more discussions me and dixon_ came up with the following terminology suggestions:

  • Name the lower-level API "TypedData" instead of Property/Metadata/Data API.
  • We realized that the objects we are working with really always are data wrappers, even in the case of properties as they wrap the plain property values. Thus, we are thinking that for the generic "PropertyInterface/ItemInterface" we better wanna go with DataWrapperInterface. All together, that makes the following suggestion:

Property\PropertyListInterface -> TypedData\DataListInterface
Property\PropertyContainerInterface -> TypedData\DataContainerInterface
Property\PropertyInterface -> TypedData\DataWrapperInterface

PropertyEntity -> EntityWrapper
PropertyString -> String
PropertyInteger -> Integer
...

Then, have
EntityInterface extends DataContainerInterface
EntityPropertyInterface extends DataListInterface
EntityPropertyItem extends DataContainerInterface, DataWrapperInterface

tim.plunkett’s picture

EntityInterface extends DataContainerInterface

This needs to be reconciled with #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc). I'm available anytime tomorrow to talk through this if you are.

yched’s picture

hate to bikeshed, but DataContainer vs DataWrapper is a bit misleading / confusing

pounard’s picture

I think #34 is true. Maybe DataWrapper should be called Value or DataValue instead (just Value sounds good to me, considering it's encapsuled in a Data* namespace).

dixon_’s picture

Yeah, we (me, @fago and @effulgentsia) talked some more about it and we realized that Container vs Wrapper is confusing. So we somewhat agreed on:

Property\PropertyContainerInterface > TypedData\DataStructureInterface

We would keep DataWrapperInterface, since it's essential for developers on this low level to understand that the data is wrapped. Then it also goes pretty much inline with the API we are familiar with from D7 entity module. Thoughts?

fago’s picture

FileSize
129.42 KB

ok, here is an updated patch using the new terminology. Class diagram follows.
Update: For interdiff/changes have a look at the git log: http://drupal.org/node/1497344/commits

fago’s picture

Status: Needs work » Needs review
fago’s picture

Here is the diagram:
d8_property_classes.png

Attached you can also find another variant showing you the methods as well.

pounard’s picture

The version with methods is unreadable because you repeat inherited methods in sub class/interfaces. Could you provide a cleaned-up version?

fago’s picture

I'm sry that's auto-generated by my IDE and it does not seem to have an option to change that. However, it's not repeating the methods for inherited classes, it's just repeating the interface methods in implementations.

pounard’s picture

Ok, no problems.

googletorp’s picture

I looked through some parts of the patch - This is quite massive.

Overall what I saw looked good.

However, I found that we are missing some data types (primitives).

The core data property types right not are:

- String
- Integer
- Language

We would also need properties for

- Decimal
- Boolean
- Date (This is not so simple - we could use unix time stamps, but also "date" and "datetime" values)
- Duration
- URI/URL

I was about to write a long rant about decimal vs floats and maybe we should support both, but it seems that PHP doesn't actually have the decimal point data type, but instead uses strings to represent these numbers. I'm still not sure if we need the decimal and float primitive, since when doing precise math, you would need to use decimal point values. I don't know what you plane on doing with primitives - if that is related to what kind of value is stored in PHP, string, int, float, etc. we probably would like to have a floats and decimal values seperated, since the float would be 1.23 but the decimal would be "1.23.

Another thing that I'm not to happy about is the get method on the EntityNG class.

The behavior is to return the value if it's set - if it's not set then get the definition and either return a value (NULL or the actual value set) or throw an exception.

I can understand from an architectual standpoint why it makes sense to throw an exception - when trying to access a value that an entity doesn't have - since it doesn't make sense to get a value that doesn't exist. However, I'm not quite sure if this is the best way of handling this.

Having been doing development with entities a lot and maintain modules that use entities - one of the most common WTF moments that I see users and developers having is exactly this for the metadata wrapper. It's very hard to figure out what's wrong - since the actually exception is thrown deep down in a class related to entities. Unless you know what to look and how - it's quite hard to figure out what's going on. All you know is that your site just died.

I'm wondering if it would be better to return a null/undefined value and raise a notice or a similar error much like what would happen if you did something like:

$account = user_load();
$value = $user->value;

Since value isn't defined on users $value would be undefined i believe and a PHP notice would be created. Doing this way - developers will be made aware of the error, but PHP can continue and render most/all of the page.

I searched a bit on the issue queue - but couldn't find a issue where error handling and exception was discussed for Drupal 8. I did find an old issue that has some interesting ideas in it #522746: Drupal Exception Wrapper Class.

The gist of it being that all exceptions thrown should be a subclass of a DrupalException that is defined in the base system. The idea was that errors could be either critical (usually file or DB issue) which should not be caught and non critical exceptions which should be caught, handled and logged. So if an non critical exception was thrown when rendering a block - maybe it wouldn't show up - but the full page would render. I don't know if something like that is going into core or not. But an alternate solution could also be to throw a minor exception which could be handled in some of the higher level systems.

I'm not sure which would be better - but I do think that it would be a good idea to not have to expose that kind of error handling for the developers for them to make sure that their site wont explode - since that might create uncertainty about using the entity APIs or bloat the code with try and catch.

// What we want
$value = $node->field->value;

// What we want to avoid having to do all the time.
try {
$value = $node->field->value;
} catch (\InvalidArgumentException $e) {
$value = NULL;
}

One line of code turned to 5 in order to make sure that a NULL value is returned in case something is off with the property data. This kind of situation where the property data is off is not something we would like ever to happen. But since we want to do some sort of caching and Drupal is quite complex - we might end up with a few edge cases where this is not the case. Since developers never can live with their site dying - we should somehow make sure this doesn't happens - either with improved error handling, maybe not even related with the entity system, or not throwing an exception in the first place.

Anyways this was a long rant - I wont change the status since I didn't read through all of the code.

geek-merlin’s picture

as for #43 datatype "decimals":
+1 for implementing these - but not necessarily now or in this place.
we should do it like in commerce: a class that stores an integer with some fixed denominator.

EDIT: also a +1 for returning an "undefined" value.

Damien Tournoud’s picture

I like seeing progress in here :)

Could we rename the different interfaces for consistency? We have a standard architecture with maps, lists and scalar values, so I suggest:

  • DataMapInterface (in place of DataStructureInterface)
  • DataListInterface
  • DataScalarInterface (in place of DataWrapperInterface)

This doesn't make any sense to me, so I probably don't understand the intent of the code:

interface EntityPropertyItemInterface extends DataStructureInterface, DataWrapperInterface

How can something be both a map and a scalar?

geek-merlin’s picture

Some review comments:

+++ b/core/lib/Drupal/Core/TypedData/DataWrapperInterface.phpundefined
@@ -0,0 +1,85 @@
+  public function __construct(array $definition, $value = NULL, $context = array());

we should typehint $context

+++ b/core/includes/common.incundefined
@@ -7316,6 +7316,48 @@ function drupal_get_updaters() {
+  return new $class($definition, $value, $context);

my gut feeling is we should do this with a factory.

+++ b/core/lib/Drupal/Core/TypedData/DataPrimitive.phpundefined
@@ -0,0 +1,50 @@
+final class DataPrimitive {

It feels strange to me that primitive data types are fixed and part of the api - or do i get this wrong.

+++ b/core/lib/Drupal/Core/TypedData/DataStructureInterface.phpundefined
@@ -0,0 +1,79 @@
+interface DataStructureInterface extends IteratorAggregate  {

we should extend ArrayAccess too and throw out toArray.

+++ b/core/lib/Drupal/Core/TypedData/DataWrapperInterface.phpundefined
@@ -0,0 +1,85 @@
+  public function getString();

should we implement __toString() ?
We might clarify: user-friedly or machine-friendly string?

+++ b/core/lib/Drupal/Core/TypedData/Type/Language.phpundefined
@@ -0,0 +1,105 @@
+   * Both the langcode and the language object may be passed as value.

my feeling is it would be cleaner to have separate methods per type.

+++ b/core/lib/Drupal/Core/TypedData/Type/String.phpundefined
@@ -0,0 +1,81 @@
+class String implements DataWrapperInterface {

looks like quite some methods of these classes should be live in a common base class: __construct, getType, getDefinition, getValue, getString

+++ b/core/modules/entity/lib/Drupal/entity/EntityNG.phpundefined
@@ -0,0 +1,355 @@
+   * For compatibility mode to work this must return a reference.

Very cool legacy support. From the code i am not sure that switching this on and off does not confuse things. This might benefit from comments.

+++ b/core/modules/entity/lib/Drupal/entity/EntityNG.phpundefined
@@ -0,0 +1,355 @@
+  public function createDuplicate() {

A comment how this is different from cloning might help (maybe @see Interface...)

+++ b/core/modules/entity/lib/Drupal/entity/EntityStorageControllerInterface.phpundefined
@@ -109,4 +109,27 @@ interface EntityStorageControllerInterface {
+   * Gets an array entity property definitions.

word missing: "array of"

+++ b/core/modules/entity/lib/Drupal/entity/EntityStorageControllerInterface.phpundefined
@@ -109,4 +109,27 @@ interface EntityStorageControllerInterface {
+   * @param array $definition
...
+   *   An array of property definitions of entity properties, keyed by property

Architecture: we should return some PropertyDefinitionArray extending ArrayObject. Same for EntityDefinition. Why? Type safety. Performance? According to this (of course to be proven) ArrayObject has practically no cost with read and write. (although it is darn slow with foreach, but i can see reasonable use case to foreach a definition, and we still can use toArray() for this)

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityApiTest.phpundefined
@@ -36,21 +36,20 @@ class EntityApiTest extends WebTestBase {
     $user1 = $this->drupalCreateUser();

"$user1" may be misleading - this is not the superuser

fago’s picture

Thanks for all the reviews. In addition to the in-person discussion here the more important remarks:

I'm not sure which would be better - but I do think that it would be a good idea to not have to expose that kind of error handling for the developers for them to make sure that their site wont explode - since that might create uncertainty about using the entity APIs or bloat the code with try and catch.

Yes, our exception handling needs work - but that's another issue. Still, you can perfectly access
$entity->field_image->title with the current patch *without* getting any exception as long as the field image actually exists. If it's NULL the code will still work, but just get you NULL.

Architecture: we should return some PropertyDefinitionArray extending ArrayObject. Same for EntityDefinition. Why? Type safety.

As discussed with you and crell, going with typed classes there would be nice but is not a priority for now. Could you open an issue in the sandbox queue for it though? Once, we've got this committed we can move it to the core queue and get it sorted out.

DataMapInterface (in place of DataStructureInterface)
DataListInterface
DataScalarInterface (in place of DataWrapperInterface)

Yeah, we already discussed that quite a bit. Map was already suggested, personally I feel like a map focuses more on mapping than on storing stuff. Still, it might be the easier understood and more precise term, so I'd be totally open with whatever name we came up with is best. Other opinions on map vs structure?

DataScalarInterface (in place of DataWrapperInterface)

The data wrapper interface is not only used for scalars, it's generally used for any kind of data wrappers. So a data wrapper can be a structure, a list or something else. Something else does not necessarily mean it's primitive, it can just be an object which we don't have any further metadata for also. Whether is it primitive is right now only determined by having a look at the definition and see whether it has the 'primitive type' key. I guess we could add a getPrimitiveType() method to the data wrapper interface, which returns FALSE if it's not a primitive?

I've updated the summary a bit to include important todos. Else, for any minor code suggestions and improvements please go to the sandbox and just open an issue there and/or talk to me. So we don't bloat this issue with minor stuff for now. If you need access to the sandbox, ping me and I'll add you.

bojanz’s picture

So far only approached this from a "user" perspective.

0) Noticed the missing datatypes (boolean, date, etc), but that has already been mentioned.
1) We're missing a way to map values (so status: 0 / 1 on nodes can become "Unpublished" / "Published". Same for mapping machine names to normal labels).
2) Trying to do just $a = $test_entity->name; dsm($a); gave me an explosion ("Property krumoXYZ is unknown."). I know the right way is $test_entity->name->value, but still.
3) If we're defining the properties on the controller (and maybe one day, in the class via annotations), do we actually need hook_entity_property_info (and the matching entity_type specific hook)? Is this hook only intended for the Field module, or do you think people will actually use it?
4) In EnttiyTestStorageController, why is the user_id translatable. And why is it a list? And does "name" need to be a list just because it's translatable? What are the rules there?
5) "Optional" is for fields only? And all fields are assumed to be required by default?
6) Do we want to add non-schema properties such as edit_url and delete_url like we do in D7, or are we planning to handle that in a cleaner way?

fago’s picture

1) We're missing a way to map values (so status: 0 / 1 on nodes can become "Unpublished" / "Published". Same for mapping machine names to normal labels).

Yes, created #1758622: Provide the options list of an entity field. I'm not sure it should be part of the initial patch though.

2) Trying to do just $a = $test_entity->name; dsm($a); gave me an explosion ("Property krumoXYZ is unknown."). I know the right way is $test_entity->name->value, but still.

Yes, krumo is broken as it writes stuff onto objects. We could add in the work-a-round from d7 entity.module, but I think we also want to improve devel.module to leverage property definitions... :)

3) If we're defining the properties on the controller (and maybe one day, in the class via annotations), do we actually need hook_entity_property_info (and the matching entity_type specific hook)? Is this hook only intended for the Field module, or do you think people will actually use it?

Good question. I think it could go away once field storage has been moved into entity storage and being a field has no UI implications. But as of now, we really have module entity storage: e.g. any module can add a property and store it on its own. In the end I'd like to see that being move to the storage controller only, but that's another issue.

4) In EnttiyTestStorageController, why is the user_id translatable. And why is it a list? And does "name" need to be a list just because it's translatable? What are the rules there?

Every entity property is by design multiple, so a list. That is as we impose the same structure on every property, see the summary and/or blog post. Translatable is optional though, but the entity_test entity type is already multi-lingual so it is translatable. That's not related to being multiple though.

5) "Optional" is for fields only? And all fields are assumed to be required by default?

Nope, the patch does not yet include a notion of properties that are mandatory. Those "optional properties" in hook_entity_property_info() are properties that not every entity of an entity type have, so everything that's bundle-specific. Then bundles just specify which properties they are making use of. Better naming suggestions are welcome of course!

6) Do we want to add non-schema properties such as edit_url and delete_url like we do in D7, or are we planning to handle that in a cleaner way?

The API allows for adding computed properties like that. Whether we want to do that for edit_url and delete_url is another question though. We could group them into a single url property or prefer to go with methods. This needs discussion.

effulgentsia’s picture

FileSize
166.54 KB

Updated patch from the sandbox.

effulgentsia’s picture

FileSize
166.54 KB

Rerolled for HEAD drift.

Status: Needs review » Needs work

The last submitted patch, d8_entity_property-51.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
168.76 KB

node.module started using Entity::translations() what the patch revamps to provide a consistent entity translation DX. Updated patch to include a deprecated Entity::translations() method to ease migration for now.

Status: Needs review » Needs work

The last submitted patch, d8_entity_property.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
168.76 KB
bojanz’s picture

Issue tags: +VDC

Another thing that I noticed is that we don't have a "schema" key (meaning, "does this property exist in the schema, can I query it?"). Are we supposed to use "computed" for that? Views will need a solution.

Added the VDC tag since the property api could really help in providing generic views integration for all entity types.

bojanz’s picture

Issue summary: View changes

updated status

fago’s picture

Issue tags: +rules, +WSCCI, +Token system, +Blocks-Layouts
FileSize
176.7 KB

I've updated the patch with the latest code. Beside more, this includes improved (shorter!) class naming, a complete set of primitives and documentation.

Another thing that I noticed is that we don't have a "schema" key (meaning, "does this property exist in the schema, can I query it?"). Are we supposed to use "computed" for that? Views will need a solution.

Yep, I've worked over the available definition keys and added 'queryable' - this should help Views I guess.

fago’s picture

Issue summary: View changes

updated summary

fago’s picture

FileSize
13.44 KB

updated class diagram attached

fago’s picture

Issue summary: View changes

updated summary with first post content

fago’s picture

Issue summary: View changes

added class diagram

fago’s picture

Issue summary: View changes

added data types

fago’s picture

Issue summary: View changes

added relationship to schema api

effulgentsia’s picture

What do you all think of breaking out the TypedData API from #57 into its own issue? Both that and this issue will become easier to review, I think, and #1648930-62: Introduce configuration schema and use for translation seems to indicate that that issue could benefit from TypedData API landing on its own.

effulgentsia’s picture

FileSize
176.55 KB

Just chasing HEAD.

Status: Needs review » Needs work
Issue tags: -Needs architectural review, -Entity system, -rules, -WSCCI, -Token system, -VDC, -Blocks-Layouts

The last submitted patch, d8_entity_property-60.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review

#60: d8_entity_property-60.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs architectural review, +Entity system, +rules, +WSCCI, +Token system, +VDC, +Blocks-Layouts

The last submitted patch, d8_entity_property-60.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
177.21 KB

Rerolled for HEAD and added a ConfigStorageController::getPropertyDefinitions() method to fix test failures.

effulgentsia’s picture

Per #59, I spun off the TypedData API into #1777268: Add a TypedData API for allowing metadata to be associated with data. That's the part that's useful to CMI and Blocks initiatives, so let's try to get that reviewed, refined, and committed quickly.

Status: Needs review » Needs work

The last submitted patch, d8_entity_property-64.patch, failed testing.

effulgentsia’s picture

+++ b/core/modules/entity/lib/Drupal/entity/Entity.php
@@ -146,14 +146,94 @@ class Entity implements EntityInterface {
+  public function set($property_name, $value) {
...
-  public function set($property_name, $value, $langcode = NULL) {

The test failure in #64 is a new test that expects to be able to set a language-specific property value on a node, and it seems the new Entity class doesn't support that. EntityNG does, via getTranslation(), but Node is an Entity, not an EntityNG yet.

fago’s picture

Status: Needs work » Needs review
FileSize
181.79 KB

Per #59, I spun off the TypedData API into #1777268: Add a TypedData API for allowing metadata to be associated with data. That's the part that's useful to CMI and Blocks initiatives, so let's try to get that reviewed, refined, and committed quickly.

I'm not so sure about that. We need to develop it based upon a use-case anyway and it might need more adjustments on the way, so I fear it's just creating work for maintaining multiple code-branches. That said, we are keeping a usable Git history here which we can't split up. :/

I see that splitting up patches helps reviewing things though. Maybe we should just provide split up patches here?

Just chasing HEAD.

Thanks for re-rolling - I've also updated the sandbox. Would be great if you could push your updates to the sandbox under entity-property-effulgentsia next time also. So I could easily merge them :)

The test failure in #64 is a new test that expects to be able to set a language-specific property value on a node, and it seems the new Entity class doesn't support that. EntityNG does, via getTranslation(), but Node is an Entity, not an EntityNG yet.

Yep. The way translated values can be used/updated with the patch changes, while there is no BC for the old way. As of now it has been used only in some test-cases, which have been overhauled anyway. I've changed this method call to d7-style direct property access for now - it needs updating anyone once node module gets converted.

@patch:
As said, I've updated it to apply against HEAD and added:

  • Added and implemented StructureInterface::isEmpty() and ListInterface::isEmpty().
  • Fixed and added tests for copying properties, using isEmpty() and adding or removing list items.

See the Git repo for details. Updated patch attached.

Status: Needs review » Needs work

The last submitted patch, d8_entity_property.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
181.66 KB
fago’s picture

Issue summary: View changes

removed duplicated not

Dries’s picture

I started looking into this, and so far I like this conceptually. I'm nervous about the added complexity, but I also see the advantages. I know a lot of thought went into the terminology, but I still think we should try to come up with better naming. I encourage all of us to try and simplify the terminology. I'll do the same over the next few days.

fago’s picture

Thanks for taking a look Dries. Regarding terminology, which parts do you think need work in particular?

Personally, I'm not too happy of not having a name for "property item values" (e.g. $node->field_image->title) that differentiates their wrapper objects from the contained values. I'll give that more thought.

Sylvain Lecoy’s picture

I am new to this issue but I am planning to deep review this.

Have you looked at doctrine and how they manage entities ? There is differences in our case to handle languages, and there is difference in their case as they manage strategies for the whole entity lifecycle. However, I found it interesting and maybe it can help to find terminology: http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/referen....

I will be able to help you in the week, the patch is not easy to dig in.

Sylvain

effulgentsia’s picture

Have you looked at doctrine and how they manage entities ?

I'm not sure about fago, but speaking for myself, while I'm not all that proficient with Doctrine ORM, PHPCR, or other ORM systems, from what little I've been able to figure out in cursory web reading is that most of them only have a single word/concept (entity or node) for an object that can have properties. Nested structures are represented as trees of nodes or entities with entity references. This breaks down with something as simple in Drupal as $node->body[0]->value, because $node->body[0] needs to contain "value", "summary", and "format", and making $node->body[0] into an entity in order to do that would be confusing, because in Drupal we distinguish between free-standing entities (like $node) and structures of information (like $node->body) that are bound to an entity, but are not in themselves free-standing entities.

However, oData is a standard that does distinguish the concept of "complex types" from "entities". Per #1787826-1: Differentiate between entity properties and properties contained in structures., I think it might make a lot of sense to therefore adopt as much of oData's terminology as possible.

gdd’s picture

Status: Needs review » Needs work
fago’s picture

FileSize
200.21 KB

Indeed. Here is an updated patch, as always for details and interdiffs see the sandbox git repo.

fago’s picture

Status: Needs work » Needs review

Note: This now incorporates an EntityFormControllerNG for doing entity forms with entities based upon the new property API and adapted entity forms tests.

Status: Needs review » Needs work

The last submitted patch, d8_entity_property.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
201.83 KB

uhm, the entity system is evolving really fast. Merged, fixed conflicts and adapted tests (once again).

fago’s picture

Issue summary: View changes

linked comment issue

Gábor Hojtsy’s picture

Priority: Normal » Critical

Right, it evolves fast, and this would Be a huge-huge task that many other subsystems are looking at Integrating with. I know CMI-D8MI wants to use typed data, blocks and layouts wants to use properties, wscii wants to rely on the unification for data representation. So having this issue as normal is a gross understatement. Elevating to critical due to all the timely dependencies.

Is the todo in the summary up to date? That sounds like a lot of stuff! If we want to let others build layers on top of this, I don't really see how we can postpone letting this land further.

joachim’s picture

Regarding data types, we also need email.

joachim’s picture

Issue summary: View changes

added cmi metadata issue

fago’s picture

Assigned: Unassigned » fago

Right, it evolves fast, and this would Be a huge-huge task that many other subsystems are looking at Integrating with. I know CMI-D8MI wants to use typed data, blocks and layouts wants to use properties, wscii wants to rely on the unification for data representation. So having this issue as normal is a gross understatement. Elevating to critical due to all the timely dependencies.

True, sounds good to me.

Is the todo in the summary up to date? That sounds like a lot of stuff! If we want to let others build layers on top of this, I don't really see how we can postpone letting this land further.

Yes and no. I've just updated it to reflect the latest plans, so it's upto date again. We figured out we want to keep converting comments in its own issue, so I've moved this off the todo for this one and the issue for converting comment to the core queue - see #1778178: Convert comments to the new Entity Field API. Also, dixon_ started working on documentation for this to ease reviewing things.

Next, we came up with some terminology improvments. I'm working those in and re-roll the patch. We plan to have all todos done until Wednesday evening.

Gábor Hojtsy’s picture

There is also #1188388: Entity translation UI in core now depending on this one, so if there would be a level above critical, I'd set this one to that :/

Gábor Hojtsy’s picture

Issue summary: View changes

updated todos

fago’s picture

Here is an updated patch reflecting the latest status. This makes now use of the plugin system, but does not yet use annotations. The typed data API has now a typed-data service and a factory for creating typed data objects.

Also, this contains updated terminology for the typed data API, which we came up with after lots of discussions:
WrapperInterface -> TypedDataInterface
WrapperBase -> TypedData( no base)
StructureInterface -> ComplexdataInterface
ListInterface stays

Some background to that terminology rename can be found at #1787826: Differentiate between entity properties and properties contained in structures..

fago’s picture

FileSize
208.15 KB

and here the actual patch.

Status: Needs review » Needs work

The last submitted patch, d8_entity_property.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
208.18 KB

fixed today's merge conflicts

fago’s picture

Issue summary: View changes

Clarified that binary is a file resource

fago’s picture

FileSize
210.43 KB
13.95 KB

updated patch containing recent improvements:

  • a62a7f7 Removed unnecessary whitespace.
  • 7560f8c Improved the documentation of typed data lists.
  • 816cf28 Updated the hook_data_type_info() documentation to match latest changes.
  • ecaf8c4 Issue #1794214: Decouple the TranslatableComplexDataInterface from complex data and rename it to TranslatableInterface.
  • ae1e872 Issue #1789998: Removed unnecessary check from EntityTranslation::set().
  • ed6f4b2 Issue #1789998: Made the entity translation getter throw an exception when retrieving an untranslatable value in strict mode.
  • 7f8c4ff Issue #1789998: Added strict mode to entity translations to properly handle property translatability.
  • 3e217c2 Various comment and code style impprovements.
  • 33664d8 Issue #1794132: Fixed setValue() implementations to only accept plain values and moved support for accepting typed data objects to the magic setters.
  • db042d1 Issue #1794132: Renamed setProperties() to setPropertyValues() and toArray() to getPropertyValues().

Also attaching an updated class diagram.

fago’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

FileSize
211.75 KB

here is again an updated patch. It contains some more terminology improvements that effulgentsia, dixon and me came up with and moves all TypedData plugin implementations below a \Type namespace.
ItemList -> EntityProperty
Item -> EntityPropertyItem

Also, we agreed to go for #1719422: Add contextual knowlege to properties what simplifies the implementation for fields and does away with the need for separate Field interfaces for now. Updated patch attached.

fago’s picture

FileSize
14.79 KB

and an updated diagram

fago’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

I've done some basic performance tests based upon the WIP comment patch, see http://drupal.org/node/1762258#comment-6525358

fago’s picture

Discussed the patch with Dries, effulgentsia, dixon and webchick in the back via skype.

We figured out it would make it easier to grasp this with the unified name "field" instead of property *now*. Also, we discussed reanming the 'string_item', 'integer_item', .. data types to 'integer_field', 'date_field', and so on.. , as 'integer_field' says more than 'integer_item'.

Also, we had the idea of moving the implementions of this fields to their respective field modules now, e.g. move integer_field to the number module. In the case of the string_item, we'd rename it to text_field and move it to the text module, whereas the previous text_item would become text_formatted_field and text_with_summary_field.

I'll take care of this renames during the next days.

fago’s picture

FileSize
212.39 KB

ok, I've done the renamings - I hope I have not overseen anything. I've not yet moved all the field classes to respective field API modules, right now this would only apply to integer_field anyway. Also, there is a string_field which we might want to rename to text_field, whereas text_field could become text_formatted_field. I think that's something we could easily do in follow-ups as well though.

This patch uses the term "Entity Field API" for the new API, whereby it calls field.module fields "configurable" at one place (see EntityStorageControllerInterface::getFieldDefinitions()). How we call field.module fields in places where we need to differentiate them definitely needs discussion, so I'd suggest a dedicated issue for this as well.

Updated patch attached.

fago’s picture

FileSize
14.23 KB

also, here is an updated diagram (including only some important classes to give an overview)

fago’s picture

Note: The updated patch also contains some typed data API improvements fixing some issues raised by chx over at http://drupal.org/node/1777268#comment-6529154.

Status: Needs review » Needs work

The last submitted patch, d8_entity_property.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
FileSize
212.35 KB

Turns out I missed renaming getPropertyDefinitions from the ConfigStorageController. Done that and also renamed two more variables (details see Git log).

Status: Needs review » Needs work

The last submitted patch, d8_entity_property.patch, failed testing.

fago’s picture

Basic sql syntax tests? Sounds unrelated and works on my box.

fago’s picture

Status: Needs work » Needs review

#97: d8_entity_property.patch queued for re-testing.

fago’s picture

Issue summary: View changes

Updated issue summary.

Dries’s picture

Status: Needs review » Fixed

Committed this patch to 8.x.

Great job everyone! This unblock a ton of other things such as web services, CMI, contextual blocks, inline editing, multilingual, etc.

Hallelujah! http://www.youtube.com/watch?v=tBKuWF-JQAQ#

effulgentsia’s picture

Title: Add a uniform Entity Property API » Change notice: Implement API to unify entity properties and fields
Status: Fixed » Active
Issue tags: +Needs change record

Yay! Needs a change notice. Retitling to reflect that we ended up with a new "field api" rather than "property api". Also, I'm not entirely sure we have a "entity field api", so much as a "typed data api" that entities and fields will use. We still need to sort out some of this terminology, but we shouldn't hold up the change notice on that.

fago’s picture

Issue tags: +Entity Field API

adding new tag

Stalski’s picture

Not the proper way, but : congratulations guys!!

catch’s picture

I've bumped #1696640: Implement API to unify entity properties and fields to major - this introduced a serious performance regression.

plach’s picture

@catch: do you mean the benchmarks issue?

catch’s picture

@plach, yes, yes I did.

Wim Leers’s picture

#107/#108: AFAICT that's #1762258: Benchmark: Bypass magic to speed up the new entity field API then? (Just trying to make sure I'm following the right issues.)

catch’s picture

Yes that's the one. Sorry for the noise.

Gábor Hojtsy’s picture

Anybody wants to go ahead and do a change notice? This is among a handful issues blocking progress elsewhere now since we are over limits :/ The patch was committed 10 days ago.

effulgentsia’s picture

Assigned: fago » Unassigned
Status: Active » Needs review

Short and sweet: http://drupal.org/node/1806650. Is anything more needed there at this time to consider this fixed?

Stalski’s picture

It's looking good for now imo

tim.plunkett’s picture

Title: Change notice: Implement API to unify entity properties and fields » Implement API to unify entity properties and fields
Status: Needs review » Fixed
Issue tags: -Needs change record

Since it is a work in progress, I think that's fine as is.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

donquixote’s picture

I am surprised that noone in this thread mentions value objects :/
#2378067: [meta] Why (mutable) TypedData instead of immutable ValueObject pattern?

It may be too late now, but I think it is still worth a discussion.