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 beforefields
andfields
should be deleted beforenode types
- For creates:
node types
should be created beforefields
andfields
should be created beforefield instances
- For updates (I think - not 100% sure):
node types
should be updated beforefields
andfields
should be updated beforefield instances
Related:
#1918926: Module dependencies are not respected when default configuration is imported
Comment | File | Size | Author |
---|---|---|---|
#33 | 2030073.33.patch | 15.73 KB | alexpott |
#33 | 30-33-interdiff.txt | 3.54 KB | alexpott |
#30 | 2030073.30.patch | 15.73 KB | alexpott |
#30 | 26-30-interdiff.txt | 4.66 KB | alexpott |
#26 | 2030073.26.patch | 16.15 KB | alexpott |
Comments
Comment #1
tayzlor CreditAttribution: tayzlor commentedtagging
Comment #2
alexpottThis will require an API change to the StorageComparer and ConfigImporter.
Comment #3
gddChanging status, as this is required for module enable/disable on import.
Comment #4
tayzlor CreditAttribution: tayzlor commentedI'm working on this and hoping to have an initial patch up in the week at some point.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedi'm concerned about the complexity of this design, particularly compared to our previous ideas. will wait for an implementation before i say any more.
Comment #6
tayzlor CreditAttribution: tayzlor commentedIt 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 :)
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedi 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?
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedi don't think this is a critical issue.
Comment #9
xjmThis 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.
Comment #10
xjmLet's get a clearer summary that explains the impact of this.
Comment #11
xjmAnd 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.
Comment #12
yched CreditAttribution: yched commentedAnd 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
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedok, 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?
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedbump.
Comment #15
larowlanTest 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.
Comment #15.0
alexpottImproved summary
Comment #15.1
alexpottUpdated issue summary.
Comment #15.2
alexpottUpdated issue summary.
Comment #15.3
mtiftAdd related issue
Comment #16
xjmComment #17
alexpottComment #18
jian he CreditAttribution: jian he commentedI 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.
Comment #19
xjmPostponing on #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall and #2004370: Batch the configuration synchronisation process per discussion with @alexpott and @mtift.
Comment #20
jessebeach CreditAttribution: jessebeach commentedComment #21
jessebeach CreditAttribution: jessebeach commentedUnpostponed!
Comment #22
alexpottPatch attached ensures that the storage comparer sorts the names correctly according to configuration entity dependencies.
Comment #23
alexpottComment #24
alexpottAdded unit tests - I think this is good to go.
Comment #25
Anonymous (not verified) CreditAttribution: Anonymous commented- can we move this to a docblock instead of in-line.
- 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 :-)
Comment #26
alexpottthanks @beejeebus
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedyay, i think this is ready to go.
Comment #28
swentel CreditAttribution: swentel commentedto add to the changelist. ?
'updated' instead of 'created'
Comment #29
Taran2LCode style in test methods is inconsistent.
Comment #30
alexpottOk thanks PHPStorm :)
Comment #31
jessebeach CreditAttribution: jessebeach commentedI've been "breaking" the tests to see if the behavior changes in an expected manner.
Removing
unset($target_data['field.field.node.body'])
fromtestCreateChangelistCreate
causes the test to fail. The list of config entities to creates is missing thefield.field.node.body
config object which, in the test, already exists because it wasn't unset.The tests
testCreateChangelistDelete
andtestCreateChangelistUpdate
fail as expected as well when altered. Dependencies are appropriately being accounted for in create, delete and update scenarios.Good to go.
Comment #32
xjmComment #33
alexpottComment #35
catchDiscussed with Alex in person, hence interdiff.
I saw the phpunit tests pass locally, honest.
Committed/pushed to 8.x, thanks!