Use snapshot to warn users if the configuration has changed since last import

Is the form of this warning intended to be different than, or an enhancement to, "view differences" on admin/config/development/configuration?

Or, is this something we want to have displayed on admin/config/development/configuration/full/import if there are currently config differences?

This looks to me like it'd be fine to add after beta - will only be an interface change no?

Yep, this was supposed to be a beta target but looks like it didn't get updated when we decided that.

Since this is my first D8 core patch, I'd really appreciate any feedback.

To option to change between which storage to compare against (active or staging) is still missing. Using active storage as default for the time being.

Changes since previous patch:

• now it is possible to switch between active and staging storage
• other small fixes

+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.php
@@ -136,4 +146,72 @@ public function diff($config_file) {
+      $compare_button = l(t('Compare snapshot with staging storage'), $url . 'staging', array('attributes' => array('class' => 'button')));
+    }

The l() function should not be used in Drupal 8. There might be a link generation service now. Maybe link_generator?

1. +++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.php
   @@ -136,4 +146,72 @@ public function diff($config_file) {
   +      $compare_button = l(t('Compare snapshot with staging storage'), $url . 'staging', array('attributes' => array('class' => 'button')));
   

   If you change the class to extend ControllerBase, you can do the following:

   $compare_button = $this->l($this->t('Compare snapshot with staging storage'), 'config.snapshot_diff', array('storage' => 'staging'), array('attributes' => array('class' => array('button'))));

   You use the route name and an array that matches the parameters. Also note that attributes=>class should be an array, not a string.

2. +++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.php
   @@ -136,4 +146,72 @@ public function diff($config_file) {
   +      '#href' => 'admin/config/development/configuration',
   

   This can also be '#route_name' => 'config.sync'

3. +++ b/core/modules/config/lib/Drupal/config/Form/ConfigImportForm.php
   @@ -54,6 +59,15 @@ public function getFormId() {
   +        '@link' => $this->urlGenerator()->generateFromRoute('config.snapshot_diff', array('storage' => 'active')))), 'warning');
   
   +++ b/core/modules/config/lib/Drupal/config/Form/ConfigSync.php
   @@ -126,6 +136,16 @@ public function getFormId() {
   +        '@link' => $this->urlGenerator()->generateFromRoute('config.snapshot_diff', array('storage' => 'active')))), 'warning');
   

   Use $this->url()

Screenshot of suggested changes:

@jessebeach, @tim.plunkett: thanks for the review!
Managed to fix all issues, I think:

• ConfigController class now extends ControllerBase
• using #route_name instead of #href
• changed $this->urlGenerator()->generateFromRoute() to $this->url()
• moved compare button to the top of the page
• placed configuration name above the table
• table headers now showing the storage name
• removed "Back to 'Synchronize configuration' page." link
• slight change in warning message

Uploading patch again because tests didn't run.

14: show-snapshot-and-active-or-staging-storage-diff-2148199-13.patch queued for re-testing.

Just a reroll

Missed a comma

Left in an extra parameter.

Should we add "write tests" to remaining tasks or will that be a separate issue?

I started sketching out the tests.

Using randomName() instead of randomString() to test the slogan change because randomString() can be interpreted as HTML.

Let me know when its ready for review.

@alexpott Correct me if I am wrong on these:

1) That controller isn't the single import form. The warning does not appear on the single import form anyway.

3) $name is the config machine name, so I don't think it is translatable.

Ok, now I see. I think I am confused between "Import" in config system parlance with "import" as in importing a file.

Assuming the error is now displayed on the correct path...

@attila.fekete First, good job on this task. I am wondering if it would make better sense to display the "Compare snapshot with staging storage" tabs instead of buttons. After considering it, that is what I would expect to see as a site admin. What do you think about that?

@Bojhan This patch works and has its basic tests. You can probably review it now.

fix comment:

+   * Tests a warning appears when unsynchronized changes exist..

has 2 dots and maybe should be "Tests that"

Also a long line in the test is wrapped using string concatenation - just put it on one line

41: show-snapshot-and-active-or-staging-storage-diff-2148199-41.patch queued for re-testing.

I'm not clear why this is a button. It would make more sense as a tab or tabs. Can we do that instead?

Would someone be a gem and post a screenshot for me :) Pretty please.

I don't think this should be a toggle button. Also the UI presentation is inconsistent with the main table showing differences. I feel like we could have 3 sub stabes.

Also, if there is no content in the staging storage, we should show a relevant message to that effect, rather than indicating no differences.

@pwolanin: something like this?

I think the alert will stay (not shown here though) and each tab will show exactly the same kind of table.

This is a prototype with the tabs, for a more consistent view of the changes.

Screenshots.

Thanks - I think bringing these to tabs and making them visually consistent is better UX.

54: show-snapshot-and-active-or-staging-storage-diff-2148199-54.patch queued for re-testing.

Making sure I can get screenshots into the summary.

Screenshots in the issue summary.

Reroll.

Whatever happens with testbot, I need to be sure I didn't cause a regression in #2262861: Add concept of collections to config storages

