Problem/Motivation

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.

Configuration entities need to be supported by the entity data type deriver (\Drupal\Core\Entity\Plugin\DataType\Deriver\EntityDeriver). Supporting configuration entities as typed data is a step to supporting config entity validation for the rest API.

Proposed resolution

Add a new \Drupal\Core\Entity\Plugin\DataType\ConfigEntityAdapter class to enhance the existing \Drupal\Core\Entity\Plugin\DataType\EntityAdapter so that configuration entities can be a supported data type.

Remaining tasks

User interface changes

None

API changes

Configuration entity data types supported by the typed data system. There are derived in a similar way to content entities - ie. "entity:$entity_type" and "entity:$entity_type:$bundle" data types.

Data model changes

None

CommentFileSizeAuthor
#107 1818574-2-107.patch15.4 KBalexpott
#102 1818574-2-102.patch15.4 KBalexpott
#102 101-102-interdiff.txt1.92 KBalexpott
#101 1818574-2-101.patch13.47 KBalexpott
#101 97-101-interdiff.txt1.15 KBalexpott
#97 1818574-2-97.patch12.76 KBalexpott
#97 92-97-interdiff.txt2.18 KBalexpott
#92 interdiff-1818574.txt1.08 KBdawehner
#92 1818574-92.patch12.21 KBdawehner
#90 interdiff-1818574.txt845 bytesdawehner
#90 1818574-90.patch11.13 KBdawehner
#82 1818574-82.patch10.99 KBalexpott
#82 80-82-interdiff.txt4.22 KBalexpott
#80 1818574-80.patch11.11 KBalexpott
#80 79-80-interdiff.txt936 bytesalexpott
#79 interdiff-1818574.txt821 bytesdawehner
#79 1818574-79.patch10.8 KBdawehner
#77 1818574-77.patch10.76 KBalexpott
#77 69-77-interdiff.txt3.81 KBalexpott
#70 interdiff-1818574.txt13.59 KBdawehner
#70 1818574-69.patch9.69 KBdawehner
#66 1818574-66.patch7.78 KBtedbow
#66 interdiff-65-66.txt1.32 KBtedbow
#65 1818574-65.patch8.25 KBtedbow
#65 interdiff-64-65.txt1013 bytestedbow
#64 1818574-64.patch7.98 KBtedbow
#64 interdiff-61-64.txt4.66 KBtedbow
#61 1818574-61.patch7.71 KBtedbow
#61 interdiff-59-61.txt662 bytestedbow
#59 1818574-59.patch7.57 KBtedbow
#40 config-typeddata-1818574-40.patch25 KBjrglasgow
#30 config-typeddata-1818574-30.patch26.24 KBtim.plunkett
#28 config-typeddata-1818574-28.patch50.71 KBtim.plunkett
#28 interdiff.txt661 bytestim.plunkett
#26 config-ng-1818574-25.patch26.05 KBtim.plunkett
#23 1818574.patch26.37 KBEclipseGc
#20 configentity-1818574-20.patch26.95 KBtim.plunkett
#20 interdiff.txt4.98 KBtim.plunkett
#18 configentity-1818574-18.patch24.51 KBtim.plunkett
#18 interdiff.txt11.32 KBtim.plunkett
#14 interdiff.txt2.76 KBtim.plunkett
#14 configentity-1818574-14.patch23.62 KBtim.plunkett
#11 configentity-1818574-11.patch23.86 KBtim.plunkett
#7 configentity-1818574-7.patch29.12 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

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?

yched’s picture

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 :-)

andypost’s picture

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

fago’s picture

>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.

gdd’s picture

Issue tags: +Configuration system
andypost’s picture

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

tim.plunkett’s picture

Status: Active » Needs review
FileSize
29.12 KB

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.

tim.plunkett’s picture

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

Tagging

xjm’s picture

Priority: Normal » Major

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
23.86 KB

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?

tim.plunkett’s picture

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

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
23.62 KB
2.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 Entity 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.

