Problem/Motivation

#2282519: Add content dependency information to configuration entities adds the ability for configuration to depend on content. However when importing configuration there is no way of creating the content that is depended on at the same time.

Proposed resolution

Add a step to the ConfigImporter that determines if there are any missing content dependencies and fire an event to allow contrib modules to respond and create the configuration.

Remaining tasks

Write patch

User interface changes

None

API changes

None - should be all additions

Files: 
CommentFileSizeAuthor
#37 2361423-swentel.37.patch21.71 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,581 pass(es). View
#37 30-37-interdiff.txt4.05 KBalexpott
#30 interdiff.txt706 bytesdixon_
#30 2361423-swentel.30.patch20.81 KBdixon_
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,542 pass(es). View
#29 2361423-swentel.29.patch20.82 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,522 pass(es). View
#29 27-29-interdiff.txt7.42 KBalexpott
#27 2361423-swentel.27.patch18.84 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,515 pass(es). View
#27 23-27-interdiff.txt8.41 KBalexpott
#23 config-import.23.patch13.23 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,411 pass(es). View
#22 config-import.22.patch13.07 KBlarowlan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch config-import.22.patch. Unable to apply patch. See the log in the details link for more information. View
#22 interdiff.txt6.42 KBlarowlan
#18 2361423.18.patch13.2 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,763 pass(es). View

Comments

alexpott’s picture

alexpott’s picture

Status: Postponed » Active

We can do this now...

xjm’s picture

larowlan’s picture

working on this

alexpott’s picture

FileSize
13.19 KB

Here's some work I already had done on this - needs tests though.

alexpott’s picture

Status: Active » Needs review
larowlan’s picture

FileSize
11.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Here's what I have, the test fails and I'm not sure how to test the context handling (batch).

larowlan’s picture

so yours is way better than mine ;)

larowlan’s picture

One thing mine does is make ConfigImporter::getProcessedConfigurations useful, making it contain the config objects instead of the keys. We have no calls to that method in core (tests or otherwise) so that's most likely why - at present I think it doesn't really help anyone who listens to a ConfigImporterEvent.

dawehner’s picture

