Skip to content

Commit 39a0718

Browse files
enejbclaudeCopilot
authored
Forms: Fix fatal error when field render value is an array (#48032)
* Fix fatal error in get_legacy_extra_values when field value is an array Guard against array values being used as array keys in the get_legacy_extra_values method. When a form field (e.g., email) receives array POST data, get_render_value() returns an array which cannot be used as a PHP array key, causing a TypeError fatal error. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * Enforce string values for single-value field types For field types that should always be strings (email, name, url, subject, textarea, ip), take the first element when array POST data is submitted. Also use first-element fallback in get_legacy_extra_values as a defensive guard for any pre-existing array values. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * Rewrite test to reproduce the fatal without test isolation dependency Use direct Feedback_Field construction with array value for a url-type field and v2 format storage, which works in isolation. The key is using 'submit' context — get_render_submit_value() returns the raw array, unlike 'default' which implodes it. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * Add parameterized tests for all non-extra field types with array values Test every field type in $non_extra_fields (email, name, url, subject, textarea, ip) with array values across both submit and default contexts. Also verify checkbox-multiple arrays are preserved in submit context and imploded in default context. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * Strengthen test assertions to verify data correctness, not just no-crash Assert that extra values contain expected field values (Comment text field in parameterized tests, checkbox-multiple array in preservation test), not just that the method returns an array. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * Fix changelog entry: add trailing period and broaden description to cover both fixed sites Agent-Logs-Url: https://github.com/Automattic/jetpack/sessions/7b5b7414-2667-4535-9781-d5ef5ee74cac Co-authored-by: enejb <[email protected]> * Add test for legacy v1 checkbox-multiple array preservation Legacy v1 format stores checkbox-multiple as JSON arrays with field type 'basic'. Verify the full array survives the round-trip and is not flattened by the reset() guard in get_legacy_extra_values(). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * Simplify fatal error fix to minimal array handling The type-aware unwrap logic added in e29b191 was broader than necessary. The minimal fix for the fatal is to handle arrays in sanitization; preserve arrays for all field types and let downstream code handle them. --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]> Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: enejb <[email protected]>
1 parent 495c63e commit 39a0718

3 files changed

Lines changed: 220 additions & 4 deletions

File tree

‎projects/packages/forms/changelog/fix-forms-fatal-error‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Significance: patch
2+
Type: fixed
3+
4+
Prevent fatal error when a non-checkbox field's render value or POST-submitted value is an array.

‎projects/packages/forms/src/contact-form/class-feedback.php‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -649,18 +649,28 @@ public function get_legacy_extra_values( $context = 'default' ) {
649649
$special_fields = array();
650650
$non_extra_fields = array( 'email', 'name', 'url', 'subject', 'textarea', 'ip' );
651651

652-
// Create a map of special fields to check agains their values.
652+
// Create a map of special fields to check against their values.
653653
foreach ( $this->fields as $field ) {
654-
if ( in_array( $field->get_type(), $non_extra_fields, true ) && $field->get_render_value( $context ) ) {
655-
$special_fields[ $field->get_render_value( $context ) ] = true;
654+
if ( in_array( $field->get_type(), $non_extra_fields, true ) ) {
655+
$value = $field->get_render_value( $context );
656+
if ( is_array( $value ) ) {
657+
$value = reset( $value );
658+
}
659+
if ( $value ) {
660+
$special_fields[ $value ] = true;
661+
}
656662
}
657663
}
658664

659665
foreach ( $this->fields as $field ) {
660666
if ( $field->compile_field( 'default' ) ) {
661667
continue;
662668
}
663-
if ( $field->get_type() === 'basic' && isset( $special_fields[ $field->get_render_value() ] ) ) {
669+
$render_value = $field->get_render_value();
670+
if ( is_array( $render_value ) ) {
671+
$render_value = reset( $render_value );
672+
}
673+
if ( $field->get_type() === 'basic' && $render_value && isset( $special_fields[ $render_value ] ) ) {
664674
++$count;
665675
continue; // Skip fields that are already present in the non-extra fields.
666676
}

‎projects/packages/forms/tests/php/contact-form/Feedback_Legacy_Compatibility_Test.php‎

Lines changed: 202 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
require_once __DIR__ . '/class-utility.php'; // phpcs:ignore WordPressVIPMinimum.Files.IncludingFile.NotAbsolutePath
1313

1414
use PHPUnit\Framework\Attributes\CoversClass;
15+
use PHPUnit\Framework\Attributes\DataProvider;
1516
use WorDBless\BaseTestCase;
1617

1718
/**
@@ -296,6 +297,207 @@ public function test_escape_legacy_special_characters_handeling_strip_new_lines(
296297
$this->assertEquals( '🙈', $response->get_field_value_by_label( 'message' ), 'Message field value should match' );
297298
}
298299

300+
/**
301+
* Data provider: every field type in $non_extra_fields that must not
302+
* fatal when its value is an array.
303+
*/
304+
public static function data_provider_non_extra_field_types() {
305+
return array(
306+
'email' => array( 'email', 'Email', array( '[email protected]', '[email protected]' ) ),
307+
'name' => array( 'name', 'Name', array( 'Alice', 'Bob' ) ),
308+
'url' => array( 'url', 'Website', array( 'https://example.com', 'https://another.com' ) ),
309+
'subject' => array( 'subject', 'Subject', array( 'Subject A', 'Subject B' ) ),
310+
'textarea' => array( 'textarea', 'Message', array( 'First message', 'Second message' ) ),
311+
'ip' => array( 'ip', 'IP', array( '127.0.0.1', '192.168.1.1' ) ),
312+
);
313+
}
314+
315+
/**
316+
* Regression test: get_legacy_extra_values() must not fatal when a
317+
* non-extra field holds an array value.
318+
*
319+
* In production this happens when POST data contains multiple values
320+
* for the same field name, causing get_field_value() to store an array.
321+
* The 'submit' context is critical because get_render_submit_value()
322+
* returns $this->value without any array-to-string conversion — unlike
323+
* 'default' context which implodes arrays.
324+
*
325+
* @dataProvider data_provider_non_extra_field_types
326+
*/
327+
#[DataProvider( 'data_provider_non_extra_field_types' )]
328+
public function test_get_legacy_extra_values_with_array_field_value_does_not_fatal( $type, $label, $array_value ) {
329+
$feedback_time = current_time( 'mysql' );
330+
$feedback_title = "Test User - {$feedback_time}";
331+
332+
$array_field = new Feedback_Field( '1_' . $label, $label, $array_value, $type );
333+
$text_field = new Feedback_Field( '2_Comment', 'Comment', 'Hello world', 'text' );
334+
335+
$content = array(
336+
'subject' => 'Test Subject',
337+
'ip' => '127.0.0.1',
338+
'fields' => array(
339+
$array_field->serialize(),
340+
$text_field->serialize(),
341+
),
342+
);
343+
344+
$post_id = wp_insert_post(
345+
array(
346+
'post_type' => 'feedback',
347+
'post_status' => 'publish',
348+
'post_title' => addslashes( wp_kses( $feedback_title, array() ) ),
349+
'post_date' => $feedback_time,
350+
'post_name' => md5( $feedback_title . $type ),
351+
'post_content' => wp_json_encode( $content, JSON_UNESCAPED_SLASHES ),
352+
'post_mime_type' => 'v2',
353+
'post_parent' => 0,
354+
)
355+
);
356+
357+
$response = Feedback::get( $post_id );
358+
$this->assertInstanceOf( Feedback::class, $response, "Feedback should load for {$type} field" );
359+
360+
// 'submit' context — the production path (class-contact-form.php:2678).
361+
// get_render_submit_value() returns the raw array, which without the fix
362+
// triggers: TypeError: Cannot access offset of type array on array
363+
$extra_values_submit = $response->get_legacy_extra_values( 'submit' );
364+
$this->assertIsArray( $extra_values_submit, "get_legacy_extra_values(submit) should not fatal for {$type} with array value" );
365+
366+
// The Comment text field should appear in extra values with correct value.
367+
$this->assertNotEmpty( $extra_values_submit, "Extra values should not be empty for {$type} with array value" );
368+
$found_comment = false;
369+
foreach ( $extra_values_submit as $value ) {
370+
if ( $value === 'Hello world' ) {
371+
$found_comment = true;
372+
break;
373+
}
374+
}
375+
$this->assertTrue( $found_comment, "Extra values should contain the Comment field value for {$type} test case" );
376+
377+
// 'default' context — implodes arrays to strings, verify it still works.
378+
$extra_values_default = $response->get_legacy_extra_values( 'default' );
379+
$this->assertIsArray( $extra_values_default, "get_legacy_extra_values(default) should not fatal for {$type} with array value" );
380+
}
381+
382+
/**
383+
* Verify that checkbox-multiple fields preserve their array values
384+
* through get_legacy_extra_values() — they are the one field type
385+
* that legitimately holds arrays.
386+
*/
387+
public function test_get_legacy_extra_values_preserves_checkbox_multiple_array() {
388+
$feedback_time = current_time( 'mysql' );
389+
$feedback_title = "Test User - {$feedback_time}";
390+
391+
$checkbox_field = new Feedback_Field( '1_Colors', 'Colors', array( 'red', 'blue', 'green' ), 'checkbox-multiple' );
392+
$name_field = new Feedback_Field( '2_Name', 'Name', 'Test User', 'name' );
393+
394+
$content = array(
395+
'subject' => 'Test Subject',
396+
'ip' => '127.0.0.1',
397+
'fields' => array(
398+
$checkbox_field->serialize(),
399+
$name_field->serialize(),
400+
),
401+
);
402+
403+
$post_id = wp_insert_post(
404+
array(
405+
'post_type' => 'feedback',
406+
'post_status' => 'publish',
407+
'post_title' => addslashes( wp_kses( $feedback_title, array() ) ),
408+
'post_date' => $feedback_time,
409+
'post_name' => md5( $feedback_title ),
410+
'post_content' => wp_json_encode( $content, JSON_UNESCAPED_SLASHES ),
411+
'post_mime_type' => 'v2',
412+
'post_parent' => 0,
413+
)
414+
);
415+
416+
$response = Feedback::get( $post_id );
417+
$this->assertInstanceOf( Feedback::class, $response );
418+
419+
// checkbox-multiple values should remain as arrays in submit/api contexts.
420+
$value_submit = $response->get_field_value_by_label( 'Colors', 'submit' );
421+
$this->assertIsArray( $value_submit, 'checkbox-multiple value should be an array in submit context' );
422+
$this->assertEquals( array( 'red', 'blue', 'green' ), $value_submit, 'checkbox-multiple value should preserve all selected options' );
423+
424+
// In default context, arrays are imploded to a comma-separated string.
425+
$value_default = $response->get_field_value_by_label( 'Colors', 'default' );
426+
$this->assertEquals( 'red, blue, green', $value_default, 'checkbox-multiple value should be imploded in default context' );
427+
428+
// get_legacy_extra_values should work without fatal and include the checkbox-multiple field.
429+
$extra_values = $response->get_legacy_extra_values( 'submit' );
430+
$this->assertIsArray( $extra_values, 'get_legacy_extra_values(submit) should work with checkbox-multiple' );
431+
432+
// checkbox-multiple is not in $non_extra_fields, so its value should appear in extra values.
433+
$found_colors = false;
434+
foreach ( $extra_values as $value ) {
435+
if ( is_array( $value ) && $value === array( 'red', 'blue', 'green' ) ) {
436+
$found_colors = true;
437+
break;
438+
}
439+
}
440+
$this->assertTrue( $found_colors, 'Extra values should contain the checkbox-multiple array value' );
441+
}
442+
443+
/**
444+
* Legacy v1 feedback stores checkbox-multiple values as JSON arrays in
445+
* post_content. When loaded, these become Feedback_Field objects with
446+
* type 'basic' (not 'checkbox-multiple') and an array value.
447+
*
448+
* Verify the full array is preserved through get_legacy_extra_values()
449+
* and not flattened to just the first element by the reset() guard.
450+
*/
451+
public function test_legacy_v1_checkbox_multiple_array_preserved() {
452+
// In legacy v1 format, checkbox-multiple values are stored as JSON arrays.
453+
// When loaded via process_legacy_values(), the field type becomes 'basic'.
454+
$post_id = Utility::create_legacy_feedback(
455+
array(
456+
'1_Colors' => array( 'red', 'blue', 'green' ),
457+
'2_Name' => 'Test User',
458+
'3_Comment' => 'Hello world',
459+
)
460+
);
461+
462+
$response = Feedback::get( $post_id );
463+
$this->assertInstanceOf( Feedback::class, $response );
464+
465+
// The array value should survive the round-trip through legacy format.
466+
// In default context, get_render_default_value() implodes arrays to strings.
467+
$value_default = $response->get_field_value_by_label( 'Colors', 'default' );
468+
$this->assertEquals( 'red, blue, green', $value_default, 'Legacy v1 checkbox-multiple should be imploded in default context' );
469+
470+
// In submit context, get_render_submit_value() returns the raw value.
471+
$value_submit = $response->get_field_value_by_label( 'Colors', 'submit' );
472+
$this->assertIsArray( $value_submit, 'Legacy v1 checkbox-multiple should remain an array in submit context' );
473+
$this->assertEquals( array( 'red', 'blue', 'green' ), $value_submit, 'Legacy v1 checkbox-multiple should preserve all values' );
474+
475+
// get_legacy_extra_values should not flatten the array via reset().
476+
$extra_values_submit = $response->get_legacy_extra_values( 'submit' );
477+
$this->assertIsArray( $extra_values_submit, 'get_legacy_extra_values(submit) should work with legacy checkbox-multiple' );
478+
479+
// The field should appear in extra values with all array items intact.
480+
$found_colors = false;
481+
foreach ( $extra_values_submit as $value ) {
482+
if ( is_array( $value ) && $value === array( 'red', 'blue', 'green' ) ) {
483+
$found_colors = true;
484+
break;
485+
}
486+
}
487+
$this->assertTrue( $found_colors, 'Legacy v1 checkbox-multiple array should not be flattened to first element' );
488+
489+
// Also verify default context preserves the imploded string.
490+
$extra_values_default = $response->get_legacy_extra_values( 'default' );
491+
$found_colors_string = false;
492+
foreach ( $extra_values_default as $value ) {
493+
if ( $value === 'red, blue, green' ) {
494+
$found_colors_string = true;
495+
break;
496+
}
497+
}
498+
$this->assertTrue( $found_colors_string, 'Legacy v1 checkbox-multiple should appear as imploded string in default context extra values' );
499+
}
500+
299501
public function test_escape_legacy_v2_special_characters_handeling() {
300502
$post_id = Utility::create_legacy_feedback_v2(
301503
array(

0 commit comments

Comments
 (0)