Problem/Motivation

At the moment ConfigImporter fires the validation event and expects events to throw a ConfigImporterException. This means that there can be only one validation error.

Proposed resolution

Add the ability to log errors in the ConfigImporter and check if there are any after the validation event.
Introduce a ConfigImportValidateEventSubscriberBase to make writing validators simple

Remaining tasks

  • Agree apporach
  • Review / update patch

User interface changes

New error messages.

API changes

Add new methods to ConfigImporter - getErrorLog() and logError().

Files: 
CommentFileSizeAuthor
#27 2236553.27.patch14.76 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,760 pass(es). View
#27 25-27-interdiff.txt697 bytesalexpott
#25 2236553.25.patch15.66 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,788 pass(es). View
#25 23-25-interdiff.txt1.26 KBalexpott
#23 less-british-interdiff.txt4.95 KBxjm
#23 config-2236553-23.patch16.13 KBxjm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,868 pass(es). View
#22 20-22-interdiff.txt1.47 KBalexpott
#22 2236553.22.patch16.13 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,804 pass(es). View
#20 interdiff.txt2.32 KBdags
#20 2236553.19.patch15.4 KBdags
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2236553.19.patch. Unable to apply patch. See the log in the details link for more information. View
#18 2236553.18.patch16.18 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,702 pass(es). View
#18 17-18-interdiff.txt734 bytesalexpott
#17 15-17-interdiff.txt4.12 KBalexpott
#17 2236553.17.patch16.22 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,665 pass(es). View
#15 drupal.multi-errors.2236553.15.interdiff.txt553 bytesGaelan
#15 drupal.multi-errors.2236553.15.patch17.55 KBGaelan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,599 pass(es), 6 fail(s), and 24 exception(s). View
#9 drupal.multi-errors.2236553.9.patch16.35 KBGaelan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Config/ConfigImporter.php. View
#6 drupal.multi-errors.2236553.6.patch2.17 MBGaelan
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal.multi-errors.2236553.6.patch. Unable to apply patch. See the log in the details link for more information. View
#1 2236553.1.patch16.46 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2236553.1.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
16.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2236553.1.patch. Unable to apply patch. See the log in the details link for more information. View
Gaelan’s picture

1: 2236553.1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: 2236553.1.patch, failed testing.

