Update phone validation regex to accept only numbers and correct amount - Fix "must consist of only numbers" validation error message to be specific #112
Conversation
|
@dkotter I think this ticket would be a good candidate for some PHP unit tests around the address validation. The changes are small, the only WP dependency is WP_Error, and some easy to run tests would help me feel better that we're covering the right validation conditions. What do you think? Assuming that you think it's worth adding the tests, I wanted to ask if 10up had a preferred testing library or "10up way" of unit testing? There's currently no PHP tests in this project. My preference would be to use Pest with WP_Mock or Brain Monkey, but I'm open to anything. |
github-actions
bot
added
the
needs:feedback
|
@MaxwellGarceau thanks for the PR! Could you please fill out the PR template with description, changelog, and credits information so that we can properly review and merge this? |
github-actions
bot
added
needs:code-review
I'm fine with spending the time to add unit tests here. As far as a preferred way, we don't have a well documented approach, though we typically use PHPUnit (as this is what WordPress core itself uses) and either use WP_Mock or use the testing framework that WordPress provides, for more complicated setups. I think PHPUnit + WP_Mock would probably work fine here. Can see how we do things on Safe SVG for one example. |
Arrow functions not caught by PHPCSTLDR:
Verifying the problem@dkotter while updating the arrow function I noticed that the current PHPCS compatibility check doesn't catch arrow functions. The specific CLI command I was running was I also tried PHP 5.0 as the test version to ensure the XML file was working and the linter picked up on several incompatibilities. Solution attemptsI tried to explicitly add the arrow function rule, tried other rulesets (PHPCompatibility), and tried adding a custom sniff to register the PRs in PHPCompatibility adding arrow function supportI looked through some of the merged PRs in the PHPCompatibility codebase and it seems like arrow functions should trigger on the
Workaround: RectorI did a quick test with rector and rector catches arrow functions. An alternative would be using rector in Not the best solution and kind of a hack, but may be nice to have for easy PHP upgrades in the future as well. 
Bug report for PHPCompatibilityDo you think this is worth making a bug report in the PHPCompatibility codebase? It's definitely possible I'm just missing something obvious. |
I'm pretty sure the issue here is the PHPCompat repo hasn't pushed out a new release since December 2019, so all new sniffs added since then don't actually run if you're not using a development version of the tool (which is unfortunate). The easiest way to address this (which I should have done when I was setting up PHPCS on this repo) is to install the dev version of that tool. Can see instructions on how to do that here: 10up/phpcs-composer#52. Basically just need to run: |
This enables us to just import the merge_validate_phone instead of loading the entire Mailchimp plugin
Following 10up namespace standards with functions
Give us the flexibility to set variables during testing and allows us to test that we are setting locations correctly
No reason to test long phone numbers because that is not a possible scenario a user can enter
There was a problem hiding this comment.
@dkotter I've added PHPUnit, WP_Mock, and created the tests for the validation logic. I moved a few functions out of mailchimp.php into their own namespaced files and turned them into classes for easier testing.
I highlighted the most import parts. It's kind of a large PR and I'm happy to make any changes you think would be beneficial.
| */ | ||
| public function __construct( $wp_error_factory = null ) { | ||
| // Default to return WP_Error objects | ||
| $this->wp_error_factory = $wp_error_factory ?? function ( string $code, string $message, $data = null ) { |
There was a problem hiding this comment.
I added a wp_error_factory in order to pass mocked WP_Error messages in the tests.
It's kind of ugly, but this is the best way I had to make assertions that the WP_Error objects returned from the validation functions had exactly the error code and message that was expected.
If you have any suggestions I'd love to take the opportunity to learn how to better handle mocking assertions on WP instantiated objects directly in the code.
includes/validation/functions.php
Outdated
| @@ -0,0 +1,41 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Kept the original functions here for backwards compatibility. The idea is work towards more class based modularization for easier unit testing.
After the composer autoload changes go into develop it would be great to get a DI container involved to avoid direct class instantiation in the function.
| $wp_error_factory = function ( $code, $message, $data = null ) use ( $wp_error ) { | ||
| // Step 3: Make assertions against the properties we want the WP_Error object to contain | ||
| // and dynamically fill them to return what our validate functions will pass in. | ||
| $wp_error | ||
| ->shouldReceive('get_error_code') | ||
| ->andReturn($code); | ||
| $wp_error | ||
| ->shouldReceive('get_error_message') | ||
| ->with($code) | ||
| ->andReturn($message); | ||
| return $wp_error; | ||
| }; |
There was a problem hiding this comment.
We're dynamically filling the WP_Error assertions here with what we actually get back from the validate functions.
| // Error code | ||
| $this->assertEquals(Validate_Merge_Fields::PHONE_VALIDATION_ERROR_CODE, $result->get_error_code()); | ||
|
|
||
| // Error message | ||
| $this->assertMatchesRegularExpression( | ||
| '/must consist of only numbers/', | ||
| $result->get_error_message(Validate_Merge_Fields::PHONE_VALIDATION_ERROR_CODE) | ||
| ); |
There was a problem hiding this comment.
Here we're testing the dynamically filled WP_Error mock with what we really should be getting back from the validation functions.
| * Strategy: | ||
| * - Unit testing only (no integration or end-to-end). | ||
| * - Mock WP core functions with WP_Mock (WP core is not loaded). | ||
| * - Promote long-term maintainability through modularization. | ||
| * | ||
| * Reasoning: | ||
| * This plugin is not modular and initializing the plugin | ||
| * will require mocking all of the WP core functions | ||
| * used in order to avoid fatal errors on loading. | ||
| * | ||
| * Instead, we'll require functionality as it's needed | ||
| * by first: | ||
| * | ||
| * 1) Modularizing the functionality into a namespaced file and then | ||
| * 2) Importing it directly into the test. | ||
| * | ||
| * End to end tests are handled in Cypress. Integration | ||
| * and E2E tests should be handled in Cypress. |
There was a problem hiding this comment.
I wrote a little strategy here for long-term app health that could be moved to over time. What do you think?
Ahh! Thank you that fixed it!! I saw the dev require in the 10up/phpcs-composer composer.json file and just assumed we were already using dev-develop. This saved me so much time and an embarrassing bug report 😂 |
The form must return null when a field is not filled out. Other values will cause a validation failure
10 numbers and 2 hyphens
Expand validation test cases
QA Update: ✅I have verified this PR in the
Accepts only numeric characters (0–9). ✅ Successfully accepts valid US-format phone numbers. ✅ Screen.Recording.2025-01-18.at.8.14.43.PM.movTesting Environment
|
github-actions
bot
added
the
needs:refresh
|
@MaxwellGarceau thanks for the PR! Could you please rebase your PR on top of the latest changes in the base branch? |
Description of the Change
Update phone validation to fix several bugs and give detailed error messages.
Issue #101 - Update phone validation regex to accept only numbers and correct amount
Issue #104 - Update phone validation regex to accept only numbers and correct amount
Closes #101
Closes #104
Both these issues required work in the same function that would have created ugly merge conflicts.
How to test the Change
Setup
General
Phone number should not accept letters
Phone number should not accept letters - Bug is fixed
Test a valid phone number
Test that other invalid characters are not accepted
The phone number field should only accept numbers. The error message should notify the user that the phone number must only contain numbers.
• Letters (A–Z, a–z).
• Special characters like @, #, $, %, *, &, !, etc.
• Whitespace, both at the ends and also in the middle.
• Multiple or misplaced delimiters, such as --, (123)), or leading/trailing symbols.
• Any character outside the ASCII range (e.g., emojis, non-standard symbols). - too broad to really test comprehensively
Phone number does not contain enough digits
Whitespace testing
All whitespace should be stripped and should return an error notifying the user that the phone number does not have enough digits.
1 3-12-234should fail validation with an error messaging stating that there are not enough digitsChangelog Entry
Credits
Props @username, @username2, ...
Checklist: