Skip to content

Conversation

@jasonbahl
Copy link
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

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
* @return array<string, array<string, mixed>> $fields
*/
$config['fields'] = function () use ( $config ) {
$config['fields'] = ! empty( $this->fields ) ? $this->fields : function () use ( $config ) {

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.

$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 ) {

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 {

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 {

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 {

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 {

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
constants.php Outdated
// Plugin version.
if ( ! defined( 'WPGRAPHQL_VERSION' ) ) {
define( 'WPGRAPHQL_VERSION', '1.23.0' );
define( 'WPGRAPHQL_VERSION', '1.24.0' );
Copy link
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
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

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