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

Add method getFieldSelectionWithAliases to class ResolveInfo #1648

Merged
merged 10 commits into from
Dec 4, 2024

Conversation

zephyx
Copy link
Contributor

@zephyx zephyx commented Nov 29, 2024

@see #1072

That was the simplest way to solve this issue with minimal breaking change due to the presence of $this->arrayMergeDeep(...).

If you have a better idea, no problem, I just needed this feature in my application.

NOTE: composer fix / check have updated several files but I have not modified them, so they are not included here.

@zephyx
Copy link
Contributor Author

zephyx commented Nov 30, 2024

I've made a few changes in order to manage the case where we have a field which is not aliased and the same one aliased.
Before we were loosing the args of the first one, not now with those modifications.

@spawnia
Copy link
Collaborator

spawnia commented Dec 1, 2024

Please add tests.

@zephyx
Copy link
Contributor Author

zephyx commented Dec 2, 2024

After writing some tests I have seen an edge case and completely refactored the code and moved it somewhere else.
It's late, I will finish the tests tomorrow or sometime this week :)

…nfo->lookAhead()->aliasArgs() to $info->getFieldSelectionWithAlias()

Add tests
@zephyx
Copy link
Contributor Author

zephyx commented Dec 2, 2024

I switched the feature from $info->lookAhead()->aliasArgs() to $info->getFieldSelectionWithAlias()
It still doesn't affect existing code ($info->getFieldSelection remains) and allow us to retrieve all fields with their aliases version and corresponding arguments.
I added some tests.

composer check is ok except for :

Parameter #3 $parentType of method GraphQL\Type\Definition\ResolveInfo::foldSelectionWithAlias() expects GraphQL\Type\Definition\Type, (GraphQL\Type\Definition\NamedType&GraphQL\Type\Definition\Type)|null given

But I can't set this typing definition if we want to keep the lib usable with php 7.4 as described in composer.json

I am not familiar with open-source pull-requests.
Do you need something else?

src/Type/Definition/ResolveInfo.php Outdated Show resolved Hide resolved
src/Type/Definition/ResolveInfo.php Outdated Show resolved Hide resolved
src/Type/Definition/ResolveInfo.php Outdated Show resolved Hide resolved
src/Type/Definition/ResolveInfo.php Outdated Show resolved Hide resolved
src/Type/Definition/ResolveInfo.php Outdated Show resolved Hide resolved
src/Type/Definition/ResolveInfo.php Outdated Show resolved Hide resolved
src/Type/Definition/ResolveInfo.php Outdated Show resolved Hide resolved
src/Type/Definition/ResolveInfo.php Outdated Show resolved Hide resolved
tests/Type/ResolveInfoTest.php Outdated Show resolved Hide resolved
@spawnia
Copy link
Collaborator

spawnia commented Dec 2, 2024

The PHPStan error should be fixable with an assert call to ensure non-null.

@zephyx
Copy link
Contributor Author

zephyx commented Dec 2, 2024

Updated

@zephyx zephyx changed the title Add an option to retrieve the arguments of each aliased field on ResolveInfo::lookAhead() Add a method getFieldSelectionWithAlias to ResolveInfo class to retrieve the arguments of each aliased field Dec 3, 2024
@spawnia
Copy link
Collaborator

spawnia commented Dec 4, 2024

I know I originally proposed it like this in #1072 (comment), but there we also included the type name per field. Given we no longer need that, is there any reason why we should nest aliases under a key aliases, as in this example:

[
    'level2' => [
        'aliases' => [
            'level2000' => [
                'args' => [
                    'width' => 1,
                    'height' => 1,
                ],
            ],
            'level2' => [
                'args' => [
                    'width' => 2,
                    'height' => 20,
                ],
            ],
        ],
    ],
]

Or can we simplify like this?

[
    'level2' => [
        'level2000' => [
            'args' => [
                'width' => 1,
                'height' => 1,
            ],
        ],
        'level2' => [
            'args' => [
                'width' => 2,
                'height' => 20,
            ],
        ],
    ],
]

@zephyx
Copy link
Contributor Author

zephyx commented Dec 4, 2024

Yes, in current state, we can simplify this
But when I started writing this code, I imagined adding other information like the type you mention.
I forgot this point during my refactoring, but for example adding the type is just one line of code.
Leaving this level “aliases” would make it possible to add other information to the return if we want to evolve the function, no?

If it's ok with you, I can add this line to the code:
$fields[$fieldName]['type'] = $fieldType->name();

This will return if you keep the types described in tests :

[
    'level2' => [
        'type' => 'Int'
        'aliases' => [
            'level2000' => [
                'args' => [
                    'width' => 1,
                    'height' => 1,
                ],
            ],
            'level2' => [
                'args' => [
                    'width' => 2,
                    'height' => 20,
                ],
            ],
        ],
    ],
     'level2bis' => [
        'type' => 'level2bis'
        'aliases' => [
            'level2bis' => [
                'args' => [],
                'fields' => [
                     'level3' => [
                         'type' => 'Int',
                         'aliases' => [
                             'level3' => [
                                 'args' => [
                                     'length' => 10
                                 ]
                             ]
                         ]
                     ]
               ]
            ]
        ],
    ],
]

@spawnia
Copy link
Collaborator

spawnia commented Dec 4, 2024

Apparently, getFieldSelection has worked fine for people without information about the field type. I don't see how this is different in the aliased variants, so let's keep it simple.

Adding the type name is kind of an arbitrary extra information anyways. Nullability or being in a list could also matter - as could other information about the field or the schema at that point.

@zephyx
Copy link
Contributor Author

zephyx commented Dec 4, 2024

Adding the type name is kind of an arbitrary extra information anyways. Nullability or being in a list could also matter - as could other information about the field or the schema at that point.

Yes, that was my point (evolution): if latter we need more information, it would have been easy to add it alongside the "aliases" level. Once removed, we won't be able to add field related informations without introducing breaking changes.
But nevermind, I can remove it if you want.

@spawnia
Copy link
Collaborator

spawnia commented Dec 4, 2024

We can always add a new API if something more complex is necessary. Let's keep the simple things simple and only add complexity when it is really necessary.

@spawnia
Copy link
Collaborator

spawnia commented Dec 4, 2024

I renamed the nested fields to selectionSet. This terminology follows the language of the GraphQL specification when talking about queries, see https://spec.graphql.org/draft/#Field. It also clearly distinguishes this from the type level, it is about the current execution and not about what is in the schema.

@spawnia spawnia changed the title Add a method getFieldSelectionWithAlias to ResolveInfo class to retrieve the arguments of each aliased field Add method getFieldSelectionWithAliases to class ResolveInfo Dec 4, 2024
@spawnia spawnia merged commit a84933c into webonyx:master Dec 4, 2024
16 checks passed
@spawnia
Copy link
Collaborator

spawnia commented Dec 4, 2024

Thank you @zephyx, released with https://github.com/webonyx/graphql-php/releases/tag/v15.19.0.

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