[UX] Config import should list all invalid configs, not just the first one

[cellear]

Description of the need

When syncing a configuration full-import, Configuration Management checks each queued config file to make sure it's valid, and refuses to sync if any are not -- for example, a configuration for a module that's not installed. It immediately quits when it finds an invalid config. If there is more than one invalid config (and there often are) the user doesn't see them until they try to run another sync. This is frustrating and scary, since there's no way to know how many config files have problems. It's easy to think there might be a lot of problems, and tempting to quit out of fear and frustration.

Proposed solution

Rather than quit after the first bad file is found, Configuration Management should continue to check all the other files, and present the user with a complete list of all the errors found. That way the user will know how big the problem is, and might be able to fix them all at once.

Draft of feature description for Press Release (1 paragraph at most)

"Backdrop's Configuration Management system now checks all config files when a full sync is performed, saving time and reducing uncertainty when importing configurations."

[cellear]

I brought this up yesterday on Zulip, and proposed a change to config.sync.inc in the core config module:

cellear (Luke McCormick): It seems that I'll need to change this function, function _config_sync_validate

/**
 * Batch API callback. Validate the changes before attempting to sync.
 */
function _config_sync_validate(&$context) {
  if (!isset($context['sandbox']['batch_size'])) {
    $context['sandbox']['batch_size'] = config_get('system.core', 'config_sync_batch_size');
    $context['sandbox']['total_size'] = count($context['results']['pending_changes']);
    $context['sandbox']['current_index'] = 0;
  }

  $config_files = array_slice($context['results']['pending_changes'], $context['sandbox']['current_index'], $context['sandbox']['batch_size']);
  try {
    foreach ($config_files as $config_file => $config_change_type) {
      config_sync_validate_file($config_file, $config_change_type, $context['results']['all_configs']);
      $context['sandbox']['current_index']++;
    }
  }
  catch (ConfigValidateException $e) {
    $context['results']['errors'][] = $e->getMessage();
    $context['finished'] = 1;
    return;
  }

  $context['finished'] = $context['sandbox']['total_size'] ? $context['sandbox']['current_index'] / $context['sandbox']['total_size'] : 1;
}

cellear (Luke McCormick): in combination with this bit of code in config_sync_validate_file:

// First check that a module claims this config file. Unclaimed configs cannot
// be managed at all, because hooks may need to be fired when performing any
// kind of change (e.g. a deleted config might need to remove database
// tables).
$config_info = config_get_info($config_name);
if (empty($config_info)) {
  if ($config_change_type === 'delete') {
    $message = t('The configuration "@name" is not owned by any enabled module, so it cannot be deleted. If you have disabled this module, either <a href="!enable">enable the module</a> and try the import again, or <a href="!uninstall">uninstall the module entirely</a> to clean up left-over configuration.', array('@name' => $config_name, '!enable' => url('admin/modules'), '!uninstall' => url('admin/modules/uninstall')));
  }
  else {
    $message = t('The configuration "@name" is not owned by any module. Try enabling the module that provides this configuration, then importing again.', array('@name' => $config_name));
  }
  throw new ConfigValidateException($message);
}

cellear (Luke McCormick): Could I move that throw to someplace outside the foreach loop?

cellear (Luke McCormick): So instead of throw-ing $message, add it to an array of errors and set a "dirty" flag of some sort, then add an if after the foreach in _config_sync_validate that checks for the flag and throws an exception with all of the errors if it finds a problem.

[cellear]

After asking a few questions, @hosef proposed this change:

$config_files = array_slice($context['results']['pending_changes'], $context['sandbox']['current_index'], $context['sandbox']['batch_size']);
foreach ($config_files as $config_file => $config_change_type) {
  try {
    config_sync_validate_file($config_file, $config_change_type, $context['results']['all_configs']);
  }
  catch (ConfigValidateException $e) {
    $context['results']['errors'][] = $e->getMessage();
  }
  $context['sandbox']['current_index']++;
}

Joseph Flatt: I think that if you replace the try catch block with that^ it should give you what you want.

[cellear]

@hosef's code seems to do everything I was hoping for! The new, more usable error message looks like this:

[docwilmot]

Joseph (who I can't seem to tag)

I believe he is @hosef

[stpaultim]

I would just like to speak in favor of the utility of this change. I've been through this process before, where config management provides error messages one by one and I have to deal with them one by one, instead of being able to fix them all at once.

I think that in theory this would make config management a bit easier to use, especially for folks that are new to using config management with Backdrop CMS.

[cellear]

I made a PR for this, but it doesn't seem to be clearing tests:

backdrop/backdrop#3683

It says it's failing two tests, but I don't know how to see what those are.

[hosef]

@cellear It looks like you included a change that removes a setting to control if it should delete the contents of the staging directory. I think that is what is causing the tests to fail.

[cellear]

@cellear It looks like you included a change that removes a setting to control if it should delete the contents of the staging directory. I think that is what is causing the tests to fail.

I thought I just copied the code that you sent over in Zulip; I'm still trying to process how it works :) Did I includes something that was outside of that? (I'm checking it myself now, to see if I can understand what you mean.)

[klonos]

