Updated: Comment #15

Problem/Motivation

At the moment the only reason field CMI import works is because field.field.* is before fieid.instance.* this sort of dependency management is not good enough.

See #2106243: Use yml files to create the forum module's comment and taxonomy term fields for where this becomes an issue.

Proposed resolution

#2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall introduces a configuration dependency manager that can be used to graph dependencies between configuration entities.

\Drupal\Core\Config\StorageComparer compares configuration storages and creates lists of changes for the ConfigImporter to process. The StorageComparer should return these operations in the correct order.

For example

  • For deletes: field instances should be deleted before fields and fields should be deleted before node types
  • For creates: node types should be created before fields and fields should be created before field instances
  • For updates (I think - not 100% sure): node types should be updated before fields and fields should be updated before field instances

Related:

#1918926: Module dependencies are not respected when default configuration is imported

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tayzlor’s picture

Issue tags: +Configuration system

tagging

alexpott’s picture

Priority: Normal » Critical

This will require an API change to the StorageComparer and ConfigImporter.

gdd’s picture

Changing status, as this is required for module enable/disable on import.

tayzlor’s picture

Assigned: Unassigned » tayzlor

I'm working on this and hoping to have an initial patch up in the week at some point.

Anonymous’s picture

i'm concerned about the complexity of this design, particularly compared to our previous ideas. will wait for an implementation before i say any more.

tayzlor’s picture

It would work similar to how modules are installed or updates are done, and calculate the dependencies using the Graph class. Then we'd cycle through the list of things to import, and install any dependencies along the way.
Having not been involved really in previous ideas or discussions, i'm happy to take a steer, and not go off and implement stuff that will never be adopted :)

Anonymous’s picture

i missed the discussions, so maybe some more explanation would be useful.

the original post doesn't make much sense with regard to what is proposed to be built.

i'm prolly missing stuff, but i don't think we need any ordering of config files to do module/themes as part of import. we do need to sort the list of modules/themes for dependencies, but we don't need events or module input for that. we can just ask the moduleHandler how to sort stuff.

regardless of whether we need a bunch of new classes and concepts for the rest of the implementation, this issue proposes a 'big bang' approach - modules get one shot at the sort, then we run the import.

maybe that's enough, but it feels like we'll run in to the usual 'who gets to go first/last' issues. module A gets a chance to sort, then module B has a go, and changes the order in a way that will break A.

a while back, we had a patch that did a simple 'module dependency order' sort, then ran the import in cycles. basically, run the changes through, and allow modules to signal that they had 'skipped' a file, and would like another chance at it on the next cycle. @tayzlor - happy to give more details in IRC if that doesn't make sense.

maybe we want both, and that could be a follow up? maybe we just live with a big-bang approach that?

Anonymous’s picture

Priority: Critical » Major

i don't think this is a critical issue.

xjm’s picture

Category: task » bug

This is definitely a bug. We're in a situation currently that the only reason field and field instance synchronization works is that "field.field" comes before "field.instance" in the alphabet. See #1944368: React to / force order of imports, which was discovered in #1735118: Convert Field API to CMI.

xjm’s picture

Let's get a clearer summary that explains the impact of this.

xjm’s picture

Title: Abstract out config import changes to a config list class and create a prioritisable array utility object » Config cannot be imported in order for dependencies
Priority: Major » Critical
Issue tags: +Needs tests

And yes, this is release-blocking. Import is unreliable as it stands currently, and it's pure luck that we haven't gotten nailed by this outside of an epic debugging session for #1735118: Convert Field API to CMI.

yched’s picture

the only reason field and field instance synchronization works is that "field.field" comes before "field.instance" in the alphabet

And I just realized the only reason "node type" import works is because 'field.*' comes before 'node.*'...
See #2069373: Race conditions on import if CUD on ConfigEntity A triggers changes in ConfigEntity B

Anonymous’s picture

ok, so what do people think of #7?

can we live with a simple module dependency sort via ModuleHandler?

if not, can we live with a single shot sort via an event?

or, do want to go back to the future and use import cycles?

or all of the above?

Anonymous’s picture

bump.

larowlan’s picture

Test case is #2106243: Use yml files to create the forum module's comment and taxonomy term fields

field.field makes a taxo field referencing a taxonomy.vocabulary
which doesn't yet exist because of sort-order.

alexpott’s picture

Issue summary: View changes

Improved summary

alexpott’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

Issue summary: View changes

Updated issue summary.

mtift’s picture

Issue summary: View changes

Add related issue

xjm’s picture

alexpott’s picture

Issue tags: +beta blocker
jian he’s picture

I just want to mention what I have solve this issue in manually way.

I use the config_reset module to import some config first, then import other configs (config_reset let us import selected configs other than import all the configs in one time). This way can solve the dependencies manually.

jessebeach’s picture