Gaelan’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigImportValidateEventSubscriberBase.php
    @@ -0,0 +1,75 @@
    +   * Checks that the configuration synchronisation is valid.
    
    +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -127,6 +127,17 @@ class ConfigImporter extends DependencySerialization {
    +   * synchronisation will not occur. If errors occur during an import then best
    +   * efforts are made to complete the synchronisation.
    
    @@ -433,19 +444,42 @@ public function import() {
    +        throw new ConfigImporterException('There were errors validating the config synchronisation.');
    
    +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImporterTest.php
    @@ -112,7 +112,10 @@ function testSiteUuidValidate() {
    +      $this->assertEqual($e->getMessage(), 'There were errors validating the config synchronisation.');
    
    +++ b/core/modules/system/lib/Drupal/system/SystemConfigSubscriber.php
    @@ -7,37 +7,32 @@
    +   * Checks that the configuration synchronisation is valid.
    

    American English: synchronisation → synchronization

  2. +++ b/core/modules/config/lib/Drupal/config/Form/ConfigSync.php
    @@ -256,21 +257,30 @@ public function submitForm(array &$form, array &$form_state) {
    +        drupal_set_message($this->t('The configuration synchronsaiotn failed validaton for the reason(s) listed below.'));
    
    +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportUITest.php
    @@ -305,6 +305,31 @@ function testImportDiff() {
    +    $this->assertText('The configuration synchronsaiotn failed validaton for the reason(s) listed below.');
    

    Typo: synchronsaiotn → synchronization

  3. +++ b/core/modules/system/lib/Drupal/system/SystemConfigSubscriber.php
    @@ -7,37 +7,32 @@
    +      $event->getConfigImporter()->logError($this->t('Site UUID in source storage does not match the target storage.'));
    

    Usability-wise, I would recommend something more like "Hey, that's not the same site!" instead of something about UUIDs.

Gaelan’s picture

Assigned: Unassigned » Gaelan

Rerolling.

Gaelan’s picture

FileSize
2.17 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal.multi-errors.2236553.6.patch. Unable to apply patch. See the log in the details link for more information. View

Rerolled.

alexpott’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: drupal.multi-errors.2236553.6.patch, failed testing.

Gaelan’s picture

FileSize
16.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Config/ConfigImporter.php. View

This applies on HEAD (pulled about 5m ago).

xjm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: drupal.multi-errors.2236553.9.patch, failed testing.

Gábor Hojtsy’s picture

  1. +++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
    @@ -473,19 +477,42 @@ public function import() {
    +        throw new ConfigImporterException('There were errors validating the config synchronisation.');
    

    So there is no translation here. Generally we don't translate exceptions because they are not front user facing. So now we are exposing these for users. Is using exceptions the right thing then? If we only translate the wrapping then this is not really resolved...

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
    @@ -29,20 +28,15 @@ class ConfigImportSubscriber implements EventSubscriberInterface {
    +          $message = $this->t('The config name @config_name is invalid due to: @reason.', array('@config_name' => $name, '@reason' => $e->getMessage()));
    

    So we only translate the wrapping here.

Gábor Hojtsy’s picture

See #2055851: Remove translation of exception messages. where xjm is explaining why we don't translate exceptions :)

xjm’s picture

A better approach might be to throw a different typed exception? Edit: @msonnabaum also suggested moving the translatability to a followup.

Gaelan’s picture

Status: Needs work » Needs review
FileSize
17.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,599 pass(es), 6 fail(s), and 24 exception(s). View
553 bytes

Have bigger problems, so will NW again after testbot passes.

Status: Needs review » Needs work

The last submitted patch, 15: drupal.multi-errors.2236553.15.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
16.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,665 pass(es). View
4.12 KB

Fixed the patch

alexpott’s picture

Assigned: Gaelan » Unassigned
FileSize
734 bytes
16.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,702 pass(es). View
+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
@@ -29,20 +28,15 @@ class ConfigImportSubscriber implements EventSubscriberInterface {
       foreach ($event->getConfigImporter()->getUnprocessedConfiguration($op) as $name) {
-        Config::validateName($name);
+        try {
+          Config::validateName($name);
+        }
+        catch (ConfigNameException $e) {
+          $message = $this->t('The config name @config_name is invalid due to: @reason.', array('@config_name' => $name, '@reason' => $e->getMessage()));
+          $event->getConfigImporter()->logError($message);
+        }
       }

Mixing a translated string with a exception message is not good.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Looks better. Mixing the untranslated messages in would be confusing...

  1. +++ b/core/lib/Drupal/Core/Config/ConfigImportValidateEventSubscriberBase.php
    @@ -0,0 +1,75 @@
    + * ConfigImportValidateEventSubscriberBase makes creating config sync validators
    + * easy.
    

    Minor: Can this somehow fit on one line instead?

  2. +++ b/core/modules/config/lib/Drupal/config/Form/ConfigSync.php
    @@ -257,21 +258,30 @@ public function submitForm(array &$form, array &$form_state) {
    +        drupal_set_message($this->t('The configuration synchronsaiotn failed validaton for the reason(s) listed below.'));
    
    +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportUITest.php
    @@ -306,6 +306,31 @@ function testImportDiff() {
    +    $this->assertText('The configuration synchronsaiotn failed validaton for the reason(s) listed below.');
    

    Typo in user facing message: "synchronsaiotn". Also it is simpler to not say "for the reason(s) below" and just list the reasons. People will understand those are the reasons, the whole "reason(s)" form where we admit we are lazy coders to handle the singular/plural case is not a good idea, its hard to translate also... :)

  3. +++ b/core/modules/system/lib/Drupal/system/SystemConfigSubscriber.php
    @@ -7,37 +7,32 @@
    +   * This event listener implements two checks:
    +   * - prevents deleting all configuration.
    +   * - checks that the system.site:uuid's in the source and target match.
    

    Minor: The list items need to be indented, no?

dags’s picture

Status: Needs work » Needs review
FileSize
15.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2236553.19.patch. Unable to apply patch. See the log in the details link for more information. View
2.32 KB

Addresses issues in #19. It's hard creating an 80 character sentence when the class name takes up 39 itself ;)

Status: Needs review » Needs work

The last submitted patch, 20: 2236553.19.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
16.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,804 pass(es). View
1.47 KB

Rerolled and improved the docs - the class name is not required :)

xjm’s picture

FileSize
16.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,868 pass(es). View
4.95 KB
+++ b/core/lib/Drupal/Core/Config/ConfigImportValidateEventSubscriberBase.php
@@ -0,0 +1,74 @@
+   * Checks that the configuration synchronisation is valid.

+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php
@@ -135,7 +135,11 @@ class ConfigImporter extends DependencySerialization {
+   * synchronisation will not occur. If errors occur during an import then best
+   * efforts are made to complete the synchronisation.

@@ -658,14 +662,19 @@ protected function getNextConfigurationOperation() {
+        throw new ConfigImporterException('There were errors validating the config synchronisation.');

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigSync.php
@@ -257,21 +258,30 @@ public function submitForm(array &$form, array &$form_state) {
+        drupal_set_message($this->t('The configuration synchronisation failed validation.'));

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportUITest.php
@@ -306,6 +306,31 @@ function testImportDiff() {
+    $this->assertText('The configuration synchronisation failed validation.');

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImporterTest.php
@@ -114,7 +114,10 @@ function testSiteUuidValidate() {
+      $this->assertEqual($e->getMessage(), 'There were errors validating the config synchronisation.');

+++ b/core/modules/system/lib/Drupal/system/SystemConfigSubscriber.php
@@ -7,37 +7,32 @@
+   * Checks that the configuration synchronisation is valid.

Still all British and stuff. Fixed in attached.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

All the changes look good. Thanks for resolving the patch so it fixes my concerns :)

alexpott’s picture

FileSize
1.26 KB
15.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,788 pass(es). View

@timplunkett point out that the inject was unnecessary since a trait is coming. I agree.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/core.services.yml
@@ -557,6 +557,7 @@ services:
+    arguments: ['@string_translation']

+++ b/core/modules/system/system.services.yml
@@ -26,6 +26,7 @@ services:
+    arguments: ['@string_translation']

Just remove these, and I think its good to go!
Please reRTBC once they're gone.

alexpott’s picture

Status: Needs work » Needs review
FileSize
697 bytes
14.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,760 pass(es). View

Doh - I thought I have removed them.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc as per #26

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Nice find!

Committed and pushed to 8.x. Thanks!

  • Commit 95e6d3a on 8.x by webchick:
    Issue #2236553 by xjm, dags, Gaelan, alexpott: Config import validation...

Status: Fixed » Closed (fixed)

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

fgm’s picture

This appears not to be complete: validation errors actually ignore the messages. Followup in #2503263: Config import validation needs to provide error messages.