Ensure that ConfigImport is taking place without outstanding updates

+1 for the general approach. Updates are designed to be executed before anything else. On the other hand we should maybe consider to allow config imports as part of post update, otherwise post update config changes would be overridden by deployment scripts.

I was never totally clear on the right order between these two steps so IMO this explicit ordering is helpful.

Initial patch...

Thanks @alexpott. I have added tests #1970534-108: Patch testing issue and working on test failures (ref: https://www.drupal.org/pift-ci-job/568413)

Rerolled the patch from the test patch issue.

Made the specific added test work - except for the post update check, but I think there's some static caching going on there.
Note also the irony of running the import before 'profile' is set, throws notices in the test, bitter irony :)

So the remaining failures are due to pending post updates. Checked whether I could replace the post update registry in the kerneltest, but seem to fail so far .. will look further this evening.

Ok, I was focusing on the wrong thing, thanks for berdir for pointing me to drupal_set_installed_schema_version() ...

Bring back the post updates, will trigger fails,

- either because of static values in update path tests (at least, that's my suspicion)
- in kerneltestbase because the registry finds the post update functions from system (so either we swap that service or we run them in kerneltestbase - first one seems more appropriate)

This should fix the fails for classes using kernelTestBase.

stupid file_put_contents left over

This should be green

1. good idea
2. done : UpdateRegistry doesn't have an interface though, introduce one as UpdateRegistryTest is a bit lying of course in it's comments :) Also, is that the right place for that class to be ? Maybe rename to UpdateRegistryNull ?
3. sad schema ..
4. fixed
5. ugh, so I can't login first to try and run an initial sync because system_update_8400() hasn't run yet .. the irony ... I've been thinking whether we should not rely on UpdatePathTestBase(), but being able to run updates is nice .. let me think about it some more .. suggestions welcome of course.

UpdateRegistry doesn't have an interface though

And hence all the fails when injecting, the fun!

+1 to general concept because developers are still confused what should goes first - updb or cim. Adding such a check will keep developers happy.

