Problem/Motivation

If you try to import invalid YAML using the admin/config/development/configuration/single/import it crashes with a WSOD and an exception.

Proposed resolution

Catch any exceptions thrown whilst parsing the incoming YAML and present a validation message to the user.

Remaining tasks

User interface changes

New form error message

API changes

None

Data model changes

None

CommentFileSizeAuthor
#20 2617152-20.patch2.04 KBchi
#18 2617152-18.patch2.04 KBchi
#13 2617152-13.patch2.04 KBchi
#10 interfiff-2617152-6_10.txt633 byteschi
#10 2617152-10.patch2.7 KBchi
#6 2617152-6.patch2.52 KBchi
#2 2617152-2.patch1.07 KBchi

Comments

alexpott created an issue. See original summary.

chi’s picture

chi commented
Status: Active » Needs review
StatusFileSize
new 2617152-2.patch1.07 KB

I am not convinced what error message we should display in this case. Exception message is not translatable I guess.

dawehner’s picture

dawehner
Primary language German
commented

Do you think we can write some simple test coverage for that case? The fix itself makes totally sense for me. Maybe we could catch like all exceptions, but that seems to be details.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Yep we can definitely test this. Plus I don't think the main part of the message should be the exception message. We need something that can be translated.

dawehner’s picture

dawehner
Primary language German
commented

Yeah, we should say its an invalid YML file, but I think we should still include the exception message, given that it contains more information that might be helpful.

chi’s picture

chi commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 2617152-6.patch2.52 KB
chi’s picture

chi commented
Related issues: +#2617152: Single import form needs to handle invalid YAML correctly and not with an exception.
chi’s picture

chi commented
Related issues: -#2617152: Single import form needs to handle invalid YAML correctly and not with an exception. +#2699247: Single import form should not accept YAML data with scalar value

Wrong reference.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/config/src/Tests/ConfigImportUITest.php
@@ -521,4 +521,18 @@ public function testExtensionValidation() {
+  public function testSingleImport() {

Nitpick: We could name the method to point out that we deal with an invalid YML here

chi’s picture

chi commented
StatusFileSize
new 2617152-10.patch2.7 KB
new interfiff-2617152-6_10.txt633 bytes

@dawehner, this form does not have any tests at all. This is why I decided to give the method more generic name. We should add more cases there.

Added todo statement for this.

dawehner’s picture

dawehner
Primary language German
commented

Added todo statement for this.

OH yeah, this is a good idea.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

@Chi this test belongs in Drupal\config\Tests\ConfigSingleImportExportTest and the @todo is unnecessary.

chi’s picture

chi commented
Status: Needs work » Needs review
StatusFileSize
new 2617152-13.patch2.04 KB

@alexpott, correct. That file with UITest suffix confused me. I thought it is the only file with UI tests.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Ah good to see that we have actual test coverage. My inner alexpott would have been disappointed by the outer alexpott.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/modules/config/src/Form/ConfigSingleImportForm.php
@@ -286,8 +287,13 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+      $form_state->setErrorByName('import', $this->t('The YAML parser failed with the following message: %message', ['%message' => $e->getMessage()]));

I think that this message, whilst technically correct, is not the most user friendly. Have we even told the user that yaml is the expected format of the string yet?

chi’s picture

chi commented

What about The import failed with the following message: %message?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@Chi looks good to me

chi’s picture

chi commented
StatusFileSize
new 2617152-18.patch2.04 KB

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2617152-18.patch, failed testing.

chi’s picture

chi commented
Status: Needs work » Needs review
StatusFileSize
new 2617152-20.patch2.04 KB

Fixed test.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs review » Reviewed & tested by the community

Feedback from @alexpott in #15 is addressed and we have tests and a fix. Looks good to go.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 037962e and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed 6632005 on 8.2.x 
    Issue #2617152 by Chi, alexpott, dawehner: Single import form needs to...

  • alexpott committed 037962e on 8.1.x 
    Issue #2617152 by Chi, alexpott, dawehner: Single import form needs to...

Status: Fixed » Closed (fixed)

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