@alexpott and I discussed on IRC and a problem with this patch is the Import (sync) button appearing on the "Compare snapshot with active" and "Compare snapshot with staging tabs". It should not be there, and the tab placement doesn't help.

I think the thing to do is just two simple tables on the config sync form: the current one for comparing staging to active, and a new one below it comparing snapshot and active. The warning would remain.

This is a new UI approach. I will update the summary with new screenshots.

New screenshots.

Rerolled for #1848266: Convert Diff into a proper, PSR-0-compatible PHP component.

Thanks @cilefen for your continued work here.

+++ b/core/modules/config/src/Form/ConfigSync.php
@@ -159,9 +170,108 @@ public function buildForm(array $form, array &$form_state) {
+      drupal_set_message($this->t('Changes have been made to your active configuration, which might be lost on the next import attempt.'), 'warning');

This message doesn't tell me what I need to know. When I read this, I think: How did the changes get made to my active configuration? What is the "next import attempt"? Does that mean what I'm trying to do right now on this form, or something else? Does "attempt" mean it's not likely to work? What am I supposed to do?

Also, the comma is ungrammatical and should be removed.

I also find the new UI very overwhelming. I don't know what the "changes to the Active Configuration" are (nor why it's capitalized). It would be better to compare the screenshots for both the previous approach and the current one in your comment rather than updating the issue summary before the suggested approach has consensus or has even been reviewed. Thanks!

I think maybe we should get the help of the usability team here.

How do we get the usability team involved?

You called? We generally use the "Needs usability review" tag.

I think its best to hop on IRC or Skype, to talk this UI over - I have not been able to understand from the comments what considerations need to be made.

We explain the problem more clearly in the issue summary, for someone who isn't familiar with the problem space and won't be looking at code. (I.e., it needs an explanation of how a site would get into this state and specific steps to follow on an 8.x installation to test both the current functionality and the patch.) Then we ask for a usability review. :)

Thinking about it a bit, a better warning message might be something like: "Your site's configuration has changed since the last configuration import. Importing new configuration (may? will?) overwrite these changes." (Regarding "may" or "will", can/should we detect if the changes since last import are not included in what's staged, and only display the warning if the changes are not in both places? Or is that too fragile?)

Edit: or, what Bojhan said. :) Crossposted.

Or maybe:

The following changes have been made to your site's configuration since the last import. Importing the staged configuration (may? will?) overwrite these changes."

[table]

The second half of the message is only relevant when there is staged configuration, obviously.

Updated the summary Problem/Motivation as I understand the issue.

Regarding the warning message—in its current incarnation all changes are indicated, regardless of what's in staging. So, we could either be vague and say "could be overwritten", or improve the patch and say "some will be overwritten" if that is the case.

Updated the summary as per the new plan.

What about "Your current configuration has changed, these changes will be lost on the next synchronization."

@bojhan: that's certainly better.

That's a comma splice. It should be two separate sentences. :)

Do we want to follow it with a list of just the names of the config objects that have changed? So the site user has a clue where to look. Edit: ie:

...Changes to these configuration items will be lost on the next synchronization: module1.foo, module2.bar.baz

@xjm We could do that because it is cryptic to not explain what these changes are. But then we are back to designing a UI, which I think is important, but I agree with @alexpott to make it a separate issue.

No, I am not saying to add a UI. I am saying simply that the message should include a list of the objects that will have their changes lost. I.e.:

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

Just in the dsm(), no UI. @alexpott agreed with this suggestion.

Why are these not bullets?

@Bojhan I am indifferent. If that is the standard should I just file a new patch?

That is generally the standard.

That looks great to me. Nice work! The only thing I'm not sure of is the early render of the item list, but I think we're partly limited by how drupal_set_message() works there. And an early render in a one-time warning seems acceptable to make the list legible.

Reroll.

"There are no configuration changes." is a confusing message when this warning appears. I recommend opening another issue to change that text to "There are no configuration changes to import."

#2336353: The message when there is no configuration to synchronize/import is misleading

The changes from #106 look good; +1 for not duplicating test coverage.

Talking with @cilefen in person. I'll re-roll this today.

showing the incremental diff to #108, since includes #114 and the fix indicated in #117

I'm not sure why the check on input was there - talking with @cilefen it's not clear why it's there, and it doesn't make sense that you want to show the message AFTER submitting the form.

Removing that check at least that one test passes, but need some evaluation as to whether this is correct.

silly me - now I see, I forgot the !

ugh. oops. get() gets form values NOT other form state properties. In other words, the suggested fix checked the equivalent of $form_state['values']['input']

I think this method will work.

I manually tested and it looks good.

The warning makes no sense on a development site where the synchronization process can be used intensively to travel in the configuration timeline.

A solution could be to check if the site is configured in dev mode, ie if the include of the file settings.local.php is uncommented in the configuration file settings.php

Another solution would be to add (in the warning area) an option "This is a development site" which remove that kind of warning.

You're welcome to open new issues to suggest improvements. This has been closed, there's no use in opening it again.