Now as the new entity field API got committed, we need to convert existing entity types to make use of it. See #1346214: [meta] Unified Entity Field API and the "Entity Field API" tag.

Files: 
CommentFileSizeAuthor
#40 config-typeddata-1818574-40.patch25 KBjrglasgow
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-typeddata-1818574-40.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#30 config-typeddata-1818574-30.patch26.24 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-typeddata-1818574-30.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#28 config-typeddata-1818574-28.patch50.71 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-typeddata-1818574-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#28 interdiff.txt661 bytestim.plunkett
#26 config-ng-1818574-25.patch26.05 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#23 1818574.patch26.37 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 51,745 pass(es), 13 fail(s), and 0 exception(s).
[ View ]
#20 configentity-1818574-20.patch26.95 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,265 pass(es).
[ View ]
#20 interdiff.txt4.98 KBtim.plunkett
#18 configentity-1818574-18.patch24.51 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,139 pass(es), 7 fail(s), and 1 exception(s).
[ View ]
#18 interdiff.txt11.32 KBtim.plunkett
#14 interdiff.txt2.76 KBtim.plunkett
#14 configentity-1818574-14.patch23.62 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,062 pass(es), 39 fail(s), and 18 exception(s).
[ View ]
#11 configentity-1818574-11.patch23.86 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,424 pass(es), 41 fail(s), and 1,186 exception(s).
[ View ]
#7 configentity-1818574-7.patch29.12 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 48,702 pass(es), 10 fail(s), and 10 exception(s).
[ View ]

Comments

With #1648930: Introduce configuration schema and use for translation happening we'd have already metadata for config objects in place, so we could use that to map it to an entity-field structure and generate entity field definitions for that.

The other way round could be us providing metadata on the entity level, which then CMI might use as well (but won't work for non-entity config). Related: #1828354: Should singular config objects be instances of a class that extends Entity?

So, #1735118: Convert Field API to CMI is turning $field and $instance definitions to config entities.

So, a $field object, being an entity, has fields (cardinality, field type...) - those are 'entity fields NG', not 'custom/old-Field-API fields' of course. However both concepts are supposed to merge at some point, right ? Aren't we heading towards the ultimate loop of hell ?

This is going to be interesting :-)

Suppose first we need to fix bugs introduced by ET. Entity hooks (ET) does not allow to save any config entity for now (entity id is not integer), so we need a way to separate config entities first or find a transparent way for using entities of both kinds.

I think EntityNG + metadata should help to unify both approches

>Aren't we heading towards the ultimate loop of hell ?

I don't think we are as the new entity fields are not necessarily backed up by config - we just need a method that gives us the entity field definitions. Thus, for supporting the case of non-configurable fields which we'd use to describe a field instance no dependency on config entities exists.

Issue tags:+Configuration system

How much work it require for each entity type? Storage, Forms, Render and Entity itself?

Status:Active» Needs review
StatusFileSize
new29.12 KB
FAILED: [[SimpleTest]]: [MySQL] 48,702 pass(es), 10 fail(s), and 10 exception(s).
[ View ]

I don't fully understand how Entity Field API will tie in for non-fieldable entities.
The first thing I guess we'll need is a TypedData type for plugins?

I was attempting to convert the config_test entity, I didn't get far but I thought I'd post some progress.

Status:Needs review» Needs work
Issue tags:+Configurables

Tagging

Priority:Normal» Major

@tim.plunkett and I agreed this should probably be major, since it affects a whole host of different entity types.

Status:Needs work» Needs review
StatusFileSize
new23.86 KB
FAILED: [[SimpleTest]]: [MySQL] 49,424 pass(es), 41 fail(s), and 1,186 exception(s).
[ View ]

Ideally #1890242: Improve the EntityBCDecorator to support converted entity properties. would help here, but it only supports fieldable entities.

I hope I'm missing something, but it seems like to add an arbitrary property to an entity type, even if its a subclass, it will need a brand new storage controller of its own. Can anyone confirm that?

Assigned:Unassigned» tim.plunkett
Status:Needs review» Needs work

Whoops, I changed the upstream controller, not the new copy. I'll revisit this tomorrow.

