-
-
Notifications
You must be signed in to change notification settings - Fork 30
feat: Add phone format properties to resolve issue #441 #475
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
base: develop
Are you sure you want to change the base?
Conversation
- Create new PhoneFormatProperties object type with label, mask, regex, instruction, and type fields - Add phoneFormatProperties field to FieldWithPhoneFormat interface with resolver - Support custom phone formats via gform_phone_formats filter - Update tests to include new phoneFormatProperties field - Register PhoneFormatProperties type in TypeRegistry Resolves: AxeWP#441
- Fix PHPCS violations in FieldWithPhoneFormat.php - Add proper inline comment punctuation - Add phpcs:ignore for third-party gform_phone_formats hook - Add changelog entry for phoneFormatProperties feature
|
Thanks so much for submitting this @chetanupare 🙇 I should hopefully have time over the weekend to review. (Feel free to ignore the copilot comments I just triggered - 80% of the time they're slop, but every once in a while they catch something helpful) |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a phoneFormatProperties field to phone fields in the GraphQL schema to expose detailed phone format configuration (label, mask, regex, instruction, and type) from Gravity Forms. This addresses issue #441 where users needed access to format properties beyond just the format name for frontend implementation.
Changes:
- Added new
PhoneFormatPropertiesGraphQL object type to represent phone format configuration details - Extended
FieldWithPhoneFormatinterface withphoneFormatPropertiesfield including resolver that uses thegform_phone_formatsfilter - Updated tests to query the new field and added test expectations
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Type/WPObject/PhoneFormatProperties.php | New GraphQL object type defining the structure for phone format properties |
| src/Type/WPInterface/FieldSetting/FieldWithPhoneFormat.php | Added phoneFormatProperties field with resolver that applies gform_phone_formats filter |
| src/Registry/TypeRegistry.php | Registered the new PhoneFormatProperties type in the type registry |
| tests/wpunit/PhoneFieldTest.php | Updated test query to include phoneFormatProperties field |
| tests/_support/Helper/GFHelpers/ExpectedFormFields.php | Added test expectations for phoneFormatProperties field |
| CHANGELOG.md | Documented the new feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'international' => [ | ||
| 'label' => 'International', | ||
| 'mask' => false, | ||
| 'regex' => false, | ||
| 'instruction' => __( 'International phone numbers must start with a + followed by the country code and phone number.', 'wp-graphql-gravity-forms' ), | ||
| 'type' => 'international', | ||
| ], |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'international' format has 'mask' and 'regex' set to boolean false, but the PhoneFormatProperties type defines these fields as String (non-nullable). This creates a type mismatch that will cause GraphQL validation errors when international format is used. Consider converting false values to null in the resolver or updating the PhoneFormatProperties type to make these fields nullable (String instead of String!).
|
|
||
| // Add phoneFormatProperties field | ||
| if ( ! empty( $field->phoneFormat ) ) { | ||
| $properties[] = $this->expectedField( 'phoneFormatProperties.label', self::NOT_NULL ); | ||
| $properties[] = $this->expectedField( 'phoneFormatProperties.mask', self::NOT_NULL ); | ||
| $properties[] = $this->expectedField( 'phoneFormatProperties.regex', self::NOT_NULL ); |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test expectations assume all fields (mask, regex, label, instruction, type) will be NOT_NULL when phoneFormat is set. However, for the 'international' format, mask and regex are false (boolean). This test will fail when testing with international phone format. The test expectations should handle the case where mask and regex can be null/false, or the resolver should be fixed to convert false to null.
| // Add phoneFormatProperties field | |
| if ( ! empty( $field->phoneFormat ) ) { | |
| $properties[] = $this->expectedField( 'phoneFormatProperties.label', self::NOT_NULL ); | |
| $properties[] = $this->expectedField( 'phoneFormatProperties.mask', self::NOT_NULL ); | |
| $properties[] = $this->expectedField( 'phoneFormatProperties.regex', self::NOT_NULL ); | |
| // Add phoneFormatProperties field | |
| if ( ! empty( $field->phoneFormat ) ) { | |
| $properties[] = $this->expectedField( 'phoneFormatProperties.label', self::NOT_NULL ); | |
| // For the 'international' phone format, mask and regex can be null/false. | |
| if ( 'international' === $field->phoneFormat ) { | |
| $properties[] = $this->expectedField( 'phoneFormatProperties.mask', self::IS_NULL ); | |
| $properties[] = $this->expectedField( 'phoneFormatProperties.regex', self::IS_NULL ); | |
| } else { | |
| $properties[] = $this->expectedField( 'phoneFormatProperties.mask', self::NOT_NULL ); | |
| $properties[] = $this->expectedField( 'phoneFormatProperties.regex', self::NOT_NULL ); | |
| } |
| // Return the properties for the selected format. | ||
| return $phone_formats[ $field->phoneFormat ] ?? null; |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The resolver returns the raw array from the gform_phone_formats filter without validation. Custom phone formats added via this filter may not include all required fields (label, mask, regex, instruction, type) or may use inconsistent types (e.g., false instead of null). Consider adding validation to ensure the returned data structure is consistent and compatible with the PhoneFormatProperties type, or add resolvers for individual fields to handle type conversion.
| // Return the properties for the selected format. | |
| return $phone_formats[ $field->phoneFormat ] ?? null; | |
| // Get the properties for the selected format. | |
| $format = $phone_formats[ $field->phoneFormat ] ?? null; | |
| if ( ! is_array( $format ) ) { | |
| return null; | |
| } | |
| // Normalize the format to ensure all required keys exist and values are of compatible types. | |
| $required_keys = [ | |
| 'label' => null, | |
| 'mask' => null, | |
| 'regex' => null, | |
| 'instruction' => null, | |
| 'type' => null, | |
| ]; | |
| $normalized = []; | |
| foreach ( $required_keys as $key => $default ) { | |
| $value = $format[ $key ] ?? $default; | |
| // Convert explicit false values to null to align with nullable GraphQL fields. | |
| if ( $value === false ) { | |
| $value = null; | |
| } | |
| // Ensure the value is a scalar; otherwise, fall back to the default (null). | |
| if ( ! is_scalar( $value ) && null !== $value ) { | |
| $value = $default; | |
| } | |
| $normalized[ $key ] = $value; | |
| } | |
| return $normalized; |
| phoneFormatProperties { | ||
| label | ||
| mask | ||
| regex | ||
| instruction | ||
| type | ||
| } |
Copilot
AI
Jan 29, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests only use the 'standard' phone format by default (see PropertyHelper.php line 360). There is no test coverage for the 'international' format case where mask and regex are false. Consider adding a test case that specifically tests the international phone format to ensure the phoneFormatProperties field works correctly with null/false values.
- Add dedicated PhoneFieldInternationalFormatTest - Normalize phone format properties for null/false values - Adjust phone format expectations for international mask/regex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test: Added international phone format coverage
- Added dedicated PhoneFieldInternationalFormatTest
- Normalized phone format properties for null/false values
- Adjusted phone format expectations for international mask/regex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: Used callable descriptions for phone format properties
Resolves: #441
What
Adds phoneFormatProperties field to phone fields returning label, mask, regex, instruction, and type.
Why
Addresses issue #441 - users need format details beyond just the format name for frontend implementation.
How
Created PhoneFormatProperties object type
Added field to FieldWithPhoneFormat interface with resolver
Supports custom formats via gform_phone_formats filter
Updated tests and registered new type
Testing Instructions
Additional Info
Checklist: