Forms: conditional logic (2/5) — the resolver, in JS and PHP - #50977
Forms: conditional logic (2/5) — the resolver, in JS and PHP#50977enejb wants to merge 8 commits into
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
c43767f to
0cc8354
Compare
0cc8354 to
ed770e6
Compare
Given a form's rules and its current answers, decide which fields are visible. Pure functions with no wiring yet -- nothing calls them. The same resolution has to happen twice: in the browser as the visitor types, and on the server at submit time, where the browser's answer cannot be trusted. So it is implemented once in each language, and pinned together by tests that fail if the two ever disagree. Resolution is a fixed point rather than a single pass, because a field's visibility can depend on a field that is itself conditional. The budget is one pass per conditional field plus one to confirm nothing moved -- the bound that guarantees convergence, and self-limiting because it cannot exceed the form. A cycle has no fixed point, so it fails open: every field still changing after the first pass is shown. Hiding a field the visitor cannot reveal is a dead end, and a form that shows too much is recoverable in a way that one showing too little is not. Dates are parsed explicitly into a comparable YYYYMMDD integer using the format the field itself writes, rather than strtotime() and Date.parse(). Those two disagree: Date.parse() reads a bare `YYYY-MM-DD` as UTC but `mm/dd/yy` as local, so any visitor away from UTC saw a different form than the server validated, and a `dd/mm/yy` field parsed on neither side. Two parity tests, because they catch different things. One pins the shared vocabulary -- operator names and the type table. The other pins comparison behaviour from a table both suites read, which is where the two implementations actually drift: the date bug above passed the vocabulary test.
ed770e6 to
ef644be
Compare
…conditional-logic-resolver
The shared behaviour table declared its data provider with only the PHPUnit attribute. The PHP 7.2 and 7.3 matrix runs on a PHPUnit old enough to ignore attributes, so the provider never ran and the test was called with no arguments. Adding the annotation alongside the attribute is what the Jetpack.PHPUnit.Attributes sniff was warning about, and it keeps both PHPUnit generations reading the same table. Reading the fixture now throws when it cannot be parsed rather than falling through. Phan flagged the unguarded array access, and the guard it wants is the one the test needs anyway: a fixture that fails to load would otherwise turn every case into a silent pass, which is the one failure mode a cross-language parity table must not have. Also documents the $format parameter the date fix added to three methods, and fixes two alignment nits and a brace placement.
kraftbj
left a comment
kraftbj
left a comment
There was a problem hiding this comment.
Went through this layer on its own — every operator and coercion helper side by side, both fixed-point loops, and the PHP class executed against the edge cases rather than reasoned about. It holds up well. The convergence bound is genuinely correct rather than just generous (Jacobi snapshot on both sides, so iteration order can't matter — which counts, because JS reorders numeric-looking keys and PHP doesn't), every error branch fails open including fields downstream of a cycle, and is_empty_value deliberately sidesteps PHP's empty() trap so 0 and "0" behave the same in both languages.
Approving. Two things I'd want fixed before 50978 and 50979 make the resolver live, both inline.
One caveat on method: the PHP results come from executing this branch's class through the CLI; the JS expectations are read from the code plus spec semantics, not observed. Those are the claims to re-check first if anything surprises you.
| return true; | ||
| } | ||
| if ( is_string( $value ) ) { | ||
| return '' === trim( $value ); |
There was a problem hiding this comment.
trim() means different things in the two languages, and this is where it shows.
JS String.prototype.trim strips the ECMAScript WhiteSpace production — U+00A0 NBSP, the whole Unicode Zs category including U+3000 IDEOGRAPHIC SPACE, U+FEFF, and form feed. PHP trim() strips exactly " \t\n\r\0\x0B": not NBSP, not U+3000, not \f — and it does strip NUL, which JS doesn't. Executing this class confirms the split:
ideographic space is_not_empty -> PHP true (JS false)
nbsp is_empty -> PHP false (JS true)
form feed is_empty -> PHP false (JS true)
NUL is_not_empty -> PHP false (JS true)
Concretely: field B is required and shown when A is_not_empty. The visitor types a single full-width space into A — routine with a Japanese IME — or pastes an NBSP. The browser reads A as empty and hides B. PHP reads A as non-empty, resolves B visible, required-validates it, and rejects the submission for a field that was never on screen. The is_empty mirror goes the other way and discards a real answer at storage.
Worth picking one set explicitly rather than inheriting each language's. On the PHP side preg_replace( '/^[\s\x{00a0}\x{feff}]+|[\s\x{00a0}\x{feff}]+$/u', '', $value ) matches JS closely (\s under /u covers \f and Zs). Then a fixture case with a U+00A0-only actual and is_empty -> true pins it in both consumers at once.
| public function test_the_shared_table_covers_every_comparison_family() { | ||
| $types = array_unique( array_column( self::load_cases(), 'type' ) ); | ||
|
|
||
| foreach ( array( 'date', 'time', 'number', 'consent', 'checkbox', 'text', 'select' ) as $type ) { |
There was a problem hiding this comment.
This guard list is exactly the set of types that already have cases, so it can't fail for a missing one. multichoice, hidden and file have zero fixture cases today and removing them wouldn't turn this red.
multichoice is the expensive gap: membership semantics (in_array(..., true) vs includes, the comma-in-a-label case, array-vs-scalar actual) are the most divergence-prone comparison in the file, and they're pinned only by two hand-mirrored suites with nothing keeping them in step. The consumers already pass $case['actual'] straight through, so array values need no consumer change — just cases.
Full coverage is 15 of the 37 (type key, operator) pairs OPERATORS_BY_TYPE_KEY offers. Also missing: does_not_contain, not_equals, gte, lte, is/is_not on string, is_not on date and time.
Related, and the reason the gap matters: nothing checks that the operators the UI offers are operators the evaluators actually implement. Conditional_Logic_Parity_Test pins OPERATORS and TYPE_KEY_BY_FIELD_TYPE but not OPERATORS_BY_TYPE_KEY against the dispatch. Add contains to date in that table and the rule builder offers it, both engines fall through their switch to null, the rule is dropped, the field is unconditionally visible — no failing test, no notice. A third parity check asserting every offered pair returns non-null from a PHP evaluation would catch drift in both directions. On the TS side, narrowing Rule.operator from Operator | string to Operator plus a never default arm covers the other half.
One more, unrelated but cheap: $cases[ $case['name'] ] a few lines up means two cases sharing a name collapse into one on the PHP side while both still run in JS. Keying on the index and carrying the name in the row avoids that.
…conditional-logic-resolver
The attribute kept its rules in a map keyed by condition kind, which cannot express "any of these AND all of those" -- so supporting more than one grouping later meant reshaping what is already stored. It is an array of groups now, each combining its own rules with its own operator, combined with each other by the top-level one. Both evaluators handle several groups already, even though the V1 panel writes exactly one: if only the storage changed, the second group would still arrive needing an evaluator change. With a single group the outer reduction is a no-op, so behaviour is unchanged. Rules now carry their own type, so further condition kinds become new rule types inside a group rather than another reshape. A rule of an unknown kind is ignored, so a form saved by a newer editor degrades to its remaining conditions.
…conditional-logic-resolver
A rating was declared as a `number`, and every rule against one was silently dead. The field submits `selected/max` -- `4/5`, not `4` -- so is_numeric() and Number() both refuse it, and every numeric operator returns false. "Show when the rating is at least 4" hid its field permanently. Only is_empty and is_not_empty worked, because they never reach the numeric comparison. Rating is its own type key rather than a special case inside `number`, because two things differ. The submitted value needs unpacking before it can be compared -- only the submitted side, since the rule stores a bare number. And the values worth offering are the field own scale, so the rule builder lists 1..max from the block max attribute instead of a free number box that would accept 6 stars out of 5. The operators are the numeric set, which is why sharing the key looked reasonable.
OPERATORS_BY_TYPE_KEY lists the operators the rule builder offers per field type. evaluate_rule_value() returns null for any pair it does not handle, and a null outcome is dropped silently: the rule never counts and the field it guards falls unconditionally visible, with no failing test. Walk the offered table and assert each of the 45 pairs evaluates to a real boolean, so adding an operator to the UI without wiring its comparison turns the build red instead of shipping a dead rule.
kraftbj
left a comment
kraftbj
left a comment
There was a problem hiding this comment.
Approving. Re-reviewed the delta since my last pass — the groups[] restructure and the dedicated rating type are correct and mirror cleanly across the JS and PHP evaluators, with the group reductions and fail-open branches covered 1:1 in both languages.
I pushed one addition (ba2bdec): a parity guard walking OPERATORS_BY_TYPE_KEY and asserting every offered (type, operator) pair dispatches to a real boolean in PHP, so offering an operator in the rule builder without wiring its comparison fails the build instead of silently dropping the rule. That closes the systemic half of my earlier coverage note and retroactively pins the new rating operators.
Remaining items are non-blocking follow-ups, at your discretion: the is_empty/numeric trim() divergence between PHP and JS on non-ASCII whitespace (now with one more call in to_rating_value()); no multichoice fixture cases yet; Rule.operator still typed Operator | string; and behaviour_cases() keyed by name so duplicate names would collapse on the PHP side. Also worth a glance when the rule-builder PR lands: rating correctness assumes it stores the bare selected number, since only the submitted side is unpacked.
|
closing this in favour of #50938 |
github-actions
Bot
removed
the
[Status] Needs Review
2 participants
Fixes FORMS-746
Proposed changes
Part 2 of 5 splitting #50938. Given a form's rules and its current answers, decide which fields are visible. Pure functions — nothing calls them yet.
The same resolution has to happen twice: in the browser as the visitor types, and on the server at submit time, where the browser's answer cannot be trusted. So it is implemented once in each language, and pinned together by a parity test that fails if the two ever disagree about an operator or a comparison.
Why a fixed point rather than a single pass
A field's visibility can depend on a field that is itself conditional, so one pass can settle A while leaving B stale. The resolver iterates to a fixed point instead.
A cycle has no fixed point, so it fails open: every field still changing after the first pass is shown. Hiding a field the visitor has no way to reveal is a dead end; a form showing too much is recoverable in a way that one showing too little is not.
Review notes
This is the intellectual core of the feature and the part most worth arguing about — particularly the fail-open choice and the operator semantics for empty values.
Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
No user-visible behaviour. Verification is the test suite:
jetpack test js packages/forms—evaluate.test.jsjetpack test php packages/forms—Conditional_Logic_Test.php(the resolver) andConditional_Logic_Parity_Test.php(the two implementations agree)To see the parity test do its job, change an operator in
util/field-types.tswithout changingclass-conditional-logic.php, and confirmConditional_Logic_Parity_Testfails.