Problem/Motivation

The intended workflow for Configuration Management is to stage configuration changes on a staging site, then to import those changes to the production site. A user who does not follow the intended workflow may make configuration changes to the production site, then later import more configurations from the staging site.

If that happens, the imported configuration changes from staging may overwrite current production values. The purpose of this issue is to provide a warning if configuration changes have been made directly on the production site that could be overwritten by staging.

Proposed resolution

After installation and configuration import we save the active configuration as a snapshot. When the user imports configuration again this can be used to determine if there have been any changes on that specific site instance and warn the user that these changes might be lost on import (unless of course the changes have been implemented on the site you are importing from too).

We should compare the snapshot storage and active storage and issue a warning on admin/config/development/configuration (synchronize) if this is the case.

Remaining tasks

  • Write patch

User interface changes

  • New warning on the configuration synchronize screen.

API changes

None.

Files: 
CommentFileSizeAuthor
#125 increment.txt873 bytespwolanin
#125 2148199-125.patch5.86 KBpwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,174 pass(es). View
#125 2148199-125.patch5.86 KBpwolanin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,174 pass(es). View
#123 increment.txt841 bytespwolanin
#123 2148199-123.patch5.85 KBpwolanin
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,145 pass(es), 1 fail(s), and 0 exception(s). View
#121 increment.txt840 bytespwolanin
#121 2148199-121.patch5.83 KBpwolanin
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,092 pass(es), 1 fail(s), and 0 exception(s). View
#119 increment.txt1.5 KBpwolanin
#119 2148199-119.patch5.85 KBpwolanin
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,069 pass(es), 1 fail(s), and 0 exception(s). View
#109 Screen Shot 2014-08-21 at 8.36.12 PM.png106.46 KBcilefen
#108 2148199-108.patch5.72 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,078 pass(es). View
#106 2148199-106.patch5.71 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,240 pass(es). View
#106 interdiff-101-106.txt887 bytescilefen
#101 Screen Shot 2014-07-21 at 5.29.32 PM.png143.6 KBcilefen
#101 2148199-101.patch6.05 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,989 pass(es). View
#101 interdiff-98-101.txt1.02 KBcilefen
#98 Screen Shot 2014-07-21 at 9.48.19 AM.png137.68 KBcilefen
#98 2148199-98.patch5.93 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,024 pass(es). View
#98 interdiff-96-98.txt851 bytescilefen
#96 2148199-96.patch5.93 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,255 pass(es). View
#96 interdiff-94-96.txt823 bytescilefen
#94 Screen Shot 2014-07-21 at 9.06.41 AM.png122.52 KBcilefen
#94 2148199-94.patch5.9 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,188 pass(es). View
#90 interdiff-87-90.txt2 KBcilefen
#90 2148199-90.patch5.35 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,420 pass(es). View
#87 2.png105.87 KBcilefen
#87 1.png90.93 KBcilefen
#87 2148199-87.patch5.37 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,201 pass(es). View

Comments

Barry Madore’s picture

Issue summary: View changes
cilefen’s picture

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?

alexpott’s picture

Issue summary: View changes
catch’s picture

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

xjm’s picture

Priority: Critical » Major
Issue tags: -beta blocker +beta target

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

attila.fekete’s picture

Assigned: Unassigned » attila.fekete
attila.fekete’s picture

Status: Active » Needs review
FileSize
9.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,724 pass(es). View

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

attila.fekete’s picture

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.

attila.fekete’s picture

FileSize
10.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,738 pass(es). View

Changes since previous patch:

  • now it is possible to switch between active and staging storage
  • other small fixes
jessebeach’s picture

Status: Needs review » Needs work
+++ 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?

tim.plunkett’s picture

  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()

jessebeach’s picture

Screenshot of suggested changes:

Only local images are allowed.

attila.fekete’s picture

FileSize
11.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,905 pass(es). View

@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
attila.fekete’s picture

FileSize
11.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch show-snapshot-and-active-or-staging-storage-diff-2148199-13_0.patch. Unable to apply patch. See the log in the details link for more information. View

