Problem/Motivation

[Drupal 8.6.x] #states breaks when OR is used.

The below Form API code from the Webform module…

select:
  '#type': select
  '#title': select
  '#options':
    1: 1
    2: 2
number:
  '#type': number
  '#title': number
textfield:
  '#type': textfield
  '#title': textfield
  '#states':
    visible:
      - ':input[name="select"]':
          value: 1
      - or
      - ':input[name="number"]':
          value: 1

…or PHP code (which can be added to any form)…


  $form['select'] = [
    '#type' =>'select',
    '#title' => 'select 1',
    '#options' => [0 => 0, 1 => 1, 2 => 2],
  ];
  $form['number'] = [
    '#type' => 'number',
    '#title' => 'enter 1',
  ];
  $form['textfield'] = [
    '#type' => 'textfield',
    '#title' => 'textfield',
    '#states' => [
      'visible' => [
        [':input[name="select"]' => ['value' => '1']],
        'or',
        [':input[name="number"]' => ['value' => '1']],
      ],
    ],
  ];

Logs the below exception…

This issue is being caused by the changes #2981652: [Aug. 9 2018] Format core JavaScript using recently add Prettier.

When I revert core/misc/states.js from the 8.6.x version to 8.5.x the issue/error is resolved.

I also confirmed (by removing the code) that the enhancements to #states contained in js/webform.states.js is not causing this issue.

Proposed resolution

TBD

Remaining tasks

TBD

User interface changes

N/A

API changes

N/A

Data model changes

N/A

CommentFileSizeAuthor
#28 interdiff-2995570-19-28.txt647 bytesAaronBauman
#28 2995570-28.patch7.7 KBAaronBauman
#20 interdiff-2995570-13-19.txt608 bytesjrockowitz
#19 interdiff.txt718 byteslauriii
#19 2995570-19.patch7.69 KBlauriii
#13 2995570-13.patch6.99 KBlauriii
#13 2995570-13-test-only.patch3.19 KBlauriii
#10 Screenshot.gif87.68 KBmattjones86
#6 interdiff-2995570-5-6.txt3.8 KBGrandmaGlassesRopeMan
#6 2995570-6.patch5.93 KBGrandmaGlassesRopeMan
#5 Screen Shot 2018-08-28 at 9.05.46 PM.png439.43 KBjrockowitz
#5 2995570-5.patch2.13 KBjrockowitz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jrockowitz created an issue. See original summary.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Version: 8.6.x-dev » 8.7.x-dev
jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Issue summary: View changes
jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Parent issue: #2995525: [Drupal 8.6.x] #states breaks when OR is used »
Related issues: +#2702233: [backport] Add JavaScript tests for Form API #states: required, visible, invisible, expanded, checked, unchecked, +#2995525: [Drupal 8.6.x] #states breaks when OR is used
jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
FileSize
2995570-5.patch2.13 KB
Screen Shot 2018-08-28 at 9.05.46 PM.png439.43 KB

The attached patch adds a test form which replicates the issue.

  • Enable the form_test.module (drush en form_test)
  • Goto /form-test/javascript-states-form
  • Confirm the below error message in JS console

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Active » Needs review
FileSize
2995570-6.patch5.93 KB
interdiff-2995570-5-6.txt3.8 KB

Alright.

@jrockowitz - This was not related to the Prettier change at all. This actually happened in #2925064: [1/2] JS codestyle: no-restricted-syntax where we tried to move away from using restricted syntax, including for...in loops.

This patch reverts that minor change + Prettier changes because this code is slightly different. I'm not 💯👌 with this and we should probably figure out a better solution.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Priority: Normal » Major

@drpal Thanks for looking at this issue.

I reviewed the patch and it fixes the regression. For the Webform module, this is a critical issue because it breaks the form builder and any generated form that uses an OR condition.

Drupal 8.6.0 is going to be released on September 5th.

Is there any way this patch can get committed to 8.6.x.? Who is the person that can make this decision?

I will ask around on Slack but I wanted to start this discussion here first.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue tags: +Needs followup

For me, this approach seems fine. Let's make sure a follow-up gets opened for an actual fix.

In my opinion, we can backport this to 8.6.x but I will double check with the release managers.

seanB’s picture

seanB
Primary language Dutch
Location Netherlands
CreditAttribution: seanB commented
Status: Needs review » Reviewed & tested by the community

We just found that some forms broke in production after updating to 8.6 and the patch at least fixes the issue. It would be nice to get this fixed in 8.6 as well.

mattjones86’s picture

mattjones86
Location 🇬🇧 GMT+0
CreditAttribution: mattjones86 commented
FileSize
Screenshot.gif87.68 KB

Hi, I applied this patch on my 8.6 install and the states now work correctly in the webform admin UI, however they don't seem to be applying to the front end until after a change() is triggered on the element.

