Follow-up to #2247291: Reorder tabs in configuration UI. This was originally addressed in that issue but there is ongoing discussion about the other changes in that issue. The confirmation dialog part of the issue is being separated here so that it isn't held back whilst the correct solution for the other changes is sought.

Problem/Motivation

There is no confirmation form and the operation is certainly destructive

Proposed resolution

Add a confirm dialog before performing a full import

Remaining tasks

#2642420: Selectively remove files from full import

User interface changes

As listed in Proposed Resolution

API changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because current UI works but is not as usable as it could be
Issue priority Major because having a usable UI is important for D8
Prioritized changes The main goal of this issue is usability.
Disruption None.
Files: 
CommentFileSizeAuthor
#18 add_confirm_dialog-2572503-18.patch31.51 KBEli-T
#2 add_confirm_dialog-2572503-2.patch30.27 KBEli-T
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,801 pass(es), 2 fail(s), and 0 exception(s). View
#5 add_confirm_dialog-2572503-5.patch32.35 KBEli-T
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,779 pass(es). View
#6 interdiff-2247291-2572503.txt3.03 KBEli-T
#15 sync-confirm.png20.11 KBswentel
#15 interdiff.txt848 bytesswentel
#15 add_confirm_dialog-2572503-15.patch31.54 KBswentel

Comments

Eli-T created an issue. See original summary.

Eli-T’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
30.27 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,801 pass(es), 2 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 2: add_confirm_dialog-2572503-2.patch, failed testing.

The last submitted patch, 2: add_confirm_dialog-2572503-2.patch, failed testing.

Eli-T’s picture

Status: Needs work » Needs review
FileSize
32.35 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,779 pass(es). View
Eli-T’s picture

Adding interdiff to configuration-2247291-98.patch, the most recent patch on the parent issue from which this was split and has previously been RTBC.

Eli-T’s picture

Assigned: Eli-T » Unassigned
jhodgdon’s picture

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.

Eli-T’s picture

Just to manage expectations, I'm away for the next two weeks so will address points above on my return.

tkoleary’s picture

Issue summary: View changes
Eli-T’s picture

Is that a remaining task, or a related issue?

tkoleary’s picture

cilefen’s picture

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.

swentel’s picture

Reroll + diff + screenshot

The last submitted patch, 15: add_confirm_dialog-2572503-15.patch, failed testing.

claudiu.cristea’s picture

Status: Needs review » Needs work
  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()/[]

Eli-T’s picture

Reroll against latest 8.1.x before addressing points in #2572503-17: Add confirm dialog before full configuration import

Eli-T’s picture

https://www.drupal.org/files/issues/add_confirm_dialog-2572503-18.patch is bad as it applies cleanly but doesn't take the changed code that made it failed to apply into account when it moves it elsewhere. I will fix later.

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

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

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.

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.