Problem/Motivation
Node, comment, and custom block fields are being created programatically during the install of modules and the standard profile. This should not occur as this means we can not have default UUIDs and this is the solution that we choose in #1969800: Add UUIDs to default configuration to resolve renames, create/deletes during a config synchronisation.
Forum, Book, Custom block and Standard profile are all affected.
This is critical since it blocks progress on #2121751: [META] Making configuration synchronisation work
Proposed resolution
Create default config and set syncing flag during module enable so that Config entity types and hooks can determine whether they should automatically create extra config on not.
Remaining tasks
None
User interface changes
None
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#11 | 2127573.11.patch | 41.33 KB | alexpott |
#11 | 7-11-interdiff.txt | 1.88 KB | alexpott |
#7 | 1-7-interdiff.txt | 11.29 KB | alexpott |
#7 | 2127573.7.patch | 41.53 KB | alexpott |
#1 | 2127573.1.patch | 32.22 KB | alexpott |
Comments
Comment #1
alexpottComment #2
alexpottComment #3
alexpottComment #4
alexpottComment #5
xjmSo, this is a functional change at a very low-level in a patch to fix a specific implementation. Why exactly do we need to set the flag, and why wasn't it set previously, and where's the test coverage for this usecase? It'd be good to document that on issue and add whatever test coverage is missing, plus expose the failures in a partial patch.
Doesn't say why. :)
Comment #7
alexpottNew patch:
Comment #8
alexpottUpdated summary to reflect full scope of the patch
Comment #9
alexpottUpdating title
Comment #10
vijaycs85Few review comments....
We can remove the typecasting now.
I may be wrong here, but Comment trying to say 'Verify that isSyncing flag was not set during a normal save of a configuration entity.'? Also checking FALSE === FALSE and message says *set to TRUE*?
missing langcode.
Comment #11
alexpott1. I think this is out-of-scope. All the config in this patch has been created by current code.
2. Fixed - comments are unnecessary - assertion says everything it needs to.
3. Definitely out-of-scope. The
EntityDisplayBase
does not have a langcode propertyComment #12
Gábor HojtsyProgramatically created configuration is also a problem for multilingual. Although the config in this patch seems to not use t() when created, that was the Drupal 7 practice. Then the config would be translated in some language instead of the original shipped English. On top of that the config created that way may also not be 'langcode: en', especially once #1966436: Default *content* entity languages are not set for entities created with the API is fixed. So the config entities are created with a mismatched language for label vs. what the langcode may say. All in all, having them as default shipped config is also the only clean solution for multilingual. Huge plus!
Comment #13
yched CreditAttribution: yched commentedI've been mulling on those subjects (config sync, UUIDs...) a lot lately, but just bumped into that issue. Trying to draw the consequences of the approach in this patch.
So this patch is making the "module/profile install" runtime context "special restricted": there are things that shouldn't happen there - like hook_update(), but different.
a) there should be no secondary config writes triggered while importing the default config. the [module]/config folder contains the only changes that should happen.
b) hook_install() should not save new config entities. Ship your stuff as CMI files in your /config folder, or (if you need runtime logic), specify a hardcoded UUID.
Just like hook_update(), "shouldn't happen" has to mean "not allowed, doing it will break with an exception", or contrib / custom code will easily overlook it and screw the whole CMI promise. How do we enforce the above ?
Also, a) means 3rd party modules cannot react to the new config entities being imported. Having 3rd party modules use their own config entities to store settings about other modules config entities is common. Typical example:
My foobar.module does things on nodes per node type. It stores the corresponding settings in foobar.settings.[node_type].yml.
- What it would do currently is implement hook_entity_node_type_save() to save a new foobar.settings.[node_type] ConfigEntity for the new node type.
- With the new logic here, foobar_entity_node_type_save() should check $node_type->isSyncing() and *not* create its config entity if TRUE.
Then it means you can no longer assume the config entities exist when they should. entity_load() cannot be used directly, and we're back to each system providing its own separate load APIs containing the wrapping logic:
(Meaning, probably, I might chose to remove foobar_entity_node_type_save() altogether and rely on runtime defaults altogether - or not).
This in a lot of contrib modules, each with their own variants (naming, methods / functions...) and chances of doing it wrong, if at all...
--> Drawbacks:
- DX overhead of another "special / restricted APIs" runtime context (hook_install() + config entity callstack when inside "import default config" process), that we need to enforce and document.
- loss of API unity: entity_load() -> somemodule_get_stuff() / SomeModuleClass::getStuff()
Comment #14
yched CreditAttribution: yched commented(side note: sorry if the above sounds negative - as i said, I'm mulling in circles on those issues right now, and am only trying to figure out the impact of the approach here)
Comment #15
gddI still don't understand the why of isSynching(). What problem does this solve? I tend to agree with yched that creating rules around what will and won't work in specific contexts gives me pause.
Well, according to what we're saying here, foobar module should provide default config as opposed to dynamically creating its config in response to another module, right? That would get installed when foobar module gets installed. However, I freely admit that I know very little about the process by which modules provide additional/modified functionality to entities, so I could very well be missing a subtlety here. For instance, thinking this through right now, I guess a module could provide some new settings for every node type. The module has no idea what node types are installed on the system, so it has to dynamically create its config per-node-type. That seems to be the kind of use case we are talking about? Obviously the process is different for required core modules, because core starts at a known state. Just trying to think it through for everyone else.
Comment #16
Gábor HojtsyRe secondary config writes, eg. the locale module kicks in after modules are enabled to update config translations. When new default configs are added, translation files may need to be created. That is a kind of secondary config write. However, this does not happen with the original config save/import outright. It happens in a batch where locale later figures out the new config files and creates the missing translations.
It sounds to me like modules would find it painful to do this by injecting further batch items after module enables for example, but if we switch to shipping configuration, indeed hooks like node type creation will not fire, because we just consider it a generic action of a config file imported and not a special action of a node type being created. I see the problem there, but cannot really think of an elegant solution. I like the elegance of no distinction between configs as to why are some dynamically created, because all would be statically shipped.
We may need to solve the "specific hooks not firing" problem by doing some discovery of the configs being imported via the config events?! Sounds a bit backwards, hm.
Comment #17
alexpott@yched I totally agree with your concerns and this for me is a serious problem with the whole default UUID approach. However until we have a working config import of all the things I'm not going to be allowed to remove them because I just can't seem to convince people of the amount of pain they are going to cause us and that there is another way. Doing what this patch does is a consequence of going with default UUIDs to fix the create/delete vs renames/update issues. In my mind we should never have allowed that patch without doing this work first to see if this was even possible. But we did - so lets follow it through to conclusion - get a working CMI system and then refactor so this problem goes away.
@Gábor Hojtsy afaik hooks for node type creation will fire just fire on config entity import. The isSyncing flag does not prevent this.
Can we please get these patches in so that we can progress to having all the things work.
Comment #18
alexpott11: 2127573.11.patch queued for re-testing.
Comment #19
Gábor Hojtsy@alexpott: oh, I was not aware that if I include a node type config entity in my module, when that is imported, the node type related hooks are invoked. Is there some magic involved there so that eg. if I have a default field, those hooks are invoked, if I have a default view, those hooks are invoked, etc?
Comment #20
alexpott@heyrocker the isSyncing() was @yched's idea in Prague :)
Whilst we are demanding default UUIDs in config entities created during install then we need some flag to know how to act. With regards to throwing an exception if a default config entity is created in a install situation without a default UUID - there is an issue for this #2124535: Prevent secondary configuration creates and deletes from breaking the ConfigImporter which was created after #2069373: Race conditions on import if CUD on ConfigEntity A triggers changes in ConfigEntity B
Comment #23
yched CreditAttribution: yched commentedClarifying : isSyncing() was my idea ;-), but in my mind it only applied to the "config sync" process, not the "default config import" process, which are very different.
In the "config sync" process, we're syncing a *full* config tree. Meaning, we have to state: "no secondary config writes should happen during the processing of a config entity, the set of files in the staging folder is the only set of changes that should happen".
Secondary writes, if any, have already happened on the site were the config changes were initially made, and are thus already present in the set of files we're syncing - "staging" is self contained. When importing a config entity as part of a config sync, no one should react to that by triggering more config changes, because the reactions are already part of the staging folder, and have already been imported earlier in the process or will be imported later, so there would be nasty race conditions. That's what #2069373: Race conditions on import if CUD on ConfigEntity A triggers changes in ConfigEntity B fixed with isSyncing() - but in current HEAD, that only involves the "config sync" process.
In the "import of default config" process, this is way different. We're only importing a couple config items, not a full config tree. We can't assume that this set of files contains the whole set of changes that should happen - we can't assume that the foo.module, that ships a new node type in its default config, knows about my random contrib / custom foobar.module and ships the foobar.settings.[the_node_type].yml that my module would have created.
If we do apply isSyncing() there too, which this patch does, then the consequences are #13.
Comment #24
yched CreditAttribution: yched commentedSide note: All in all, this is is all a consequence of "we try to support config sync between any random drupal install, and that forces us to make that config entities create during (site or module) install have the same UUIDs everywhere".
I'm myself finding this unsustainable, the impact is just too painful - mind bending special cases, impact on DX, easily breakable... (what I understand from @alexpott's #17 is he's in a similar position ?).
If we targeted "config sync works only between *copies* of a single initial install, that share the same set of UUIDs for their config entities" - which still fulfills what is IMO the real promise of D8 CMI : deploy between dev / stage / prod -, then all of those would disappear.
Comment #25
Gábor HojtsyI think having all shipped config as .yml files is useful as a DX *improvement* so long as it works. Lots of config is now in .yml files in core, and some are not. That is confusing. Why would I ship some in .yml files and others in code. Also the translatability is a challenge, because you want to create those default configs in English (no t() in the creation process), but you also want it to be translatable, so you need all pieces that will need to be translated somehow in t() somewhere. So you need to add some dead code at least then to ensure the static code parser will find those strings. That is not needed if all shipped config is in YAMLs.
Comment #26
alexpottYES, YES, YES!
For me here is the start of a different solution to this and the default UUID issue #2133325: Create a site UUID on install and only allow config sync between sites with the same UUID
Comment #27
xjmComment #28
xjmComment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedi don't think we're on the right track here. #24 is not quite right.
the problem does not originate with site installation and 'copy an existing install' vs 'two separate installs'. the problem originates with 'how do we tell the difference between an update and delete/create and rename during sync'. as we're not going to implement a log-shipping mechanism for deploying changes, we're solving that via a hash in config objects (which we're calling a uuid now, but the name is unimportant).
very broadly speaking, we have two basic options to ensure that we can use these hashes to solve the deploy rename/update problem.
the first approach is: ban all config entities changes during sync other than those directly flowing from iterating over the staging tree during sync. this ensures that no config object hash is ever generated during a sync, and we can do rename/update without issue.
that means during config sync:
- no config entities will be installed by config_install_default_config()
- no module code is allowed to react to config entity CRUD operations by creating another config object
- no module can create a config object during hook_install() or hook_modules_installed()
that is, we run some very different code paths during sync for module installation and config entity CRUD. during normal site operation, those three points are reversed. (i'm assuming we're not considering massively regressing the ability of module code to react to config entity operations.)
this has consequences. first of all, we'll need to special case all the config entity things for the cases above. module install will require a different code path, config entity CRUD will require a different code path, module install hooks will require a different code path.
module install hooks will also need to be aware that sometimes they can rely on default config, sometimes they can't. yep, a module which is installed during a config sync will not be able to rely on it's own default config being available during hook_install(). same sort of problem for any module that implements hook_modules_installed(). i have NFI what issues that may cause, but i'm 100% sure i think the conceptual overhead is not worth it.
in case it's not clear, i don't think this approach is the way to go.
the second broad approach is - make sure that the config object hash is always the same for config objects created during module install. that covers both default config objects, and secondary config objects created via CRUD hooks or module install hooks.
default UUIDs in shipped configuration was our first cut at this broad approach. this implementation seems to be lacking, because it doesn't cover the 'secondary config objects' created during module install, and we've had to implement another work around. it also requires pre-generating UUIDs for all default config objects, which sucks.
however, i still think this approach is broadly the way to go, and i think we can make it work with these changes:
- stop using 'uuid' as the name for the hash we're using to solve 'rename/update during sync', instead use 'creationHash' or some better name
- always generate the same 'creationHash' for new config entities created during module install
afaics, those two things will solve the 'rename/update' problem, and remove the need for hard-coded UUIDs in default config.
Comment #30
yched CreditAttribution: yched commentedSure, the issue originates in "config sync relies on UUIDs to tell between 'update' and 'delete/create new'". But I don't think I see how relying on a new 'creationHash' instead would make things different ?
We'd move from "config entities created at install time need to have hardcoded UUIDs" to "... need to have hardcoded creationHashes" :
- I don't see how that's simpler to solve ?
In my example in #13, foobar_entity_node_type_save() would still have code to create a foobar.settings.[node_type] config entity when a new node type is created. This code runs both at install time when importing a node_type.yml shipped in a default config, and at "regular" runtime when a node type gets created through the UI or some other code. How do you "hardcode" something in one case and not in the other ?
- What would those "random, non clashing, automatically generatable" hashes be, if not something very much like a UUID ?
- Plus drawback of now having two properties dealing with "object identity", sounds like a recipe for hair loss ?
(side note: can we stop saying "config sync" when talking about "default config import" ;-) ? The two are widely different, collating them adds to the confusion..)
Comment #31
Anonymous (not verified) CreditAttribution: Anonymous commentedi don't think the creationHash has to be random, and that's actually why i think we should stop using UUID for the name.
if the creationHash set on a config entity is a predictable string when created during default config import, all of the existing problems go away.
Comment #32
yched CreditAttribution: yched commentedOk, sure :-) but how do we do that ?
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedwe pass in initial data when creating entities. entity code already does 'if the uuid is set, don't generate one', so we can do the same for creationHash.
ConfigEntityBase will need something like this so that secondary CRUD operations can know if they are running during module install:
and config_install_default_config() will need something like this so that the creationHash is always set:
and i think we can then remove isSyncing(), because we've solved the issue another way. crazy?
EDIT: oh, and we can remove all default UUIDs :-)
Comment #34
yched CreditAttribution: yched commentedNot relateed AFAICS. isSyncing() is there to prevent race conditions in the *config sync* process, this has nothing to do with UUIDs, and will still be needed whatever the outcome of this *default config import* discussion.
$config_values['creationHash'] = md5("module-install-uuid-$entity_type-$name");
Meaning, the creationHash is a strict function of the config $name (i.e entity id) ? The files already have a name, how would adding a mere hash of it help ?
If two separate install profiles happen to create two entirely different fields that they both choose to name field_blabla, they would have the same creationHash, so later on, config sync would consider them to be "the same field" (meaning, sync tries an "update" instead of a "delete/create new"), which is exactly what we need to prevent. "Same name" is not enough to be "the same config entity" for config sync.
I'm still missing the plan here.
Comment #35
Anonymous (not verified) CreditAttribution: Anonymous commentedi wonder if we should try to discuss this in IRC / hangout.
two install profiles - we don't support that. we shouldn't test it, we shouldn't think about it.
within an install, at any point in time, you can't create a config entity with a name that isn't unique? so yes, creationHash, when a config entity is created during module install, is derived from the name directly, but that's kinda the point. when created outside of that, creationHash is a random hash.
that gives us enough to work with, so that config entities created during module install will match, whether that module install runs during sync or not, and we can sidestep our existing problems without hard-coding default UUIDs.
Comment #36
yched CreditAttribution: yched commentedWell, the problem is "what we support" is not clear at all :-). There's currently no restriction on "which A and B sites can you sync config from and to", and there is indeed a expectation that "sync between any two random installs" should "just work" - that's exactly what the OP here is about.
Summary of the situation as I understand it:
It seems at least @beejeebus, @alexpott and myself all think that expectation (in short: "config sync can take any random arbitrary drupal site, and magically turn it into any arbitrary and totally different drupal site") is crazy.
- this issue tries to go to the bottom of that expectation, and (for the people mentioned above), acknowledge that this is not doable / too complex / crazy.
- #2133325: Create a site UUID on install and only allow config sync between sites with the same UUID, opened by @alexpott, that I support too, is about exploring one possible restriction: "config sync is only between copies of one single initial install (e.g dev / stage / prod)", and, similarly, figuring out the consequences.
- @beejeebus in #21 / #33 proposes another approach, which is not entirely clear to me yet (still need to mull over #33).
Would it then make sense to explore it in its own issue ?[edit: done, that's #2138559: Replace UUIDs in configuration entities with a creation ID, with an initial patch]Is that a correct assessment ?
Sigh, yeah, it's really hard to keep this discussion not confusing :-/
I guess an IRC/hangout might help, yes - but it's gonna be hard for me until DCVienna ends (or maybe during the sprints there - sat/mon/tue ?)
Comment #37
gddI would really love to get some kind of hangout discussion to hash this out, even if it means a less-than-ideal time for me. It really is an important issue, and while I'm not 100% against #2133325: Create a site UUID on install and only allow config sync between sites with the same UUID, it introduces a restriction that I never intended the system to have, and that will be difficult to explain to people. I'll try and coordinate a meeting via email.
Comment #38
sunNote sure whether it helps here, but I attempted a clarification in
#2110615-41: Do not ship with default UUIDs
Comment #39
alexpottDiscussed with yched, heyrocker, xjm, and mtift. We need to be able to support programmatic creation of config entities on install time due to the following scenario:
Agreed to close this issue in favour of exploring other solutions which will be documented on #2121751: [META] Making configuration synchronisation work
Comment #40
Gábor HojtsySo do we have a guideline for which configuration should be created with an API and which should be shipped as-is a file with core / a module? What makes them different? If I'm a module developer, how do I decide?
Also I'm a bit puzzled as to what is the recommendation for their translatability. If they are created without runtime t() calls, they will get created in English but will not get any translations or translation updates (only shipped configuration gets those, configuration that users create don't, for performance reasons). If it is runtime t()-ed, then the langcode of the created config should be set to the language used on the page, so the config knows the proper language it was created with. Even then there will be no translation updates provided obviously. So either way, programatically created default configuration looks like a step-child in the current scheme of things for translation.
Comment #41
xjm