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

[GraphQL] Fix FieldsBuilder not always fully unwrapping a type before deciding if a resolver is needed. #4251

Merged

Conversation

DavidBennettUK
Copy link
Contributor

Q A
Branch? main
Bug fix? yes
New feature? no
Deprecations? no
Tickets N/A
License MIT
Doc PR N/A

This fixes FieldsBuilder not using the recursive version of getWrappedType. Without this, if the type is wrapped in both NonNull and then ListOfType, then ListOfType is returned instead of the actual type of the list items - which I think was the original intention of this code, as it's trying to decide if the base type needs a special resolver. For example, a non-null list of non-null strings ([String!]!) will now correctly return String as the wrapped type with this fix. This means that simple arrays such as this, which are already deserialized by the Symfony deserializer and therefore do not need to use a special resolver, can now work.

@alanpoulain
Copy link
Member

Hello David, thanks for the PR.
Could you:

  • target 2.6 since it's a bug fix,
  • add a unit test?

@DavidBennettUK DavidBennettUK force-pushed the graphql-fieldsbuilder-wrapped-type branch from 79d068f to 00a17ec Compare April 27, 2021 12:01
@DavidBennettUK DavidBennettUK changed the base branch from main to 2.6 April 27, 2021 12:02
@DavidBennettUK
Copy link
Contributor Author

@alanpoulain Thanks, all done 👍

@alanpoulain alanpoulain force-pushed the graphql-fieldsbuilder-wrapped-type branch from 00a17ec to 8ef71c2 Compare April 27, 2021 13:22
@alanpoulain alanpoulain merged commit 1be4676 into api-platform:2.6 Apr 27, 2021
@alanpoulain
Copy link
Member

Thank you @DavidBennettUK.

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.

None yet

2 participants