tobyzerner / json-api-server

A JSON:API server implementation in PHP.
https://tobyzerner.github.io/json-api-server/
MIT License
63 stars 21 forks source link

`Str` validation on `is_string` is always `true`, even when another type is passed #70

Closed bertramakers closed 1 year ago

bertramakers commented 1 year ago

The Str class has a validation on the type of the value being a string, as I'd expect to be checked, here: https://github.com/tobyzerner/json-api-server/blob/main/src/Schema/Field/Str.php#L21

However, the value is always converted to a string here before it is passed to the validation: https://github.com/tobyzerner/json-api-server/blob/main/src/Schema/Field/Str.php#L18

This results in warnings/errors.

For example, given a POST payload like this:

{
    "data": {
        "type": "posts",
        "attributes": {
            "title": ["Foo"] // Str field
        }
    }
}

PHP triggers a warning:

Warning: Array to string conversion in /var/www/vendor/tobyz/json-api-server/src/Schema/Field/Str.php on line 18

Even when ignoring the warning (since it's not fatal), the final value is "Array" which obfuscates the problem but is not a good conversion.

Instead, I'd expect the "must be a string" error defined in the validation function.

The same issue occurs with other types that cannot be safely cast to a string, like an object.

In some other cases, the value can be safely cast to a string but it's probably not optimal either. For example, a boolean "false" will be converted to an empty string. But if a client passes "false" to something that expects a string, it's possible that something went wrong on the client instead and it would be better to return an error so that the issue is detected early on.

Simple solution would be to not cast the type to string in the deserialize function, but I'm not sure if that would influence/break something else.

Note: The Integer, Number and Boolean fields have a similar issue.

bertramakers commented 1 year ago

@tobyzerner I can make a PR with a fix if you'd like, but I wanted to check with you first if you agree with a change like this or if there are reasons that I'm overlooking why this automatic conversion is needed

tobyzerner commented 1 year ago

Thanks for the report @bertramakers! I've fixed in the above commit.

bertramakers commented 1 year ago

Thanks @tobyzerner !