Forms: conditional logic (1/5) — vocabulary and feature flag - #50976
Forms: conditional logic (1/5) — vocabulary and feature flag#50976enejb wants to merge 4 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?
|
github-actions
Bot
added
[Block] Contact Form
[Feature] Contact Form
[Package] Forms
[Plugin] Jetpack
|
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! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 20 files. Only the first 5 are listed here.
2 files are newly checked for coverage.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
648cdb3 to
82c8b14
Compare
Any field can be shown or hidden based on another field's answer. Disabled by default: the whole feature sits behind the `forms-conditional-logic` flag, registered with the jetpack-feature-flags package, while it is in testing. Five pieces, reviewable separately as #50976-#50980: - the shared vocabulary both sides speak -- the comparison behaviour each field type has, the operators it offers, the value input each operator needs -- plus a `conditional_logic` declaration on each field block, beside `form_editor` and outside the registered block settings so it cannot collide with core block metadata - the resolver, written once in JS and once in PHP, because the browser decides what the visitor sees while the server decides what it accepts, and the browser's answer cannot be trusted at submit time. Resolution is a fixed point, since a field's visibility can depend on a field that is itself conditional; a cycle fails open, because hiding a field the visitor cannot reveal is a dead end - the front-end runtime, which shows and hides fields as the visitor answers and skips hidden fields when validating - the server side: hidden fields are skipped by validation, so a required field the visitor cannot see can never block submission; initial visibility is resolved during render so a hidden field never flashes into view; and a hidden field's answer reaches nothing downstream - the editor panel, added by an `editor.BlockEdit` filter rather than per-block wiring, loaded through a lazy boundary so a site with the feature off never fetches it Rebased onto trunk by content rather than by replaying commits: the branch had 36 commits against 103 of drift, and the conflicts landed in intermediate states that later work removed outright, so replaying them would have produced commits that never existed in a working form. The tree here is byte-identical to the verified pre-rebase branch merged with trunk.
82c8b14 to
4312554
Compare
Foundation for showing or hiding a form field based on another field's answer. Nothing renders or evaluates yet: this establishes the shared vocabulary the editor and the runtime both speak, and the flag that gates the whole feature. - the comparison behaviours a field can have (string, choice, multichoice, number, date, time, boolean, hidden, file), the operators each one offers and the value input each operator needs - the `conditionalLogic` block attribute, keyed by control slug so that further condition types become sibling keys rather than a reshape - a `conditional_logic` declaration on each field block, beside `form_editor` and outside the registered block settings, so there is no chance of colliding with core block metadata - the `forms-conditional-logic` flag, registered with the jetpack-feature-flags package and bridged into the editor so PHP and JS answer from one source Declaring support per block means a field block that omits it simply gets no conditional-logic support, so the feature can be turned on one block at a time. image-select deliberately opts out: it submits a JSON document rather than the label the rule builder offers, so a rule against it could never match. block-names.test.js reads the block sources rather than restating the block list: two blocks register under a name that differs from their directory (field-single-choice as jetpack/field-radio, field-multiple-choice as jetpack/field-checkbox-multiple), and a hand-written table got both wrong.
4312554 to
cc52eb6
Compare
…l-logic-foundation
kraftbj
added
the
Coverage tests to be added later
kraftbj
left a comment
kraftbj
left a comment
There was a problem hiding this comment.
Reviewed this layer on its own. One thing worth catching before the vocabulary sets, plus a note; everything else checks out — the flag registration and naming match the jetpack-feature-flags README, the composer constraint matches its siblings, the changelog is right, and the other 17 type declarations line up with what those fields actually submit.
Detail inline on the rating declaration.
| * input. A block that omits this simply gets no conditional-logic support. | ||
| */ | ||
| export const conditional_logic = { | ||
| type: 'number', |
There was a problem hiding this comment.
A rating field doesn't submit a bare number. render_rating_field() builds each radio as value="%3$s/%4$s" (class-contact-form-field.php:3198 on trunk), so picking four stars posts "4/5", and class-feedback-field.php's get_rating_value() confirms that string is what gets stored — the array shape with type => 'rating' only appears later in display formatting.
is_numeric('4/5') is false and Number('4/5') is NaN. Once the resolver lands in 50977, that means every numeric operator against a rating is permanently false in both engines — equals, greater_than, gte, and also not_equals, since the null pair short-circuits before the operator switch. The author gets a rule that looks fine in the panel and never fires, with nothing indicating it's dead.
This is the same mismatch that got field-image-select excluded a few files over, with a comment explaining why. Two options: exclude rating the same way, or give it its own type key and parse the numerator in both evaluators. The value is trivially parseable, so a real rating key is the more useful answer — but the parse belongs to 50977, and what this PR should avoid is asserting 'number' as the vocabulary entry everyone downstream reads.
Related, and cheap to settle while the tables are still being written: util/field-types.ts:85 still maps 'image-select' => 'choice'. Dropping the per-block conditional_logic export stops image-select getting its own panel, but this map is what the runtime and PHP consult when an image-select field is referenced as the subject of someone else's rule, so it can still offer is/is not against the same JSON blob the adjacent comment says is unparseable. Worth dropping the entry too, or cross-referencing the exclusion so a later PR doesn't wire it up naively.
Any field can be shown or hidden based on another field's answer. Disabled by default: the whole feature sits behind the `forms-conditional-logic` flag, registered with the jetpack-feature-flags package, while it is in testing. Five pieces, reviewable separately as #50976-#50980: - the shared vocabulary both sides speak -- the comparison behaviour each field type has, the operators it offers, the value input each operator needs -- plus a `conditional_logic` declaration on each field block, beside `form_editor` and outside the registered block settings so it cannot collide with core block metadata - the resolver, written once in JS and once in PHP, because the browser decides what the visitor sees while the server decides what it accepts, and the browser's answer cannot be trusted at submit time. Resolution is a fixed point, since a field's visibility can depend on a field that is itself conditional; a cycle fails open, because hiding a field the visitor cannot reveal is a dead end - the front-end runtime, which shows and hides fields as the visitor answers and skips hidden fields when validating - the server side: hidden fields are skipped by validation, so a required field the visitor cannot see can never block submission; initial visibility is resolved during render so a hidden field never flashes into view; and a hidden field's answer reaches nothing downstream - the editor panel, added by an `editor.BlockEdit` filter rather than per-block wiring, loaded through a lazy boundary so a site with the feature off never fetches it Rebased onto trunk by content rather than by replaying commits: the branch had 36 commits against 103 of drift, and the conflicts landed in intermediate states that later work removed outright, so replaying them would have produced commits that never existed in a working form. The tree here is byte-identical to the verified pre-rebase branch merged with trunk.
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.
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.
2 participants
Fixes FORMS-745
Proposed changes
Part 1 of 5 splitting #50938 into reviewable pieces. This is the foundation: the shared vocabulary the editor and the runtime both speak, plus the flag that gates the whole feature. Nothing renders or evaluates yet.
string,choice,multichoice,number,date,time,boolean,hidden,file— the operators each one offers, and the value input each operator needs.conditionalLogicblock attribute. Rules are keyed by control slug so that further condition types (query string, user role, date and time) become sibling keys rather than a reshape.conditional_logicdeclaration on each of the 19 field blocks, besideform_editorand outside the registered block settings, so there is no chance of colliding with core block metadata.forms-conditional-logicflag, registered with thejetpack-feature-flagspackage and bridged into the editor, so PHP and JS answer from one source under one name.Declaring support per block is what makes the rest of the series safe to land incrementally: a block that omits the declaration simply gets no conditional-logic support.
Review notes
block-names.test.jsreads the block sources rather than restating the block list. Two blocks register under a name that differs from their directory —field-single-choiceasjetpack/field-radio,field-multiple-choiceasjetpack/field-checkbox-multiple— and a hand-written table got both wrong, silently dropping the panel for those two.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
Nothing is user-visible in this PR — the flag is off and no code path consumes the vocabulary yet. Verification is the test suite:
jetpack test js packages/forms— the operator/type table and the per-block declarationsjetpack test php packages/formsTo confirm the flag is wired, on a site with the package installed:
wp eval 'echo Automattic\Jetpack\Forms\Jetpack_Forms::is_conditional_logic_enabled() ? "on" : "off";'→offadd_filter( 'jetpack_feature_flag_enabled_forms-conditional-logic', '__return_true' );in a mu-plugin →on