Allow overwriting internal types to improve result coercion

[Rendez]

The 0.11.x update seem to have improved variable coercion and some of the scalar types as well. The coercion adheres the definition in http://facebook.github.io/graphql/October2016/#sec-Scalars.

Now, we've tried to extend BooleanType to make it more strict, so that it throws if the passed value isn't a boolean in order to avoid false positives within our app:

class ExtendedBooleanType extends BooleanType {
    /**
     * @param mixed $value
     * @return bool
     */
    public function serialize($value) {
        if (!is_bool($value)) {
            throw new InvariantViolation("Boolean must be represented by strict value. " . Utils::printSafe($value) . " given.");
        }
        return $value;
    }
}

The need for this relates to running in weak mode, where even methods with return types :bool end up in coercion. Therefore defining an error inside serialize is important for us.

However, the Instrospection class (among others) uses static calls, including calls to Type::boolean(), so building our schema throws once again:

Schema must contain unique named types but contains multiple types named

• What are your thoughts on strictly checking for boolean to prevent errors?
• Is there a way we could point to our boolean definition from the different calling contexts?

[vladar]

I don't think that replacing standard types with custom types is a good idea because their parsing behavior is defined in spec and clients rely on it (except maybe ID type).

On the other hand, I do agree that serializing process is a bit different and could be customizable to some extent. Two options I see:

1. Introduce strict/weak mode for serialization
2. Allow defining optional custom serialization function

I'd like to keep this issue open for a while for discussion (maybe others will post their thoughts as well). Maybe I'll reconsider this and just allow defining own standard types.

In the meantime, the only workaround for you is to use Reflection to override GraphQL\Definition\Type::$internalTypes.

[Rendez]

@vladar thanks for the very quick answer. I agree let's keep this opened for a bit. Personally I do like option 1. because it plays nice when configuring PHP to run in either weak or strong typing mode.

[Rendez]

Reading the spec for Boolean closer and given than PHP does have a bool type (unlike other langs), we're not limited to returning a coerced type. Therefore, even though is not coercing, I believe the next could be safe:

class BooleanType extends ... {
    /**
     * @param mixed $value
     * @return bool
     */
    public function serialize($value) {
        return $value ? true : false;
    }
}

"The Boolean scalar type represents true or false. Response formats should use a built‐in boolean type if supported; otherwise, they should use their representation of the integers 1 and 0."

Again, the point I'm trying to make is that PHP is already loose enough coercing types, and strict mode makes no difference to this behavior, leaving us with either tackling it here or using strict type with method return types: serialize($value): boolean { ... } which we do not use. As I understand, GraphQL philosophy is to avoid returning typing errors and instead coerce.

Thoughts?

[vladar]

Sorry, missed your last comment somehow. But I am not sure I am following it. $value ? true : false is a current behavior. But then what's your problem with it (again, I feel that I am missing something)?

[Rendez]

Sure...

return !!$value; // returns coerced value, e.g.:
// IF $value = 'foo' THEN return !!$value EQUALS (int) 1

return $value ? true : false; // returns boolean value

With the second approach we would make sure the coerced value is a boolean value, as expected. This IMO plays a very important role in type validation, avoiding potential pitfalls that come from checkbox value conversions, etc.

[vladar]

It's the same:

$foo = 'foo';
$bar = !!$foo;
var_dump($bar);

// outputs: bool(true)

[Rendez]

Oh my... echo VS var_dump, echo is obviously stringifying the real output. My bad...

I feel after a few days even I got lost about the real issue, which was not being able to extend the scalar types. However, I'm closing this for now.