Title: Config cannot be imported in order for dependencies » [PP-1] Config cannot be imported in order for dependencies
Issue summary: View changes
jessebeach’s picture

Title: [PP-1] Config cannot be imported in order for dependencies » Config cannot be imported in order for dependencies
Status: Postponed » Active

Unpostponed!

alexpott’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
7.9 KB

Patch attached ensures that the storage comparer sorts the names correctly according to configuration entity dependencies.

alexpott’s picture

Issue summary: View changes
alexpott’s picture

Added unit tests - I think this is good to go.

Anonymous’s picture

+  protected function addChangelistUpdate() {
+    // Organise updates in such a way that dependencies are updated before
+    // configuration entities that depend on them. For example, field instances
+    // should be updated after fields and fields should be updated after
+    // content types.

- can we move this to a docblock instead of in-line.

+   * Sets up the data required to determine and sort the change lists.
+   */
+  protected function setUpData() {

- this function seems to actually do the dependency sorting, so we should update the docblock to reflect it. actually, perhaps we can do getData() and sortData(), so it's clearer what's going on?

otherwise, this looks all good to me :-)

alexpott’s picture

FileSize
3.5 KB
16.15 KB

thanks @beejeebus

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

yay, i think this is ready to go.

swentel’s picture

  1. +++ b/core/lib/Drupal/Core/Config/StorageComparer.php
    @@ -101,51 +112,69 @@ public function getChangelist($op = NULL) {
    +   *   Array of changes to add the changelist.
    

    to add to the changelist. ?

  2. +++ b/core/lib/Drupal/Core/Config/StorageComparer.php
    @@ -101,51 +112,69 @@ public function getChangelist($op = NULL) {
    +   * should be created after fields.
    

    'updated' instead of 'created'

Taran2L’s picture

+++ b/core/tests/Drupal/Tests/Core/Config/StorageComparerTest.php
@@ -0,0 +1,232 @@
+      ->method('listAll')
+      ->will($this->returnValue($config_files));
...
+      ->method('listAll')
+      ->will($this->returnValue($config_files));
...
+      ->method('readMultiple')
+      ->will($this->returnValue($config_data));
...
+      ->method('readMultiple')
+      ->will($this->returnValue($config_data));
...
+                        ->method('listAll')
+                        ->will($this->returnValue(array_keys($source_data)));
...
+                        ->method('listAll')
+                        ->will($this->returnValue(array_keys($target_data)));
...
+                        ->method('readMultiple')
+                        ->will($this->returnValue($source_data));
...
+                        ->method('readMultiple')
+                        ->will($this->returnValue($target_data));
...
+                        ->method('listAll')
+                        ->will($this->returnValue(array_keys($source_data)));
...
+                        ->method('listAll')
+                        ->will($this->returnValue(array_keys($target_data)));
...
+                        ->method('readMultiple')
+                        ->will($this->returnValue($source_data));
...
+                        ->method('readMultiple')
+                        ->will($this->returnValue($target_data));
...
+                        ->method('listAll')
+                        ->will($this->returnValue(array_keys($source_data)));
...
+                        ->method('listAll')
+                        ->will($this->returnValue(array_keys($target_data)));
...
+                        ->method('readMultiple')
+                        ->will($this->returnValue($source_data));
...
+                        ->method('readMultiple')
+                        ->will($this->returnValue($target_data));

Code style in test methods is inconsistent.

alexpott’s picture

FileSize
4.66 KB
15.73 KB

Ok thanks PHPStorm :)

jessebeach’s picture

I've been "breaking" the tests to see if the behavior changes in an expected manner.

public function testCreateChangelistCreate() {
    $target_data = $source_data = $this->getConfigData();
    //unset($target_data['field.field.node.body']);
    unset($target_data['field.instance.node.article.body']);
    unset($target_data['views.view.frontpage']);

Removing unset($target_data['field.field.node.body']) from testCreateChangelistCreate causes the test to fail. The list of config entities to creates is missing the field.field.node.body config object which, in the test, already exists because it wasn't unset.

Array (
-    0 => 'field.field.node.body'
-    1 => 'views.view.frontpage'
-    2 => 'field.instance.node.article.body'
+    0 => 'views.view.frontpage'
+    1 => 'field.instance.node.article.body'
)

The tests testCreateChangelistDelete and testCreateChangelistUpdate fail as expected as well when altered. Dependencies are appropriately being accounted for in create, delete and update scenarios.

Good to go.

xjm’s picture

Assigned: tayzlor » catch
alexpott’s picture

FileSize
3.54 KB
15.73 KB

  • Commit 2e7b209 on 8.x by catch:
    Issue #2030073 by alexpott: Config cannot be imported in order for...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Discussed with Alex in person, hence interdiff.

I saw the phpunit tests pass locally, honest.

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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