Status:Needs work» Needs review
StatusFileSize
new23.62 KB
FAILED: [[SimpleTest]]: [MySQL] 49,062 pass(es), 39 fail(s), and 18 exception(s).
[ View ]
new2.76 KB

So the main issue seems to be the initial set()-ing of originalID in ConfigEntityNGBase. It seems that during entity_create(), in the __construct, before the create() method finishes, all of the values are junk. That's why DatabaseStorageControllerNG loops and sets the $values again (which I don't understand), after setting them once in __construct...

Anyway, because the values haven't yet been re-nested under a langcode, accessing the string as an array gives you this: Value 'o' is identical to value 'oxcuXmir'. or Value 'g' is identical to value 'geStfuev'.

Also, if I understand #1869574: Support single valued EntityNG fields correctly, that would make be a HUGE help for DX out of the gate, I had no idea why I was mucking about with these multivalue fields for something like id or label.

Status:Needs review» Needs work

Yeah, I must say that requiring multi-value fields for every config-entity property/field feels like an unnecessary overhead. Thus, I think we should consider doing #1869574: Support single valued EntityNG fields.

Also, if I understand #1869574: Consider supporting single valued EntityNG fields correctly, that would make be a HUGE help for DX out of the gate, I had no idea why I was mucking about with these multivalue fields for something like id or label.

Well, you can do $entity->field->value although "field" is multi-valued?

So the main issue seems to be the initial set()-ing of originalID in ConfigEntityNGBase. It seems that during entity_create(), in the __construct, before the create() method finishes, all of the values are junk. That's why DatabaseStorageControllerNG loops and sets the $values again (which I don't understand), after setting them once in __construct...

I think we should add in applying-default values here (once we have them), thus this logic needs to differentiate between create/load - so it's better handled in entity_create() instead of construct?

Ideally #1890242: Improve the EntityBCDecorator to support converted entity properties. would help here, but it only supports fieldable entities.

For what would we use BC-mode here? If possible I'd go with a straight conversion as the BC-mode is not uncomplicated.

Status:Needs work» Needs review
StatusFileSize
new11.32 KB
new24.51 KB
FAILED: [[SimpleTest]]: [MySQL] 49,139 pass(es), 7 fail(s), and 1 exception(s).
[ View ]

Parts of the problem were ConfigEntityNGBase::getExportProperties() relying on Entity::get(), not expecting EntityNG::get().
The other was TypedData deciding it didn't accept '0' as a string :)
Also, moving the setOriginalID out of __construct and into load/create.
Lastly, removing the specific code for ConfigQueryTest, it just wasn't needed.

StatusFileSize
new4.98 KB
new26.95 KB
PASSED: [[SimpleTest]]: [MySQL] 49,265 pass(es).
[ View ]

Yay! It passed!
Here's my commentary on the changes made here:

+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -326,7 +327,12 @@ public function set($key, $value) {
-    $value = $this->castValue($value);
+    if ($value instanceof TypedDataInterface) {
+      $value = $value->getString();
+    }
+    else {
+      $value = $this->castValue($value);
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -122,7 +122,7 @@ public function load(array $ids = NULL) {
-        $passed_ids[$entity->{$this->idKey}] = $entity;
+        $passed_ids[$entity->id()] = $entity;
+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -331,7 +331,7 @@ public function getTranslationLanguages($include_default = TRUE) {
-        if (isset($this->values[$name])) {
+        if (isset($this->values[$name]) && is_array($this->values[$name])) {
+++ b/core/lib/Drupal/Core/Entity/Field/FieldItemBase.phpundefined
@@ -105,7 +105,7 @@ public function getString() {
-    return implode(', ', array_filter($strings));
+    return implode(', ', $strings);
+++ b/core/lib/Drupal/Core/Entity/Field/Type/Field.phpundefined
@@ -112,7 +112,7 @@ public function getString() {
-      return implode(', ', array_filter($strings));
+      return implode(', ', $strings);
+++ b/core/lib/Drupal/Core/TypedData/TypedData.phpundefined
@@ -34,6 +34,10 @@ public function __construct(array $definition) {
+  public function __toString() {
+    return $this->getString();

These are all of the indirect changes that I'd like some real review on. The rest is pretty standard stuff.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityStatusTest.phpundefined
@@ -19,7 +19,7 @@ class ConfigEntityStatusTest extends DrupalUnitTestBase {
-  public static $modules = array('config_test');
+  public static $modules = array('config_test', 'system');

system.module provides the TypedData types needed, so it has to be added to the unit tests.

ok, I did quickl review the patch. I've not looked into details for now, but more on the general architecture.

    // Retrieve the desired properties and set them in config.
    foreach ($entity->getExportProperties() as $key => $value) {
      $config->set($key, $value);
    }

This really duplicates the existing entity field definitions which tell you exactly the same: which fields should be stored? Imo we should do away with getExportProperties(), it's duplicating.

Instead, looping over $entity properties/fields and setting their values should do it.

-      return implode(', ', array_filter($strings));
+      return implode(', ', $strings);

discussed that with fubhy a bit - I guess you too :-) So this would be relevant if we set a complex typed-data object for a simple config string value, right?

Generally, I don't think getString() is sufficient here. getString() gives us a human readable string of the data, which generally is not a re-usable serialization format. (Thus leaving out the details of getString() and null values here)

For now, I think we should have a method that is easily overridable/extensible for the whole conversion to the updated $config object, as we have DatabaseStorageControllerNG::mapToStorageRecord(). Then we can apply a simple default logic as there, e.g. just access ->value for now. We can improve the default mapping logic later on.

In the case of config entities the stored entity really is just an entity serialization format, so I wonder whether it would make sense to leverage our new entity serialzation API for that? We could easily do a generic typed-data based serialization that produces a sane config structure for the core stuff by default, and allow modules to play into that via the general entity serialization API. Howsoever, this could/should be introduced later on. For now I think the simple-default mapping approach makes most sense.

-    $value = $this->castValue($value);
+    if ($value instanceof TypedDataInterface) {
+      $value = $value->getString();
+    }
+    else {
+      $value = $this->castValue($value);

That mapping from data to storage format should be the job of the entity storage layer.

StatusFileSize
new26.37 KB
FAILED: [[SimpleTest]]: [MySQL] 51,745 pass(es), 13 fail(s), and 0 exception(s).
[ View ]

This is an updated version of tim's patch in 20 and should apply to HEAD. No consideration was given for any subsequent comments, this is just a reroll.

Eclipse

Assigned:tim.plunkett» Unassigned
Status:Needs review» Needs work

We need a EntityStorageControllerBase class first, the amount of public methods that DatabaseStorageController/DatabaseStorageControllerNG has silently added without updating the interface is really making this very hard.

Status:Needs work» Needs review
StatusFileSize
new26.05 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Rerolled again, I had a local branch. It's at http://drupalcode.org/sandbox/tim.plunkett/1698392.git/shortlog/refs/hea..., most people have commit access there.

StatusFileSize
new661 bytes
new50.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-typeddata-1818574-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Added the new ConfigEntityInterface::setStatus() to ConfigEntityNGBase.

StatusFileSize
new26.24 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-typeddata-1818574-30.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Grrr. Forgot to rebase first. No changes.

Patch is green! It looks like Tim outlined next steps in #25. Anyone up for taking over now that Tim has unassigned? This would be a big help for REST Services. We could expose all views, image styles, Field/Instance definitions, etc. via REST.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityNGBase.phpundefined
@@ -0,0 +1,137 @@
+  public function getExportProperties() {
+    // Configuration objects do not have a schema. Extract all key names from
+    // class properties.
+    $class_info = new \ReflectionClass($this);
+    $properties = array();
+    foreach ($class_info->getProperties(\ReflectionProperty::IS_PUBLIC) as $property) {

But each configurable has limited set of properties to save. So maybe better to use them

+++ b/core/lib/Drupal/Core/Entity/EntityNG.phpundefined
@@ -336,7 +336,7 @@ public function getTranslationLanguages($include_default = TRUE) {
-        if (isset($this->values[$name])) {
+        if (isset($this->values[$name]) && is_array($this->values[$name])) {

Please add a comment why this could be not an array!

+++ b/core/lib/Drupal/Core/Entity/Field/FieldItemBase.phpundefined
@@ -116,7 +116,7 @@ public function getString() {
-    return implode(', ', array_filter($strings));
+    return implode(', ', $strings);

so now there's no empty properties any more?

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityListTest.phpundefined
@@ -76,7 +76,7 @@ function testList() {
-    $this->assertIdentical($expected_operations, $actual_operations);
+    $this->assertIdentical($expected_operations, $actual_operations, 'The operations are built correctly.');

It makes harder to debug if values are wrong

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.phpundefined
@@ -36,11 +36,11 @@ public static function getInfo() {
+    $this->assertIdentical($empty->style->value, NULL);
@@ -50,11 +50,11 @@ function testCRUD() {
+    $this->assertIdentical($empty->get('style')->value, NULL);
@@ -91,12 +91,12 @@ function testCRUD() {
+    $this->assertIdentical($config_test->style->value, $expected['style']);
+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestStorageController.phpundefined
@@ -45,4 +45,48 @@ public function importDelete($name, Config $new_config, Config $old_config) {
+    $fields['style'] = array(
+      'label' => t('Style'),
+      'description' => t('The ID of the associated image style.'),
+      'type' => 'entity_reference_field',
+      'settings' => array('target_type' => 'image_style'),

maybe better to use ->target_id here?

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.phpundefined
@@ -179,7 +179,7 @@ function testCRUD() {
-    $this->assertEqual($config_test->get('foo'), 'baz', 'Initial value correctly populated');
+    $this->assertEqual($config_test->foo, 'baz', 'Initial value correctly populated');
+++ b/core/modules/config/tests/config_test/config_test.moduleundefined
@@ -155,7 +155,7 @@ function config_test_cache_flush() {
-    $config_test->set('foo', 'baz');
+    $config_test->foo = 'baz';

this changed to make sure that BC works?

Still looking for brave helper to finish this. Perhaps Tim can give guidance. REST would also gain access to shortcut sets, user roles, text formats, ...

But each configurable has limited set of properties to save. So maybe better to use them

I'm not sure what this means

Please add a comment why this could be not an array!

I should have added a comment here, because I no longer remember why I did this.

so now there's no empty properties any more?

array_filter would remove 0, '0'.

It makes harder to debug if values are wrong

Without the assertion message, it prints out the full values, and there was some "nesting too deep, recursion" or whatever that error says

this changed to make sure that BC works?

I don't remember why I did this either.

I don't see the benefit of EntityNG for ConfigEntity, so I have no motivation to work on this.

Status:Needs review» Needs work

The last submitted patch, config-typeddata-1818574-30.patch, failed testing.

The last submitted patch, config-typeddata-1818574-30.patch, failed testing.

Assigned:Unassigned» jrglasgow

StatusFileSize
new25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config-typeddata-1818574-40.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

rerolled the patch - hopefully it will apply now

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

Status:Needs review» Needs work

The last submitted patch, config-typeddata-1818574-40.patch, failed testing.

Issue tags:+Needs reroll

This needs another reroll.

Tag didn't stick, trying again.

Status:Needs work» Needs review
Issue tags:-Configuration system, -Needs reroll, -Configurables, -Entity Field API

#40: config-typeddata-1818574-40.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Configuration system, +Needs reroll, +Configurables, +Entity Field API

The last submitted patch, config-typeddata-1818574-40.patch, failed testing.

Here is a simple patch starting with another approach, as discussed:
- Loosen the contract on entity fields so it can be something simple, see #1869574: Support single valued EntityNG fields-
- Leverage the same metadata system: Patch starts with that.
- Solve the get() naming problem - not sure how do that best yet. We need to somehow free up get() for whatever makes sense I guess. Probably related to the solution of #2002134: Move TypedData metadata introspection from data objects to definition objects

@fago - patch missing?

Version:8.x-dev» 9.x-dev
Priority:Major» Normal

No longer major I think with #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface which makes sense of this. It would still be nice to support config entities (better), but I guess that's 9.x material?

Issue tags:-Needs reroll

If this is postponed to D9 there's no urgent need to reroll this for the moment. Removing tag so it disappears from the "Issues needing reroll" page on the CMI initiative: http://drupal8cmi.org/node/573.

We can add the tag back when we start working on D9 :)