Please ignore, the bug I was experiencing is specific to the webform module (#3002812: Default #state not honoured for select field).

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
@@ -0,0 +1,55 @@
+class JavascriptStatesForm extends FormBase {

Could we add some actual tests that are using this form? It would be best if we could write a test that fails at the moment since the bug still exists.

Pascal-’s picture

Pascal- CreditAttribution: Pascal- commented

Amazing how this issue was known & patched a week before 8.6 was released and nothing was done with it ...
This is happening more and more and I'm really starting to dislike Drupal because of it!

I'm a junior/medior developer and finding these issues is not exactly easy for me either.

On to fix the next bug updating to 8.6 caused!

(Thx for the patch, it works.)

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs work » Needs review
FileSize
2995570-13-test-only.patch3.19 KB
2995570-13.patch6.99 KB

Here's a very minimal test coverage using Nightwatch for this.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented

I manually tested my original example and the patch from #13 fixes the regression.

@Pascal- The challenge here is that the JavaScript Modernisation is working hard on improving and replacing Drupal's legacy JavaScript which lacks test coverage. As a community, not enough people, including myself, are fully up-to-speed on modern JavaScript. Finally, as usually more people need to get involved. I know a lot of people who are using this patch but did not provide any simple feedback and thanks as you did.

The last submitted patch, 13: 2995570-13-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/misc/states.es6.js
@@ -59,6 +59,30 @@
+   * @param {*} a
+   *   Value a.
+   * @param {*} b
+   *   Value b

The tiniest of nitpicks (fixable on commit): "Value b" should end with a period.

Otherwise, this looks right to me.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented

@phenaproxima - I think this was an issue with the original comment block? This just reverts a change to a previous bit of code.

@lauriii - Looks good to me. 🎉✌️👍

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue tags: -Needs followup
Related issues: +#3005682: Remove no-restricted-syntax eslint rule override from states.es6.js

Created a follow-up #3005682: Remove no-restricted-syntax eslint rule override from states.es6.js.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Needs review
FileSize
2995570-19.patch7.69 KB
interdiff.txt718 bytes

I discussed this with @xjm and she mentioned that we should create a post-update hook to ensure that aggregated JS files are being regenerated when updating a site.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Status: Needs review » Reviewed & tested by the community
FileSize
interdiff-2995570-13-19.txt608 bytes

Attached is the interdiff between #10 and #19 which shows that only the post-update hook was added.

Using the post-update hook to clear cache makes complete sense to me.

The patch was otherwise marked as RTBC via #16 and #17, so I think this can back to RTBC.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Hypothesis: Running update.php already clears the css/js aggregates
Experiment:
Visit the frontpage, look for JS aggregate
Visit update.php, finish the update without running any actual updates
Visit the frontpage again, compare the JS aggregates
Result:
No updates result in the same css/js aggregates.

Given that an update function is needed everytime we change JS/CSS and we want it to land at the user.

shimar007’s picture

shimar007 CreditAttribution: shimar007 commented

Hey,

Just wanted to check by this patch would be ready to use on production sites ?

Regards
Shiva

itamair’s picture

itamair CreditAttribution: itamair as a volunteer commented

Hi, I confirm all this.
Indeed this strongly affects the Geofield Map module (version up to 8.x-2.15) in the not-correct rendering of the Map Theming Options section of the Geofield Google Map View Display Style, even without the OR #states option use.
Everything was working correctly till D8 core 8.5.x) ...

Really needed this fixes committed into new stable D8 core release.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 2995570-19.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Needs review

Seems like some weird Nightwatch fail.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Bah, meant to make that RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 2995570-19.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

AaronBauman’s picture

AaronBauman
he/him
CreditAttribution: AaronBauman at Message Agency commented
Status: Needs work » Needs review
FileSize
2995570-28.patch7.7 KB
interdiff-2995570-19-28.txt647 bytes

#19 RTBC +1

This patch addresses 2 coding standards complaints (spacing), otherwise identical.

Oulalahakabu’s picture

Oulalahakabu CreditAttribution: Oulalahakabu commented

#28 works on drupal 8.6.2 thanks

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC. Thanks for addressing the coding standard violations.

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz as a volunteer commented
Related issues: +#3007836: States.js - Uncaught TypeError
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Related issues:

This adds very minimal test coverage that proves that the bug has been fixed. We should add additional test coverage in #2702233: [backport] Add JavaScript tests for Form API #states: required, visible, invisible, expanded, checked, unchecked.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented

Alright. This looks good to me. Thanks for the test @lauriii 👏

  • lauriii committed 2662480 on 8.7.x 
    Issue #2995570 by lauriii, jrockowitz, drpal, aaronbauman: #states...

  • lauriii committed f3e0022 on 8.6.x 
    Issue #2995570 by lauriii, jrockowitz, drpal, aaronbauman: #states...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Fixed

There's sign-off from a subsystem maintainer and multiple comments stating people are using this patch in production - so I'm going ahead and committing this myself prior to 8.6.3 release.

Committed 2662480 and pushed to 8.7.x. Also cherry-picked to 8.6.x. Thanks!

Status: Fixed » Closed (fixed)

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

idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
Version: 8.7.x-dev » 8.6.x-dev