Problem/Motivation

Content dependencies are also soft dependencies. This missing that if they are missing then the configuration should not remove them if resaved. This means that if the content does not exist during configuration import and the config entity is created the content dependency should not be removed. For example, Block content blocks lose their dependencies when exported after being imported before the block content entity existed.

If I have a module that provides some block placements and default content for the block entities themselves, the block placements are imported first and then the block content. This is allowed because of the fallback plugin for blocks. If you immediately attempt to re-export the block placements, the dependencies will be lost.

Proposed resolution

Don't clear out content dependencies when recalculating configuration dependencies.

Remaining tasks

Discuss approach, review patch, add tests.

User interface changes

None

API changes

tba

Data model changes

tba

Comments

benjy created an issue. See original summary.

larowlan’s picture

+1 from me assuming that this doesn't prevent importing (but I think all content entity dependencies are soft, so we're good)

Status: Needs review » Needs work

The last submitted patch, block-content-dependency.patch, failed testing.

benjy’s picture

Failures all look like config schema, will wait for some input from @alexpott before changing any tests.

alexpott’s picture

Something about this feels wrong. Can @benjy can you post such a module so I can see what's going on.

benjy’s picture

FileSize
673 bytes

See the attached module, simplified to the bare minimum of what is needed to reproduce this.
SHA1 25cc8257d0432cef8a79b7ddb6e66917f1307e58 test-content.tar.gz

Steps to reproduce

  1. Install Drupal with standard
  2. drush en -y test_content
  3. drush config-get block.block.testblock // Note, the content dependency has been lost, refer to config/install/block.block.testblock.yml
  4. Re-save "Test Block" on the blocks page.
  5. drush config-get block.block.testblock // Now shows the dependencies again.
alexpott’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +Configurables
FileSize
1.06 KB
1.06 KB

So here is a better fix - i think. I think given the fact that all content dependencies in configuration are soft we can't remove them on recalculation as the whole point is that don't have to be met.

The last submitted patch, 7: 2597854-7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 7: 2597854-7.patch, failed testing.

benjy’s picture

Here's a test for the issue. The solution in #7 only partly works, the dependency on the block_content module is still dropped, as shown by this test.

(array(
   'type' => 'delete',
   'orig' =>
  array (
    0 => '  module:',
    1 => '    - block_content',
  ),
   'closing' => false,
))

The last submitted patch, 10: 2597854-10-FAIL.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 10: 2597854-FAIL-with-7.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
3.78 KB

Now creating the block which was missing from the test.

#7 still doesn't fix the issue though because config.storage->read() doesn't trigger the re-calculation of the dependencies.

Status: Needs review » Needs work

The last submitted patch, 13: 2597854-13.patch, failed testing.

alexpott’s picture

Priority: Normal » Major
Issue tags: +rc target triage

This is a major bug - dependencies shouldn't go missing like this.

alexpott’s picture

+++ b/core/modules/block_content/tests/modules/block_content_test/config/install/block.block.testblock.yml
@@ -0,0 +1,24 @@
+provider: null
...
+  provider: block_content

This looks weird

roynilanjan’s picture

Is it possible to import block_content to import? This is found that after full import the block content unable to import in the db, the import block is missing/showing broken in the original region see http://drupal.stackexchange.com/questions/146037/block-error-on-cmi

alexpott’s picture

Title: Block content dependencies lost on export » Content dependencies lost on export and importing again if the content entities do not exist
Issue summary: View changes
Issue tags: -rc target triage

Updated issue summary

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.26 KB
7.26 KB

