Skip to content
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

ResolveInfo::getFieldSelectionWithAliases() => now add instance types and the folded union types to the returned schema. #1681

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zephyx
Copy link
Contributor

@zephyx zephyx commented Mar 17, 2025

Hi,

After using the ResolveInfo::getFieldSelectionWithAliases() method I needed some additionnal data in order to go through the all schema properly.
So I have updated the method to add:

  • Type instance for each alias level (not set at field level in order to keep the schema simple and avoid breaking changes)
  • Union type management: This allows us to properly go through unions types and retrieve all their respective subfields/args

Before this update, we were able to go through union types but it was unreliable, some fields with the same name in different unioned types could be substituted for others.
Now it's consistent and it allows me in my application to handle a proper infinite level eager loading across the entire schema with unions.

Copy link
Collaborator

@spawnia spawnia left a comment

Choose a reason for hiding this comment

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

Please fix the PHPStan issues, then mark as ready.

* }
* }
*
* Given this ResolveInfo instance is a part of "root" field resolution, and $depth === 1,
* Given this ResolveInfo instance is a part of "root" field resolution, $depth === 1, and nested represent an ObjectType with a configured name "Nested",
Copy link
Collaborator

Choose a reason for hiding this comment

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

What exactly is meant by nested here? A field?

Copy link
Contributor Author

@zephyx zephyx Mar 18, 2025

Choose a reason for hiding this comment

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

Yes, I mean that the field described on line 233 represents an ObjectType with a property 'name' (or the result of the ->name() method) = "Nested".
I wrote this in order to emphasize the link between this field and the first ObjectType field in "myUnion" which contains the same field type.
It's probably not clear enough without the ObjectType + UnionType class config descriptions :/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Given this ResolveInfo instance is a part of "root" field resolution, $depth === 1, and nested represent an ObjectType with a configured name "Nested",
* Given this ResolveInfo instance is a part of root field resolution,
* $depth === 1,
* and fields "nested" represents an ObjectType named "Nested",

* 'nested1' => [
* 'args' => [
* 'myArg' => 2,
* 'mySecondAg' => "test,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks broken

* 'myUnion' => [
* 'myUnion' => [
* 'args' => [
* 'myArg' => 3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Always add trailing commas

Comment on lines 305 to 340
* @throws \Exception
* @return array<string, mixed>
*
* @throws Error
* @throws InvariantViolation
*
* @return array<string, mixed>
*
* @throws \Exception
Copy link
Collaborator

Choose a reason for hiding this comment

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

I liked the previous order better

Comment on lines 413 to 445
assert($parentType instanceof HasFieldsType, 'ensured by query validation');

$fieldDef = $parentType->getField($fieldName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing the assert causes PHPStan failures.

$result10 = GraphQL::executeQuery(
new Schema(['query' => $query]),
<<<GRAPHQL
query {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
query {
{

Comment on lines 11 to 17
$config = [
'fields' => [
'customA' => new MyCustomType(),
'customB' => new OtherCustom(),
],
];
parent::__construct($config);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$config = [
'fields' => [
'customA' => new MyCustomType(),
'customB' => new OtherCustom(),
],
];
parent::__construct($config);
parent::__construct([
'fields' => [
'customA' => new MyCustomType(),
'customB' => new OtherCustom(),
],
]);

@spawnia spawnia marked this pull request as draft March 18, 2025 09:52
@zephyx
Copy link
Contributor Author

zephyx commented Mar 18, 2025

Hi, thank you for the review.

How should I fix this PHPStan error ?

Cannot access offset 'customA' on (callable(): iterable)|iterable.

On tests/Type/ResolveInfoTest.php

Line 741

Something like this ?

assert(is_array($myCustomWithObjectType->config['fields']), 'ensured by type config');

@spawnia
Copy link
Collaborator

spawnia commented Mar 19, 2025

Hi, thank you for the review.

How should I fix this PHPStan error ?

Cannot access offset 'customA' on (callable(): iterable)|iterable.

On tests/Type/ResolveInfoTest.php

Line 741

Something like this ?

assert(is_array($myCustomWithObjectType->config['fields']), 'ensured by type config');

I would do something like this:

$fields = $myCustomWithObjectType->config['fields'];
assert(is_array($fields), 'ensured by type config');

$myCustomType = $fields['customA'];

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

Successfully merging this pull request may close these issues.

2 participants