@larowlan
I don't get why you reimplemented the feature in a different way. The advantage of the proposal of alexpott is that
you can recursivly resolve content dependencies, which is kind of helpful for entity references and what not.

  1. +++ b/core/lib/Drupal/Component/Plugin/composer.json
    @@ -1,4 +1,4 @@
    -{
    +n{
    

    nice one :)

  2. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -298,9 +301,12 @@ public function getProcessedConfiguration($collection = StorageInterface::DEFAUL
    +   * @param \Drupal\Core\Config\Config|bool $config
    +   *   The processed configuration object or FALSE if the object was deleted or
    +   *   doesn't exist.
    

    It seems a bit odd to use FALSE and not just NULL

  3. +++ b/core/lib/Drupal/Core/Config/ConfigImporterEvent.php
    @@ -18,13 +18,23 @@ class ConfigImporterEvent extends Event {
       /**
    +   * Batch context.
    +   *
    +   * @var array
    +   */
    +  protected $context = array();
    

    Does this really has to be a batch context? Is the system really built in a way to rely on something like a batch process? It would be really helpful to describe what is part of the $context, currently you as a user would have no clue what is in here.

  4. +++ b/core/modules/config/src/Tests/ConfigEventsTest.php
    @@ -56,6 +56,11 @@ function testConfigEvents() {
    +    // Test config dependencies event.
    +    $config->set('dependencies.content', ['entity_test:entity_test:8A521545-F2EC-4BBF-9722-8DAB4F4FCBD7']);
    +    $config->save();
    +    $this->assertIdentical(\Drupal::state()->get('config_import_test.content_dependencies'), ['entity_test:entity_test:8A521545-F2EC-4BBF-9722-8DAB4F4FCBD7']);
    
    +++ b/core/modules/config/tests/config_import_test/src/EventSubscriber.php
    @@ -52,6 +52,27 @@ public function onConfigImporterValidate(ConfigImporterEvent $event) {
    +          $context = $event->getContext();
    +          $context['hi_there'] = 'indeed';
    +          $event->setContext($context);
    

    You don't test 'hi_there' at the moment.

larowlan’s picture

I don't get why you reimplemented the feature in a different way. The advantage of the proposal of alexpott is that
you can recursivly resolve content dependencies, which is kind of helpful for entity references and what not.

Sorry - should have said that @alexpott and I spoke on irc and realised we were both working on it at the same time - so we decided to put our patches up and then join forces and iterate from there.

I think @alexpott's is better too - but I like the changes to getProcessedConfigurations in mine, but no reason that can't go in a new issue

nice one :)

larowlan-- no idea what happened there but tipping wrong window was open :)

alexpott’s picture

@larowlan the issue with getProcessedConfigurations() actually keeping a reference to the config means that all config is loaded on all steps of the batch - and possibly could go stale if there are side effects.

larowlan’s picture

Ah, so maybe we just need more documentation of how to get the objects from the event subscriber

Status: Needs review » Needs work

The last submitted patch, 7: 2361423-content-config-sync.5.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: 2361423-content-config-sync.5.patch, failed testing.

isntall’s picture

Patch from comment 7, 2361423-content-config-sync.5.patch, does not seem to pass the testbot checks, and it doesn't seem to fail properly either. Please reroll.

alexpott’s picture

Status: Needs work » Needs review
FileSize
13.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,763 pass(es). View

The patch from #5 rerolled. @isntall core generally is self-service :) see http://drupal.org/patch/reroll

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -543,7 +544,7 @@ public function initialize() {
         $this->moduleHandler->alter('config_import_steps', $sync_steps, $this);
    

    Oh nice, you can already add your own steps in contrib

  2. +++ b/core/lib/Drupal/Core/Config/Importer/EventSubscriber.php
    @@ -0,0 +1,39 @@
    +class EventSubscriber implements EventSubscriberInterface {
    

    What about using "FinalMissingContentSubscriber" as name?

  3. +++ b/core/lib/Drupal/Core/Config/Importer/EventSubscriber.php
    @@ -0,0 +1,39 @@
    +    // This should always be the final event as it will mark all content
    +    // dependencies as resolved.
    +    $events[ConfigEvents::IMPORT_MISSING_CONTENT][] = array('onMissingContent', 1024);
    

    ... mh, so 1024 is a pretty high priority, I think you wanted to use -1024

  4. +++ b/core/lib/Drupal/Core/Config/Importer/MissingContentEvent.php
    @@ -0,0 +1,73 @@
    +  /**
    +   * Missing content.
    +   *
    +   * @var array
    +   */
    +  protected $config;
    ...
    +    $this->missingContent = $missing_content;
    

    Wrong variable name, probably.

  5. +++ b/core/lib/Drupal/Core/Config/Importer/MissingContentEvent.php
    @@ -0,0 +1,73 @@
    +  public function resolveMissingContent($uuid) {
    +    if (isset($this->missingContent[$uuid])) {
    +      unset($this->missingContent[$uuid]);
    +      // Stop propagation if the subscriber resolves any conflicts. This allows
    +      // multiple events to listen to the missing content event and interact
    +      // with the config importer in a batch.
    +      // \Drupal\Core\Config\Importer\EventSubscriber implements an event to
    +      // catch any unresolved content dependencies and mark them all as resolved
    +      // so that config importing can complete.
    +      $this->stopPropagation();
    +    }
    

    From an API point of view, I think its wrong to stop always after the first resolveMissingContent call. Doesn't stopPropagation much more belongs into EventSubscriber?

yched’s picture

Same remarks as @dawehner :
- EventSubscriber is a bit short / non-telling
- seems surprising to stop propagation on the first entity being resolved ?

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -613,6 +614,31 @@ protected function processConfigurations(array &$context) {
+  protected function processMissingContent(array &$context) {
+    if (!isset($context['sandbox']['config']['missing_content'])) {

Nitpick : the whole method could be a bit leaner if it started by extracting the piece of the $context it's going to use for its processing, like
$sandbox = &$context['sandbox']['config'];
instead of fully dereferencing it again and again ?

dawehner’s picture

Status: Needs review » Needs work

.

larowlan’s picture

Status: Needs work » Needs review
FileSize
6.42 KB
13.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch config-import.22.patch. Unable to apply patch. See the log in the details link for more information. View

fixes #19 and #20 plus a re-roll

swentel’s picture

FileSize
13.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,411 pass(es). View

rerolled

dixon_’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Config/ConfigManagerInterface.php
    @@ -178,4 +178,9 @@ public function supportsConfigurationEntities($collection);
    +   *   A list of missing dependencies()
    

    Let's replace () with a . here :)

  2. +++ b/core/lib/Drupal/Core/Config/Importer/MissingContentEvent.php
    @@ -0,0 +1,66 @@
    +  /**
    +   * Resolves the missing content by removing it from the list.
    +   *
    +   * Calling this also stops event propagation so that if the event subscriber
    +   * does actually creates content it will work nicely with the batch system.
    +   *
    +   * @param string $uuid
    +   *   The UUID of the content entity to mark resolved.
    +   *
    +   * @return $this
    +   *   The MissingContentEvent object.
    +   */
    +  public function resolveMissingContent($uuid) {
    

    The docs seems wrong here since we no longer stop propagation here.

  3. +++ b/core/lib/Drupal/Core/Config/Importer/MissingContentEvent.php
    @@ -0,0 +1,66 @@
    +    else {
    +      // @todo throw an exception? Atm I don't think so.
    +    }
    +    return $this;
    

    Agreed, I don't think we need to do anything here. So let's remove the else block here?

isntall queued 22: config-import.22.patch for re-testing.

The last submitted patch, 22: config-import.22.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
8.41 KB
18.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,515 pass(es). View

Patches addresses everything in #24 and adds a test implementation of an event listener and uses it to test :)

jibran’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -614,6 +615,38 @@ protected function processConfigurations(array &$context) {
    +      $event = new MissingContentEvent($missing_content);
    

    If we are passing all the missing content in one go why would we need a sandbox? Are we missing some kind of array_slice here?

  2. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
    @@ -447,4 +447,30 @@ protected function callOnDependencyRemoval(ConfigEntityInterface $entity, array
    +      // Can we do some multiple loading?
    

    I think we can but it has to be a different loop. We can create an array of uuids per entity_type and then load it in next loop using loadByProperties() or see EntityReferenceFieldItemList::processDefaultValue() for similar kind of implementation.

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.42 KB
20.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,522 pass(es). View

re #28

  1. We are sandboxing so that we can allow multiple events to handle missing content in multiple batch steps. Adding tests for this.
  2. I tried to implement this but it just adds heaps of complexity because we are looking for entities that don't exist.
dixon_’s picture

FileSize
20.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,542 pass(es). View
706 bytes

Code looks good. I'll be working on a proof-of-concept implementation in Deploy this week at DrupalCon that utilizes this.

Marking this as RTBC, with a very minor documentation fix to the patch (please don't give me commit credit because I haven't really worked on the patch itself).

dixon_’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: 2361423-swentel.30.patch, failed testing.

alexpott’s picture

Unrelated fail in Drupal\locale\Tests\LocaleJavascriptTranslationTest.

alexpott queued 30: 2361423-swentel.30.patch for re-testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
catch’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -614,6 +615,38 @@ protected function processConfigurations(array &$context) {
    +    if ($current_count) {
    

    It's not clear from me what happens if there's no subscriber to deal with the missing content? Could this get stuck?

  2. +++ b/core/lib/Drupal/Core/Config/Importer/FinalMissingContentSubscriber.php
    @@ -0,0 +1,45 @@
    + *
    + * Ensure that all missing content dependencies are removed from the event so
    + * the importer can complete.
    + *
    

    Oh here it is!

However I didn't immediately see test coverage that covers this case (i.e. that the final subscriber successfully removes content depenencies when nothing else has) - or did I miss it?

alexpott’s picture

FileSize
4.05 KB
21.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,581 pass(es). View

@catch well there is implicit coverage in things like ConfigImportAllTest but I've added explicit coverage in the attached patch.

catch’s picture

Status: Reviewed & tested by the community » Fixed

That looks better. Committed/pushed to 8.0.x, thanks!

  • catch committed 66c9f70 on 8.0.x
    Issue #2361423 by alexpott, larowlan, dixon_, swentel: Add a step to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.