Calculate configuration entity dependencies on install

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -341,7 +341,7 @@ public function preSave(EntityStorageInterface $storage) {
   -    if (!$this->isSyncing() && !$this->trustedData) {
   

   So, would the test fail without this change ? Because I can't find calls to trustData when creating an entity anywhere.

2. +++ b/core/modules/config/src/Tests/ConfigInstallTest.php
   @@ -7,6 +7,7 @@
   +use Drupal\config_test\Entity\ConfigTest;
   

   The use statement doesn't seem to be used, so can go away ?

Removed redundant use statement identified in #3, otherwise the original patch looks good.

Also to confirm the test does fail without the change.

Patch in #2 also currently fails testing locally when applied to current HEAD.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -301,8 +301,11 @@ public function hasNewEntity() {
+
+    $target_entity_type = \Drupal::entityManager()->getDefinition($field_definition->getFieldStorageDefinition()->getSetting('target_type'));
+    $dependencies['module'][] = $target_entity_type->getProvider();
+

This patch seems not to add test coverage for this bit?

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -341,7 +341,7 @@ public function preSave(EntityStorageInterface $storage) {
   -    if (!$this->isSyncing() && !$this->trustedData) {
   +    if (!$this->isSyncing()) {
   

   This is the key change. I've verified locally that there is test coverage: reverting this change causes the updated ConfigInstallTest to fail.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -301,8 +301,11 @@ public function hasNewEntity() {
   +    $target_entity_type = \Drupal::entityManager()->getDefinition($field_definition->getFieldStorageDefinition()->getSetting('target_type'));
   +    $dependencies['module'][] = $target_entity_type->getProvider();
   

   So this was a missing dependency.

   Isn't that technically out of scope for this issue? And agreed with @dawehner that test coverage for this is missing.

3. +++ b/core/modules/aggregator/config/install/core.entity_view_display.aggregator_item.aggregator_item.summary.yml
   @@ -5,7 +5,6 @@ dependencies:
   -    - entity_reference
   
   +++ b/core/modules/book/config/install/core.entity_form_display.node.book.default.yml
   @@ -5,7 +5,6 @@ dependencies:
   -    - entity_reference
   

   I don't understand what change in this patch can cause some dependencies to be omitted.

4. +++ b/core/profiles/standard/config/install/field.field.node.article.field_image.yml
   @@ -5,6 +5,7 @@ dependencies:
   +    - file
   
   +++ b/core/profiles/standard/config/install/field.field.node.article.field_tags.yml
   @@ -6,6 +6,7 @@ dependencies:
   +    - taxonomy
   

   I do understand that additional dependencies can appear.

5. +++ b/core/profiles/standard/config/install/field.storage.node.field_image.yml
   @@ -2,8 +2,8 @@ langcode: en
   -    - node
   ...
   +    - node
   

   I don't understand how this patch can cause listed dependencies to have a different order.

6. +++ b/core/profiles/standard/config/install/field.storage.node.field_tags.yml
   @@ -2,8 +2,8 @@ langcode: en
   +    - entity_reference
   ...
   -    - taxonomy
   

   I don't understand how the changes in this patch can cause dependencies to change.

Regarding entity_reference : this is why we really should proceed with #2429191: Deprecate entity_reference.module and move its functionality to core (which is 99% ready)

Atm, entity_reference.module is not needed for e_r base fields (Core now contains the full-featured e_r field type), but only to create configurable e_r fields. That is an arbitrary split at this point. (I think that explains the hunks mentioned by @Wim)

critical so the rc deadline tag is not needed.

filled out more in the summary. seems like no api or data model changes.

Seems ready to me.

Thank you alex!

This looks like it needs an upgrade path (or several actually) -- if there are all these config entities on existing sites that are missing dependencies, and furthermore shipped entities with wrong dependencies, then we also need to update those existing sites.

Also, some manual testing would be a really good idea, given the broad impact of this bug and fix.

What is a scenario that you would like to be tested?

Is #2574903: Examples should uninstall configuration a related item?

Looking at the update path.

There we go.

1. +++ b/core/modules/system/src/Tests/Update/RecalculatedDependencyTest.php
   @@ -0,0 +1,45 @@
   +  public function testUpdate() {
   +    $this->runUpdates();
   +
   +    $data = \Drupal::config('field.field.node.article.field_tags')->get();
   +    $this->assertEqual(['taxonomy'], $data['dependencies']['module']);
   +
   +    $data = \Drupal::config('field.field.user.user.user_picture')->get();
   +    $this->assertEqual(['file', 'image', 'user'], $data['dependencies']['module']);
   +
   +    $data = \Drupal::config('field.storage.node.field_image')->get();
   +    $this->assertEqual(['image', 'node'], $data['dependencies']['module']);
   +
   +    $data = \Drupal::config('field.storage.node.field_tags')->get();
   +    $this->assertEqual(['node'], $data['dependencies']['module']);
   

   I think it'd be clearer (and safer) to also test the expectations *before* running the updates, so that it's clear what exactly you expect to change.

2. +++ b/core/modules/system/system.post_update.php
   @@ -31,5 +34,30 @@ function system_post_update_fix_enforced_dependencies() {
   +  // Loads scan all module install folders and resave the entities in there.
   +  $modules = array_keys(\Drupal::moduleHandler()->getModuleList());
   +  foreach ($modules as $module) {
   +    $default_install_path = drupal_get_path('module', $module) . '/' . InstallStorage::CONFIG_INSTALL_DIRECTORY;
   +    $storage = new FileStorage($default_install_path, StorageInterface::DEFAULT_COLLECTION);
   

   Does this then also include optional config?

   Either way: would be good to have explicit test coverage for optional config too.

3. +++ b/core/profiles/standard/config/install/field.field.node.article.field_tags.yml
   @@ -5,7 +5,6 @@ dependencies:
   -    - entity_reference
   
   +++ b/core/profiles/standard/config/install/field.storage.node.field_tags.yml
   @@ -2,7 +2,6 @@ langcode: en
   -    - entity_reference
   

   Why these changes?

#34-3 #2429191: Deprecate entity_reference.module and move its functionality to core but also prior to that patch entity_reference was only required for field_ui not for programmatic creation.

trying to fix some of the test fails

This fixes some of them

Might fix the other 2.

Added entity tags to alert team entity. Removed upgrade path and upgrade path test tags as we have them now.

Not familiar with the exact stakes of this patch, but the changes in the last interdiff in FieldItemInterface & EntityReferenceItem make total sense. We have FieldItemInterface::calculateDependencies() for the field config, there should definitely be an equivalent for the storage config.

Oh yeah I totally think that this is how you expect it to be.

+++ b/core/modules/field/src/Entity/FieldStorageConfig.php
@@ -343,6 +343,16 @@ public function calculateDependencies() {
+    // Plugins can declare additional dependencies in their definition.
+    if (isset($definition['config_dependencies'])) {
+      $this->addDependencies($definition['config_dependencies']);
+    }

Is this something existing or did you invented this here ?

Looking at the failures for a while.

I'm actually quite happy that Drupal\system\Tests\Installer\StandardInstallerTest failed ...

Here is a fix for the rest of the failures. Now the dependencies actually make more sense to be honest, like the tags field storage depending on taxonomy module.

+++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
@@ -419,6 +419,39 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state);
+   * return an array of dependencies listing module that provides the entity

"dependencies listing module" does not make sense. I think it wanted to be "dependencies listing THE module"?

Wrote change notice draft at https://www.drupal.org/node/2581375 for the new API addition. Found two more nits while writing it:

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -446,6 +446,16 @@ public static function calculateDependencies(FieldDefinitionInterface $field_def
   +   * @inheritDoc
   

   {@inheritdoc}

2. +++ b/core/modules/field/tests/src/Unit/FieldStorageConfigEntityUnitTest.php
   @@ -94,8 +107,37 @@ public function testCalculateDependencies() {
   +class TestFieldType extends FieldItemBase {
   

   Let's add one line phpdoc on what does this supposed to test IMHO.

1. +++ b/core/modules/system/src/Tests/Entity/EntityReferenceFieldTest.php
   @@ -406,4 +407,35 @@ public function testTargetEntityNoLoad() {
   +          'handler' => 'default'
   

   comma needed at the end of the line.

Thanks for the reviews, well someone please make a patch, I'm not necessarily online.

I'll do it.

This addresses all but #50.1.

Because the class is not a test class but is only needed for FieldStorageConfigEntityUnitTest it is not appropriate to have @coversDefaultClass so could some who working on that provide an example doc please.

More nits.

1. +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
   @@ -419,6 +419,39 @@ public function fieldSettingsForm(array $form, FormStateInterface $form_state);
   +   * Dependencies added here affect the storage and therefore are always
   

   field storage or other storage?

2. +++ b/core/modules/book/config/install/core.entity_view_mode.node.print.yml
   @@ -1,11 +1,11 @@
    dependencies:
   -  module:
   -    - node
      enforced:
        module:
          - book
   +  module:
   +    - node
   

   Why the ordering change?

3. +++ b/core/modules/system/system.post_update.php
   @@ -31,5 +34,30 @@ function system_post_update_fix_enforced_dependencies() {
   +  // Loads scan all module install folders and resave the entities in there.
   

   Loads scan all?

   Also what about optional config that doesn't actually depend on the module, then that module got uninstalled but the config stuck around?

Two nits one actual question.

#59: RE: optional config, I asked about that in #34.2. Which is why I think #34.1 is worth having/doing too: it makes the expectations of pre-update and post-update much clearer, hence preventing accidental regressions.

1. +++ b/core/modules/system/src/Tests/Update/RecalculatedDependencyTest.php
   @@ -27,8 +27,28 @@ protected function setDatabaseDumpFiles() {
   +
   +    // Explicitly break an optional configuration dependencies to ensure it is
   +    // recalculated.
   +    \Drupal::configFactory()->getEditable('search.page.node_search')->clear('dependencies')->save();
   +
   

   Urgs, it feels just bad to execute code before the update. Do we really need that and can't just manipulate the objects in the DB directly?

2. +++ b/core/modules/system/system.post_update.php
   @@ -36,24 +36,26 @@ function system_post_update_fix_enforced_dependencies() {
   +  $install_storage = new InstallStorage();
   +  $config_names = $install_storage->listAll();
   +  $install_storage = new InstallStorage(InstallStorage::CONFIG_OPTIONAL_DIRECTORY);
   +  $config_names = array_merge($config_names, $install_storage->listAll());
   

   Maybe some docs about why we want to scan optional config as well would be nice

1. +++ b/core/modules/system/system.post_update.php
   @@ -31,5 +31,38 @@ function system_post_update_fix_enforced_dependencies() {
   + * Drupal did not calculate dependencies on install, so recalculate them by
   + * saving all matching configuration entities.
   

   I didnt think we wrote comments talking about what used to happen. I know this is in an update though...

2. +++ b/core/modules/system/system.post_update.php
   @@ -31,5 +31,38 @@ function system_post_update_fix_enforced_dependencies() {
   + * @see https://www.drupal.org/node/2520526
   

   and I dont think we reference the issue that is changing code. that is what blame is for eh?

Well for what it's worth the patch looks good to me. The only quibble I could find was that EntityReferenceFieldTest and UpdatePostUpdateTest didn't have any @covers directives. But then I noticed they weren't unit tests so I guess that's alright.

RTBC++ from me.

I will be bold.

This looks great to me as well. I'm about to commit it. Ticking credit boxes for everyone who participated.

And pushed!

FYI: One of the ways I manually tested this was installing 2 Standard Profile sites: one from beta-16 and then updating to HEAD post commit of this patch, and another by installing straight to HEAD post commit of this patch. Then I exported the full configuration of each site and diffed them, and all dependencies were the same.

@effulgentsia
You would not have had to do that. The \Drupal\system\Tests\Installer\StandardInstallerTest is doing exactly that, and did, see some of the failures earlier.