fago’s picture

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 Entity 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?

fago’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
11.32 KB
24.51 KB

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.

tim.plunkett’s picture

tim.plunkett’s picture

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.

fago’s picture

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.

EclipseGc’s picture

FileSize
26.37 KB

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

tim.plunkett’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
26.05 KB

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.

tim.plunkett’s picture

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

tim.plunkett’s picture

Grrr. Forgot to rebase first. No changes.

moshe weitzman’s picture

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.

andypost’s picture

+++ 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?

moshe weitzman’s picture

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, ...

tim.plunkett’s picture

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.

tim.plunkett’s picture

benjifisher’s picture

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.

jrglasgow’s picture

Assigned: Unassigned » jrglasgow
jrglasgow’s picture

rerolled the patch - hopefully it will apply now

jrglasgow’s picture

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

Status: Needs review » Needs work

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

star-szr’s picture

Issue tags: +Needs reroll

This needs another reroll.

star-szr’s picture

Tag didn't stick, trying again.

Xano’s picture

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.

fago’s picture

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 Entity 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

star-szr’s picture

@fago - patch missing?

Berdir’s picture

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?

pfrenssen’s picture

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 :)

Berdir’s picture

Title: Convert config entities to the new Entity Field API » Support config entities in typed data EntityAdapter
Version: 9.x-dev » 8.0.x-dev
Issue summary: View changes
Issue tags: +Needs issue summary update

With #2002138: Use an adapter for supporting typed data on ContentEntities, it might now be possible to support config entities without API change or any negative impact on them.

moshe weitzman’s picture

Great news! Would be huge to get this one in. We could then support REST operations on config entities

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Priority: Normal » Major
Status: Needs work » Postponed
Issue tags: +Triaged core major

It'd be great if we could do this in a minor in a BC way for the reason @moshe weitzman describes and for making our validation more consistent overall, but no longer feasible in 8.0.x. Reference: https://www.drupal.org/core/d8-allowed-changes

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Status: Postponed » Needs work

There is no reason that we cannot do that at this point. Maybe it would be great to first have a look what we actually want to support instead of just rerolling the current patch, which is totally outdated at this point in time.

tedbow’s picture

Status: Needs work » Needs review
FileSize
7.57 KB

It looks like #58 change the scope of this.

So here is start to try to get EntityAdapter to support ConfigEntities.

I don't know the history of this issue but this would a start to get #2300677: JSON:API POST/PATCH support for fully validatable config entities to be able to validate entities.

Status: Needs review » Needs work

The last submitted patch, 59: 1818574-59.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tedbow’s picture

Status: Needs work » Needs review
FileSize
662 bytes
7.71 KB

Whoops forgot @group

dawehner’s picture

I think one important part should be calling to validate() and ensuring that the basic data types are validated.

Status: Needs review » Needs work

The last submitted patch, 61: 1818574-61.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tedbow’s picture

Status: Needs work » Needs review
FileSize
4.66 KB
7.98 KB

This should fix some but not all of test failures.

tedbow’s picture

#62 agreed.

The previous patch set the weight property to a string which did produce an error in validate() but it wasn't checking the message to see if it was correct. Adding that.

tedbow’s picture

I messed when I updated \Drupal\Core\Entity\Plugin\DataType\EntityAdapter::getIterator and I wasn't returning a value.

The last submitted patch, 64: 1818574-64.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 65: 1818574-65.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityAdapter.php
    @@ -78,9 +79,7 @@ public function get($property_name) {
         if (!$this->entity instanceof FieldableEntityInterface) {
    -      // @todo: Add support for config entities in
    -      // https://www.drupal.org/node/1818574.
    -      throw new \InvalidArgumentException("Unable to get unknown property $property_name.");
    +      return $this->getConfigTypedData()->get($property_name);
         }
    

    not fieldable doesn't have to mean config entity, so this should be changed to an explicit config entity check I guess. Not sure what to do if it's neither... maybe throw an exception at the end?

  2. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityAdapter.php
    @@ -93,13 +92,14 @@ public function set($property_name, $value, $notify = TRUE) {
    -      throw new \InvalidArgumentException("Unable to set unknown property $property_name.");
    +    if ($this->entity instanceof ConfigEntityInterface) {
    +      $this->entity->set($property_name, $value, $notify);
         }
    -    // This will throw an exception for unknown fields.
    -    $this->entity->set($property_name, $value, $notify);
    +    else {
    +      // This will throw an exception for unknown fields.
    +      $this->entity->set($property_name, $value, $notify);
    

    similar to here, but probably if/elseif?

dawehner’s picture

Looking at EntityAdapter it starts to have a lot of special cases which assume that non fieldable entities are config entities.
I propose to actually have a config specific entity adapter, which contains the specific logic for config entities.

On top of that I made a range of other improvements.

tedbow’s picture

@dawehner this is much better. #70 that sounds good. I wasn't sure where the EntityAdapter class was set. Now I see \Drupal\Core\Entity\Plugin\DataType\Deriver\EntityDeriver set in the annotation.

I was wondering if we should log some type of warning in \Drupal\Core\Entity\Plugin\DataType\EntityAdapter::createFromEntity if it is called with a Config entity. Would anybody ever call this directly since we already have \Drupal\Core\Entity\Entity::getTypedData()?

Also should there a follow up to mark EntityAdapter and ConfigEntityAdapter as @internal? Or at least \Drupal\Core\Entity\Plugin\DataType\EntityAdapter::createFromEntity()? Outside code should always be using \Drupal\Core\Entity\Entity::getTypedData() correct? Since EntityAdapter::createFromEntity() wasn't internal and before this patch it would have been effectively the same as calling getTypedData().

dawehner’s picture

@tedbow
You asked some good questions. Ideally fago would answer how the API was planned to be used.

alexpott’s picture

I like the idea of making createFromEntity internal but it already has quite a few usages in contrib - search_api tests for example. Given that contrib is already extending the EntityAdapter making the whole thing internal is probably not right.

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/ConfigEntityAdapter.php
@@ -0,0 +1,92 @@
+/**
+ * Enhances EntityAdapter for config entities.
+ */
+class ConfigEntityAdapter extends EntityAdapter {

Should we override some of the annotation? This is what similar code in entity_reference_revisions has done.

Should we test all the inherited methods from EntityAdapter to ensure they work on the ConfigEntity? I think the meothds are ::applyDefaultValue() and ::getString()?

dawehner’s picture

Should we test all the inherited methods from EntityAdapter to ensure they work on the ConfigEntity? I think the meothds are ::applyDefaultValue() and ::getString()?

That sounds like a good idea!

Wim Leers’s picture

Issue tags: +API-First Initiative, +blocker

So exciting to see progress here! 😄 🎉

#2300677: JSON:API POST/PATCH support for fully validatable config entities is the next big thing for the API-First Initiative, and based on the conversations at DrupalCon Vienna with @fago, @dawehner, @amateescu, @hchonov, @alexpott, @tedbow and I, it sounds like we feel this is a must-have to properly implement that. Tagging…

alexpott’s picture

I've tried working on a test that calls ConfigEntityAdapter::applyDefaultValue(). This brings up a couple of problems.

  • The method in \Drupal\Core\Entity\Plugin\DataType\EntityAdapter::applyDefaultValue() does not actually change apply any default values to the config entity (I'm not sure it would on a config entity either). It just set's the value on the property object but this is never applied on the entity level like both adapter's ::set() methods do.
  • Then if you create a version that does apply the default values to the entity for config entities this creates an interesting problem because the ID get's nulled and we can't find a schema for config_test.dynamic..

I think having default values for configuration entities might be something we want to support via schema so I'm not sure what to do here. Maybe as we don't have such a concept at the moment we could punt and just throw a \BadMethodCallException or something like that.

alexpott’s picture

Here's a patch that implements the exception.

dawehner’s picture

The method in \Drupal\Core\Entity\Plugin\DataType\EntityAdapter::applyDefaultValue() does not actually change apply any default values to the config entity (I'm not sure it would on a config entity either). It just set's the value on the property object but this is never applied on the entity level like both adapter's ::set() methods do.

Couldn't we copy the values from the properties object to the entity on every onChange, which is probably fired on applyDefaultValues?

dawehner’s picture

Here are just some minor fixes.

alexpott’s picture

Re #78 - the problem is we can't copy all the default values (which are NULL) because the ID is part of the schema name. I think punting on default values for config is okay ATM because they don't have any.

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/ConfigEntityAdapter.php
@@ -0,0 +1,100 @@
+  /**
+   * Get typed data for config entity.
+   *
+   * @return \Drupal\Core\TypedData\ComplexDataInterface
+   *   The typed data.
+   */
+  protected function getConfigTypedData() {
+    return $this->getTypedDataManager()->createFromNameAndData($this->entity->getConfigDependencyName(), $this->entity->toArray());
+  }

This is quite inefficient. We re-create the typed data on every get - I'm not sure this is necessary. But it does make things a bit easier with respect to state. Patch attached adds to a test to ensure this is true.

tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/EntityDeriver.php
    @@ -86,7 +89,9 @@ public function getDerivativeDefinitions($base_plugin_definition) {
    +      $class = $entity_type instanceof ConfigEntityTypeInterface ? ConfigEntityAdapter::class : EntityAdapter::class;
    

    Nit:
    $entity_type->entityClassImplements(ConfigEntityInterface::class) is more correct

  2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ConfigEntityAdapterTest.php
    @@ -0,0 +1,146 @@
    + * @see \Drupal\Core\Entity\Plugin\DataType\ConfigEntityAdapter
    ...
    +  /**
    +   * Tests applying default values to the config entity is not supported.
    +   */
    +  public function testApplyDefaultValue() {
    

    Can KTB also use @coversDefaultClass and @covers? I think that would be preferable.

alexpott’s picture

Thanks for the review @tim.plunkett

I think KernelTestBase can use @covers... I held off because of \Drupal\KernelTests\Core\Entity\ConfigEntityAdapterTest::testEntityDeriver() but then again we can just use the full class there.

dawehner’s picture

Nice! It would be nice to have @fago reviewing these issues, as always ...

dawehner’s picture

This is quite inefficient. We re-create the typed data on every get - I'm not sure this is necessary. But it does make things a bit easier with respect to state. Patch attached adds to a test to ensure this is true.

Should we open up a follow up to explore that but keep it like that for now?

Wim Leers’s picture

+1

dawehner’s picture

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

fago’s picture

Awesome to see this coming along so well! Patch looks already great!

Also should there a follow up to mark EntityAdapter and ConfigEntityAdapter as @internal? Or at least \Drupal\Core\Entity\Plugin\DataType\EntityAdapter::createFromEntity()? Outside code should always be using \Drupal\Core\Entity\Entity::getTypedData() correct? Since EntityAdapter::createFromEntity() wasn't internal and before this patch it would have been effectively the same as calling getTypedData().

I'd see EntityAdapter::createFromEntity() as the main API, while getTypedData() being an optional shortcut with performance optimizations for multiple calls. I'd see similar \Drupal\Core\Entity\EntityInterface::save() - a convenient wrapper around the underling Entity Storage API. Thus, keeping it public seems fine to me. We could add an @see to the helper though.

Re #78 - the problem is we can't copy all the default values (which are NULL) because the ID is part of the schema name.

sry, I don't follow :/ Isn't the ID empty when you create a new entity also? It would empty the ID for content entities also. Well, when you revert the data back to defaults that's what you get.

1.

+  /**
+   * Gets the typed data manager.
+   *
+   * @return \Drupal\Core\Config\TypedConfigManagerInterface
+   *   The typed data manager.
+   */
+  public function getTypedDataManager() {
+    if (empty($this->typedDataManager)) {
+      $this->typedDataManager = \Drupal::service('config.typed');
+    }
+
+    return $this->typedDataManager;
+  }

This is confusingly named as it's not getting the typed data manager, but the typed config manager.

2.
+ * Get typed data for config entity.
nit pick: Gets typed data for *a* config entity.

dawehner’s picture

I'd see EntityAdapter::createFromEntity() as the main API, while getTypedData() being an optional shortcut with performance optimizations for multiple calls. I'd see similar \Drupal\Core\Entity\EntityInterface::save() - a convenient wrapper around the underling Entity Storage API. Thus, keeping it public seems fine to me. We could add an @see to the helper though.

👏👏👏

sry, I don't follow :/ Isn't the ID empty when you create a new entity also? It would empty the ID for content entities also. Well, when you revert the data back to defaults that's what you get.

I created a follow up to deal with that. Default values could be nice for some reasons.

This is confusingly named as it's not getting the typed data manager, but the typed config manager.

Well, this is inherited by \Drupal\Core\TypedData\TypedDataTrait

Status: Needs review » Needs work

The last submitted patch, 90: 1818574-90.patch, failed testing. View results

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.21 KB
1.08 KB

Let's fix the remaining failures.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready. It'd be great to link to the follow-up @dawehner talked about in #90.

sry, I don't follow :/ Isn't the ID empty when you create a new entity also? It would empty the ID for content entities also. Well, when you revert the data back to defaults that's what you get.

from #89.

You can't create a configuration entity without an ID and use schema because we expect the ID when we resolve a schema definition for example taxonomy.vocabulary.*

Wim Leers’s picture

OMG OMG OMG! Yay, thanks, @alexpott! Config entity validation and hence REST support can now move forward again!

dawehner’s picture

I think this is ready. It'd be great to link to the follow-up @dawehner talked about in #90.

Sure: Here we go: #2945635: Figure out how to deal with applyDefaultValue on ConfigEntityAdapter

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

This patch looks great! Just some minor things:

  1. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/ConfigEntityAdapter.php
    @@ -0,0 +1,102 @@
    +  public function set($property_name, $value, $notify = TRUE) {
    

    Needs to return $this.

  2. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityAdapter.php
    @@ -94,8 +94,6 @@ public function set($property_name, $value, $notify = TRUE) {
    -      // @todo: Add support for config entities in
    -      // https://www.drupal.org/node/1818574.
    

    This is great. Can we also remove the same comment from get()?

  3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ConfigEntityAdapterTest.php
    @@ -0,0 +1,148 @@
    +  public function testSet() {
    ...
    +    $adapter->set('weight', 2);
    

    Should we add an assertion that $adapter is returned? If so, should we also add a similar assertion to EntityAdapterUnitTest::testSet() as well?

Also, this issue is still tagged with "Needs issue summary update", which I think is accurate. Anyone up for doing that?

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
2.18 KB
12.76 KB

Patch attached addresses #96. I don't think we should change EntityAdapterUnitTest::testSet() here. But +1 to asserting on the return in the new test.

I've updated the issue summary.

alexpott’s picture

Issue summary: View changes

I've started on a CR - perhaps someone else can flesh it out.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @effulgentsia for the review. Thank you @alexpott for addressing it. I expanded the CR with one example usage.

jibran’s picture

  1. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ConfigEntityAdapterTest.php
    @@ -0,0 +1,149 @@
    +    $expected_properties = [
    +      'uuid' => StringData::class,
    +      'langcode' => StringData::class,
    +      'status' => BooleanData::class,
    +      'dependencies' => Mapping::class,
    +      'id' => StringData::class,
    +      'label' => StringData::class,
    +      'weight' => IntegerData::class,
    +      'style' => StringData::class,
    +      'size' => StringData::class,
    +      'size_value' => StringData::class,
    +      'protected_property' => StringData::class,
    +    ];
    

    This is kind of config schema validation? If yes then we should verify all the config entities schema.

  2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ConfigEntityAdapterTest.php
    @@ -0,0 +1,149 @@
    +    $properties = ConfigEntityAdapter::createFromEntity($this->entity)->getProperties();
    ...
    +    foreach ($properties as $key => $property) {
    

    Why not use the EntityAdapter object directly in the loop? It will verify the ::getIterator() method as well.

alexpott’s picture

Re #100.1 We're testing typed data hence asserting against typed data classes.

Re #100.2 Yep we can add test coverage of that. But not here we're testing getProperties explicitly in that test.

alexpott’s picture

realised we had some left over config entity test coverage in \Drupal\Tests\Core\Entity\TypedData\EntityAdapterUnitTest::testGetIterator() that's no longer relevant.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 102: 1818574-2-102.patch, failed testing. View results

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC

tedbow’s picture

Plus 1! This looks great!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 102: 1818574-2-102.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
15.4 KB
Using index info to reconstruct a base tree...
M	core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/EntityDeriver.php
Falling back to patching base and 3-way merge...
Auto-merging core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/EntityDeriver.php

Thanks git!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 107: 1818574-2-107.patch, failed testing. View results

Mixologic’s picture

Status: Needs work » Reviewed & tested by the community

Testbot Snafu.

fago’s picture

You can't create a configuration entity without an ID and use schema because we expect the ID when we resolve a schema definition for example taxonomy.vocabulary.*

I see. I guess the default value application could just not touch the ID and keep it. Not ideal, but I don't see any harm in it.

But anyway, there is a follow-up to discuss this in detail which is linked in the patch + the rest of the patch is great, so I think we should move on with this!

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/tests/Drupal/KernelTests/Core/Entity/ConfigEntityAdapterTest.php
@@ -0,0 +1,176 @@
+    parent::setUp();
+    $this->installConfig(static::$modules);
+
+    // ConfigTest::create doesn't work with the following exception:
+    // "Multiple entity types found for Drupal\config_test\Entity\ConfigTest."
+    $this->entity = \Drupal::entityTypeManager()->getStorage('config_test')->create([

Is this a problem that needs following up, or are we just documenting something we don't care about? It's not clear from the comment which it is.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

That just means we can't use the <EntityTypeName>::create() easy-DX-shortcut. This is why for a while now, we've been refraining from doing for example Node::create() or Node::load(), instead preferring $node_storage->create() and $node_storage->load(). Because that pattern works always. And that's exactly what's happening here, and documented in the code you cited.

Berdir’s picture

I wouldn't say it's wrong, it is IMHO in non-injected places and test perfectly valid to use the ::create() shortcuts. But so is using the entity type manager/storage, I would even say that there is no reason for this comment, it does indeed sounds like a justification for something that doesn't need one.

For the record, the reason ConfigTest::create() doesn't work is because of some trickery in \config_test_entity_type_alter() that clones the entity type definition and then as a result we have two entity types with the same class.

andypost’s picture

the reason ConfigTest::create() doesn't work is because of some trickery in \config_test_entity_type_alter() that clones the entity type definition and then as a result we have two entity types with the same class.

There was issue about it filed years ago

Gábor Hojtsy’s picture

Adjusting issue credits. The test particularity will not be resolved in this issue as it is not introduced here. I think its fine to have a comment to that effect though.

  • Gábor Hojtsy committed 0daa2b5 on 8.6.x
    Issue #1818574 by tim.plunkett, alexpott, tedbow, dawehner, EclipseGc,...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

A lot of core committers worked on this, especially @alexpott, thanks! The fact that we are down to details of how we document particularities of the test implementation that preceeded this issue is great :) I am sure we can keep improving.

Committed and published the change record at https://www.drupal.org/node/2954182 :)

Wim Leers’s picture

OMG OMG! 🎉

That means #2300677: JSON:API POST/PATCH support for fully validatable config entities is finally unblocked again! 🎉🎉🎉🎉🎉

Status: Fixed » Closed (fixed)

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