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

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