Add a step to config import to allow contrib to create content based on config dependencies

working on this

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

so yours is way better than mine ;)

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.

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

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

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

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.

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?

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 ?

.

fixes #19 and #20 plus a re-roll

rerolled

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?

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.

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

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?

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