Ensure that all config can not be deleted using the config importer

Looks great, thanks for providing the interdiff. I like!

That was just a bot hickup, still RTBC.

diff --git a/core/modules/system/lib/Drupal/system/SystemConfigSubscriber.php b/core/modules/system/lib/Drupal/system/SystemConfigSubscriber.php
old mode 100644
new mode 100755

We shouldn't be changing the file mode

+++ b/core/modules/system/lib/Drupal/system/SystemConfigSubscriber.phpundefined
@@ -30,7 +30,6 @@ static function getSubscribedEvents() {
     $importer = $event->getConfigImporter();
-    $importList = $importer->getStorageComparer()->getSourceStorage()->listAll();
     if (empty($importerList)) {

This looks slightly more complex since $importerList is not defined.

We need to is actually fix this code :)

Hmm, yeah that makes sense.

So $importList should not be removed, and instead in the if() it should be called $importList as well. This also means, however, that we're missing a test on this. Removing the Novice tag for that.

The variable $importList is required and will resolve the purpose of getting all Configuration stuff for delete. So renamed $importerList to $importList.

Please combine this patch with #2080343: Remove Unused local variables from system module.

Actually, looks like this one has an actual bug associated with it? If so let's add tests. Sorry for closing pre-emptively; trying to update too many issues at once. :)

@xjm what will be the next step?

@xjim, Can we close this as i have added changes for this in https://drupal.org/node/2080343?

rahul.shinde, we should keep this separate, since this fixes a bug. The bug fix should get reviewed here before merging it in with the other unused variables.

So, I was trying to see what test needs to be written for this. However, I don't know if this event ever gets injected. SystemConfigSubscriber has a getSubscribedEvents() function. But, I don't think the function ever gets called. Anyone know why?

There is already a test for this - but it was very broken and yep the event is never fired because it is not registered.

I tried to give you a hand on this one but I do not get why if the function was created to fail and it actually fails, what is the change that needs to be done here?

This patch looks good to me. onConfigImporterValidate() now gets called and the test validates that it throws an exception when the importList is empty.

+++ b/core/modules/system/lib/Drupal/system/SystemConfigSubscriber.php
@@ -19,18 +18,27 @@
+   * Checks that we not importing an from an empty source storage.

Wut ? :)

@swentel thanks for the review!

Great, RTBC if green.

+++ b/core/modules/system/lib/Drupal/system/SystemConfigSubscriber.php
@@ -19,18 +18,27 @@
+      throw new ConfigImporterException("This import will delete all your active configuration, bailing out now.");

If it's bailing out now, then it's not actually going to delete all the active configuration. Could we say something like 'This import is empty and if applied would delete all of your configuration, so has been rejected.' or similar?

Makes sense.

OK I still feel like that message doesn't quite cover it, but can't think of anything better either and presumably this won't happen often, so committed/pushed to 8.x, thanks!