Skip to content

fix: recursion issues with interfaces#3100

Merged
jasonbahl merged 11 commits into
wp-graphql:developfrom
jasonbahl:fix/interface-recursion
Apr 22, 2024
Merged

fix: recursion issues with interfaces#3100
jasonbahl merged 11 commits into
wp-graphql:developfrom
jasonbahl:fix/interface-recursion

Conversation

@jasonbahl
Copy link
Copy Markdown
Collaborator

@jasonbahl jasonbahl commented Apr 15, 2024

This Pull Request refactors some under-the-hood logic for how interface fields are applied to Interfaces and Object Types that implement interfaces to make things more performant.

Instead of using array_merge_recursive this instead does some diffing on the Implementing Type and the interface(s) being implemented and more efficiently applies any missing fields from the Interface to the implementing Type.

For context, below is a summary of the existing functionality this refactor impacts.

Summary of Existing Functionality this Impacts

When implementing an Interface on another Interface or ObjectType, WPGraphQL "automatically" ensures that any fields defined on the Interface(s) are added to the implementing Interface or Object Type if the implementing Type does not already have the field.

This allows for plugins to do things like:

add_action( 'graphql_register_types', function() {

	register_graphql_interface_type( 'MyTestInterface', [
		'fields' => [
			'fieldA' => [
				'type' => 'String',
				'resolve' => function() { return 'test value...'; },
			],
		]
	] );

	register_graphql_interfaces_to_types( [ 'MyTestInterface' ], [ 'Post' ] );

});

This code will apply the MyTestInterface to the Post Type, but will also ensure that the Post type has fieldA, which it previously did not.

Now I can successfully query like so:

{
  posts {
    nodes {
      id
      fieldA
    }
  }
}

CleanShot 2024-04-17 at 14 51 46

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 15, 2024

Coverage Status

coverage: 84.395% (+0.02%) from 84.373%
when pulling 2e00cd1 on jasonbahl:fix/interface-recursion
into 0de7266 on wp-graphql:develop.

@jasonbahl jasonbahl marked this pull request as ready for review April 17, 2024 20:45
Comment thread src/Type/WPObjectType.php Outdated
* @return array<string, array<string, mixed>> $fields
*/
$config['fields'] = function () use ( $config ) {
$config['fields'] = ! empty( $this->fields ) ? $this->fields : function () use ( $config ) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Identical blocks of code found in 2 locations. Consider refactoring.

Comment thread src/Type/WPInterfaceType.php Outdated
$name = ucfirst( $config['name'] );
$config['name'] = apply_filters( 'graphql_type_name', $name, $config, $this );
$config['fields'] = function () use ( $config ) {
$config['fields'] = ! empty( $this->fields ) ? $this->fields : function () use ( $config ) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Identical blocks of code found in 2 locations. Consider refactoring.

* @return array<mixed>
* @throws \Exception
*/
protected function get_fields( array $config, TypeRegistry $type_registry ): array {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function get_fields has a Cognitive Complexity of 26 (exceeds 5 allowed). Consider refactoring.

…e implementing Type does not have a description defined on the field already
* @return array<mixed>
* @throws \Exception
*/
protected function get_fields( array $config, TypeRegistry $type_registry ): array {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function get_fields has a Cognitive Complexity of 29 (exceeds 5 allowed). Consider refactoring.

* @return array<mixed>
* @throws \Exception
*/
protected function get_fields( array $config, TypeRegistry $type_registry ): array {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function get_fields has a Cognitive Complexity of 30 (exceeds 5 allowed). Consider refactoring.

* @return array<mixed>
* @throws \Exception
*/
protected function get_fields( array $config, TypeRegistry $type_registry ): array {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method get_fields has 44 lines of code (exceeds 40 allowed). Consider refactoring.

@jasonbahl jasonbahl self-assigned this Apr 17, 2024
@jasonbahl jasonbahl requested a review from josephfusco April 17, 2024 21:39
Comment thread constants.php Outdated
// Plugin version.
if ( ! defined( 'WPGRAPHQL_VERSION' ) ) {
define( 'WPGRAPHQL_VERSION', '1.23.0' );
define( 'WPGRAPHQL_VERSION', '1.24.0' );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the version be bumped in this PR going into develop?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josephfusco good catch. I made this change as I needed to test something local using version_compare() but it shouldn't have been committed in this PR. I'll back it out.

…e registered that have the same fieldName, toType and ConnectionTypeName

- add isConnectionField identifier to fields registered via WPConnectionType
- fix typo in docblock
@qlty-cloud-legacy
Copy link
Copy Markdown

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

Here's the issue category breakdown:

Category Count
Complexity 2

View more on Code Climate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants