Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug/when US phone is not included as a merge field in a Mailchimp form it causes fatal error on submission #108

Conversation

MaxwellGarceau
Copy link
Collaborator

@MaxwellGarceau MaxwellGarceau commented Jan 3, 2025

Description of the Change

Fixed a bug causing a fatal PHP error when:

  • Phone number is set to US style in the Mailchimp account
  • Phone number is NOT included as a merge field in the WP plugin
us-phone-fatal-error-when-merge-field-not-included.mov

Closes #103

How to test the Change

The video us-phone-fatal-error-when-merge-field-not-included.mov above can be used as a guide for recreating the testing conditions. However, instead of a fatal error you should see a successful form submission.

Phone number

  1. Set the phone format in the test Mailchimp account (/lists/settings/merge-tags) to US style
  2. Select the "Update List" button to refresh the Mailchimp data with WP
  3. Ensure that Phone Number is not an included merge field. The error will only occur when Phone Number is not included in the form.
  4. Submit a form submission using only email (any merge field is okay and the error should occur regardless)
  5. The form submission should submit successfully

Address

I added a conditional check to ensure address fields were included for submission.

With address
  1. Include address in the Mailchimp admin
  2. Fill out and submit a form
  3. The form submission should be successful
Without address
  1. Ensure that address is NOT included in the Mailchimp admin
  2. Fill out and submit a form
  3. The form submission should be successful

Changelog Entry

Fixed - Fatal PHP error that would occur when the phone merge field was set to US format, but the merge field was not included in the Mailchimp plugin.

Credits

Props @MaxwellGarceau

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change. Small change, not worth writing tests
  • All new and existing tests pass.

@github-actions github-actions bot added this to the 1.7.0 milestone Jan 3, 2025
@MaxwellGarceau MaxwellGarceau marked this pull request as ready for review January 3, 2025 00:25
@github-actions github-actions bot added the needs:code-review This requires code review. label Jan 3, 2025
@MaxwellGarceau MaxwellGarceau requested a review from dkotter January 3, 2025 00:25
@MaxwellGarceau MaxwellGarceau changed the title Bug/us phone not included in mailchimp form causes fatal error on submission Bugfix: When US phone is not included as a merge field in a Mailchimp form it causes fatal error on submission Jan 3, 2025
@MaxwellGarceau MaxwellGarceau changed the title Bugfix: When US phone is not included as a merge field in a Mailchimp form it causes fatal error on submission Bug/when US phone is not included as a merge field in a Mailchimp form it causes fatal error on submission Jan 3, 2025
@qasumitbagthariya
Copy link
Collaborator

QA Update ✅


I have verified this PR in the bug/us-phone-not-included-in-mailchimp-form-causes-fatal-error-on-submission branch which has been fixed and is functioning as intended.

Screen.Recording.2025-01-16.at.7.20.45.PM.mov

Testing Environment

  • WordPress: 6.7.1
  • Theme: Twenty Twenty-Four 1.3
  • WooCommerce - 9.5.1
  • PHP: 8.0.30
  • Web Server: Nginx 1.20.2
  • Browser: Chrome
  • OS: macOS 15.2
  • Branch: bug/us-phone-not-included-in-mailchimp-form-causes-fatal-error-on-submission

Steps to Test- As mentioned in the PR description.
Test Results - It is working as expected.
Functional Demo / Screencast -
Special Notes - Ready for code review (Woo)
Testing Document status:
Cases related to this Issue/PR are added to the Critical Flow Wiki pages:

  • Yes
  • Not Required/Applicable for this PR

@qasumitbagthariya
Copy link
Collaborator

Regression / Smoke Test Report ✅

Tested with the smoke-testing branch, it works as expected, similar to the fix-specific branch.

Testing Environment

  • WordPress: 6.7.1
  • Theme: Twenty Twenty-Four 1.3
  • WooCommerce - 9.5.1
  • PHP: 8.0.30
  • Web Server: Nginx 1.20.2
  • Browser: Chrome
  • OS: macOS 15.2
  • Branch: smoke-testing

@vikrampm1 vikrampm1 modified the milestones: 1.7.0, 1.6.3 Jan 28, 2025
@vikrampm1 vikrampm1 merged commit 478adea into develop Jan 28, 2025
18 checks passed
@vikrampm1 vikrampm1 mentioned this pull request Jan 28, 2025
27 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review.
Projects
None yet
5 participants