Problem/Motivation

Follow-up to #2388867: Notifying user of config changes when config has never been synched makes no sense. Discovered during #2416109: Validate configuration dependencies before importing configuration.

In #2148199: Use snapshot to warn users if the configuration has changed since last import, we added functionality to warn the user when configuration has changed since the last config import, to protect site owners from unintentionally overwriting their live configuration with older configuration (for example, if content authors changed configuration on a production site and then additional changes were deployed from the development environment).

Your current configuration has changed. Changes to these configuration items will be lost on the next synchronization:

This warning is displayed when the config differs from the last snapshot, and before #2388867: Notifying user of config changes when config has never been synched makes no sense, a snapshot was created on installation, resulting any configuration change after installation results on the message being "stuck" on the config sync page even when there were no changes to import.

#2388867: Notifying user of config changes when config has never been synched makes no sense resolved the issue immediately after installation by no longer creating a snapshot on installation, but instead only whenever configuration was imported. However, it is still possible to end up in a situation where there are not actually any changes that will be "lost" on the next import, and so the message is still confusing. One way to reproduce this is to install using https://www.drupal.org/project/config_installer

  1. Make a site_config directory and run git init --bare.
  2. Install standard for site A.
  3. Clone the site_config repo into site A's staging directory.
  4. Run drush cex -y on site A, commit, push.
  5. Create a site B from site A (either by copying codebase, files, and db, or by installing using the config_installer project).
  6. Clone the site_config repo into site B's staging directory as well.
  7. On site B, add the tags field to the page content type.
  8. Export the configuration, commit, push.
  9. Visit /admin/config/development/configuration/. You will see the worrying warning about changes that will be lost, without really understanding how or why you would lose them.

In general, D8 users (including me) also repeatedly confuse the contents of this message with the contents of the form below it, which displays the differences between the active config and staging. The warning, on the other hand, only displays the differences between the last snapshot and staging, which could be a subset of the changes between active and staging, or could be changes that were made and then reverted, or (as in the example above) be completely irrelevant to the user's workflow.

Proposed resolution

See #2388867: Notifying user of config changes when config has never been synched makes no sense for a list of potential solutions that were ruled out previously.

@alexpott and I discussed this a bit and came up with three things to improve the usability:

  1. There is probably no reason to display the warning at all if there are no changes to import.
  2. Improve the message so it explains what's actually happening: the configuration is different from the snapshot, which may result in the changes being list. Maybe something like:

    The following changes have been made to your active configuration since the last import, and may be lost on the next import:

  3. See if we can improve the UI somehow so that it doesn't seem like the message is part of the form below it? Not really sure what this would mean.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because the purpose of the warning is to help the user understand what will happen when configuration is synchronized, but the current behavior instead obfuscates that under certain circumstances.
Issue priority Normal because (unlike #2388867: Notifying user of config changes when config has never been synched makes no sense) this "stuck" message will not be encountered that frequently using just core.
Prioritized changes The main goal of this issue is usability.

Remaining tasks

None.

User interface changes

The active configuration changed because on snapshot storage message is only displayed when a configuration sync can be performed. The text has been changed to make it easier to understand.

API changes

None.

CommentFileSizeAuthor
#11 1-10-interdiff.txt2.82 KBalexpott
#10 notifying_user_of-2486475-10.patch5.69 KBEli-T
#6 Screen Shot 2015-05-16 at 13.47.52.png106.85 KBalexpott
#4 edit_issue_notifying-2486475-1.patch5.71 KBEli-T
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue summary: View changes
Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented
Assigned: Unassigned » Eli-T
Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented
Issue summary: View changes
Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented
Issue summary: View changes
Status: Active » Needs review
FileSize
edit_issue_notifying-2486475-1.patch5.71 KB

This patch implements suggestions 1 and 2 of the proposed resolutions.

Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented
Assigned: Eli-T » Unassigned
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
Screen Shot 2015-05-16 at 13.47.52.png106.85 KB

I've manually tested the patch and it works as I expect. There is test coverage and the patch complies with coding standards.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs review

Nice work, thanks @Eli-T. I think this is the best way to fix it, and the added test coverage is good. And thanks @alexpott for the manual testing. 

+++ b/core/modules/config/src/Tests/ConfigExportImportUITest.php
@@ -187,14 +187,25 @@ public function testExportImport() {
+    $this->assertNoText(t('Warning message'));
+    $this->assertNoText('The following changes have been made to your active configuration since the last import, and may be lost on the next import: system.site');

+1 for this first assertion as it helps ensure that the second is less likely to be a false pass if the string gets changed later.

Looking at the screenshot, I think that the message I suggested in the OP is not quite correct. What's listed under the header isn't changes... it's items that have changed. So should it be:

The following items in your active configuration have changes since the last import that may be lost on the next import.

Thoughts? (Sorry... this is my earlier mistake.) Setting NR for that.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: +Needs usability review

Maybe some usability feedback would help.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue summary: View changes
Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented
FileSize
notifying_user_of-2486475-10.patch5.69 KB

Changed text as suggested in #7

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs review » Reviewed & tested by the community
FileSize
1-10-interdiff.txt2.82 KB

Looks good to me. Uploading interdiff.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs usability review
Related issues: +#2247291: Reorder tabs in configuration UI

Cool, I think that works. Untagging for UX review since it seems we agree. :)

+++ b/core/modules/config/src/Form/ConfigSync.php
@@ -166,29 +166,6 @@ public function getFormId() {
-    if ($this->snapshotStorage->exists('core.extension')) {

@@ -211,11 +188,35 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    if ($this->snapshotStorage->exists('core.extension')) {

I confirmed this hunk is mostly just moved code so that we only check the snapshot when there are changes to report and site UUID is valid. Other than the code flow, the changes between this and ConfigSync in HEAD are:

-        drupal_set_message($this->t('Your current configuration has changed. Changes to these configuration items will be lost on the next synchronization: !changes', array('!changes' => $change_list_html)), 'warning');
+        drupal_set_message($this->t('The following items in your active configuration have changes since the last import that may be lost on the next import. !changes', array('!changes' => $change_list_html)), 'warning');

(So updating to the newer, more correct warning.) And:

-    else {
-      // Store the comparer for use in the submit.
-      $form_state->set('storage_comparer', $storage_comparer);
-    }
+    // A list of changes will be displayed, so check if the user should be
+    // warned of potential losses to configuration.
+
+    // Store the comparer for use in the submit.
+    $form_state->set('storage_comparer', $storage_comparer);

(Getting rid of the else, because the other two branches in the code flow have already returned anyway, plus adding a clarifying comment.)

As a usability improvement and a bugfix, this issue is a prioritized change as per https://www.drupal.org/core/beta-changes and its benefits outweigh any disruption. Committed and pushed to 8.0.x. Thanks @Eli-T and @alexpott!

  • xjm committed 1d82edf on 8.0.x 
    Issue #2486475 by Eli-T, alexpott, xjm: Notifying user of config changes...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue summary: View changes

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.