computerminds / cm_config_tools

DEPRECATED: Mirror of http://cgit.drupalcode.org/cm_config_tools
1 stars 0 forks source link

[META] [DANGER] Don't use config/install #16

Closed darthsteven closed 7 years ago

darthsteven commented 7 years ago

The config/install directory has special handling in Drupal core. There are things you can do with it, and things that you can't.

Primarily it's designed for installing config when a module is installed.

We are (by default) overloading the usage to additionally store config that we want to be synced on the next deployment.

This seems to be causing issues where we have two modules that provide the config in config/install or we don't actually want the config installed at the install time of the module, 'after'.

Could we move to using some other directory?

etc.

We could then implement something that does a cm-config-tools-import after a module is installed. Developers wouldn't notice much difference, except the config would be in a different directory.

Discuss.

anotherjames commented 7 years ago

we don't actually want the config installed at the install time of the module, 'after'

I'd be interested in this use case. Could you elaborate please, with an example? Thankees!

darthsteven commented 7 years ago

19 is an example

darthsteven commented 7 years ago

Also, at the moment if we want to override some config otherwise provided by another module, that seems to cause issues, because it's in two config/install folders.

We've worked around this by deleting the config from the original providing module.

But if we moved away from using config/install then we just wouldn't have this issue, because our directory could be scanned after, replacing any config that it needed to.

anotherjames commented 7 years ago

Hmm, the config being in two config/install folders shouldn't be a problem... unless it's in two modules that have the cm_config_tools key in their info files? Do you have a specific example please, i.e. which config name(s) & which modules?

I'd like to understand the problem(s) before moving onto potential solutions (e.g. using a different sub-directory). (Part of my aim for cm_config_tools was to try and stick to known/understood things where possible, e.g. so it could be phased out as core/contrib caught up with usage. The more we diverge, the more that becomes harder later in life.)

darthsteven commented 7 years ago

https://github.com/computerminds/SPX/commit/e6fc32fdf4bf279ad060f875b166a8b61e085163

darthsteven commented 7 years ago

https://github.com/computerminds/SPX/commit/5c62068fe5c17143cf6e845d9183e5871a18dcb2

darthsteven commented 7 years ago

Looks like those are actually optional views, but they aren't optional for our install profile, if that makes sense?

anotherjames commented 7 years ago

Right, so let's straighten one thing out: those views were config/optional directories in their original core modules. And yes, that makes sense that they should not be 'optional' for our install profile, but that misses what the 'config/optional' directory means / is for.

So because our install profile does depend on views, their presence in config/optional or config/install is (ought to be) exactly the same.

So I think the actual problem here is that overriding something in another module's config/optional with something in a cm_config_tools-managed module/profile's config/install directory does not work correctly. I suspect simply having the view in our profile's config/optional dir would suffice, no need for core patches. I could be wrong, so I'll test.

If there are other reasons to move away from using the existing config/install directory, fair enough, but let's deal with them separately from this actual observed & identified bug.

anotherjames commented 7 years ago

Also, regardless of the view perhaps needing to go into /config/optional in our profile, yes, cm_config_tools ought to be smarter and allow the view being in /config/install despite it being in the providing modules' /config/optional directories.

darthsteven commented 7 years ago

Yep, okay, so we can work around this optional nastiness, but #19 still remains.

anotherjames commented 7 years ago

Yes, comment added related to that :-P I'll try & resolve that first then and then we may be able to move on in this ticket.

darthsteven commented 7 years ago

We could remove the install_only thing too, because if you really wanted that, you'd just pop the config in config/install

anotherjames commented 7 years ago

Sorry, that's not how it works. IIRC, anything in config/install gets imported/reverted when running cmci unless flagged as 'create_only/unmanaged'.

darthsteven commented 7 years ago

Yeah, that's what I mean.

Leave the semantics of the config/install directory alone and add some new ones for a config/sync directory.

I very quickly end up back at core's workflow here, of just having a big directory of config. The only way this falls down is that we'd want this in git, and we'd want the production site to be committing to that git repo. If that could be done, then one could ditch all of this custom stuff, and just use that custom stuff instead, which may or may not be better.

anotherjames commented 7 years ago

one could ditch all of this custom stuff, and just use that custom stuff instead, which may or may not be better.

Yes, very true that that is indeed another approach that could be tried, and each has their own pros & cons. I'm prioritising getting as far down this current approach first. Feel free to fork / branch / start a new project to explore the other approach in the meantime.

anotherjames commented 7 years ago

19 is now resolved.

So this issue is now around either:

1) Moving cm_config_tools-based-workflow config into a separate directory, but I don't think that's necessary, as the keys in the info file now work as expected/specified, having discussed with @stevetweeddale . There is a (valid) argument for avoiding overloading /config/install, but the recent changes now mean that the original core use case of config that is only saved on installation, and ignored by cm_config_tools otherwise, is possible again (whereas it hadn't been until now, sorry, I hadn't recognised this issue!). Simply having config in the /config/install, without mentioning it in the info file, is sufficient to achieve this, which matches core behaviour.

2) Fixing overrides of things that were in /config/optional of their original modules - this still needs investigating.

3) Using a different custom workflow that is closer to core's original single big directory of config, whilst still tracking it in git. For a start on that, I suggest building on from https://github.com/previousnext/drush_cmi_tools (and maybe more specifically, https://github.com/previousnext/drush_cmi_tools/pull/3).

anotherjames commented 7 years ago

Opened #23 to cover item 2 from above.

Item 3 is frankly outside of the scope of cm_config_tools [currently].

I believe item 1 is resolved, @darthsteven you are welcome to disagree if you feel a separate directory is still necessary, but I don't believe there's any reason to prioritise any such change even so.

anotherjames commented 7 years ago

A recent article on the weekly drop also covers using https://www.drupal.org/project/config_split which kinda works similar to that blacklist + core config sync directory approach, which you may be interested in @darthsteven .