fix: Allow compatible interface field override with register_graphql_field()#3539
Conversation
Adds comprehensive test coverage for issue #3096: - Test that compatible interface field overrides are allowed - Test that incompatible overrides still error - Test that non-interface duplicates still error (preserves existing behavior) - Test that type modifiers (non_null, list_of) work correctly These tests will initially fail until the fix is implemented.
…field() Implements compatibility check in TypeRegistry::register_field() to allow overriding interface fields with compatible object types that implement the interface. When a duplicate field is detected: - If existing field is callable (from interface) and new field is string/array (object type), check if new type implements the interface - If compatible, allow the override; otherwise throw DUPLICATE_FIELD error - Uses static flag to prevent infinite recursion during type resolution Fixes #3096
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3539 +/- ##
========================================
Coverage 83.4% 83.4%
- Complexity 5217 5282 +65
========================================
Files 286 286
Lines 22601 22745 +144
========================================
+ Hits 18854 18980 +126
- Misses 3747 3765 +18
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Fixes PHPCS PSR2.Files.EndFileNewline.NoneFound error
… guard Replace static variable with class property for checking_compatibility flag to improve consistency with codebase style and testability.
|
@justlevine more changes coming. Hold off on review. |
Add tests that verify interface field overrides work correctly when overriding with the same type (e.g., Post.field with type Post). These tests will fail without the fix, demonstrating the bug. Tests cover: - Dovid's exact scenario (both register_graphql_field and filter) - Multiple interfaces - Page type (not just Post) - Custom post types - Filter-only approach Related to #3540
Fix infinite recursion and DUPLICATE_FIELD errors when overriding an interface field on the same type (e.g., Post.field with type Post). The fix: - Detects when an existing field type is an interface - Checks if the new type implements that interface - Allows the override when types are compatible - Handles both callable and string field types - Prevents recursion by checking if types are loaded before resolving - Uses a class property flag to prevent infinite recursion during compatibility checks This fixes the issue where narrowing an interface field to the same type that's currently being loaded would cause infinite recursion. Fixes #3540
|
@justlevine ok, I reworked the tests and had to rework the fix as well |
- Fix variable alignment issues - Fix PHPStan null coalescing issue by using explicit empty check - Fix else/if structure to use elseif - Fix indentation issues
…test coverage
- Fix interface field argument merging across inheritance chains
- Update get_fields_from_implemented_interfaces() to merge arguments
from multiple interfaces instead of overwriting
- Ensures arguments from parent interfaces are preserved when
child interfaces define the same field
- Add comprehensive test coverage for register_graphql_field
- Test connection field duplicate handling scenarios
- Test field name formatting with allowFieldUnderscores
- Test interface field override edge cases
- Test field registration with array type modifiers
- Test various field type configurations
- Unskip and fix testInterfaceFieldInheritanceAndMerging
- Test now verifies that arguments from parent interfaces,
child interfaces, and object types are properly merged
- Add comprehensive interface documentation
- Document interface field inheritance
- Document interface field argument merging feature
- Include examples and best practices
- Cover type narrowing and resolver overrides
- Fix interface field argument merging across inheritance chains - Add PHPStan suppressions for type compatibility in merge_field_args - Fix Settings type registration to always register (even with empty fields) - Add placeholder _empty field with TODO for issue #2459 - Fix undefined $schema variables in test methods - Fix testSchemaIsValid test by ensuring Settings type is always valid - All wpunit tests passing (1002 tests, 6538 assertions)
- Add test for string type existing field (covers string type check path) - Add test for different type when loaded (covers loaded type check path) - Add test for error handling during type resolution - Update code comment to explain when string type check path is executed - All tests passing, improving code coverage for TypeRegistry
|
@justlevine I updated the tests to test your specific case. Let me know when you have a chance to test it further. 🙏 |
There was a problem hiding this comment.
Pull request overview
This PR updates WPGraphQL’s type/field registration behavior to better support narrowing interface-returned fields via register_graphql_field() (avoiding false DUPLICATE_FIELD errors), and expands tests/docs around interface behavior.
Changes:
- Add compatibility/recursion-handling logic to
TypeRegistry::register_field()to permit compatible overrides of interface-derived fields. - Update interface field inheritance behavior and add extensive wpunit coverage for interface overrides, connection duplicates, and field-name/type-modifier scenarios.
- Expand interface documentation and adjust schema/type initialization behavior in a few tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/wp-graphql/src/Registry/TypeRegistry.php | Adds compatibility checks to allow overriding interface fields when types are compatible and attempts to avoid recursion. |
| plugins/wp-graphql/src/Type/WPInterfaceTrait.php | Changes how fields from multiple implemented interfaces are combined (merging args instead of overwriting). |
| plugins/wp-graphql/src/Type/ObjectType/Settings.php | Ensures the Settings type always registers by injecting a placeholder field when empty. |
| plugins/wp-graphql/tests/wpunit/TypesTest.php | Adds many new wpunit tests covering override compatibility, recursion, type modifiers, connection duplicates, etc. |
| plugins/wp-graphql/tests/wpunit/InterfaceTest.php | Reformats/adjusts tests and un-skips (and expands) a test for interface arg merging. |
| plugins/wp-graphql/docs/interfaces.md | Replaces placeholder page with comprehensive interface documentation and examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Sadly this isnt working for me downstream.
First thing to note, is the schema docs still thinks the field type is from the interface ( When I try to access fields specific to downstream, you can see that validation fails, assumedly because the Duplicate Field check is preventing the application of the narrowed field.
You can see the relevant change in this commit: AxeWP/wp-graphql-gravity-forms@a4fd6bc Hoping it's just because the function doesn't account for list_of/non_null, but it's very noisy and hard for me to tell. To test on that branch (so you don't need to manually test gf. |
…w-compatible-interface-field-override
…w-compatible-interface-field-override
…interface-field-override
Refactor duplicate-field compatibility checks to only allow verified interface-compatible overrides, including list/non-null and lazy-loaded type scenarios. Add downstream-inspired regression coverage and conditionally expose allSettings only when settings fields exist to keep schema behavior correct without placeholder fields.
Expose the monorepo phpcs directory inside wp-env so plugin lint scripts can resolve shared WPGraphQL sniffs in containerized runs.
Add regression tests for unregistered and non-object override types so duplicate-field compatibility guards remain covered and future refactors don't reopen this behavior.
Add regression tests for loaded and loader-backed interface name resolution paths plus guard behavior for non-interface and empty type-name string scenarios.
…override
When the compat check resolves the override's target type via get_type, the
graphql_get_type filter runs InstrumentSchema which re-enters the
graphql_{Type}_fields filter chain while the outer compat call is still
in-flight. Each callback in the re-entered chain then hit the
checking_compatibility recursion guard, returned false, and logged a
spurious DUPLICATE_FIELD entry even though the outer pass had already
accepted the override. Guard the duplicate-field branch with a fast no-op
when checking_compatibility is set so the outer decision is preserved
without debug noise.
Also broaden the override compatibility check to a second path: when the
existing field's type is a concrete/scalar but the field itself came from
an implemented interface, look up the interface's declared field type via
the implementing ObjectType and accept the override if the wrapping
structure matches and the override's base type is either identical or a
concrete implementation of the interface's base type. This covers
same-type resolver overrides (e.g. Post.related: Post inherited from a
HasSelfRef interface) without loosening protection for unrelated duplicate
registrations.
The register_field filter callback now receives the WPObjectType instance
(filter accepted-args bumped to 2) so the inherited-field lookup can read
implemented interfaces without round-tripping through get_type.
Squiz.Commenting.FunctionComment.SpacingAfterParamType requires the variable column to align based on the longest parameter type. The docblocks for is_compatible_interface_field_override and is_override_compatible_with_interface_field_type added in the previous commit had a few extra padding spaces, tripping phpcs in CI.
Adds 4 wpunit tests exercising the inherited-field (Path 2) compatibility branches that were uncovered by patch coverage: - testCompatibleInterfaceFieldOverrideWithListOfScalar: drives the list_of branch through unwrap_field_type_config and unwrap_field_type_instance. - testCompatibleInterfaceFieldOverrideWithNonNullScalar: drives the non_null branch through both unwrap helpers. - testIncompatibleInterfaceFieldOverrideWithMismatchedWrappersStillErrors: covers wrapper-mismatch rejection (interface declares list_of String, override declares non_null String). - testCompatibleInterfaceFieldOverrideWhereInheritedFieldTypeIsInterface: covers the inherited-interface-typed-field path through resolve_interface_name_from_type_name + type_implements_interface.
…fety Adds four short subsections to interfaces.md so extension authors know exactly what overrides are accepted by the new compatibility check: - "Compatible Override Rules": wrappers must match exactly; base type must equal the interface's base or implement it. - "Narrowing Wrapped Interface Field Types": list_of/non_null example mirroring the downstream use case from the PR (and the new wpunit coverage). - "Same-Type Overrides": notes the recursion-safe handling that fixes issue #3540, with a Post.related example. - "Behavioral Change Note": flags that previously permissive duplicate overrides may now consistently emit DUPLICATE_FIELD when not verifiably compatible, framed as a correctness fix.


Fixes #3096
Problem
When using
register_graphql_field(), WPGraphQL could throwDUPLICATE_FIELDeven when an override was compatible with an inherited interface field.Example: if an interface defines
seo: SeoInterface, overridingPost.seotoPostSeo implements SeoInterfaceshould be valid, but duplicate checks previously happened before compatibility could be confirmed.Solution
TypeRegistry::register_field()now performs an explicit compatibility check for duplicate overrides that appear to come from interface inheritance:list_of/non_null)DUPLICATE_FIELDThis includes recursion-safe handling and registered-but-lazy-loaded type scenarios.
Correctness Fix Note (Behavioral Change)
This PR intentionally tightens duplicate-field override validation.
Downstream plugins that accidentally relied on those permissive edge cases may now see
DUPLICATE_FIELDwhere they previously did not.We are documenting this as a correctness fix, not a formal breaking change, because it aligns behavior with valid schema/interface rules and preserves intended extension behavior for compatible overrides.
Additional Schema Safety Adjustment
To avoid exposing an invalid root field in environments with no exposed settings:
Settingstype is only registered when real settings fields existRootQuery.allSettingsis conditionally exposed only when settings fields are availableThis avoids synthetic placeholder fields while keeping schema validity.
Tests Added / Updated
Interface override compatibility
testIncompatibleInterfaceFieldOverrideStillErrorstestCompatibleInterfaceFieldOverrideWithWrappedListInterfaceTypetestCompatibleInterfaceFieldOverrideWithListOfInterface(based on downstream real-world plugin scenario)Interface field inheritance cleanup
Settings schema behavior
testAllSettingsFieldIsHiddenWhenNoSettingsAreExposedValidation
tests/wpunit/TypesTest.phptests/wpunit/InterfaceTest.phptests/wpunit/SettingsQueriesTest.phpcomposer run phpstan -- --memory-limit=2G) for@wpgraphql/wp-graphqlAll passing locally in wp-env.
Impact
Related
register_graphql_field()#3096