Add confirm dialog before full configuration import

Hm... I have a few questions/concerns about this patch:

a) Why is the confirm form being generated from a form state redirect? That is not the normal way to get to a confirm form. Normally, when you have a destructive operation, the trigger for getting to the confirm form is an ordinary link, while here for some reason it is a form submit with redirect set, which doesn't make sense to me and might not always work (sometimes the form system doesn't redirect the way you think it should, like for instance if some URL query parameters are set).

I guess I don't understand why the original page is still a form at all, since the submit just redirects. Couldn't it be an ordinary page with a link that would be used to import the shown config?

b) The new confirm form class has no documentation header. It definitely must have one.

c) For future reviews, a screen shot would be helpful. It's not so easy to set up a site to test this.

Added to config management usability meta #2642404: [meta] Usability improvements to configuration management post 8.0

From the issue summary:

There is no confirmation form and the operation is certainly destructive

The argument can be made that this is a critical bug.

Reroll + diff + screenshot

1. +++ b/core/modules/config/src/Form/ConfigSync.php
   @@ -7,18 +7,12 @@
    use Symfony\Component\EventDispatcher\EventDispatcherInterface;
   

   Not in use. Remove.

2. +++ b/core/modules/config/src/Form/ConfigSync.php
   @@ -115,35 +74,20 @@ class ConfigSync extends FormBase {
   -  public function __construct(StorageInterface $sync_storage, StorageInterface $active_storage, StorageInterface $snapshot_storage, LockBackendInterface $lock, EventDispatcherInterface $event_dispatcher, ConfigManagerInterface $config_manager, TypedConfigManagerInterface $typed_config, ModuleHandlerInterface $module_handler, ModuleInstallerInterface $module_installer, ThemeHandlerInterface $theme_handler, RendererInterface $renderer) {
   +  public function __construct(StorageInterface $sync_storage, StorageInterface $active_storage, StorageInterface $snapshot_storage, ConfigManagerInterface $config_manager, RendererInterface $renderer, KeyValueStoreExpirableInterface $key_value_expirable) {
   

   Well, I guess no contrib tried to extend this because this is a BC break. In a conservative approach I would suggest to keep the signature and create a protected getter for the key-values store. Also we add a @todo to inject the service in 9.0.x. But normally there should be no use-case ti extend this form. Opinions?

3. +++ b/core/modules/config/src/Form/ConfigSync.php
   @@ -247,8 +186,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -        // @todo A table caption would be more appropriate, but does not have the
   -        //   visual importance of a heading.
   +        // @todo A table caption would be more appropriate, but does not have
   +        //   the visual importance of a heading.
   
   @@ -298,7 +237,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -                'width' => 700
   +                'width' => 700,
   

   Look as not-related?

4. +++ b/core/modules/config/src/Form/ConfigSyncConfirmForm.php
   @@ -0,0 +1,290 @@
   +          'title' => t('Synchronizing configuration'),
   +          'init_message' => t('Starting configuration synchronization.'),
   +          'progress_message' => t('Completed @current step of @total.'),
   +          'error_message' => t('Configuration synchronization has encountered an error.'),
   

   $this->t()

5. +++ b/core/modules/config/src/Form/ConfigSyncConfirmForm.php
   @@ -0,0 +1,290 @@
   +            array(get_class($this), 'processBatch'),
   +            array($config_importer, $sync_step),
   

   s/array()/[]

6. +++ b/core/modules/config/src/Form/ConfigSyncConfirmForm.php
   @@ -0,0 +1,290 @@
   +        drupal_set_message(\Drupal::translation()->translate('The configuration was imported with errors.'), 'warning');
   ...
   +        drupal_set_message(\Drupal::translation()->translate('The configuration was imported successfully.'));
   ...
   +      $message = \Drupal::translation()->translate('An error occurred while processing %error_operation with arguments: @arguments', array(
   

   $this->t()

7. +++ b/core/modules/config/src/Form/ConfigSyncConfirmForm.php
   @@ -0,0 +1,290 @@
   +      // An error occurred.
   +      // $operations contains the operations that remained unprocessed.
   

   Why line break? The 2nd phrase should continue on the 1st line.

8. +++ b/core/modules/config/src/Tests/ConfigExportImportUITest.php
   @@ -173,6 +173,11 @@ public function testExportImport() {
   +    $this->drupalPostForm('admin/config/development/configuration/sync/confirm', array(), t('Import'));
   

   s/array()/[]

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

wanted to bump 1 more time if still needed.