Allow field types, widgets, formatters to specify config dependencies

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

Awesome! also covered with tests

What about first remark in #2 ?

@yched calculateDependencies() is used by core already the same way

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

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

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

We need this asap, suppose naming things are API change so needs own issue

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

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

So 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 implements ConfigurablePluginInterface we have an awful lot of work to do. So the patch attached makes EntityDisplayBase extend EntityWithPluginCollectionInterface and splits up ConfigurablePluginInterface to add a new DependentPluginInterface. This means we can use the generic PluginDependencyTrait 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 from EntityDisplayBase::calculateDependencies. The addition of third party setting providing modules is tested in Drupal\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.

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

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

@alexpott why would the fallback been broken, don't see it immediately.

Because I look for plugins using the #type - if they don't exist it throws an exception - hence all the tests that suddenly broke.

What needs to happen is that WidgetPluginManager and FormatterPluginManager should implement FallbackPluginManagerInterface

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

Fix some of the test failures.

Should be green - this change has found bugs all over the place :)

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

Missed one

Added a test so show that field type plugins can add dependencies to configuration.

Patch attached implements dependencies for default values for entity reference fields - it works but it is super ugly. See code for why:

  /**
   * {@inheritdoc}
   */
  public function calculateDependencies() {
    $dependencies = [];

    // Does not work - it turns out that that the way the plugin is constructed
    // in FieldConfigBase::calculateDependencies does not provide a parent
    // so lots of methods will not work.
    // $field_definition = $this->getFieldDefinition();
    $field_definition = $this->definition->getFieldDefinition();
    // Public access of default_value and we're not using
    // $field_definition->getDefaultValue() because we don't have an entity
    // which is problematic because we're ignoring default value callbacks. But
    // since callback can vary depending on the entity perhaps they should not
    // by supported anyway.
    if (is_array($field_definition->default_value) && count($field_definition->default_value)) {
      $target_entity_type = \Drupal::entityManager()->getDefinition($field_definition->getFieldStorageDefinition()->getSetting('target_type'));
      if ($target_entity_type instanceof ConfigEntityType) {
        $key = 'config';
      }
      else {
        $key = 'content';
      }
      $dependencies = [
        $key => []
      ];
      foreach ($field_definition->default_value as $uuid) {
        $entity = \Drupal::entityManager()->loadEntityByUuid($target_entity_type->id(), $uuid);
        $dependencies[$key][] = $entity->getConfigDependencyName();
      }
    }
    return $dependencies;
  }

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

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -242,4 +243,39 @@ public function hasUnsavedEntity() {
   +    // Does not work - it turns out that that the way the plugin is constructed
   +    // in FieldConfigBase::calculateDependencies does not provide a parent
   +    // so lots of methods will not work.
   +    // $field_definition = $this->getFieldDefinition();
   +    $field_definition = $this->definition->getFieldDefinition();
   +    // Public access of default_value and we're not using
   +    // $field_definition->getDefaultValue() because we don't have an entity
   +    // which is problematic because we're ignoring default value callbacks. But
   +    // since callback can vary depending on the entity perhaps they should not
   +    // by supported anyway.
   +    if (is_array($field_definition->default_value) && count($field_definition->default_value)) {
   

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

2. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
   @@ -243,4 +244,23 @@ public function __wakeup() {
   +    return array(
   +      'widgets' => new EntityDisplayPluginCollection($this->pluginManager, $configurations)
   +    );
   

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

3. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -228,6 +229,12 @@ public function getTargetBundle() {
   +    /** @var $typed_data_manager \Drupal\Core\Field\FieldTypePluginManagerInterface */
   +    $typed_data_manager = \Drupal::service('typed_data_manager');
   

   Wicked - you typehint TypedDataManager as a FieldTypePluginManagerInterface ?
   Could we add a @todo on "TypedDataManager should have its own TDMInterface" ? :-)

4. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -228,6 +229,12 @@ public function getTargetBundle() {
   +    $instance = $typed_data_manager->create($this->getItemDefinition());
   +    if ($instance instanceof DependentPluginInterface) {
   +      $this->calculatePluginDependencies($instance);
   +    }
   

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

5. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
   @@ -289,4 +290,11 @@ public static function fieldSettingsFromConfigData(array $settings) {
   +  public function calculateDependencies() {
   

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

6. +++ b/core/lib/Drupal/Core/Field/PluginSettingsBase.php
   @@ -108,4 +109,17 @@ public function setThirdPartySetting($module, $key, $value) {
   +  public function calculateDependencies() {
   +    if (!empty($this->thirdPartySettings)) {
   +      // Create dependencies on any modules providing third party settings.
   +      return array(
   +        'module' => array_keys($this->thirdPartySettings)
   +      );
   +    }
   +    return array();
   +  }
   

   Shouldn't this be also added to ThirdPartySettingsTrait ?

Hm - #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 :-/

re #37

1. yep it does - but we can handle that in
2. it corresponds to a property on the config entity - usually - but here we don't use it - although once you do #1875974: Abstract 'component type' specific code out of EntityDisplay you probably should. This is part of that refactor :)
3. Patch in #36 made this moot
4. Patch in #36 made this moot
5. well yes and no - these are the dependencies regardless of whether its for config or some other purpose
6. Nope this is a plugin - the trait is for config entities

Added tests and fixed tests.

re #37 1. yep it does - but we can handle that in

Ooh, cliffhanger :-)

re #37 5. well yes and no - these are the dependencies regardless of whether its for config or some other purpose

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 :

1. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -228,6 +229,19 @@ public function getTargetBundle() {
   +    // Add dependencies from field item. We can not use
   +    // self::calculatePluginDependencies() because instantiating a field type
   +    // plugin without the entity is not possible.
   ...
   +    // Plugins can declare additional dependencies in their definition.
   

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

2. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -228,6 +229,19 @@ public function getTargetBundle() {
   +    $this->addDependencies($definition['class']::calculateDependencies($this));
   

   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.

3. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
   @@ -26,6 +26,13 @@
      /**
   +   * The data definition.
   +   *
   +   * @var \Drupal\Core\Field\TypedData\FieldItemDataDefinitionInterface
   +   */
   +  protected $definition;
   +
   +  /**
   

   Still needed ?

4. +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
   @@ -372,4 +372,31 @@ public function storageSettingsForm(array &$form, FormStateInterface $form_state
   +  /**
   +   * Calculates dependencies for the configured plugin.
   +   *
   +   * Dependencies are saved in the plugin's configuration entity and are used to
   

   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 ?

5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -242,4 +243,36 @@ public function hasUnsavedEntity() {
   +    // Public access of default_value and we're not using
   +    // $definition->getDefaultValue() because we don't have an entity
   +    // which is problematic because we're ignoring default value callbacks. But
   +    // since callback can vary depending on the entity perhaps they should not
   +    // by supported anyway.
   

   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 ?

6. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -242,4 +243,36 @@ public function hasUnsavedEntity() {
   +      if ($target_entity_type instanceof ConfigEntityType) {
   +        $key = 'config';
   +      }
   +      else {
   +        $key = 'content';
   +      }
   

   Ternary ?

7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -242,4 +243,36 @@ public function hasUnsavedEntity() {
   +      $dependencies = [
   +        $key => []
   +      ];
   +      foreach ($definition->default_value as $default_value) {
   +        if (is_array($default_value) && isset($default_value['target_uuid'])) {
   +          $entity = \Drupal::entityManager()->loadEntityByUuid($target_entity_type->id(), $default_value['target_uuid']);
   +          $dependencies[$key][] = $entity->getConfigDependencyName();
   +        }
   +      }
   +    }
   

   Arguably slightly clearer :

   $names = array();
   foreach (...) {
     $names[] = ..;
   }
   return array(
     $key => $names;
   }
   

re #37.1 we can handle that in #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods

re #42

1. Fixed
2. Fixed
3. Fixed
4. Fixed
5. Fixed
6. Good idea fixed
7. Simplified it even further i think

Thanks !

Looks good (oops, aside from fails), but :

+++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
@@ -255,7 +255,7 @@ public function getPropertyInstance(TypedDataInterface $object, $property_name,
-      $key .= ':' . implode(',', $settings);
+      $key .= ':' . serialize($settings);

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 ?

re #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 :(

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

So it looks like we do need the fix to TypedDataManager::getPropertyInstance().

This is happening because of the following method in ConfigurableEntityReferenceItem

  /**
   * {@inheritdoc}
   */
  public static function defaultFieldSettings() {
    return array(
      'handler_settings' => array(),
    ) + parent::defaultFieldSettings();
  }

This causes the settings array in TypedDataManager::getPropertyInstance() to be:

array(
  'target_type' => 'entity_test',
  'handler_settings' => array(),
  'handler' => 'default',
);

which fails in HEAD.

This issue is exposing this because EntityFieldTest now installs the entity_reference module because EntityTest actually has a dependency on the entity_reference module that is unmet in HEAD.

Just wondering : why is EntityWithPluginCollectionInterface added to EntityDisplayBase rather than EntityDisplayInterface ?
(EntityDisplayInterface already extends ConfigEntityInterface)

re #37.1 [no API for "get the litteral (non-callback non-runtime) default values for a FieldDefinition] we can handle that in #2030637: Expand FieldInstance with methods

Well, that issue is about FieldConfig, we would need that API on FieldDefinitionInterface for all fields ?

re #51 yep which why #2030637: Expand FieldConfig/BaseFieldOverride/FieldConfigBase with methods will be a complex beast

Rerolled 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

One final round of review, sorry :-/

1. +++ b/core/lib/Drupal/Core/Plugin/PluginDependencyTrait.php
   @@ -8,6 +8,7 @@
    use Drupal\Component\Plugin\ConfigurablePluginInterface;
   +use Drupal\Component\Plugin\DependentPluginInterface;
   

   Nit : ConfigurablePluginInterface is not needed anymore.

2. +++ b/core/lib/Drupal/Core/Entity/Display/EntityDisplayInterface.php
   @@ -9,11 +9,12 @@
   +interface EntityDisplayInterface extends EntityWithPluginCollectionInterface, ThirdPartySettingsInterface, ConfigEntityInterface {
   

   Uber-nit - would be clearer as :
   ConfigEntityinterface, EntityWithPluginCollectionInterface, ThirdPartySettingsInterface ?

   It is a EntityWithPluginCollection because, first of all, it's a (Config)Entity ?

3. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
   @@ -289,4 +289,11 @@ public static function fieldSettingsFromConfigData(array $settings) {
   +  public static function calculateDependencies(FieldDefinitionInterface $definition) {
   

   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() ?

Thanks @yched! re #54

1. Fixed
2. Fixed
3. Fixed the parameter name - I would leave the FieldItemInterface::calculateDependencies() method alone - it should be kept in-sync with DependentPluginInterface so if we ever can use calculatePluginDependencies we have less to change.

if we ever can use calculatePluginDependencies we have less to change

Well, 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 !

Minor conflict with #2364337: $typedConfigManager dinamically defined in FieldConfigEntityUnitTest

+++ b/core/modules/field/tests/src/Unit/FieldConfigEntityUnitTest.php
@@ -61,6 +62,13 @@ class FieldConfigEntityUnitTest extends UnitTestCase {
+  ¶

Extra space. Can be fixed while committing.

+    if (is_array($field_definition->default_value) && count($field_definition->default_value)) {
+      $target_entity_type = \Drupal::entityManager()->getDefinition($field_definition->getFieldStorageDefinition()->getSetting('target_type'));

nit- why count() and not !empty() - I'm always looking for a comparison when I see count().

+        if (is_array($default_value) && isset($default_value['target_uuid'])) {
+          $entity = \Drupal::entityManager()->loadEntityByUuid($target_entity_type->id(), $default_value['target_uuid']);
+          $dependencies[$key][] = $entity->getConfigDependencyName();
+        }

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

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.

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

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

So wow... I forgot.

    if (!$this->isSyncing()) {
      // Ensure the correct dependencies are present. If the configuration is
      // being written during a configuration synchronisation then there is no
      // need to recalculate the dependencies.
      $this->calculateDependencies();
    }

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.

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

Well if EntityRefFieldItemList::processDefaultValue() filters out non-existing entities then the answer is clear we should remove dependency - patch incoming.

Here is a patch.

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

+++ b/core/modules/entity_reference/src/Tests/EntityReferenceIntegrationTest.php
@@ -139,25 +139,21 @@ public function testSupportedEntityTypesAndWidgets() {
+      $this->assertConfigEntityImport($field);

#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

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 ?

Doesn'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?

[on entity delete] remove the entity from the default values, or prevent deletion of the entity ?

Well, 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 :-/

"find all the config entities that have a dependency on this content entity"

Yep that is doable.

\Drupal::service('config.manager')->findConfigEntityDependents('content', array($entity->getConfigDependencyName()));

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?

"find all the config entities that have a dependency on this content entity"

Yep that is doable.

\Drupal::service('config.manager')->findConfigEntityDependents('content', array($entity->getConfigDependencyName()));

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?

I'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 ?

Personally I think that we have to accept that content dependencies are a little bit "softer" than the others

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)

I'm OK deciding exactly what to do in a follow-up - is there already an issue?

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

Committed/pushed to 8.0.x, thanks!

I opened #2369933: Remove references to an exception class that doesn't exist (\Drupal\entity_reference\Exception\MissingDefaultValueException) as a small follow-up.