Forms: conditional logic (4/5) — server-side render and validation - #50979
Forms: conditional logic (4/5) — server-side render and validation#50979enejb wants to merge 6 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! |
aec5512 to
9358fbb
Compare
9358fbb to
3336b8a
Compare
Problems the browser cannot solve on its own. A required field that conditional logic hides used to block submission: it was invalid, but the visitor could not see it to fill it in. Validation now skips fields the rules hide, evaluated server-side from the submitted values rather than trusting anything the browser reports. Parse-time validation is deferred for conditional fields, which otherwise wrote a static error before any answers existed -- and every submission path now validates once the form is whole, which the legacy path did not. Fields that start hidden used to render visible and then disappear once the runtime booted. Initial visibility is resolved while rendering and stamped onto the markup, on the element that holds the field's slot in the row, so the form is correct in its first paint and the row does not keep a hole. Values prefilled through the query string are taken into account. Validation and storage consult one resolution rather than resolving separately over different value sources, and a hidden field's answer is stripped once on the way in -- so the stored response, the comment content, the consent flag, the author details and the integrations all agree that it was never answered. The rules survive the shortcode roundtrip: they serialize to a JSON array, and both the brackets and a bare `<` used to break the value out of the attribute, dropping the condition while keeping the field required.
3336b8a to
7d52857
Compare
…nditional-logic-server
PHPCS treats the double-arrow misalignment as a warning, and this repo's phpcs run exits non-zero on warnings, so it fails the build.
kraftbj
left a comment
kraftbj
left a comment
There was a problem hiding this comment.
Reviewed the server layer on its own. The architecture is right — one cached visibility map that validation and storage genuinely share, the flag checked at a single choke point, and the shortcode wire format is the most careful part of the PR. I traced [ -> [ -> esc_html with double_encode: false -> html_entity_decode(ENT_COMPAT) and it round-trips correctly for &, ", ,, \ and < inside rule values; JSON_HEX_AMP is what makes that safe, and esc_shortcode_val's backslash handling protects the JSON escapes from shortcode_parse_atts's stripcslashes. Also confirmed the round-1 enforcement gap is genuinely closed: only two paths reach storage and both now call $form->validate().
Two P1s though, both reachable through the editor UI with no forged input, and both cheaper to fix before the action signature and the wire format are in the field. Inline.
Two test notes worth pulling out, since they're the reason these survived: Feedback_Conditional_Logic_Test::test_storage_agrees_with_validation_about_a_cleared_prefilled_trigger asserts storage agrees with the form's own resolution rather than with what the browser rendered — so it currently pins the first P1 as correct behavior. And its scenario (a text field absent from $_POST) isn't reachable, since a cleared text input still posts an empty string. Rebuilt around a checkbox with default => 'true', that test becomes the regression test for it.
One behavior change to call out separately: the legacy path now also runs the "Please fill out at least one field." check and validate_ref(), neither gated on the flag. Almost certainly intended, but it changes what that path accepts on every site.
| // default, then the logged-in user's details. Reading $_POST alone would make a | ||
| // prefilled form resolve against an empty one, so a field the visitor can already | ||
| // see satisfying a condition would render hidden and then flash into view. | ||
| $values[ $field_id ] = $field->get_computed_field_value( |
There was a problem hiding this comment.
The comment above is right for render and wrong for submit, and this function serves both.
get_computed_field_value() is POST -> GET -> default -> logged-in autofill. Every other reader of a submission is POST-only: has_value() at class-contact-form-field.php:336 and validate() at :384. So inside the single loop in Contact_Form::validate() the decision whether to validate a field comes from one value source and the validation itself from another.
Harmless for anything that always posts. It bites the controls that post nothing when unset — checkbox, explicit consent, radio, checkbox-multiple. No query string needed; the editor's own "Checked by default" toggle (field-checkbox/edit.jsx:46 sets defaultValue: 'true', mapped to default) is enough:
subscribeis a checkbox, checked by default.detailsis required, shown whensubscribeis_checked.- Render: server and client agree,
detailsvisible. - Visitor unchecks
subscribe. Client value becomes'',detailsgetsdisplay: none. - Submit.
FormDataomits the unchecked box entirely. - Server:
$_POSTunset,$_GETunset, falls through to'true'-> is_checked true ->detailsvisible -> validated -> "Details is required" for a field that isn't on screen and can't be cleared.
Invert the rule and you get the other half: the visitor unchecks, fills in the now-visible details, and the server resolves it hidden, so without_hidden_answers() strips the answer with no error anywhere.
The $_GET half is narrower than it looks — the AJAX action is admin-ajax.php and the non-AJAX action is get_permalink(), so query args only survive for widget and block-template forms. The default fallback is the reliably reachable one.
apply_initial_field_visibility() should keep this chain, since the reasoning in the comment genuinely applies to render. get_resolved_field_visibility() should build values the way has_value() does — isset( $_POST[ $id ] ) ? wp_unslash( $_POST[ $id ] ) : ''. A $source argument covers both; that getter is only ever called from validation and storage, so it can be POST-only unconditionally.
| // particular reads this payload directly for explicit consent and the subscriber's | ||
| // email, so a forged POST naming a hidden consent field could otherwise subscribe | ||
| // someone off a question that was never on screen. | ||
| $visible_fields = $this->fields; |
There was a problem hiding this comment.
This filter is defeated immediately — all three integrations recover the unfiltered set from $fields[0]->form:
service/class-post-to-url.php:159—foreach ( $form->fields as $field ), so every hidden answer is POSTed to the configured webhook or Salesforce endpoint.service/class-mailpoet-integration.php:177(get_subscriber_data_from_fields) and:281(the consent gate).service/class-hostinger-reach-integration.php:137and:180.
The v2 question resolves against this PR rather than in its favour: Feedback writes 'post_mime_type' => 'v3' (class-feedback.php:1598) while both integrations compute $is_v2_data = ( $post->post_mime_type === 'v2' ). So for every submission this code creates, the else branch is the live one — the branch that reads $form->fields and $consent_field->value instead of the Feedback API.
And hidden fields are posted: grunion.scss:1669 hides with display: none !important only, nothing disables or removes the inputs, and FormData includes them. So this is the ordinary path — fill a field, then change the trigger so it hides — not a forged request. The comment's own example lands: a hidden explicit-consent field ticked before it hid still passes has_consent(), and a hidden email field still supplies the subscriber address.
Everything else you filtered does hold — I checked comment_content, the consent flag, the subject, the author record, notification recipients, and Form_Webhooks, and they all read the $post_data that without_hidden_answers() already stripped. The leak is confined to these three $form->fields readers.
They only walk $fields to recover $form, so switching them to use the $fields argument they're already handed is mechanical. The alternative — unsetting hidden entries from $this->fields around the action — works too but is easier to get wrong.
|
|
||
| $value = $this->get_field_value( $field_id, $post_data, $type ); | ||
| $label = wp_strip_all_tags( $field->get_attribute( 'label' ) ); | ||
| $key = $i . '_' . $label; |
There was a problem hiding this comment.
$i now advances only for fields that get stored (the continue four lines up, ++$i at the bottom of the loop), where before it advanced for every renderable field.
For [Name, Extra(conditional), Message] one response persists 1_Name, 2_Extra, 3_Message and the next persists 1_Name, 2_Message — the same question on a different key depending on which siblings that visitor happened to see. The key is persisted into _feedback_all_fields in post_content and exposed over REST in the collection shape, so it's a stored identifier rather than a display detail.
Moving ++$i above the visibility continue fixes it — the counter is meant to encode position in the form, which is what makes it stable.
| // the runtime hides. | ||
| $visibility_attrs = " data-jp-visibility-root='" . esc_attr( $id ) . "'" | ||
| . ( $this->has_conditional_logic() ? " data-jp-conditional='1'" : '' ) | ||
| . ' data-wp-class--jetpack-field--conditionally-hidden="state.isFieldHidden"' |
There was a problem hiding this comment.
data-jp-conditional one line up is gated on has_conditional_logic(); these two directives aren't gated on anything — not the flag, not whether the field has logic, not whether the form has any. So every field of every form on every site running the package ships a class binding and a watch callback for a feature that's off by default, which contradicts the "adds nothing to the page" claim in the get_conditional_logic_context() docblock.
It's inert rather than wrong today — with the flag off the context is array(), resolveFormVisibility returns null, and both the getter and the callback early-return. But manageConditionalFocus is a data-wp-watch, so it re-runs on every context signal change on every field on sites that have nothing to do with this feature.
Wrapping the $visibility_attrs construction in Jetpack_Forms::is_conditional_logic_enabled() matches how every other new path here is gated. Worth noting Conditional_Logic_Initial_Render_Test::test_nothing_is_marked_when_the_feature_is_off only asserts on the hidden class, so it passes either way.
| * | ||
| * @return Contact_Form | ||
| */ | ||
| private function parse_form( $trigger_value, $dependent_value ): Contact_Form { |
There was a problem hiding this comment.
This never reaches the defer it's testing. The parse-time gate at class-contact-form.php:2659 requires $_POST['action'] === 'grunion-contact-form', a matching contact-form-id, and a hash_equals match on contact-form-hash. parse_form sets only the two field values, so the whole if block — including the new $defer_to_full_form_validation decision at :2680 — is skipped.
test_the_parse_pass_defers_a_conditional_field and the first assertion of test_a_visible_required_field_is_only_caught_once_validate_runs therefore pass identically with the defer logic deleted. The branch has no coverage at all.
Which is a shame, because this file's header specifically says these tests exist "because the defer decision lives inside Contact_Form::parse_contact_field()" and that restating the decision "is how the missing legacy-path validation got through review" — the same shape of gap, one level further out. Setting the three $_POST keys in parse_form opens the gate.
Related coverage gap while you're in here: nothing exercises the production encode/decode chain end to end. parse_form hand-rolls the wire format rather than calling block_attributes_to_shortcode_attributes(), so esc_shortcode_val is never in the loop and the encoding can drift silently. One test running a rule value containing [, &, ", ,, \ and < through block_attributes_to_shortcode_attributes() -> parse_contact_field() -> do_shortcode() and asserting the decoded array equals the input would pin the most delicate code in the PR.
…nditional-logic-server
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.
…nditional-logic-server
2 participants
Fixes FORMS-748
Proposed changes
Part 4 of 5 splitting #50938. Two problems the browser cannot solve on its own.
A required field that conditional logic hides used to block submission. It was invalid, but the visitor could not see it to fill it in. Validation now skips fields the rules hide, evaluated server-side from the submitted values rather than trusting anything the browser reports. Parse-time validation is deferred for conditional fields, which otherwise wrote a static error before any answers existed.
Fields that start hidden used to render visible and then disappear once the runtime booted. Initial visibility is now resolved while rendering and stamped onto the markup, so the form is correct in its first paint. Values prefilled through the query string are taken into account, so a link that prefills the field a rule depends on renders the resulting state directly.
Only hidden fields are dropped from the stored feedback, so a response records what the visitor was actually asked.
Related product discussion/links
Does this pull request change what data or activity we track or use?
A form response no longer stores values for fields that conditional logic hid at submit time. Nothing new is collected.
Testing instructions
With the flag on and a form that hides a required field:
?fieldname=valueprefilling the field a rule depends on → the dependent field renders in its resulting state immediately.Also:
jetpack test php packages/forms.