Updated: Comment #14

Problem/Motivation

When there are updates to Drupal core, contributed modules dealing with configuration overrides need to react to schema changes. It's currently not reliable to use the update helper functions, because there is no guarantee your update function is actually run before Drupal adds a new version of the helper function (update dependencies don't help because as per our instructions, you first need to update core and only after that update contrib modules, at least for major updates)

Conversions to the config system have been using config() during the upgrade path. We have a convention that API functions (apart from raw database queries) can't be used during the upgrade path, because the API might change the the upgrade path has to be completely static as much as possible.

Proposed resolutions

  • Add the old configuration as an update-only service and have future update functions change the service appropriately by establishing an UpdateConfigFactory that extends ConfigFactory (much like UpdateModuleHandler extends ModuleHandler)
  • add a config_update_0() API that is locked to the existing configuration API and that we'll only update until the upgrade path is supported in some way (usually around beta). After there's an expectation that 7.x sites can upgrade to Drupal 8, any change to CMI file storage would result in config_update_x() being added on top
  • wait until this becomes a problem then add this retrospectively, the problem with this is that contrib modules will also need to use config_update_x() and right now they'll already be using config(), it's not going to be acceptable to change things for existing updates once the upgrade path is supported
  • never under any circumstances change the configuration file format once we reach beta/rc, historically we never make that kind of guarantee just in case but it might be OK to make it here
  • possibly work around it with update dependencies, but again that's hard once you have contrib modules relying on the API

Remaining tasks

  • Agree on an approach to solving the problem.
  • Create the patch.

User interface changes

None

API changes

TBD

#2001310: Disallow firing hooks during update
#1871700: Provide an upgrade path to the new Block architecture from Drupal 7
#1977498: Add version tracking for configuration schema and data

Original report by catch

Conversions to the config system have been using config() during the upgrade path. We have a convention that API functions (apart from raw database queries) can't be used during the upgrade path, because the API might change the the upgrade path has to be completely static as much as possible.

config(), as a raw storage API the configuration is a bit different so I've been ignoring this issue to some extent, but the more I think about it I don't think it's an exception to the rule.

Here's where it can go wrong:

- system_update_xx() (and various hundreds of others) use config() during the upgrade path.

- at some point Drupal 8.0 is released. Some sites upgrade to 8.0.

- Drupal 8.6 is released and changes the way that configuration files are written changes in some way that requires an update to existing configuration.

- Drupal 8.6 adds an update function to update all existing configuration files to the new format.

- sites upgrading from 7.x direct to Drupal 8.6 now have all of their files written out into the new format by the pre-8.x update function. Then an upgrade path added in 8.6 running on those new configuration files, which attempts to upgrade them from the old format to the new one.

- Drupal 8.15 changes the structure again.

Ways we can deal with this:

- add a config_update_0() API that is locked to the existing configuration API and that we'll only update until the upgrade path is supported in some way (usually around beta). After there's an expectation that 7.x sites can upgrade to Drupal 8, any change to CMI file storage would result in config_update_x() being added on top.

- wait until this becomes a problem then add this retrospectively, the problem with this is that contrib modules will also need to use config_update_x() and right now they'll already be using config(), it's not going to be acceptable to change things for existing updates once the upgrade path is supported.

- never under any circumstances change the configuration file format once we reach beta/rc, historically we never make that kind of guarantee just in case but it might be OK to make it here.

- possibly work around it with update dependencies, but again that's hard once you have contrib modules relying on the API.

If the example above isn't any good and this whole issue confuses you, please read #1134088: Properly document update-api functions and #1239370: Module updates of new modules in core are not executed to see some more background on how broken our current major upgrade system is and why all this weird crap is required to stop it from completely collapsing in on itself.

Opening this as a critical task because we absolutely have to resolve this issue before people start trying to upgrade sites with any hope of keeping them around.

Comments

Berdir’s picture

- wait until this becomes a problem then add this retrospectively, the problem with this is that contrib modules will also need to use config_update_x() and right now they'll already be using config(), it's not going to be acceptable to change things for existing updates once the upgrade path is supported.

