From 486488a8f8a6428cf2ce45f1a8ecb9253e051aae Mon Sep 17 00:00:00 2001 From: Miguel Lezama Date: Mon, 13 Jan 2025 10:34:32 -0300 Subject: [PATCH 1/8] unique id --- .../packages/forms/src/contact-form/class-contact-form.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/projects/packages/forms/src/contact-form/class-contact-form.php b/projects/packages/forms/src/contact-form/class-contact-form.php index 5f820fa3c2440..86b965c79749f 100644 --- a/projects/packages/forms/src/contact-form/class-contact-form.php +++ b/projects/packages/forms/src/contact-form/class-contact-form.php @@ -118,6 +118,10 @@ public function __construct( $attributes, $content = null ) { $default_to .= $post_author->user_email; } + if ( ! empty( self::$forms ) ) { + $attributes['id'] = $attributes['id'] . '-' . count( self::$forms ) + 1; + } + $this->hash = sha1( wp_json_encode( $attributes ) ); self::$forms[ $this->hash ] = $this; From 27fbbeec681b76a86320ab4be9568807daf42bd9 Mon Sep 17 00:00:00 2001 From: Miguel Lezama Date: Mon, 13 Jan 2025 10:38:44 -0300 Subject: [PATCH 2/8] fix lint --- projects/packages/forms/src/contact-form/class-contact-form.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/projects/packages/forms/src/contact-form/class-contact-form.php b/projects/packages/forms/src/contact-form/class-contact-form.php index 86b965c79749f..8774b28063f4b 100644 --- a/projects/packages/forms/src/contact-form/class-contact-form.php +++ b/projects/packages/forms/src/contact-form/class-contact-form.php @@ -119,7 +119,7 @@ public function __construct( $attributes, $content = null ) { } if ( ! empty( self::$forms ) ) { - $attributes['id'] = $attributes['id'] . '-' . count( self::$forms ) + 1; + $attributes['id'] = $attributes['id'] . '-' . ( count( self::$forms ) + 1 ); } $this->hash = sha1( wp_json_encode( $attributes ) ); From b10663e2b1b02f6250e5dd09cb88b227b6fda21a Mon Sep 17 00:00:00 2001 From: Miguel Lezama Date: Mon, 13 Jan 2025 10:41:22 -0300 Subject: [PATCH 3/8] changelogs --- projects/packages/forms/changelog/update-forms-unique-id | 4 ++++ projects/plugins/jetpack/changelog/update-forms-unique-id | 5 +++++ 2 files changed, 9 insertions(+) create mode 100644 projects/packages/forms/changelog/update-forms-unique-id create mode 100644 projects/plugins/jetpack/changelog/update-forms-unique-id diff --git a/projects/packages/forms/changelog/update-forms-unique-id b/projects/packages/forms/changelog/update-forms-unique-id new file mode 100644 index 0000000000000..a9fa70192e97b --- /dev/null +++ b/projects/packages/forms/changelog/update-forms-unique-id @@ -0,0 +1,4 @@ +Significance: patch +Type: fixed + +Forms: Add unique ids to each form diff --git a/projects/plugins/jetpack/changelog/update-forms-unique-id b/projects/plugins/jetpack/changelog/update-forms-unique-id new file mode 100644 index 0000000000000..b9412e76b338b --- /dev/null +++ b/projects/plugins/jetpack/changelog/update-forms-unique-id @@ -0,0 +1,5 @@ +Significance: patch +Type: bugfix +Comment: Forms: Add unique ids to each form + + From c1d6f01d5af48aa902ed30d68642addce24a13ee Mon Sep 17 00:00:00 2001 From: Miguel Lezama Date: Mon, 13 Jan 2025 11:51:04 -0300 Subject: [PATCH 4/8] fix tests --- .../forms/tests/php/contact-form/test-class.contact-form.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/projects/packages/forms/tests/php/contact-form/test-class.contact-form.php b/projects/packages/forms/tests/php/contact-form/test-class.contact-form.php index 0148b98822261..51e65b7ebbbb9 100644 --- a/projects/packages/forms/tests/php/contact-form/test-class.contact-form.php +++ b/projects/packages/forms/tests/php/contact-form/test-class.contact-form.php @@ -98,6 +98,11 @@ public function tear_down() { remove_all_filters( 'wp_mail' ); remove_all_filters( 'grunion_still_email_spam' ); remove_all_filters( 'jetpack_contact_form_is_spam' ); + + // Reset the forms array + Contact_Form::$forms = array(); + Contact_Form::$last = null; + Contact_Form::$current_form = null; } /** From 2f42b0d196c71fd7888445ca3bb8896533c67c4b Mon Sep 17 00:00:00 2001 From: Miguel Lezama Date: Mon, 13 Jan 2025 13:02:07 -0300 Subject: [PATCH 5/8] add test --- .../contact-form/test-class.contact-form.php | 36 +++++++++++++++++-- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/projects/packages/forms/tests/php/contact-form/test-class.contact-form.php b/projects/packages/forms/tests/php/contact-form/test-class.contact-form.php index 51e65b7ebbbb9..6186698128a65 100644 --- a/projects/packages/forms/tests/php/contact-form/test-class.contact-form.php +++ b/projects/packages/forms/tests/php/contact-form/test-class.contact-form.php @@ -108,11 +108,17 @@ public function tear_down() { /** * Adds the field values to the global $_POST value. * - * @param array $values Array of field key value pairs. + * @param array $values Array of form fields and values. + * @param string $form_id Optional form ID. If not provided, will use $this->post->ID. */ - private function add_field_values( $values ) { + private function add_field_values( $values, $form_id = null ) { + $prefix = $form_id ? $form_id : 'g' . $this->post->ID; foreach ( $values as $key => $val ) { - $_POST[ 'g' . $this->post->ID . '-' . $key ] = $val; + if ( strpos( $key, 'contact-form' ) === 0 || strpos( $key, 'action' ) === 0 ) { + $_POST[ $key ] = $val; + } else { + $_POST[ $prefix . '-' . $key ] = $val; + } } } @@ -2072,4 +2078,28 @@ public function test_grunion_contact_form_apply_block_attribute() { Util::grunion_contact_form_apply_block_attribute( $original, array( 'foo' => 'bar' ) ) ); } + + /** + * Tests that multiple instances of the same form work correctly with unique IDs. + */ + public function test_multiple_form_instances_with_unique_ids() { + // Create two instances of the same form + $form1 = new Contact_Form( array(), "[contact-field label='Name' type='name' required='1'/][contact-field label='Message' type='textarea' required='1'/]" ); + $form2 = new Contact_Form( array(), "[contact-field label='Name' type='name' required='1'/][contact-field label='Message' type='textarea' required='1'/]" ); + + // Verify that the forms have different IDs + $this->assertNotEquals( $form1->get_attribute( 'id' ), $form2->get_attribute( 'id' ), 'Forms should have unique IDs' ); + + // Submit first form + $this->add_field_values( + array( + 'name' => 'First form name', + 'message' => 'First form message', + ), + $form1->get_attribute( 'id' ) + ); + $result1 = $form1->process_submission(); + $this->assertTrue( is_string( $result1 ), 'First form submission should be successful' ); + // TODO: Test that the both froms submissions are saved with the correct information. + } } // end class From e1e5eb1194f77d91d5eae12155bed16297756b8d Mon Sep 17 00:00:00 2001 From: Enej Bajgoric Date: Wed, 15 Jan 2025 17:34:10 -0800 Subject: [PATCH 6/8] Update the test test_multiple_form_instances_with_unique_ids --- .../contact-form/test-class.contact-form.php | 57 +++++++++++++++---- 1 file changed, 46 insertions(+), 11 deletions(-) diff --git a/projects/packages/forms/tests/php/contact-form/test-class.contact-form.php b/projects/packages/forms/tests/php/contact-form/test-class.contact-form.php index 6186698128a65..de22b9428ee43 100644 --- a/projects/packages/forms/tests/php/contact-form/test-class.contact-form.php +++ b/projects/packages/forms/tests/php/contact-form/test-class.contact-form.php @@ -2,6 +2,8 @@ /** * Unit Tests for Automattic\Jetpack\Forms\Contact_Form. * + * To run the test visit the packages/forms directory and run composer test-php + * * @package automattic/jetpack-forms */ @@ -21,6 +23,8 @@ class WP_Test_Contact_Form extends BaseTestCase { private $post; + private $track_feedback_inserted; + private $plugin; /** @@ -113,6 +117,7 @@ public function tear_down() { */ private function add_field_values( $values, $form_id = null ) { $prefix = $form_id ? $form_id : 'g' . $this->post->ID; + $_POST = array(); foreach ( $values as $key => $val ) { if ( strpos( $key, 'contact-form' ) === 0 || strpos( $key, 'action' ) === 0 ) { $_POST[ $key ] = $val; @@ -2078,28 +2083,58 @@ public function test_grunion_contact_form_apply_block_attribute() { Util::grunion_contact_form_apply_block_attribute( $original, array( 'foo' => 'bar' ) ) ); } - + /** + * Helper function that tracks the ids of the feedbacks that got created. + */ + public function track_feedback_inserted( $post_id ) { + $this->track_feedback_inserted[] = $post_id; + } /** * Tests that multiple instances of the same form work correctly with unique IDs. */ public function test_multiple_form_instances_with_unique_ids() { - // Create two instances of the same form - $form1 = new Contact_Form( array(), "[contact-field label='Name' type='name' required='1'/][contact-field label='Message' type='textarea' required='1'/]" ); - $form2 = new Contact_Form( array(), "[contact-field label='Name' type='name' required='1'/][contact-field label='Message' type='textarea' required='1'/]" ); + global $post; - // Verify that the forms have different IDs - $this->assertNotEquals( $form1->get_attribute( 'id' ), $form2->get_attribute( 'id' ), 'Forms should have unique IDs' ); + $this->track_feedback_inserted = array(); + add_action( 'grunion_after_feedback_post_inserted', array( $this, 'track_feedback_inserted' ), 10, 1 ); - // Submit first form $this->add_field_values( array( - 'name' => 'First form name', - 'message' => 'First form message', + 'name' => 'First form name 1', + 'message' => 'First form message 1', ), - $form1->get_attribute( 'id' ) + 'g' . $post->ID ); + + $form1 = new Contact_Form( array(), "[contact-field label='Name' type='name' required='1'/][contact-field label='Message' type='textarea' required='1'/]" ); + // Submit first form $result1 = $form1->process_submission(); + $this->assertTrue( is_string( $result1 ), 'First form submission should be successful' ); - // TODO: Test that the both froms submissions are saved with the correct information. + + $this->add_field_values( + array( + 'name' => 'First form name 2', + 'message' => 'First form message 2', + ), + 'g' . $post->ID . '-2' + ); + + $form2 = new Contact_Form( array(), "[contact-field label='Name' type='name' required='1'/][contact-field label='Message' type='textarea' required='1'/]" ); + $result2 = $form2->process_submission(); + + $this->assertTrue( is_string( $result2 ), 'First form submission should be successful' ); + + // Verify that the forms have different IDs + $this->assertNotEquals( $form1->get_attribute( 'id' ), $form2->get_attribute( 'id' ), 'Forms should have unique IDs' ); + + remove_action( 'grunion_after_feedback_post_inserted', array( $this, 'track_feedback_inserted' ), 10, 1 ); + + $this->assertTrue( count( $this->track_feedback_inserted ) === 2 ); + + foreach ( $this->track_feedback_inserted as $feedback_id ) { + $feedback = get_post( $feedback_id ); + $this->assertStringContainsString( 'First form name', $feedback->post_content ); + } } } // end class From f83b7fb2de5d250b9b646e52d8329fd7fdafb461 Mon Sep 17 00:00:00 2001 From: Enej Bajgoric Date: Wed, 15 Jan 2025 17:53:02 -0800 Subject: [PATCH 7/8] Ensure 'id' exists in before trying to modify it --- .../packages/forms/src/contact-form/class-contact-form.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/projects/packages/forms/src/contact-form/class-contact-form.php b/projects/packages/forms/src/contact-form/class-contact-form.php index 8774b28063f4b..ee5da82e748b8 100644 --- a/projects/packages/forms/src/contact-form/class-contact-form.php +++ b/projects/packages/forms/src/contact-form/class-contact-form.php @@ -119,6 +119,10 @@ public function __construct( $attributes, $content = null ) { } if ( ! empty( self::$forms ) ) { + // Ensure 'id' exists in $attributes before trying to modify it + if ( ! isset( $attributes['id'] ) ) { + $attributes['id'] = ''; + } $attributes['id'] = $attributes['id'] . '-' . ( count( self::$forms ) + 1 ); } From 8f51a811b99ce0ae11cb8886f2181623fe051b29 Mon Sep 17 00:00:00 2001 From: Enej Bajgoric Date: Thu, 16 Jan 2025 11:16:39 -0800 Subject: [PATCH 8/8] Fix PHP Phan issues --- .../forms/src/contact-form/class-contact-form.php | 4 ++-- .../php/contact-form/test-class.contact-form.php | 15 ++++++++++----- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/projects/packages/forms/src/contact-form/class-contact-form.php b/projects/packages/forms/src/contact-form/class-contact-form.php index ee5da82e748b8..610fb85205c0e 100644 --- a/projects/packages/forms/src/contact-form/class-contact-form.php +++ b/projects/packages/forms/src/contact-form/class-contact-form.php @@ -43,14 +43,14 @@ class Contact_Form extends Contact_Form_Shortcode { /** * The most recent (inclusive) contact-form shortcode processed. * - * @var Contact_Form + * @var Contact_Form|null */ public static $last; /** * Form we are currently looking at. If processed, will become $last * - * @var Contact_Form + * @var Contact_Form|null */ public static $current_form; diff --git a/projects/packages/forms/tests/php/contact-form/test-class.contact-form.php b/projects/packages/forms/tests/php/contact-form/test-class.contact-form.php index de22b9428ee43..ce3d5588c45a1 100644 --- a/projects/packages/forms/tests/php/contact-form/test-class.contact-form.php +++ b/projects/packages/forms/tests/php/contact-form/test-class.contact-form.php @@ -58,7 +58,7 @@ private function set_globals() { public function set_up_test_case() { // Avoid actually trying to send any mail. add_filter( 'pre_wp_mail', '__return_true', PHP_INT_MAX ); - + $this->track_feedback_inserted = array(); $this->set_globals(); $author_id = wp_insert_user( @@ -2095,7 +2095,6 @@ public function track_feedback_inserted( $post_id ) { public function test_multiple_form_instances_with_unique_ids() { global $post; - $this->track_feedback_inserted = array(); add_action( 'grunion_after_feedback_post_inserted', array( $this, 'track_feedback_inserted' ), 10, 1 ); $this->add_field_values( @@ -2128,13 +2127,19 @@ public function test_multiple_form_instances_with_unique_ids() { // Verify that the forms have different IDs $this->assertNotEquals( $form1->get_attribute( 'id' ), $form2->get_attribute( 'id' ), 'Forms should have unique IDs' ); - remove_action( 'grunion_after_feedback_post_inserted', array( $this, 'track_feedback_inserted' ), 10, 1 ); + remove_action( 'grunion_after_feedback_post_inserted', array( $this, 'track_feedback_inserted' ), 10 ); + + $this->assertCount( 2, $this->track_feedback_inserted, 'The number of feedback forms that were inserted does not match! Expected 2.' ); - $this->assertTrue( count( $this->track_feedback_inserted ) === 2 ); + // Add assertion to ensure array is not empty + $this->assertNotEmpty( $this->track_feedback_inserted, 'No feedback forms were inserted' ); + $count = 1; foreach ( $this->track_feedback_inserted as $feedback_id ) { $feedback = get_post( $feedback_id ); - $this->assertStringContainsString( 'First form name', $feedback->post_content ); + $this->assertStringContainsString( 'First form name ' . $count, $feedback->post_content ); + $this->assertStringContainsString( 'First form message ' . $count, $feedback->post_content ); + ++$count; } } } // end class