fix: cleanup `WPInterfaceTrait` logic #3383

justlevine · 2025-05-21T21:35:45Z

What does this implement/fix? Explain your changes.

This PR refactors WPInterfaceTrait to improve the recursive handling of inherited interfaces.

The code quality and cyclomatic complexity of the class has been improved as well.

Important

~~This PR is based on #3382 which should be merged first.~~
~~Relevant diff: https://github.com/wp-graphql/wp-graphql/pull/3383/files/3eaa153b2399bfd4ca5732cecd9595093d145326..9d85d28915c2e71d58f7203cf6dd993cb507fc02~~
Rebased on develop

Does this close any currently open issues?

Possibly closes Error: Return value must be of type GraphQL\\Utils\\InterfaceImplementations #3341

Any other comments?

Replication steps:

src/Registry/TypeRegistry.php

src/Type/WPInterfaceTrait.php

jasonbahl · 2025-05-21T21:43:55Z

@justlevine thanks for the PR! I'll take a look this evening or tomorrow and share any thoughts I might have 🙏🏻

coveralls · 2025-05-21T21:57:13Z

coverage: 84.17% (+0.03%) from 84.14%
when pulling 2f84d5b on justlevine:fix/cleanup-WPInterfaceTrait-logic
into ccdffe1 on wp-graphql:develop.

jasonbahl · 2025-06-02T22:04:32Z

@justlevine this looks good. I'll see if I can get a test written for this case tomorrow 🤞🏻

…-graphql#3383

qlty-cloud-legacy · 2025-06-16T17:14:28Z

src/Type/WPInterfaceTrait.php

