Problem/Motivation
Over in #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form we need a field type to be able to nominate some config-dependencies to any field config instances that use that type.
Proposed resolution
Ideally we would be able to make FieldItemBase implement ConfigurablePluginInterface but because of the whole Typed Data Plugin Manager <-> Field Type Plugin manager interdependency where we can't use the field-type plugin manager to create a field item interface, and we can't boot a field item interface without an entity - we are forced to create a static method on the FieldItemInterface/FieldItemBase - in a similar fashion to what we do for schema etc.
We also need widgets and formatters to be able to do the same for Entity*Display config entities. One interesting thuing is that Views include formatters configurations too, so ultimately the dependencies should bubble up in Views config entities as well. Not sure Views is ready for that atm though.
Remaining tasks
Reviews
User interface changes
None
API changes
Only additions, FieldItemInteface::calculateDependencies added, base implementation in FieldItemBase.
Comment | File | Size | Author |
---|---|---|---|
#66 | 2271419.66.patch | 55.54 KB | alexpott |
#66 | 63-66-interdiff.txt | 5.58 KB | alexpott |
#63 | 2271419.63.patch | 57.05 KB | alexpott |
#63 | 58-63-interdiff.txt | 6.57 KB | alexpott |
#58 | 2271419.58.patch | 52.31 KB | alexpott |
Comments
Comment #2
yched CreditAttribution: yched commented"calculateDependencies()" method name lacks some context / namespacing to make it clear it's about *config* dependencies.
Also, feels weird to put CMI-related methods in FieldItemInterface / FieldItemBase, which should be agnostic about the fact that some fields come from config :-/.
Is there a way this could plug into the hooks / lifecycle of FieldConfig / FieldInstanceConfig entities instead ?
Comment #3
larowlanWe already do the same with block plugins, they implement ConfigurablePluginInterface.
Ideally we could do the same here but we can't because of reasons outlined in the issue summary.
Comment #4
larowlanFixes fails
Comment #5
andypostAwesome! also covered with tests
Comment #6
yched CreditAttribution: yched commentedWhat about first remark in #2 ?
Comment #7
andypost@yched
calculateDependencies()
is used by core already the same wayComment #8
yched CreditAttribution: yched commented@andypost : well, the rest of core uses calculateDependencies() as a method in ConfigEntityInterface, so the "config" context is implied.
This patch adds a method on FieldItemInterface that is just proxied to, and that is *not* an implementation of ConfigEntityInterface::calculateDependencies(), since FieldItems are not ConfigEntities. So the method could and IMO should be named whatever makes sense in context of the interface it's added to.
Still feel FieldItemInterface::calculateDependencies() weird. But I'll shut up now :-)
Comment #9
Berdirandypost was referring to ConfigurablePluginInterface I think, which is also not a config entity :)
I agree that the name is not optimal but I don't have any better ideas.
Comment #10
tim.plunkettI don't like the name of ConfigurablePluginInterface::calculateDependencies() either, I'd prefer it to be calculateConfigDependencies. We could either rename this one here, and ConfigurablePluginInterface in a follow-up, or just commit this RTBC patch and discuss a better name for both in a new issue.
Comment #11
andypostWe need this asap, suppose naming things are API change so needs own issue
Comment #13
yched CreditAttribution: yched commented@tim.plunkett : yep, I was also thinking that calculateDependencies was lacking context on ConfigurablePluginInterface too.
Same old : "secondary" interfaces, that are intended to be used in various business domains alongside various domain interfaces, should be polite and clearly namespace their method names - but I said in #8 I'd shut up :-).
So +1 for
- calculateDependencies() in ConfigEntityInterface
- calculateConfigDependencies() in both ConfigurablePluginInterface and FieldItemInterface here.
Comment #14
yched CreditAttribution: yched commentedMoving tags and status over from the duplicate #2350031: let field types, widgets & formatters expose configDependencies, that was created after the CMI "hard discussion" at DC AMS yesterday.
@alexpott suggested we'd do field types, widgets and formatters in the same issue, so adding the last two to the issue summary too.
Comment #15
alexpottSo I've do some work on making widgets and formatters expose their dependencies to the config system through the generic ConfigurablePluginInterface. This is problematic since the entity system has not leveraged any of the generic function to tied a configuration entity to a plugin yet (ConfigurablePluginInterface and EntityWithPluginCollectionInterface). If
Drupal\field\Plugin\PluginSettingsBase
implementsConfigurablePluginInterface
we have an awful lot of work to do. So the patch attached makesEntityDisplayBase
extendEntityWithPluginCollectionInterface
and splits upConfigurablePluginInterface
to add a newDependentPluginInterface
. This means we can use the genericPluginDependencyTrait
during a config save to add dependencies due to third party settings, the provider of the plugin and any dependencies defined in the definition. It also means we can lose custom code fromEntityDisplayBase::calculateDependencies
. The addition of third party setting providing modules is tested inDrupal\field_ui\Tests\ManageDisplayTest
I think given the state of the FieldUI and moment of release this is the best approach since I think it will take considerable time and changes to config structure to implement
ConfigurablePluginInterface
.We can do the same for fields.
Comment #16
alexpottComment #18
alexpottWow - so the rdf field tests were using non existing field formatters, Aggregator has an unmet dependency on entity_reference, and entity_test needs entity_reference installed in KernelTestBase tests.
Patch attached also makes field items implement the DependentPluginInterface. Need to add tests for this.
Comment #19
alexpottI'm also slightly concerned that I've broken how entity display's fallback but given the number of bugs that has found I'm not upset about it :)
Comment #20
swentel CreditAttribution: swentel commented@alexpott why would the fallback been broken, don't see it immediately.
Comment #21
alexpottBecause I look for plugins using the #type - if they don't exist it throws an exception - hence all the tests that suddenly broke.
Comment #22
alexpottWhat needs to happen is that WidgetPluginManager and FormatterPluginManager should implement FallbackPluginManagerInterface
Comment #23
alexpottBut also EntityDisplayBase::onDependencyRemoval() makes the fallback ability somewhat obsolete as you never should have a non existing plugin configured in your entity display config now.
Comment #25
alexpottFix some of the test failures.
Comment #26
alexpottComment #28
alexpottShould be green - this change has found bugs all over the place :)
Comment #30
alexpottWow this issue has uncovered more than it's fair share of bugs. It has exposed an issue with the assumptions made in the ConfigDependencyManager and the how arrays are sorted following a merge. This is exposed because now both the field and field storage depend on the field type providing module. Before only the field storage did but since the field depends on the field storage there was a second hand dependency on the module.
Comment #32
alexpottMissed one
Comment #33
alexpottAdded a test so show that field type plugins can add dependencies to configuration.
Comment #34
alexpottPatch attached implements dependencies for default values for entity reference fields - it works but it is super ugly. See code for why:
Comment #36
alexpottHere is an alternative approach using a static method on FieldItemBase since we are never going to be able to instantiate a field type plugin on it's own in 8.x
Fixes
Drupal\migrate_drupal\Tests\d6\MigrateFieldInstanceTest
- migrate was adding an empty default value.Comment #37
yched CreditAttribution: yched commentedOuch. Doesn't that rather mean we should have a FieldDefinitionInterface::getRawLitteralDefaultValue() ?
Currently you set litteral default values or a default value callback, but there's no API way to *get* those. The only API we have is getDefaultValue(), which processes the above (litteral or callback) for runtime computation of "the default value to use for a given $entity".
That sounds like an issue of its own ? which would mean leaving the "entity ref default values" thing out of this issue ?
Not fully sure what the 'widgets' key corresponds to ?
Will be interesting when combining this with #1875974: Abstract 'component type' specific code out of EntityDisplay, which abstracts out "component types" within an EntityDisplay ("Field API fields" using 'widget'/'formatter' plugins being just one of the possible component types, DisplaySuite adding its own component type using its own supporting plugin type)
Also, FWIW, I still hope to internally refactor the list of instanciated widgets/formatters to an actual PluginCollection at some point (PluginCollections/PluginBags were introduced after EntityDisplays were written).
Wicked - you typehint TypedDataManager as a FieldTypePluginManagerInterface ?
Could we add a @todo on "TypedDataManager should have its own TDMInterface" ? :-)
- $instance, in this specific code here, is a FieldItemInterface; given our, hem, complex history with the word "instance" in Field API, could we name it $item for clarity ?
- $this->calculatePluginDependencies() already does an "$instance instanceof DependentPluginInterface" check, so do we also need to do it here ?
- Related : patch also adds DependentPluginInterface to FieldItemBase, with a default empty implementation of calculateDependencies(). In practice, I don't expect any FieldItem class to not extend FieldItemBase. So why not add DependentPluginInterface directly to FieldItemInterface (keeping the default impl. in FieldItemBase)
- There are a couple places where we have to do similar "create a dummy Item just so that we can call some non-static method for the field type". I'd be totally up for promoting such a method as FieldDefinitionInterface::createDummyItem().
Probably not for this issue though...
Still sad that this is not correctly namespaced to calculateConfigDependencies() :-/
Now we have TypedData *and* Config putting "secondary API" methods in there not related to the primary job of "being a field item".
Shouldn't this be also added to ThirdPartySettingsTrait ?
Comment #38
yched CreditAttribution: yched commentedHm - #37 crossposted with latest patch in #36, that made some of the remarks moot.
Yeah - FieldItems / FieldItemLists without a parent entity are hard to support :-/
Comment #40
alexpottre #37
Added tests and fixed tests.
Comment #42
yched CreditAttribution: yched commentedOoh, cliffhanger :-)
Hmmmmmmmmyeah... Saying "we'll pretend this can be used for many different things that we thus won't bother to further clarify" makes more confusing APIs than "this is used in this context by this subsystem".
Anyway. That method name is already in HEAD, battle lost.
Down to nitpicks :
Suggestion:
"Add dependencies from the 'field type' plugin. We can not use self::calculatePluginDependencies() because instanciation a plugin (i.e. a field item) requires a parent entity" ?
Add a "Let the field type specifiy its own dependencies" comment above that line ?
And maybe move the existing "If the target entity type uses entities to manage its bundles..." comment at the toplevel of the next, last section ?
And maybe harmonize empty-line spacing ? The code in that method is a bit weird visually.
Still needed ?
OK, this is copy pasted from DependentPluginInterface, since FieldItem clases need a static method and thus don't implement that interface.
The text should probably be adjusted though. As-such, it makes little sense in the context of FieldItemInterface ?
The wording is a bit weird, at first I thought the beginning of the sentence was missing :-)
Also, we don't need to care about default value callbacks for dependencies (a callback lives in code, not relevant for config dependencies). So maybe no need to mention them here ?
Ternary ?
Arguably slightly clearer :
Comment #43
alexpottre #37.1 we can handle that in #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods
re #42
Comment #45
yched CreditAttribution: yched commentedThanks !
Looks good (oops, aside from fails), but :
This is a fix for #2221577: Fix assumption that field settings is not a nested array, which we'd need to sort out first. Although, sorry, as explained in my last comment over there, this issue is now a can of worms in my head :-/
Do we really/still need that change here ?
Comment #46
alexpottre #45 @yched - yep we do - if we don't this issue exposes the bug. Without this fix
Drupal\aggregator\Tests\FeedCacheTagsTest
for example.And the fails in the latest patch are not reproducible locally :(
Comment #47
alexpottFixed test failure - it was full only failed on testbot due to the way different versions of php order arrays :)
Removed change from
TypedDataManager::getPropertyInstance()
- not 100% sure if change is still required.Comment #49
alexpottSo it looks like we do need the fix to
TypedDataManager::getPropertyInstance()
.This is happening because of the following method in ConfigurableEntityReferenceItem
This causes the settings array in
TypedDataManager::getPropertyInstance()
to be:which fails in HEAD.
This issue is exposing this because
EntityFieldTest
now installs the entity_reference module because EntityTest actually has a dependency on theentity_reference
module that is unmet in HEAD.Comment #50
yched CreditAttribution: yched commentedJust wondering : why is EntityWithPluginCollectionInterface added to EntityDisplayBase rather than EntityDisplayInterface ?
(EntityDisplayInterface already extends ConfigEntityInterface)
Comment #51
yched CreditAttribution: yched commentedWell, that issue is about FieldConfig, we would need that API on FieldDefinitionInterface for all fields ?
Comment #52
alexpottre #51 yep which why #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods will be a complex beast
Comment #53
alexpottRerolled after #2221577: Fix assumption that field settings is not a nested array landed and moved interface as per #50. Thanks for all the review @yched
Comment #54
yched CreditAttribution: yched commentedOne final round of review, sorry :-/
Nit : ConfigurablePluginInterface is not needed anymore.
Uber-nit - would be clearer as :
ConfigEntityinterface, EntityWithPluginCollectionInterface, ThirdPartySettingsInterface ?
It is a EntityWithPluginCollection because, first of all, it's a (Config)Entity ?
So - this is a newcomer in the family of:
static FieldItemInterface::schema(FDI $field_definition)
static FieldItemInterface::propertyDefinitions(FDI $field_definition)
static FieldItemInterface::defaultFieldSettings()
static FieldItemInterface::defaultStorageSettings()
aka "the 'per field type' info for which we don't want to have to instantiate a FieldItem object with a parent entity".
For consistency:
- Can we rename the $definition param to $field_definition ?
- (For clarity for implementors, would be nice to group those static methods together in FieldItemInterface - but the current order is a mess anyway, so that's for a followup)
- The current naming pattern is that those methods have no verb ("get"), because they're the "raw source", and that some other "official API getter" somewhere is in charge of collecting/massging that data:
FieldStorageDefinitionInterface::getSchema(),
ComplexDataDefinitionInterface::getPropertyDefinitions(),
FieldTypePluginManagerInterface::getDefaultFieldSettings(),
FieldTypePluginManagerInterface::getDefaultStorageSettings(),
Accordingly, should this one here be just "static dependencies()",
with the official getter being ConfigEntityInterface::calculateDependencies() ?
Comment #55
alexpottThanks @yched! re #54
FieldItemInterface::calculateDependencies()
method alone - it should be kept in-sync with DependentPluginInterface so if we ever can use calculatePluginDependencies we have less to change.Comment #56
yched CreditAttribution: yched commentedWell, as long as "the instanciated field type plugins" are "the field value objects that need a parent entity", which I don't see changing at any point in D8, the current situation where FieldItemBase cannot implement DependentPluginInterface, and dependecies need to be fetched using a different, static method, is likely here to stay ?
Anyway, no biggie. Bumping to RTBC.
Thanks a lot Alex !
Comment #58
alexpottMinor conflict with #2364337: $typedConfigManager dinamically defined in FieldConfigEntityUnitTest
Comment #59
jibranExtra space. Can be fixed while committing.
Comment #60
catchnit- why count() and not !empty() - I'm always looking for a comparison when I see count().
+ return $dependencies;
+ }
+
This looks like it leaves open a situation where the entity is specified in dependencies, does not exist, and then we're calling getConfigDependencyName() on FALSE.
Looks like it's missing an if/else on the return value of loadEntityByUuid() and then I'm not sure what happens in the else {. The dependency can't be calculated in that case so it feels like it'd have to be an exception otherwise it'd get lost. However we're allowing content dependencies to be looser handled than config, so that probably isn't an option.
Only thing that occurred to me was changing getConfigDependencytName() to be a static method on the entity class so we don't have to load anything and pass the entity type + uuid into that - that would allow the content dependencies to be preserved even if the content itself is not there.
Comment #61
yched CreditAttribution: yched commentedThe fact that we have to load the entities first before being able to set them as dependencies bugged me a bit as well. Probably no biggie for "default values", since there will not be 100s of them on a field, but might be more of an issue when scaled to the whole config tree ?
Comment #62
alexpott@catch that is a superb point - during an import this information would be lost if the content entity does not exist. The problem with not loading them is that we can not provide the bundle information - which will be important if creating stubs or mock content. And this is a problem with the static method idea too - it is just not for sure that we have even the entity type let alone the bundle.
It seems as though want we need is some way that content dependencies can be preserved in special situations - a run time we don't want that but during import we do.
Comment #63
alexpottSo wow... I forgot.
So we will not overwrite or recalculate dependencies on import (phew) which means we can work out missing content dependencies after the config entities have been saved over in #2361423: Add a step to config import to allow contrib to create content based on config dependencies
I've added a test assertion that makes this type of thing really easy to test.
I've also added an exception for the situation when you save an entity reference field with a default value that does not exist.
Comment #64
yched CreditAttribution: yched commentedThe Exception scares me a bit.
If an entity is used as a default value and is deleted at some point, the field will continue to work (because at runtime EntityRefFieldItemList::processDefaultValue() filters out non-existing entities), but saving the FieldConfig will break ?
Or, in a scenario with config sync:
- on dev, a field uses a local existing entity as a default value
- on sync onto prod, with just Core (no stubbing for missing content dependencies), the field is deployed and runs fine (without the default value), but then the prod breaks next time the FieldConfig is saved ?
Not sure which would be best - break with an exception or silently drop the dependency ?
Comment #65
alexpottWell if EntityRefFieldItemList::processDefaultValue() filters out non-existing entities then the answer is clear we should remove dependency - patch incoming.
Comment #66
alexpottHere is a patch.
Comment #67
yched CreditAttribution: yched commentedThanks Alex. Looks good to me.
Just wondering :
Agreed that we need to put the bundle name in content dependency names, and thus that we need to load the entities when building the dependencies on ConfigEntity::save(). This is only viable because we don't do it during config sync of the snippet mentioned in #63 (only calculate depencies if not syncing).
Maybe we should refine the comment there (the current comment only says "we don't need to do it"), and maybe add an explicit test for that, so that it doesn't get changed unknowingly at some point in the future ?
Not a blocker for that issue here though.
Comment #68
alexpott#67 the patch includes some test coverage to ensure that these content dependencies are not removed :)
I agree that we should open a follow-up to improve the comment and find a way to unit test this - created #2366899: Improve comment about not calculating dependencies whist syncing config and add a unit test to prove it
Comment #69
catchDoesn't this issue enable handling that differently though? i.e. remove the entity from the default values, or prevent deletion of the entity? We don't have to do that here but I don't think just silently removing dependencies is a good idea - why bother having the dependencies at all?
Comment #70
yched CreditAttribution: yched commentedWell, that would mean being able run a query like "find all the config entities that have a dependency on this content entity". Is that even doable ?
Referential integrity is a slippery slope :-/
Comment #71
alexpottYep that is doable.
Personally I think that we have to accept that content dependencies are a little bit "softer" than the others.
Now that EntityReferenceItem is doing the same as EntityRefFieldItemList::processDefaultValue() any chance we can punt the issue of exactly what to do here to a follow up?
Comment #72
alexpottYep that is doable.
Personally I think that we have to accept that content dependencies are a little bit "softer" than the others.
Now that EntityReferenceItem is doing the same as EntityRefFieldItemList::processDefaultValue() any chance we can punt the issue of exactly what to do here to a follow up?
Comment #73
yched CreditAttribution: yched commentedI'd tend to leave it be for now too.
- We have no unified way to prevent UI deletion of content entities based on some 3rd party criteria ?
- Starting to raise exceptions on API $entity->delete() seems like a non-minor DX change at this point ?
Yep, that was the idea in Amsterdam IIRC.
@catch, what do you think ?
(not sure i should play RTBC ping-pong to push that back in committerland)
Comment #74
catchI'm OK deciding exactly what to do in a follow-up - is there already an issue?
Comment #75
alexpottIt does now #2367317: Decide what to do about entity reference content dependencies if the content does not exist - setting to rtbc given #73 and #74.
Comment #76
catchCommitted/pushed to 8.0.x, thanks!
Comment #78
amateescu CreditAttribution: amateescu commentedI opened #2369933: Remove references to an exception class that doesn't exist (\Drupal\entity_reference\Exception\MissingDefaultValueException) as a small follow-up.