Stringable ID types

[theofidry]

In IDType, you are checking that the value is a scalar and return a string value:

    /**
     * @param mixed $value
     * @return string
     */
    public function serialize($value)
    {
        if ($value === true) {
            return 'true';
        }
        if ($value === false) {
            return 'false';
        }
        if ($value === null) {
            return 'null';
        }
        if (!is_scalar($value)) {
            throw new InvariantViolation("ID type cannot represent non scalar value: " . Utils::printSafe($value));
        }
        return (string) $value;
    }

This is actually wrong as there is one more kind of value that could be valid: classes implementing __toString. For example you could have an ID implementing a Uuid class or even something more specific to your domain like UserId.

[vladar]

For now, it is recommended to use your own custom ID scalar type if you need custom behavior. ID is the only built-in scalar type which can be overridden by passing it via types option to the schema.

[theofidry]

@vladar I'm using it with OverblogGraphQLBundle so maybe it's an issue there but I wouldn't override the ID type as it's internal and used in Type::getInternalTypes() which is static (so even if we can extend type to override it it's not enough). So you would need to register a new type with a different alias and use that new alias, which is too much of a change as it requires to update all the front-ends as well.

We have a dirty workaround for now, but the fix in the core is rather simple...:

    /**
     * @param mixed $value
     * @return string
     */
    public function serialize($value)
    {
        if ($value === true) {
            return 'true';
        }
        if ($value === false) {
            return 'false';
        }
        if ($value === null) {
            return 'null';
        }
        try {
             return (string) $value;
        } catch (\Throwable $throwable) {
             throw new InvariantViolation("ID type cannot represent non scalar value: " . Utils::printSafe($value));
        }
    }

[vladar]

OK, I'll reopen.

This lib's minimum PHP version is 5.5+ which has no Throwable support. Also, it will silently convert arrays to "Array" string which is not desirable (although I guess we could ignore this case).

So the only good way I see how this can be fixed is by additionally checking for:

!(is_scalar($value) || (is_object($value) && method_exists($value, '__toString'))

Do you see any drawbacks to it?

[theofidry]

That would work but there is a performance overhead as you would try to check if the method exists but if you want to support PHP 5.5 I don't think there's another way.

Is there any reason not to push that library to PHP 7.1?

[vladar]

Someday it will happen, but not yet as there are still projects on PHP5 who may use this lib.

[theofidry]

True but:

• PHP 5.5 already reached EOL
• PHP 5.6 & 7.0 are reaching EOL in 1 year
• the adoption rate for PHP 7+ is quite good

And sticking to PHP 5.5+:

• prevents you to leverage scalar typehints
• requires you to check more PHP versions

And we are talking for the new versions. It's not like the library is disappearing for the current PHP 5.5 users, they just wouldn't access to the latest version.

But ultimately, the burden is on the maintainer, you. So you're the one doing the call depending of what you need from this lib and what you're ready to maintain :)

[vladar]

I personally know legacy projects on PHP5 who haven't transitioned yet to PHP7, but who do use this library, so we won't switch before the next major version of this lib.

[vladar]

Fixed in #210