@cellear I believe that @hosef is referring to the removal of the if (config_get('system.core', 'config_sync_clear_staging')) conditional, at line ~100 of the core/modules/config/config.sync.inc file. This seems unrelated (or at least I don't see how it helps with issue here), and would need to be reverted.

[cellear]

@cellear I believe that @hosef is referring to the removal of the if (config_get('system.core', 'config_sync_clear_staging')) conditional, at line ~100 of the core/modules/config/config.sync.inc file. This seems unrelated (or at least I don't see how it helps with issue here), and would need to be reverted.

Sooo...if I put that back, it might pass the tests?

I imagine I inadvertently deleted it, when I manually patched the file.

I'll give that a shot

[cellear]

Hi guys. I made the change, and I'm getting an error that doesn't seem to be related to the code I changed at all. Is it possible that the base I was working from wasn't passing these? (I may need to resync my branch.)

Test run duration: 8 min 7 sec

Detailed test results
---------------------


---- Date: Date Theme (DateThemeTestCase) ----


Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Other      date_themes.test   150 DateThemeTestCase->testDateDisplayC
    Start and end date on same day with timezone, with remaining days is
    rendered correctly.

[klonos]

Yes, this is unrelated @cellear (this is one of the "usual" random failures that I thought we had gotten rid off, but apparently not 🙄 ...I even mentioned it during our dev meeting this past week)

I can usually get it to go away by waiting for 1hr, and then closing/reopening the PR.

[klonos]

@cellear the only thing I see now wrong with the PR is indentation (2 additional leading spaces in your code).

[cellear]

I'll fix that and resubmit in an hour. Thanks, @klonos!

[cellear]

Hmm...I can't figure out what's wrong with my indentation. Is it in this function?

/**
 * Batch API callback. Finish the config import.
 */
function _config_sync_finished($status, $results, $operations) {
  if ($status === FALSE) {
    backdrop_set_message(t('Configuration sync failed. Check that the configuration files are properly formatted.'), 'error');
  }
  if (!empty($results['errors'])) {
    backdrop_set_message(t('Configuration sync failed. The following errors were reported:') . ' ' . theme('item_list', array('items' => $results['errors'])), 'error');
  }
  if (!empty($results['completed_changes'])) {
    backdrop_set_message(t('Configuration sync completed. @files configuration files synced.', array('@files' => count($results['completed_changes']))));
    if (config_get('system.core', 'config_sync_clear_staging')) {
      // Clean up the staging directory.
      $config_dir = config_get_config_directory('staging');
      $config_storage = new ConfigFileStorage($config_dir);
      $config_storage->deleteAll();
    }
  }

  backdrop_flush_all_caches();
  state_del('config_sync');
}

I tried running phpcbf against it, but it found 64 errors just in that file! (Presumably I need to configure it to understand our conventions, it's set to Acquia/BLT D8.)

[klonos]

This is what I see when I visit https://github.com/backdrop/backdrop/pull/3683/files @cellear:

My view shows spaces as "dots", and you can see on the right side that there are 4 dots instead of 2 when compared to the left side. I hope it helps.

[klonos]

...it's in _config_sync_validate() - not in _config_sync_finished().

[cellear]

Cool, found it. And learned that I can put Github diffs into side-by-side and dark mode, which I didn't realize and have now set.

Though...my previous push just passed, so I don't know if I want to mess with it :)

[klonos]

Though...my previous push just passed, so I don't know if I want to mess with it :)

Your previous push didn't fix the indentation that was wrong though, so it can't be merged as is 😅

Sorry that the random failures have caused you to be scared of "breaking things" @cellear; we are hoping to sort them out, but in the meantime you will need to learn to close/wait/reopen your PRs till they go away. If it helps, I have come to believe that the specific random failure is being triggered by opening the PR a specific hour of the day (which happens to be between the 11pm to midnight window in some part of the world - the timezone of the server running the tests). Once you have waited for about 1hr, then close/wait/reopen, then that window of hours that triggers the failure will have passed.

[cellear]

Ok, I've fixed the spacing...I think :)

Checks running now.

[klonos]

Ok, I've fixed the spacing...I think :)

Not all of it @cellear 😅 ...see my code suggestion here: https://github.com/backdrop/backdrop/pull/3683/files#r693607879

[cellear]

At last:

[cellear]

So, @klonos: Are we good to go? Can it be merged? Or are there more steps? Does it need to be RTBCed now, or is that something different?

[stpaultim]

@cellear - Someone other than yourself has to test it and review the code, before they mark it as RTBC.

[klonos]

One last indentation issue @cellear (I could not add this in my code suggestion, since that line was unchanged). This line needs to be reduced by 2 spaces: https://github.com/backdrop/backdrop/pull/3683/files#diff-356e569f8df8bf2cd86a287254047428d456326b7ec1e986ef4cc932a22cffe6R56 ...after this has been done, the code is RTBC by me, and all that remains is testing and setting the issue to "works for me".

[stpaultim]

Success. I tested the patch on a site that I've been using to test config sync and it worked great.

Instead of getting these messages one at time, I get them all at once. In my pre-patch tests I was never able to see all the problems, because as I tried to resolve them one at a time my site eventually broke before I got to the end of the list (but that is another issue). Anyway, I never knew if I was close to the end of the list or not. (Turns out I was).

I'm going to mark this "Works for Me."

@klonos - Can you take another look at the code? @cellear indicated in Slack that he has dealt with your other comments.

[klonos]

Yup, code looks good to me now 👍🏼 ...thanks for persisting through the multiple code reviews and requests for changes @cellear 🙏🏼

Now, if someone can set the milestone, we're set 😉

[quicksketch]

+1 RTBC from me. backdrop/backdrop#3683 looks great.

[stpaultim]

Discussion during meeting today: https://youtu.be/CoXHmSUGsXg?t=854

[laryn]

Thanks to @cellear for persisting on this one, and thanks to @hosef for the code suggestion that helped out. Thanks for comments and reviews @klonos, @quicksketch, and @stpaultim. Given all the attention and testing and approval I've merged backdrop/backdrop#3683 for 1.x and 1.19.x.