Whilst trying to explain some of the test fails @tstoeckler and I discovered #2625216: LocaleConfigManager::getTranslatableDefaultConfig() reads config from uninstalled modules

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Block/Plugin/Block/Broken.php
    @@ -49,4 +49,14 @@ protected function brokenMessage() {
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function calculateDependencies() {
    +    // Broken blocks should preserve the provider of the broken plugin.
    +    $dependencies = parent::calculateDependencies();
    +    $dependencies['module'][] = $this->configuration['provider'];
    +    return $dependencies;
    +  }
    +
     }
    

    In that case fixing \Drupal\views\Plugin\views\BrokenHandlerTrait would be neat.

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -354,7 +354,7 @@ public function preSave(EntityStorageInterface $storage) {
       public function calculateDependencies() {
         // All dependencies should be recalculated on every save apart from enforced
         // dependencies. This ensures stale dependencies are never saved.
    -    $this->dependencies = array_intersect_key($this->dependencies, ['enforced' => '']);
    +    $this->dependencies = array_intersect_key($this->dependencies, ['enforced' => '', 'content' => '']);
    

    The comment is now missing some information

  3. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentDependencyTest.php
    @@ -0,0 +1,59 @@
    +/**
    + * @group block_content
    + */
    +class BlockContentDependencyTest extends KernelTestBase {
    

    Is that valid? I would be totally happy about that.

alexpott’s picture

Thanks @dawehner. I think 1 should be addressed in a followup since it will require tests of its own.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me, great to have this fixed.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Hmmm... I think there might be a problem with the approach. If you create an entity reference field to content and then change the default value I don't think the dependency to the initial node will be removed :( testing this theory...

alexpott’s picture

Unfortunately... #23 is right...

uuid: 0f936f13-18cb-4940-ad00-a9764e1d9177
langcode: en
status: true
dependencies:
  config:
    - field.storage.node.field_ref
    - node.type.article
  content:
    - 'node:article:19b0f140-4e17-4858-8400-e78a83076e0b'
    - 'node:article:e6ccb833-139f-4c90-9000-b6345b9caceb'
id: node.article.field_ref
field_name: field_ref
entity_type: node
bundle: article
label: ref
description: ''
required: false
translatable: false
default_value:
  -
    target_uuid: 19b0f140-4e17-4858-8400-e78a83076e0b
default_value_callback: ''
settings:
  handler: 'default:node'
  handler_settings:
    target_bundles:
      article: article
    sort:
      field: _none
field_type: entity_reference
alexpott’s picture

So one option would be to only preserve during a configuration sync. However this would mean that any post update that saved all config entities to fix something would risk losing these dependencies too. So going back to first principles - is it right for configuration to have dependencies listed for things that are not there? I don't think so. It is okay for configuration to be broken is something is missing. (By broken I mean report to the user that something is missing).

So is this a bug? As long as the missing content dependency event is fired with the correct information and the system has the chance to create the content then everything is working as expected. Also if there is missing content dependencies after an import the user should be informed so they know stuff is broken.

benjy’s picture

Also if there is missing content dependencies after an import the user should be informed so they know stuff is broken.

Yeah, +1 to this. Because this wasn't obvious, this is why the original issue described the problem as dropping off when re-exporting, because that was the first time I actually noticed.

Documenting what we discussed in IRC, processMissingContent() is currently too late to figure out which dependencies are removed, we need to calculate which dependencies are removed during sync so that processMissingContent() can send the correct information in the MissingContentEvent event.

Whereabouts exactly does the dependency currently get dropped off? Somewhere within $config->save()?

alexpott’s picture

So testing the ConfigImporter reveals that importing a block through the config importer does indeed fire the ConfigEvents::IMPORT_MISSING_CONTENT event missing the correct missing content - and the dependencies are not removed during sync because of the following code:

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

So actually everything is behaving as expected for config sync. The question is more complicated in terms of install profiles but I think the answer there is to use a config sync based installer instead of our current installer (see the config_installer project). A further question is what do we do for module installation where the config depends on missing content - this is the test added by the current patch. Also given that content_block placements can not exist at all when the content block is missing should the content block module listen and stub if the content is missing - and should be apply this generally?

alexpott’s picture

Here's what I think we should do:

  1. Inform users that certain configuration is missing content dependencies after importing
  2. Prevent installation of configuration from modules if content dependencies are missing (this is the simplest thing to do) - at the moment they are ignored. This will fix install profiles for now whilst the installer is using regular module install
  3. Discuss whether or not we want to provided the ability to create stubs for the missing content in core
alexpott’s picture

Also I just tested importing and re-exporting a block with a missing content dependency and is it maintained.

catch’s picture

#28 works for me.

larowlan’s picture

28.2 will break one of the primary uses of default_content module
Pretty sure agov and Berdir's/md-systems' profile are using it that way.

Berdir’s picture

Yes, I don't think that is an option, how are modules supposed to create that content then. There's nothing that runs before config import AFAIK and content might depend on other config. So the config import must happen first (think custom block type > custom block > plugin > block placement) and then modules can create content, e.g. with default_content or hook_install or whatever they want.

That definitely seems like an API change if we'd change that now?

alexpott’s picture

@Berdir thinking custom block type > custom block > plugin > block placement during a module install - in HEAD we have a problem. The block placement will be saved during the module install and this will recalculate the dependencies and at that point the content does not exist - so the plugin does not exist - so the block is not saved correctly.

Berdir’s picture

I don't think BlockContentBlock even adds a dependency in the first place? It's the *plugin* that depends on the content entity, not the config entity, because it's a derivative (which I still think is bad architecture, but that's a different topic).

But that's kind of OK, we have that covered. You'd simply get that fallback plugin as long as the content entity doesn't exist.

So that example isn't actually really related, but there could be others that do involve config dependencies like that.

IIRC #2407853: TaxonomyIndexTid Views plugin stores selected terms with the ID instead of UUID is also related to this, that's a patch I need for our install profile.

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should 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.

xjm’s picture

Issue tags: +Triaged D8 major

@alexpott, @catch, and I agreed that this is a major bug because of the lost dependencies.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.