This pattern doesn't work for contrib anyway. It's currently not reliable to use the update helper functions, because there is no guarantee your update function is actually run before Drupal adds a new version of the helper function (update dependencies don't help because as per our instructions, you first need to update core and only after that update contrib modules, at least for major updates). The only thing that works for contrib is using the normal function and require to run the update with the latest available core version...

sun’s picture

Almost identical / closely related: #1848998: Problems with update_module_enable()

xjm’s picture

This pattern doesn't work for contrib anyway. It's currently not reliable to use the update helper functions, because there is no guarantee your update function is actually run before Drupal adds a new version of the helper function (update dependencies don't help because as per our instructions, you first need to update core and only after that update contrib modules, at least for major updates)

Let me see if I understand this:

  • Core changes the configuration API in system_update_8090() for 8.6 and adds config_update_8090() (or whatever) based on that API.
  • A contrib module updates to the 8.6 API and tries to use config_update_8090() to upgrade its data in mymodule_update_8001().
  • Core changes the configuration API in system_update_8092() in 8.15 and adds config_update_8092() (or whatever).
  • The same contrib module adds another update using config_update_8092() in mymodule_update_8002(), but sites that are not already on the 8090 schema from 8.6 choke on the previous update in mymodule_update_8001(), because core was updated to 8092 before mymodule_update_8001() was run.
xjm’s picture

Do config files need their own schema version, like stored in the file?

Berdir’s picture

The same contrib module adds another update using config_update_8092() in mymodule_update_8002(), but sites that are not already on the 8090 schema from 8.6 choke on the previous update in mymodule_update_8001(), because core was updated to 8092 before mymodule_update_8001() was run.

Exactly. Unlike the core modules, where we have 100% control over the order in which updates are run, contrib modules aren't so lucky. Their only option really is just using the normal API function (or maybe an update_something_latest()) and update it if necessary when core changes something.

catch’s picture

update_something_latest() would work if contrib updates are being run after core has been upgraded from 7.x to 8.x already, but for that major version upgrade, they'd probably still need to use a specific version and hook_update_dependencies() since otherwise the _latest() function could be operating against any schema version, not necessarily with the latest one?

Berdir’s picture

update_something_latest() would work if contrib updates are being run after core has been upgraded from 7.x to 8.x already, but for that major version upgrade, they'd probably still need to use a specific version and hook_update_dependencies() since otherwise the _latest() function could be operating against any schema version, not necessarily with the latest one?

According to our upgrade instructions, for major updates, contrib *always* needs to run after the main 7.x -> 8.x upgrade path was executed. But there is no way of saying of that core upgrade means 7.x => 8.0 or 7.x => 8.15 which have have changed the update helper function.

gdd’s picture

- never under any circumstances change the configuration file format once we reach beta/rc, historically we never make that kind of guarantee just in case but it might be OK to make it here.

I think I am OK with this answer to be honest. By the time we reach beta we will have worked through some of the most complicated possible implementations of the config system, namely views and fields/content types. The format is already very open and malleable for specific use cases. I'd be fine making this statement and essentially punting on the problem.

sun’s picture

Issue tags: +Configuration system
andypost’s picture

xjm’s picture

Gábor Hojtsy’s picture

Related: #1977498: Add version tracking for configuration schema and data for post-upgrade path updates of config overrides when the base config structure changes and overrides are not anymore overlaying the config as needed.

Gábor Hojtsy’s picture

@heyrocker:

- never under any circumstances change the configuration file format once we reach beta/rc, historically we never make that kind of guarantee just in case but it might be OK to make it here.

I think I am OK with this answer to be honest. By the time we reach beta we will have worked through some of the most complicated possible implementations of the config system, namely views and fields/content types. The format is already very open and malleable for specific use cases. I'd be fine making this statement and essentially punting on the problem.

So views and field config are just containers for in many cases contrib data structures (displays, filters, field types, etc). So I'd question how deep do we want the config structure freeze to take hold (and whether this is enforceable on contrib). The problem from the point of view of config overrides is that the override will be in the structure of the config. If *any* substructure changes that was overriden (by language, domain module, etc), then the override will not apply anymore, and may even be harmful given that it "stamps over" the base config. So in case any contribs update config structures, be it a view plugin or field configuration, we'd need to find a way to trickle that down to the overrides. That is where #1977498: Add version tracking for configuration schema and data comes in.

If we have a ban on changing config structure altogether, this is not an issue obviously. Is that realistic? If not, and we can apply the structure change ban on core only, we'll need to find ways for how these contrib modules update their structure, how that affects core wrapping structures and then how overrides can / should react. Eg. hooks/events in place to react with config before/after and schema before/after as arguments.

chx’s picture

If Drupal 8.6 changes config then we can add the old config as an update-only service and have that 8.6 update function change the service appropriately. I think.

mtift’s picture

Issue summary: View changes

Updated issue summary.

mtift’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue tags: +Needs tests

So there are two separate problems here

  1. Changing schemata
  2. The invocation of hooks during configuration entity creation

For both issues, I think the next step is a failing test that demonstrates that these are actually still bugs.

mtift’s picture

Issue tags: +sprint

tagging

chx’s picture

> The invocation of hooks during configuration entity creation

Except UpdateModuleHandler will boink you over the head if you write an update that saves an entity cos saving an entity fires hooks and you can not fire hooks during update. Just save the raw config.

webchick’s picture

Issue tags: +beta blocker

Tagging all critical D8 upgrade path issues as "beta blocker."

catch’s picture

Title: config() isn't safe to use during upgrade path » config() isn't safe to use during minor updates if there's a change to configuration storage
Priority: Critical » Major
Issue tags: -beta blocker

We need to discuss this again now there's no upgrade path. This could still be an issue if we make a change to the configuration storage during a minor release but seems like that could be handled with update dependencies - it'd only affect a module updating its configuration where the update runs at the same time as the core update for the storage.

Since that's considerably more of an edge case than when trying to support both minor and major version updates with the same API and having minor updates run immediately after major updates, downgrading to major.

effulgentsia’s picture

One thing we discussed in Prague is that we should introduce separate hooks for schema changes than for data changes (both for "config" data and "content" data). Then we could run all the schema changes (across all modules) first (with very limited APIs allowed, and no hooks firing), followed by the data change functions (which could go through normal config() and entity CRUD API calls). If we did that, would that fully solve this issue?

catch’s picture

Yes I think that'd be fine.

catch’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue tags: -sprint
chx’s picture

How often will config storage change? Can we tell people to re-migrate?

catch’s picture

Status: Active » Closed (duplicate)