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

Make it possible for scalar annotation to register Doctrine and PHP types to map #751

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

Conversation

bartv2
Copy link
Contributor

@bartv2 bartv2 commented Aug 20, 2020

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Documented? not yet
License MIT

When creating a scalar class it should be easy to also tell which types always map to this scalar. When the idea is accepted i will also update the documentation.

@Vincz
Copy link
Collaborator

Vincz commented Aug 21, 2020

Hey @bartv2 !

I like the PHP types <-> Scalar conversion, but I don't like the fact that we would need to dive into the app code to check which PHP type convert to which Scalar.
At the moment, we can add Doctrine types conversion with https://github.com/overblog/GraphQLBundle/blob/master/docs/annotations/index.md#field-type-auto-guessing-from-doctrine-orm-annotations.
Can't we do what you want in the config as well ?
Something like :

overblog_graphql:
    php:
        types_mapping:
            DateTime:  ScalarWithTypeMapping

Or maybe, am I missing something ?

@bartv2
Copy link
Contributor Author

bartv2 commented Aug 21, 2020

I was adding a scalar type and it wasn't working directly. These mappings are only used for the auto-guessing, i think. And with annotations (most of) the config will already be in the php code.

This PR also makes it possible to register the doctrine mapping in the annotation, maybe you didn't notice that. So all the mapping data can still be in one place.

Adding the php mapping to the config is a good idea for those who prefer that.

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