Uploading patch again because tests didn't run.

attila.fekete’s picture

Status: Needs work » Needs review
mtift’s picture

Status: Needs review » Needs work
mtift’s picture

Status: Needs work » Needs review
FileSize
10.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/config/lib/Drupal/config/Form/ConfigSync.php. View

Just a reroll

Status: Needs review » Needs work

The last submitted patch, 18: snapshot-diff-2148199-18.patch, failed testing.

mtift’s picture

FileSize
10.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,974 pass(es), 64 fail(s), and 23 exception(s). View
639 bytes

Missed a comma

mtift’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 20: snapshot-diff-2148199-20.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
11.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,123 pass(es). View
cilefen’s picture

FileSize
969 bytes
11.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,148 pass(es). View
cilefen’s picture

cilefen’s picture

FileSize
11.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,062 pass(es). View
545 bytes

Left in an extra parameter.

cilefen’s picture

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

cilefen’s picture

alexpott’s picture

Issue tags: +Needs tests
  1. +++ b/core/modules/config/lib/Drupal/config/Form/ConfigImportForm.php
    @@ -11,6 +11,7 @@
    @@ -30,8 +31,10 @@ class ConfigImportForm extends FormBase {
    

    Not entirely sure the single import form should have this message. In fact I'm sure. It should not since it does not make a snapshot after you are done.

  2. +++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.php
    @@ -131,9 +141,66 @@ public function diff($source_name, $target_name = NULL) {
    +      $build['#title'] = t('View changes between snapshot and active configuration.');
    ...
    +      $build['#title'] = t('View changes between snapshot and staging configuration.');
    ...
    +              array('data' => t('Snapshot'), 'colspan' => '2'),
    +              array('data' => ($storage == 'active') ? t('Active storage') : t('Staging storage'), 'colspan' => '2'),
    

    $this->t() instead of t()

  3. +++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.php
    @@ -131,9 +141,66 @@ public function diff($source_name, $target_name = NULL) {
    +          $build[$name] = array(
    +            '#prefix' => '<h3>' . $name . '</h3>',
    

    This is needs to be translatable. So we need to do a switch and use $this->t() like the config synchronisation form does.

  4. +++ b/core/modules/config/lib/Drupal/config/Form/ConfigSync.php
    @@ -153,6 +164,16 @@ public function getFormId() {
    +    if ($active_snapshot_comparer->createChangelist()->hasChanges()) {
    +      drupal_set_message($this->t('Changes have been made to your active configuration, which might be lost on the next import attempt. You can find and review the differences between snapshot and active/staging storage here: <a href="@link">Compare snapshot and active/staging storage</a>.', array(
    +        '@link' => $this->url('config.snapshot_diff', array('storage' => 'active')))), 'warning');
    +
    +    }
    

    We should only be doing this is the form has not been submitted... so check that $form_state['input'] is empty. Otherwise the message will appear after the import is completed.

Yep we need to add tests in the this issue.

alexpott’s picture

Status: Needs review » Needs work
cilefen’s picture

Status: Needs work » Needs review
FileSize
12.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,149 pass(es), 1 fail(s), and 0 exception(s). View
1.28 KB

I started sketching out the tests.

Status: Needs review » Needs work
cilefen’s picture

Status: Needs work » Needs review
FileSize
13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,139 pass(es). View
1.34 KB

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

Bojhan’s picture

Let me know when its ready for review.

cilefen’s picture

Status: Needs review » Needs work
cilefen’s picture

Status: Needs work » Needs review
FileSize
14.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,710 pass(es). View
5.8 KB

@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.

alexpott’s picture

1) you are right - still the check does not need to be there. The form is for uploading an config tar ball.
3) you are totally right

cilefen’s picture

FileSize
11.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 69,745 pass(es). View
3.16 KB

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

cilefen’s picture

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.

pwolanin’s picture

Status: Needs review » Needs work

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

cilefen’s picture

Status: Needs work » Needs review
FileSize
11.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,358 pass(es). View
1.29 KB
cilefen’s picture

pwolanin’s picture

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

jessebeach’s picture

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

cilefen’s picture

pwolanin’s picture

Status: Needs review » Needs work

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.

cilefen’s picture

@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.

cilefen’s picture

FileSize
15.25 KB
17.27 KB

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

cilefen’s picture

cilefen’s picture

cilefen’s picture

Issue summary: View changes
cilefen’s picture

FileSize
18.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,424 pass(es), 1 fail(s), and 0 exception(s). View
2.8 KB

Status: Needs review » Needs work
cilefen’s picture

Status: Needs work » Needs review
FileSize
20.57 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch show-snapshot-and-active-or-staging-storage-diff-2148199-54.patch. Unable to apply patch. See the log in the details link for more information. View
2.04 KB
cilefen’s picture

pwolanin’s picture

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

pwolanin’s picture

Status: Needs review » Needs work
cilefen’s picture

Issue summary: View changes

Making sure I can get screenshots into the summary.

cilefen’s picture

Issue summary: View changes
cilefen’s picture

Issue summary: View changes

Screenshots in the issue summary.

cilefen’s picture

Status: Needs work » Needs review
FileSize
21.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,775 pass(es). View

Reroll.

cilefen’s picture

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

cilefen’s picture

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
cilefen’s picture

Status: Needs review » Needs work

@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.

alexpott’s picture

cilefen’s picture

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.

cilefen’s picture

Status: Needs work » Needs review
FileSize
19.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,587 pass(es), 9 fail(s), and 1 exception(s). View
158.14 KB
62.64 KB
60.89 KB
142.66 KB
99.47 KB
94.37 KB

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

cilefen’s picture

Issue summary: View changes

New screenshots.

Status: Needs review » Needs work

The last submitted patch, 71: 2148199-71.patch, failed testing.

cilefen’s picture

FileSize
19.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,705 pass(es), 9 fail(s), and 2 exception(s). View
pwolanin’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 74: use_snapshot_to_warn-2148199-74.patch, failed testing.

xjm’s picture

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!

xjm’s picture

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

cilefen’s picture

How do we get the usability team involved?

Bojhan’s picture

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.

xjm’s picture

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.

xjm’s picture

Assigned: attila.fekete » Unassigned
xjm’s picture

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.

cilefen’s picture

Issue summary: View changes

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.

alexpott’s picture

Lets not do the whole UI thing here. We should just be adding a message that warns the user that the snapshot is different from the active and therefore a config sync with staging might produce unexpected results. We can deal with a the UI in a follow up.

cilefen’s picture

Issue summary: View changes

Updated the summary as per the new plan.

cilefen’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
5.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,201 pass(es). View
90.93 KB
105.87 KB

Bojhan’s picture

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

cilefen’s picture

@bojhan: that's certainly better.

cilefen’s picture

FileSize
5.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,420 pass(es). View
2 KB
xjm’s picture

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

cilefen’s picture

@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.

xjm’s picture

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.

cilefen’s picture

FileSize
5.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,188 pass(es). View
122.52 KB

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/config/src/Form/ConfigSync.php
@@ -153,14 +164,31 @@ public function getFormId() {
+      drupal_set_message($this->t('Your current configuration has changed. Changes to these configuration items will be lost on the next synchronization: ' . $change_list_text), 'warning');

Should use a placeholder.

cilefen’s picture

Status: Needs work » Needs review
FileSize
823 bytes
5.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,255 pass(es). View
alexpott’s picture

+++ b/core/modules/config/src/Form/ConfigSync.php
@@ -153,14 +164,31 @@ public function getFormId() {
+      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_text)), 'warning');

I think we should be using % instead of @ here.

cilefen’s picture

FileSize
851 bytes
5.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,024 pass(es). View
137.68 KB

Bojhan’s picture

Why are these not bullets?

cilefen’s picture

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

cilefen’s picture

FileSize
1.02 KB
6.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,989 pass(es). View
143.6 KB

Bojhan’s picture

That is generally the standard.

cilefen’s picture

Issue summary: View changes
xjm’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/config/src/Form/ConfigSync.php
@@ -153,14 +164,35 @@ public function getFormId() {
+      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');

+++ b/core/modules/config/src/Tests/ConfigExportImportUITest.php
@@ -121,6 +121,16 @@ public function testExportImport() {
+    $this->assertNoText(t('Your current configuration has changed, these changes will be lost on the next synchronization.'));

The full stop here is problematic since this text can never appear in the UI. I would remove this assertion. The update of the snapshot after an import is tested elsewhere.

cilefen’s picture

Status: Needs work » Needs review
FileSize
887 bytes
5.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,240 pass(es). View
cilefen’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
cilefen’s picture

Status: Needs work » Needs review
FileSize
5.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,078 pass(es). View

Reroll.

cilefen’s picture

"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."

cilefen queued 108: 2148199-108.patch for re-testing.

cilefen’s picture

Issue tags: -Needs reroll
cilefen’s picture

xjm’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed fcc5b62 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/config/src/Form/ConfigSync.php b/core/modules/config/src/Form/ConfigSync.php
index a6e3e20..45478e2 100644
--- a/core/modules/config/src/Form/ConfigSync.php
+++ b/core/modules/config/src/Form/ConfigSync.php
@@ -103,11 +103,11 @@ class ConfigSync extends FormBase {
    * Constructs the object.
    *
    * @param \Drupal\Core\Config\StorageInterface $staging_storage
-   *   The source storage object.
+   *   The source storage.
    * @param \Drupal\Core\Config\StorageInterface $active_storage
-   *   The target storage manager.
+   *   The target storage.
    * @param \Drupal\Core\Config\StorageInterface $snapshot_storage
-   *   The target storage manager.
+   *   The snapshot storage.
    * @param \Drupal\Core\Lock\LockBackendInterface $lock
    *   The lock object.
    * @param \Symfony\Component\EventDispatcher\EventDispatcherInterface $event_dispatcher

Fixed on commit. The new @param doc was incorrect and the just made everything consistent.

  • alexpott committed fcc5b62 on
    Issue #2148199 by cilefen, attila.fekete, mtift | alexpott: Use snapshot...

  • alexpott committed a782ad5 on
    Revert "Issue #2148199 by cilefen, attila.fekete, mtift | alexpott: Use...
alexpott’s picture

Status: Fixed » Needs work

This broke head - patch needs to be fixed with changes from #114 and...

+++ b/core/modules/config/src/Form/ConfigSync.php
@@ -154,14 +165,35 @@ public function getFormId() {
+    if (empty($form_state['input']) && $snapshot_comparer->createChangelist()->hasChanges()) {

Can no longer use array access on $form_state - this needs to be !$form_state->has('input')

pwolanin’s picture

Assigned: Unassigned » pwolanin

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

pwolanin’s picture

Assigned: pwolanin » Unassigned
Status: Needs work » Needs review
FileSize
5.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,069 pass(es), 1 fail(s), and 0 exception(s). View
1.5 KB

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

Status: Needs review » Needs work

The last submitted patch, 119: 2148199-119.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
5.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,092 pass(es), 1 fail(s), and 0 exception(s). View
840 bytes

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.

Status: Needs review » Needs work

The last submitted patch, 121: 2148199-121.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
5.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,145 pass(es), 1 fail(s), and 0 exception(s). View
841 bytes

silly me - now I see, I forgot the !

Status: Needs review » Needs work

The last submitted patch, 123: 2148199-123.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
5.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,174 pass(es). View
5.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,174 pass(es). View
873 bytes

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.

cilefen’s picture

Status: Needs review » Reviewed & tested by the community

I manually tested and it looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6a6e528 and pushed to 8.0.x. Thanks!

  • alexpott committed 6a6e528 on 8.0.x
    Issue #2148199 by cilefen, pwolanin, attila.fekete, mtift | alexpott:...

Status: Fixed » Closed (fixed)

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

adraoo’s picture

Status: Closed (fixed) » Needs review

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.

Berdir’s picture

Status: Needs review » Closed (fixed)

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