Problem/Motivation

When user accidentally fills in invalid YAML into the attributes input (type webform_codemirror) of a WebformElementAttributes form element and then tryies to submit the form, nothing happens. There no indication about any error, and especially if the webform field config form was open into a dialog (which is 99% of the cases I guess), the only feedback is the error counter of DevTools (if it is open).

Steps to reproduce

  1. Install Webform and Webform UI
  2. Log in as a user which has the "adminsiter webforms" permission.
  3. Visit the edit form of the default contact form replacement provided by Webform: admin/structure/webform/manage/contact
  4. Chose one of the elements (e.g. Your name) and click its "Edit" link.
  5. Chose the "Advanced" tab and open the details element of either Wrapper attributes or Element attributes.
  6. Fill in some invalid YAML string into the codemirror/textarea input with the label element custom attributes (YAML) or Wrapper custom attributes (YAML), e.g. this: is: invalid: YAML
  7. Hit the Save button

Expected: Form is reloaded and a validation error message is displayed (ideally, in the modal in the sidebar).
Actual: Exception is throw; user sees what described at the first point.

Proposed resolution

Add a try-catch around the YAML unserialization attempt in WebformElementAttributes::validateWebformElementAttributes.

Remaining tasks

Maybe the right tab and the right details should be open if there is an error with one of the inputs. I'§d solve this in another ticket.

User interface changes

Error message about the invalid YAML apperas on the page.

API changes

Nothing.

Data model changes

CommentFileSizeAuthor
#8 3526756-commit_4d0bc68381-6.2.x.diff4.16 KBhuzooka
#4 3526756-commit_23922a8f7f-6.2.9.diff3.64 KBhuzooka

Issue fork webform-3526756

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

huzooka created an issue. See original summary.

huzooka opened merge request !634

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Assigned: huzooka » Unassigned
Status: Active » Needs review

CS and Eslint violations are preexisting imho.
Asking for a (quick) review and about further tasks.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
StatusFileSize
new 3526756-commit_23922a8f7f-6.2.9.diff3.64 KB

I also created a patch which can be applied on Webform 6.2.9; uploading it here if anyone is interested.

dxvargas’s picture

dxvargas commented

I have tested the patch and it fixes the problem for malformed YAML (e.g. "this: is: invalid: YAML").
This part is good.

When testing I've initially filled the field "Wrapper custom attributes (YAML)" with just a dummy string "BLABLABLA".
For this, the patch is not working. There is an error that is only visible in the browser's console:

TypeError: Unsupported operand types: array + string in Drupal\\webform\\Element\\WebformElementAttributes::validateWebformElementAttributes() (line 185 of modules/contrib/webform/src/Element/WebformElementAttributes.php). call_user_func_array(Array, Array) (Line: 281

Can we make this dummy proof and also display an error when a random string is filled in these fields?

dxvargas’s picture

dxvargas commented
Status: Needs review » Needs work
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Assigned: Unassigned » huzooka
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Assigned: huzooka » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3526756-commit_4d0bc68381-6.2.x.diff4.16 KB

Re #5,
Thank you for testing this! I also addressed the issue you found, and added it as a test case.

I'm asking for a second round review.

Created a patch on top of 6.2.x.

dxvargas’s picture

dxvargas commented
Status: Needs review » Reviewed & tested by the community

It works flawless! Code and tests are good.
I mark the issue as RTBC.

liam morland made their first commit to this issue’s fork.

jrockowitz made their first commit to this issue’s fork.

jrockowitz’s picture

jrockowitz commented
Status: Reviewed & tested by the community » Needs review

I appreciate all the work everyone has done to resolve this issue.

The codemirror element validates the YAML and sets an error on the attributes. Still, the problem was that the decoding in WebformElementAttributes was not checking if the attributes element had any validation errors.

Please review the new MR.

jrockowitz opened merge request !683

  • jrockowitz committed 7766de1b on 6.3.x 
    Issue #3526756 by huzooka, liam morland, jrockowitz, dxvargas: Exception...
jrockowitz’s picture

jrockowitz commented
Status: Needs review » Fixed

Committing my change which has test coverage and fixes the issue.

  • jrockowitz committed 7766de1b on 6.x 
    Issue #3526756 by huzooka, liam morland, jrockowitz, dxvargas: Exception...

Status: Fixed » Closed (fixed)

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