Problem/Motivation

Talks about #2628144: Ensure that ConfigImport is taking place against the same code base making config import more reliable. There is some contention because it reduces some of the abilities of developers to do what they please. This issue is going to do something simpler. Ensure that when running configuration import there are no outstanding database updates to run.

Proposed resolution

Add validator to ensure that no outstanding updates need running.

Remaining tasks

User interface changes

Additional configuration import validation messages

API changes

None

Data model changes

None

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

alexpott created an issue. See original summary.

dawehner’s picture

+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.

alexpott’s picture

@dawehner this is why #2628144: Ensure that ConfigImport is taking place against the same code base - I think that that is the only real way to deal with this - but this is a first and less controversial step.

alexpott’s picture

Status: Needs review » Needs work
moshe weitzman’s picture

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.79 KB

Initial patch...

alexpott’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
+++ b/core/modules/system/src/SystemConfigSubscriber.php
@@ -78,6 +78,40 @@ public function onConfigImporterValidateSiteUUID(ConfigImporterEvent $event) {
+    $updates = update_get_update_list();

There are also post updates to check for. This is the code from system_requirements()...

    // Check installed modules.
    $has_pending_updates = FALSE;
    foreach (\Drupal::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) {
      /** @var \Drupal\Core\Update\UpdateRegistry $post_update_registry */
      $post_update_registry = \Drupal::service('update.post_update_registry');
      $missing_post_update_functions = $post_update_registry->getPendingUpdateFunctions();
      if (!empty($missing_post_update_functions)) {
        $has_pending_updates = TRUE;
      }
    }
vijaycs85’s picture

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)

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

swentel’s picture

Status: Needs work » Needs review
FileSize
7.3 KB

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 :)

Status: Needs review » Needs work

The last submitted patch, 11: 2762235-11.patch, failed testing.

swentel’s picture

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.

swentel’s picture

Status: Needs work » Needs review
FileSize
7.99 KB

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

swentel’s picture

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)

Status: Needs review » Needs work

The last submitted patch, 15: 2762235-15.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
9.22 KB

This should fix the fails for classes using kernelTestBase.

swentel’s picture

stupid file_put_contents left over

The last submitted patch, 17: 2762235-17.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 18: 2762235-18.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
9.72 KB

This should be green

alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/src/SystemConfigSubscriber.php
    @@ -78,6 +78,53 @@ public function onConfigImporterValidateSiteUUID(ConfigImporterEvent $event) {
    +  protected function hasModuleUpdates() {
    

    Given that most of the code in this method is a copy of system_requirements() we might want to open a follow up to create an update list service or something similar.

  2. +++ b/core/modules/system/src/SystemConfigSubscriber.php
    @@ -78,6 +78,53 @@ public function onConfigImporterValidateSiteUUID(ConfigImporterEvent $event) {
    +    foreach (\Drupal::moduleHandler()->getModuleList() as $module => $filename) {
    ...
    +        if (max($updates) > $default) {
    ...
    +      $post_update_registry = \Drupal::service('update.post_update_registry');
    

    let's inject these things

  3. +++ b/core/modules/system/src/SystemConfigSubscriber.php
    @@ -78,6 +78,53 @@ public function onConfigImporterValidateSiteUUID(ConfigImporterEvent $event) {
    +      $updates = drupal_get_schema_versions($module);
    ...
    +        $default = drupal_get_installed_schema_version($module);
    

    There's nothing else to do other than call these procedural methods :(

  4. +++ b/core/modules/system/src/Tests/Update/UpdatePendingConfigImportTest.php
    @@ -0,0 +1,79 @@
    +class UpdatePendingConfigImportTest extends UpdatePathTestBase {
    +
    +
    +
    +  /**
    

    Lots of space :)

  5. +++ b/core/modules/system/src/Tests/Update/UpdatePendingConfigImportTest.php
    @@ -0,0 +1,79 @@
    +    // Use the UI to import (for some reason I can't get the config importer to
    +    // get rid of it's error log of pending database updates, and that error log
    +    // is probably stuck somewhere deeper).
    

    Static hmmm.... services are static. But after running updates we could rebuild the container and the config importer as before. I think to keep us sane do everything via the UI and assert on the error messages that would appear to a user. The test should have a comment that nothing should be done of the parent site and everything should be done on the system-under-test to test what a user would see.

swentel’s picture

Status: Needs work » Needs review
FileSize
11.75 KB
3.81 KB

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.

The last submitted patch, 23: 2762235-23.patch, failed testing.

swentel’s picture

UpdateRegistry doesn't have an interface though

And hence all the fails when injecting, the fun!

alexpott’s picture

So the question is do we want people to override the UpdateRegistry - I'm not sure about that. The simple thing is to make the test class extend the real class.

Status: Needs review » Needs work

The last submitted patch, 26: 2762235-26.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
503 bytes
12.16 KB

Shouldn't post a patch without running a couple of tests first...

zaporylie’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

+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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.4 KB
12.49 KB

Thanks for the review @zaporylie.

  1. fixed
  2. fixed
  3. The early returns are a good idea. The loops around modules exist inside $this->postUpdateRegistry->getPendingUpdateFunctions() so there is no performance benefit to switching the order - especially now we're doing early returns.
zaporylie’s picture

+++ 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()

Status: Needs review » Needs work

The last submitted patch, 30: 2762235-30.patch, failed testing.

fago’s picture

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.

bircher’s picture

#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.

fago’s picture

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?

bircher’s picture

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.

moshe weitzman’s picture

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.

fago’s picture

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.

moshe weitzman’s picture

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

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

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.

catch’s picture

Status: Reviewed & tested by the community » Needs work

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.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

bircher’s picture

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 if we decide to block this issue on having the hooks in core.