+	 *
+	 * @return array<\GraphQL\Type\Definition\InterfaceType>
+	 */
+	private function get_all_interfaces( InterfaceType $interface_type, TypeRegistry $registry ): array {


Function get_all_interfaces has a Cognitive Complexity of 14 (exceeds 5 allowed). Consider refactoring.

What does this do, why do we need it, how is it different than the all the other times in this (and implementing) class that'were trying to get interfaces?

jasonbahl · 2025-06-16T17:32:27Z

@justlevine I pushed up some tests that pass on this PR but fail on Master (see #3388).

I made some tweaks to the WPInterfaceTrait as well. Might be good to confirm that my changes to help support the test cases are also practical for your real-world scenarios.

Copilot

Pull Request Overview

This PR refactors the WPInterfaceTrait to improve recursive handling of inherited interfaces and field configurations. Key changes include:

Fallback to parent interfaces when none are explicitly provided.
Recursive resolution of inherited interfaces with improved validation.
More robust merging of field configurations from implemented interfaces.

Comments suppressed due to low confidence (1)

src/Type/WPInterfaceTrait.php:133

The docblock uses '@@param-out' which appears to be a typo. It should likely be corrected to '@param-out' to adhere to proper docblock conventions.

* @@param-out array<string, \GraphQL\Type\Definition\InterfaceType> $target_interfaces The array to add interfaces to (passed by reference).

Copilot · 2025-06-16T18:02:48Z

src/Type/WPInterfaceTrait.php

+		// We use array_unique as a final safeguard against duplicate entries.
+		// While we're already using interface names as array keys which generally prevents duplicates,
+		// this provides an extra layer of protection for edge cases or future modifications.
+		return array_unique( $implemented_interfaces );


Since $implemented_interfaces is built as an associative array keyed by interface names, using array_unique may be redundant. Consider removing it or using array_values if a sequentially indexed array is needed.

Suggested change

// We use array_unique as a final safeguard against duplicate entries.

// While we're already using interface names as array keys which generally prevents duplicates,

// this provides an extra layer of protection for edge cases or future modifications.

return array_unique( $implemented_interfaces );

// Since $implemented_interfaces is an associative array keyed by interface names,

// duplicates are inherently prevented. We return the values as a sequentially indexed array.

return array_values( $implemented_interfaces );

justlevine · 2025-06-16T18:03:49Z

@jasonbahl I'm having trouble making sense of the diff from mobile, will get to the computer to review shortly.

justlevine · 2025-06-16T18:44:04Z

src/Type/WPInterfaceTrait.php

 		foreach ( $fields as $field_name => $field ) {
-			$merged_field_config = $this->inherit_field_config_from_interface( $field_name, $field, $interface_fields );
+			// Get all interfaces that define this field
+			$interfaces_with_field = [];
+			foreach ( $this->getInterfaces() as $interface ) {
+				if ( ! $interface instanceof InterfaceType ) {
+					$interface = $type_registry->get_type( $interface );
+				}

-			if ( null === $merged_field_config ) {
-				unset( $fields[ $field_name ] );
-				continue;
+				if ( ! $interface instanceof InterfaceType ) {
+					continue;
+				}
+
+				$interface_fields = $interface->getFields();
+				if ( isset( $interface_fields[ $field_name ] ) ) {
+					$interfaces_with_field[] = $interface;
+				}
 			}


How is this different from what happens in ::get_fields_from_implemented_interfaces() ?

justlevine · 2025-06-16T18:45:21Z

src/Type/WPInterfaceTrait.php

+			// If this field is defined in any interfaces, merge their configs
+			if ( ! empty( $interfaces_with_field ) ) {
+				$merged_field_config = $field;
+				foreach ( $interfaces_with_field as $interface ) {
+					$interface_fields    = $interface->getFields();
+					$interface_field     = $interface_fields[ $field_name ];
+					$merged_field_config = $this->inherit_field_config_from_interface( $field_name, $merged_field_config, [ $field_name => $interface_field->config ] );
+				}
+
+				if ( null === $merged_field_config ) {
+					unset( $fields[ $field_name ] );
+					continue;
+				}
+
+				// Update the field.
+				$fields[ $field_name ] = $merged_field_config;
+			}


What does this level of nesting do?

justlevine · 2025-06-16T18:53:04Z

src/Type/WPInterfaceTrait.php

@@ -227,20 +252,65 @@ private function get_fields_from_implemented_interfaces( TypeRegistry $registry
 				continue;
 			}

-			$interface_config_fields = $interface_type->getFields();
+			// Get all interfaces in the inheritance chain
+			$all_interfaces = $this->get_all_interfaces( $interface_type, $registry );


but ::get_type() gives us the constructed type with all the resolved fields (including from the interface) because of our get_fields() this seems like weird recursion or am I missing something?

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…-graphql#3383

justlevine · 2025-06-16T18:57:58Z

(force-push was a rebase on develop to test again GF with the other regression merged)

justlevine · 2025-06-16T19:14:29Z

@jasonbahl tests are still passing with WPGraphQL for GF, so whatever changes you made seem to have no breaking effect there, but I havn't performance tested the new code.

However, even after pulling I cannot make sense of the changes you made. I left feedback how I could above since my availability is limited.

I'd also stress that the PR (before your last commit) was aimed at a regression in WPGraphQL v2.0x that has prevented dozens of teams that I'm aware of from upgrading their dev environments, and affects production environments for anything relying on WPGraphQL Smart Cache + varying extensions. If these change's relevant to addressing the regression - maybe they can happen in a separate PR (perhaps even after the patch release has been cut?)

jasonbahl · 2025-06-16T20:03:31Z

@jasonbahl tests are still passing with WPGraphQL for GF, so whatever changes you made seem to have no breaking effect there, but I havn't performance tested the new code.

However, even after pulling I cannot make sense of the changes you made. I left feedback how I could above since my availability is limited.

I'd also stress that the PR (before your last commit) was aimed at a regression in WPGraphQL v2.0x that has prevented dozens of teams that I'm aware of from upgrading their dev environments, and affects production environments for anything relying on WPGraphQL Smart Cache + varying extensions. If these change's relevant to addressing the regression - maybe they can happen in a separate PR (perhaps even after the patch release has been cut?)

@justlevine ok, I was working on adding some test cases for what I believe were some missing cases with the merging of interfaces.

The test testInterfaceFieldInheritanceAndMerging is the test that fails and that I was able to address, however I've determined that the test also fails in v1.32 and other versions, so this indeed was a pre-existing bug and not something that necessarily needs to be addressed in this PR, so I can flag that test as incomplete and revert the changes I made to WPInterfaceTrait and we can follow up on this functionality in the future if we deem that it should be considered supported functionality.

qlty-cloud-legacy · 2025-06-16T20:10:44Z

Code Climate has analyzed commit 2f84d5b and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	2

View more on Code Climate.

justlevine · 2025-06-16T20:51:56Z

That test case sounds strangely similar to #3096 🤔👀🤞

justlevine requested a review from Copilot May 21, 2025 21:36

qlty-cloud-legacy bot reviewed May 21, 2025

View reviewed changes

src/Registry/TypeRegistry.php Outdated Show resolved Hide resolved

src/Registry/TypeRegistry.php Outdated Show resolved Hide resolved

src/Type/WPInterfaceTrait.php Show resolved Hide resolved

src/Type/WPInterfaceTrait.php Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

justlevine mentioned this pull request May 21, 2025

Error: Return value must be of type GraphQL\\Utils\\InterfaceImplementations #3341

Closed

3 tasks

justlevine requested a review from colinmurphy May 21, 2025 21:43

justlevine marked this pull request as ready for review May 21, 2025 22:10

justlevine force-pushed the fix/cleanup-WPInterfaceTrait-logic branch from aed7e55 to e21f262 Compare June 2, 2025 21:39

justlevine requested a review from jasonbahl June 7, 2025 00:17

jasonbahl added a commit to justlevine/wp-graphql that referenced this pull request Jun 16, 2025

- Add tests for Interface Inheritance to cover some cases fixed in wp…

073c868

…-graphql#3383

jasonbahl added a commit to jasonbahl/wp-graphql that referenced this pull request Jun 16, 2025

- Add tests for Interface Inheritance to cover some cases fixed in wp…

70f9181

…-graphql#3383

jasonbahl mentioned this pull request Jun 16, 2025

ci: test interface inheritance #3388

Closed

qlty-cloud-legacy bot reviewed Jun 16, 2025

View reviewed changes

justlevine requested a review from Copilot June 16, 2025 18:02

Copilot AI reviewed Jun 16, 2025

View reviewed changes

justlevine commented Jun 16, 2025

View reviewed changes

justlevine and others added 4 commits June 16, 2025 21:56

fix: refactor WPInterfaceTrait to resolve interfaces recursively

ae6d801

Apply suggestions from code review

0df7a86

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix: use the correct interface name

a0da2e7

- Add tests for Interface Inheritance to cover some cases fixed in wp…

2fd29a7

…-graphql#3383

jasonbahl added 3 commits June 16, 2025 21:56

no message

eb351c7

no message

8663ebe

- phpcs

f81098b

justlevine force-pushed the fix/cleanup-WPInterfaceTrait-logic branch from 8920bbe to f81098b Compare June 16, 2025 18:56

no message

2f84d5b

jasonbahl approved these changes Jun 16, 2025

View reviewed changes

jasonbahl merged commit f2eedea into wp-graphql:develop Jun 16, 2025
35 of 36 checks passed

jasonbahl mentioned this pull request Jun 16, 2025

release: next version 📦 #3375

Merged

jasonbahl pushed a commit that referenced this pull request Jun 17, 2025

chore: update changeset for PR #3383

0267d44

fix: cleanup WPInterfaceTrait logic #3383

fix: cleanup WPInterfaceTrait logic #3383

Uh oh!

Conversation

justlevine commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

jasonbahl commented May 21, 2025

Uh oh!

coveralls commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasonbahl commented Jun 2, 2025

Uh oh!

qlty-cloud-legacy bot Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

justlevine Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

jasonbahl commented Jun 16, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

justlevine commented Jun 16, 2025

Uh oh!

justlevine Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

justlevine Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

justlevine Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

justlevine commented Jun 16, 2025

Uh oh!

justlevine commented Jun 16, 2025

Uh oh!

jasonbahl commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qlty-cloud-legacy bot commented Jun 16, 2025

Uh oh!

Uh oh!

justlevine commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

fix: cleanup `WPInterfaceTrait` logic #3383

fix: cleanup `WPInterfaceTrait` logic #3383

justlevine commented May 21, 2025 •

edited

Loading

coveralls commented May 21, 2025 •

edited

Loading

jasonbahl commented Jun 16, 2025 •

edited

Loading

justlevine commented Jun 16, 2025 •

edited

Loading