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
Comment | File | Size | Author |
---|---|---|---|
#107 | 1818574-2-107.patch | 15.4 KB | alexpott |
Comments
Comment #1
fagoWith #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?
Comment #2
yched CreditAttribution: yched commentedSo, #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 :-)
Comment #3
andypostSuppose 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
Comment #4
fago>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.
Comment #5
gddComment #6
andypostHow much work it require for each entity type? Storage, Forms,
Renderand Entity itself?Comment #7
tim.plunkettI 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.
Comment #9
tim.plunkettTagging
Comment #10
xjm@tim.plunkett and I agreed this should probably be major, since it affects a whole host of different entity types.
Comment #11
tim.plunkettIdeally #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?
Comment #13
tim.plunkettWhoops, I changed the upstream controller, not the new copy. I'll revisit this tomorrow.
Comment #14
tim.plunkettSo 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: 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.
Comment #16
fagoYeah, 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.
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?
Comment #17
fagoFor what would we use BC-mode here? If possible I'd go with a straight conversion as the BC-mode is not uncomplicated.
Comment #18
tim.plunkettParts 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.
Comment #20
tim.plunkettAh, had to update for #1826602: Allow all configuration entities to be enabled/disabled.
Comment #21
tim.plunkettYay! It passed!
Here's my commentary on the changes made here:
These are all of the indirect changes that I'd like some real review on. The rest is pretty standard stuff.
system.module provides the TypedData types needed, so it has to be added to the unit tests.
Comment #22
fagook, I did quickl review the patch. I've not looked into details for now, but more on the general architecture.
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.
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.
That mapping from data to storage format should be the job of the entity storage layer.
Comment #23
EclipseGc CreditAttribution: EclipseGc commentedThis 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
Comment #25
tim.plunkettWe 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.
Comment #26
tim.plunkettRerolled 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.
Comment #28
tim.plunkettAdded the new ConfigEntityInterface::setStatus() to ConfigEntityNGBase.
Comment #30
tim.plunkettGrrr. Forgot to rebase first. No changes.
Comment #31
moshe weitzman CreditAttribution: moshe weitzman commentedPatch 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.
Comment #32
andypostBut each configurable has limited set of properties to save. So maybe better to use them
Please add a comment why this could be not an array!
so now there's no empty properties any more?
It makes harder to debug if values are wrong
maybe better to use ->target_id here?
this changed to make sure that BC works?
Comment #33
moshe weitzman CreditAttribution: moshe weitzman commentedStill looking for brave helper to finish this. Perhaps Tim can give guidance. REST would also gain access to shortcut sets, user roles, text formats, ...
Comment #34
tim.plunkettI'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.
Comment #35
tim.plunkett#1978870: Add an EntityStorageControllerBase
Comment #36
benjifisher#30: config-typeddata-1818574-30.patch queued for re-testing.
Comment #39
jrglasgow CreditAttribution: jrglasgow commentedComment #40
jrglasgow CreditAttribution: jrglasgow commentedrerolled the patch - hopefully it will apply now
Comment #41
jrglasgow CreditAttribution: jrglasgow commentedComment #43
star-szrThis needs another reroll.
Comment #44
star-szrTag didn't stick, trying again.
Comment #45
Xano#40: config-typeddata-1818574-40.patch queued for re-testing.
Comment #47
fagoHere 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
Comment #48
star-szr@fago - patch missing?
Comment #49
BerdirNo 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?
Comment #50
pfrenssenIf 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 :)
Comment #51
BerdirWith #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.
Comment #52
moshe weitzman CreditAttribution: moshe weitzman commentedGreat news! Would be huge to get this one in. We could then support REST operations on config entities
Comment #53
xjmIt'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
Comment #58
dawehnerThere 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.
Comment #59
tedbowIt 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.
Comment #61
tedbowWhoops forgot @group
Comment #62
dawehnerI think one important part should be calling to validate() and ensuring that the basic data types are validated.
Comment #64
tedbowThis should fix some but not all of test failures.
Comment #65
tedbow#62 agreed.
The previous patch set the
weight
property to a string which did produce an error invalidate()
but it wasn't checking the message to see if it was correct. Adding that.Comment #66
tedbowI messed when I updated
\Drupal\Core\Entity\Plugin\DataType\EntityAdapter::getIterator
and I wasn't returning a value.Comment #69
Berdirnot 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?
similar to here, but probably if/elseif?
Comment #70
dawehnerLooking 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.
Comment #71
tedbow@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
andConfigEntityAdapter
as @internal? Or at least\Drupal\Core\Entity\Plugin\DataType\EntityAdapter::createFromEntity()
? Outside code should always be using\Drupal\Core\Entity\Entity::getTypedData()
correct? SinceEntityAdapter::createFromEntity()
wasn't internal and before this patch it would have been effectively the same as callinggetTypedData()
.Comment #72
dawehner@tedbow
You asked some good questions. Ideally fago would answer how the API was planned to be used.
Comment #73
alexpottI 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.
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()?
Comment #74
dawehnerThat sounds like a good idea!
Comment #75
Wim LeersSo 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…
Comment #76
alexpottI've tried working on a test that calls
ConfigEntityAdapter::applyDefaultValue()
. This brings up a couple of problems.::set()
methods do.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.
Comment #77
alexpottHere's a patch that implements the exception.
Comment #78
dawehnerCouldn't we copy the values from the properties object to the entity on every onChange, which is probably fired on
applyDefaultValues
?Comment #79
dawehnerHere are just some minor fixes.
Comment #80
alexpottRe #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.
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.
Comment #81
tim.plunkettNit:
$entity_type->entityClassImplements(ConfigEntityInterface::class)
is more correctCan KTB also use @coversDefaultClass and @covers? I think that would be preferable.
Comment #82
alexpottThanks 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.
Comment #83
dawehnerNice! It would be nice to have @fago reviewing these issues, as always ...
Comment #84
Wim LeersAs of #2907163-32: Split validation-related methods from FieldableEntityInterface into a separate ValidatableInterface, #2907163 is blocked on this. Both that issue and this one are blockers of #2300677: JSON:API POST/PATCH support for fully validatable config entities.
Comment #85
dawehnerShould we open up a follow up to explore that but keep it like that for now?
Comment #86
Wim Leers+1
Comment #87
dawehnerHere is the issue: #2929042: Optimise ConfigEntityType::getConfigTypedData()
Comment #89
fagoAwesome to see this coming along so well! Patch looks already great!
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.
1.
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.
Comment #90
dawehner👏👏👏
I created a follow up to deal with that. Default values could be nice for some reasons.
Well, this is inherited by
\Drupal\Core\TypedData\TypedDataTrait
Comment #92
dawehnerLet's fix the remaining failures.
Comment #93
alexpottI think this is ready. It'd be great to link to the follow-up @dawehner talked about in #90.
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.*
Comment #94
Wim LeersOMG OMG OMG! Yay, thanks, @alexpott! Config entity validation and hence REST support can now move forward again!
Comment #95
dawehnerSure: Here we go: #2945635: Figure out how to deal with applyDefaultValue on ConfigEntityAdapter
Comment #96
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis patch looks great! Just some minor things:
Needs to return
$this
.This is great. Can we also remove the same comment from
get()
?Should we add an assertion that
$adapter
is returned? If so, should we also add a similar assertion toEntityAdapterUnitTest::testSet()
as well?Also, this issue is still tagged with "Needs issue summary update", which I think is accurate. Anyone up for doing that?
Comment #97
alexpottPatch 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.
Comment #98
alexpottI've started on a CR - perhaps someone else can flesh it out.
Comment #99
dawehnerThank you @effulgentsia for the review. Thank you @alexpott for addressing it. I expanded the CR with one example usage.
Comment #100
jibranThis is kind of config schema validation? If yes then we should verify all the config entities schema.
Why not use the EntityAdapter object directly in the loop? It will verify the
::getIterator()
method as well.Comment #101
alexpottRe #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.
Comment #102
alexpottrealised we had some left over config entity test coverage in \Drupal\Tests\Core\Entity\TypedData\EntityAdapterUnitTest::testGetIterator() that's no longer relevant.
Comment #104
dawehnerBack to RTBC
Comment #105
tedbowPlus 1! This looks great!
Comment #107
alexpottThanks git!
Comment #109
MixologicTestbot Snafu.
Comment #110
fagoI 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!
Comment #111
catchIs 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.
Comment #112
Wim LeersThat 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 exampleNode::create()
orNode::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.Comment #113
BerdirI 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.
Comment #114
andypostThere was issue about it filed years ago
Comment #115
Gábor HojtsyAdjusting 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.
Comment #117
Gábor HojtsyA 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 :)
Comment #118
Wim LeersOMG OMG! 🎉
That means #2300677: JSON:API POST/PATCH support for fully validatable config entities is finally unblocked again! 🎉🎉🎉🎉🎉