Posted by fago on October 20, 2012 at 4:32pm
34 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | configuration system |
| Category: | task |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | Configurables, Configuration system, Entity Field API |
Issue Summary
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.
Comments
#1
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?
#2
So, #1735118: Change notice: 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 :-)
#3
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
#4
>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.
#5
#6
How much work it require for each entity type? Storage, Forms,
Renderand Entity itself?#7
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.
#9
Tagging
#10
@tim.plunkett and I agreed this should probably be major, since it affects a whole host of different entity types.
#11
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?
#13
Whoops, I changed the upstream controller, not the new copy. I'll revisit this tomorrow.
#14
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'.orValue 'g' is identical to value 'geStfuev'.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.
#16
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: Consider supporting single valued EntityNG fields.
Well, you can do $entity->field->value although "field" is multi-valued?
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?
#17
For what would we use BC-mode here? If possible I'd go with a straight conversion as the BC-mode is not uncomplicated.
#18
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.
#20
Ah, had to update for #1826602: Allow all configuration entities to be enabled/disabled.
#21
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.
#22
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.
#23
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
#25
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.
#26
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.
#28
Added the new ConfigEntityInterface::setStatus() to ConfigEntityNGBase.
#30
Grrr. Forgot to rebase first. No changes.
#31
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.
#32
+++ 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?
#33
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, ...
#34
I'm not sure what this means
I should have added a comment here, because I no longer remember why I did this.
array_filter would remove 0, '0'.
Without the assertion message, it prints out the full values, and there was some "nesting too deep, recursion" or whatever that error says
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.
#35
#1978870: Add an EntityStorageControllerBase