1. +++ b/core/lib/Drupal/Core/Test/TestUpdateRegistry.php
   @@ -0,0 +1,52 @@
   + * UpdateRegistryTest used for tests.
   ...
   +class TestUpdateRegistry extends UpdateRegistry {
   

   docblock and class name doesn't match

2. +++ b/core/modules/system/src/SystemConfigSubscriber.php
   @@ -23,13 +25,32 @@ class SystemConfigSubscriber implements EventSubscriberInterface {
   +   * The update registry
   

   Missing full stop.

3. +++ b/core/modules/system/src/SystemConfigSubscriber.php
   @@ -78,6 +99,52 @@ public function onConfigImporterValidateSiteUUID(ConfigImporterEvent $event) {
   +    foreach ($this->moduleHandler->getModuleList() as $module => $filename) {
   +      $updates = drupal_get_schema_versions($module);
   +      if ($updates !== FALSE) {
   +        $default = drupal_get_installed_schema_version($module);
   +        if (max($updates) > $default) {
   +          $has_pending_updates = TRUE;
   +          break;
   +        }
   +      }
   +    }
   +    if (!$has_pending_updates) {
   +      $missing_post_update_functions = $this->postUpdateRegistry->getPendingUpdateFunctions();
   +      if (!empty($missing_post_update_functions)) {
   +        $has_pending_updates = TRUE;
   +      }
   +    }
   +
   

   Can we change the order? First check for missing post update functions and then, if needed, iterate over module list? That will, potentially, make it faster(?).

   Instead of setting value for $has_pending_updates we could just return TRUE. There's no need to continue.

+++ b/core/modules/system/src/SystemConfigSubscriber.php
@@ -78,6 +100,53 @@ public function onConfigImporterValidateSiteUUID(ConfigImporterEvent $event) {
+    $has_pending_updates = FALSE;
...
+    if (!$has_pending_updates) {
...
+    }
...
+    return $has_pending_updates;

I think we can remove $has_pending_updates variable and condition wrapping $missing_post_update_functions and return FALSE in last line of hasModuleUpdates()

I don't think that the patch follow the right approach regarding post updates.

Post update hooks are for applying content changes. However, content changes might require config changes to work, e.g. having a new field added. Thus, imo the right order of things would be
1. updates
2. config import
3. post updates

There is also a PR to drush what the above that approach possible using drush commands: https://github.com/drush-ops/drush/pull/2740

That said, I think the patch should check for all updates being applied, but not for all post-updates being applied.

#31: I agree, attached the patch. Also I tested the failing test on 8.4.x and it worked there, so lets see if the CI likes it better now.

#33: Well, this is a very common problem when building sites indeed.
But I think the workflow you propose is flawed because hook_post_update_NAME is for fixing content you broke in a hook_update_N function since in those functions you can not assume to have the API available. So post_update hooks should always follow the update hooks immediately.
On the other hand what you describe is a hook that should be run after configuration deployment to fix content that had been broken due to configuration changes. It is natural to turn to the hook that is already there, and we had a couple of projects that manually adjusted the configuration in the hook so that the content could be fixed in that hook. But then I discovered that such a post_config_import hook is not difficult to do. Here is the code we use to this effect: https://github.com/bircher/drupal-config_import_n I would have to add some more automated tests and create a project on d.o, but maybe instead we would want to have that as part of drupal core directly.

But I think the workflow you propose is flawed because hook_post_update_NAME is for fixing content you broke in a hook_update_N function since in those functions you can not assume to have the API available.

I can't see why post-update should be only valid for this case. The documentation says:

Executes an update which is intended to update data, like entities.

This is exactly what I'm trying to do in #33 - so the hook seems to be fine?

Adding a separate post-config-import update would be an option, but I'm not convinced we need pre + post config import updates. After running updates, the system should be in a working state and it should be able to process config imports, no? I don't see data migrations would have to be run before config import when we have it afterwards?

You are right, there is a perfectly valid use case for updating entities in a post_update hook and the documentation is not wrong.

But I think it makes sense to distinguish between an update to an entity because of configuration changes due to schema changes in a (contrib) module, and due to configuration change as part of the site building process. Because in the first one the configuration changes in the update hook and hence the post_update hook is the perfect place to change the content entity. But the second changes the configuration as part of the configuration import and in my opinion the content should change in the post_config_import hook.

Of course since we currently only have the post_update hook it seems natural to use it for both cases and not differentiate between the two. But that leads to the problem that sometimes you want to import the configuration before the update. And that is because (as of now) both drush and the drupal UI do hook updates and post_updates at the same time (one after the other). And in my opinion that is a good thing.
"Sometimes" is very bad for repeatable scripts, and ergo I am suggesting to use a different hook in those times.

You are right that we don't necessarily need a pre and post config import hook since the post_update hook *is* also the pre config import hook. But I added both to my proof of concept to differentiate what kind of changes we are talking about. And as such a pre import hook would save the data of a field to be removed and the post import hook would add it to the new field. This is for the use case of site building where in the site development process you remove a field and add a new one (of say a new type with the same name).
So the argument is more for DX that you can say these two pre and post import hooks go together and this post_update goes with that update hook.

Right now without this patch there is:

1. config import
2. update hooks
3. post_update hooks

(bad) and (good)

1. update hooks
2. post_update hooks
3. config import

With the patch there is only:

1. update hooks
2. post_update hooks
3. config import

You would like:

1. update hooks
2.  
3. config import
4. post_update hooks

And I propose as a follow up:

1. update hooks
2. post_update hooks
3. config import
4. post_config_import hooks

or possibly:

1. update hooks
2. post_update hooks
3. pre_config_import hooks
4. config import
5. post_config_import hooks

But I think that should be a follow-up and not block the issue at hand.

update hooks
post_update hooks
pre_config_import hooks
config import
post_config_import hooks

That gets my vote. The pre_config_import hook is a great place to pull out config before it gets changed. It would naturally get used by the post_config_import hook.

ok, I can follow the reasoning outlined by bircher - I agree it makes to have a post update hook + a post config import hook.

That gets my vote. The pre_config_import hook is a great place to pull out config before it gets changed. It would naturally get used by the post_config_import hook.

I cannot follow that use case though, can you elaborate on this? What do you mean with pulling out config and how does that relate to post-config?

This makes total sense to me, but I'm wondering whether we need the pre-config hook:

update hooks
post_update hooks
pre_config_import hooks ??
config import
post_config_import hooks

Still, of course this would need a documentation update to clarify when to use what.

Sorry, "pull out config" is a vague term. I think whats desired is a way to store in memory relevant config values in a pre_config_import hook and then use those values in a post_config_import hook

I guess we should get this in, and work on pre/post_config_import hooks in a new issue. Maybe @bircher is up for creating it?

I did a code review of this patch and it LGTM. Further, I retested and bot is happy.

I have at least one concern with this issue in its current state.

Especially on custom projects, let's say you have a configuration change that adds a field, then a post update hook that populates the field for some entities. This patch would require you to write a hook_update_N() for the configuration change instead of being able to do a drush cim -y && drush updb. Sometimes field content is set dynamically, say if data is being denormalized, so the hook_update_N() exists purely to load and save affected entities with no other logic.

In this case you may very well have zero hook_update_N() to run at all - you're deploying one changeset which you control entirely. You might not even be deploying the change to production but only to dev/staging during medium stages of a project.

If we shut that workflow off (which I've used personally, why write a hook_update_N() unless you have to), it undoes some of the convenience of configuration management.

Also missing a change record and the issue summary isn't clear, but I think we probably need to either add the new hooks here rather than in a follow-up.

Hmm

I see your concern. However, I do not really understand the difference between writing a `hook_update_N` and a `hook_post_config_import_NAME` for a one-off hook that doesn't do anything dangerous and is meant to run after configuration imports.

The above mentioned hook exists in https://github.com/bircher/drupal-config_import_n which I can make into either a contrib project or a core patch #2901418: Add hook_post_config_import_NAME after config import if we decide to block this issue on having the hooks in core.

We had a production discussion on this topic during Drupalcon Vienna sprints. We being: alex pott, bircher, jibran, mbm80 and me.

Here is a short summary of our conclusions:

• post-update hooks cannot be reliable for content imports for projects as content imports may rely on the (field) config changes, which are done during config import.
• content imports relying on config could be done with an event subscriber reacting on the config import and running the content import once the required config was imported. However this is not a nice DX as the developer would have to take care of the run-only-once update logic, plus the code would be in the code-base all the time vs only during running updates.
• to improve DX for this content-import use-case a hook_post_config_import_NAME() receiving the config importer $event is the way to go -> let's do this in #2901418: Add hook_post_config_import_NAME after config import
• given that we can require all updates to be run before config-import is run (this issue)
• we also noted that hook-install() cannot be used for content imports in modules as module cannot rely on default config to be there then (it's not duing config import). But that's another topic.

we also noted that hook-install() cannot be used for content imports in modules as module cannot rely on default config to be there then (it's not duing config import). But that's another topic.

Created #2914213: Don't create content in install hooks if module is installed during config import for this.

+++ b/core/modules/system/src/SystemConfigSubscriber.php
@@ -78,6 +100,52 @@ public function onConfigImporterValidateSiteUUID(ConfigImporterEvent $event) {
+    // Check post update hooks.
+    $missing_post_update_functions = $this->postUpdateRegistry->getPendingUpdateFunctions();
+    if (!empty($missing_post_update_functions)) {
+      return TRUE;
+    }

I think we should only check for hook_update_N(). Those updates are supposed to repair the DB. If such updates are successful the config import should be safe.

As an alternative to #2901418: Add hook_post_config_import_NAME after config import, we're using this sequence, in order to run content manipulations with the new installed modules and updated configs:

drush updb --no-post-updates
drush config:import
drush updb

In this way we are able to run content manipulations, in hook_post_update_NAME(), that are relying on the updated configs and new modules installed by config:import.

EDIT: And this was supposed to be the initial scope of hook_post_update_NAME(), to run updates on top of the healed API. With config import, we're installing new modules & updating config, and that is part of “fixing the API”, in the same way as hook_update_N() is.

EDIT2: I've missed the @fago's comment from #33. It's exactly the same solution.

Since I closed #2901418: Add hook_post_config_import_NAME after config import which was listed here as a solution for why some people were against this change I will also link the better way to handle this situation here: Drush deploy documentation

So there is nothing stopping us from doing this.

Re-roll the patch for 8.9.x-dev

opss sorry @kapilkumar0324, I haven't seen you assign this issue yourself

hear a reroll patch.

1. +++ b/core/modules/system/src/SystemConfigSubscriber.php
   @@ -81,6 +115,50 @@ public function onConfigImporterValidateSiteUUID(ConfigImporterEvent $event) {
   +    foreach ($this->moduleHandler->getModuleList() as $module => $filename) {
   

   We are not using the $filename in the loop, than we therefor change it to: foreach (array_keys($this->moduleHandler->getModuleList()) as $module) {.

2. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdatePendingConfigImportTest.php
   @@ -0,0 +1,102 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function doSelectionTest() {
   +    if ($this->getName() === 'testConfigImportWithHookUpdateN') {
   +      $this->assertSession()->pageTextContains('9001 - Pending update.');
   +      $this->assertSession()->pageTextNotContains('Pending post update.');
   +    }
   +    else {
   +      $this->assertSession()->pageTextContains('Pending post update.');
   +      $this->assertSession()->pageTextNotContains('9001 - Pending update.');
   +    }
   +  }
   

   I think that this method should be added to the method doTest().

3. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -739,6 +739,52 @@ protected function installSchema($module, $tables) {
   +   * Sets the update registry state for the provided modules.
   

   Nitpick: can we add "as fully updated"?

4. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -739,6 +739,52 @@ protected function installSchema($module, $tables) {
   +  protected function installUpdateRegistryState(string|array $modules): void {
   

   The testbot failing for PHP 7.4. Probably for string|array.

Can't apply the Patch on #72 against Drupal 9.4.x

Thanks for your suggestions @daffie
I have addressed the point 1. and 3. on comment #73
Here, I have Attached patch against Drupal 9.4.x

There's a lot of references to hook_post_config_import_NAME in these comments and in related issues, but according to api.drupal.org, no such hook exists. I'd like to know what the solution to the problem posed in #41 will be. I've always run config import before database updates. That's because the primary purpose of database updates (in my opinion) is to modify content and the purpose of config is to modify structure. We need structure in place before we can create content that relies on that structure. Doing the opposite feels very counterintuitive to me but I can see that almost everyone disagrees with me. It's certainly got me scratching my head.

I'm not here to tell you not to add this requirement. I think the fact that there's confusion about which to run first is a problem. But I'd really like to know how you all would solve the issue of the hook_update creating content for a field that does not exist yet because config has not been imported. EDIT: Without deploying twice (one deploy with the config and the next with the hook_update).

The hook_post_config_import_NAME idea has been replaced with hook_deploy_NAME in drush.
https://www.drush.org/latest/deploycommand/

The deploy hooks are not supported by drupal (yet) but that is a different issue. Deploy hooks are not meant for contrib modules or core, since they are meant to be "owned" by the same git repository as the exported config. But if this is a need, then the UI for deploy hooks would be relatively easy to implement as a contrib module.

From my understanding update hooks are for fixing the database schema to work with the latest codebase, post update hooks are for fixing the data that the update hook broke. Config import changes the structure and deploy hooks can update content knowing the config has been imported.

Ah thank you for the info! So there is no working solution for my question yet, but eventually it will be to use hook_deploy_NAME and then modify my deploy script like this: drush updb -y; drush cim -y; drush deploy:hook -y? Does the "NAME" part of hook_deploy_NAME matter or does it just need to be unique?

Ah no! you misunderstood!
Drupal does not have a UI to run deploy hooks (yet). But deploy hooks are a thing already for a while and work in production with the stable drush version since two years.

So since you are using drush this is already available to you.
Deploy hooks work the same way as post update hooks.