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

Inconsistency between system scalar type serialize/parseValue methods #254

Closed
mcg-web opened this issue Feb 27, 2018 · 7 comments
Closed

Comments

@mcg-web
Copy link
Collaborator

mcg-web commented Feb 27, 2018

Hi, after upgrading my project from 0.10 to 0.11 version I get some weird behaviors with system scalar types when using inputs with variables. I don't really get why we have a differents between serialize/parseValue methods.
Here an example IntType::parseValue no more accepts string but IntType::serialize does:

(new IntType())->serialize('9') !== (new IntType())->parseValue('9'); // 9 !== null

In JS implementation both methods has the same behavior so why in PHP we add these differents?
I override system scalar types to use 0.10 version right now but this is not the best solution... I'm just trying to understand the reason why...

How I override system scalar types without using reflection:

// Override scalar types
$overrideScalarTypes = \Closure::bind(function () {
    self::$internalTypes = [
        self::ID => new MyType\IDType(),
        self::STRING => new MyType\StringType(),
        self::FLOAT => new MyType\FloatType(),
        self::INT => new MyType\IntType(),
        self::BOOLEAN => new MyType\BooleanType()
    ];
}, null, \GraphQL\Type\Definition\Type::class);
$overrideScalarTypes();

This can maybe helps some people encountering same issue.

@vladar
Copy link
Member

vladar commented Mar 3, 2018

As far as I remember it was done in #170. It was actually a valid issue (at least for strings and booleans). A related issue in graphql-js - graphql/graphql-js#771 (yet they seem to fix only string case, not boolean or int).

So do you need to accept strings as integer input?

@mcg-web
Copy link
Collaborator Author

mcg-web commented Mar 3, 2018

My trouble is that string is no more accepted as int also int is not accepted as string in parseValue method but with serialize both works. I can submit a PR that show what I'm waiting for if you want?

@vladar
Copy link
Member

vladar commented Mar 16, 2018

Sorry for the long delay with a response, I suggest to send a PR after I merge #248 (hopefully on this weekend)

@danez
Copy link
Contributor

danez commented Mar 22, 2018

This problem should be fixed in #248 as the serialize and parseValue methods have been aligned together. https://github.com/danez/graphql-php/blob/3e067cc60fb720703453904f1351f3acbfb4b453/src/Type/Definition/IntType.php#L39..L52

@mcg-web
Copy link
Collaborator Author

mcg-web commented Mar 22, 2018

Nice! Thank you @danez 👍 .

@vladar
Copy link
Member

vladar commented Mar 26, 2018

@mcg-web Can you check if the new version (in master) works for you and close if it is OK?

@mcg-web
Copy link
Collaborator Author

mcg-web commented Mar 26, 2018

It seems to be OK! Thank you @vladar and @danez 👍

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

No branches or pull requests

3 participants