Deprecate callbacks in field types

[vladar]

Deprecate following field definition form:

[
    'name' => 'Foo',
    'fields' => [
        'bar'  => ['type' => function()  use(&$barType) { return $barType;}],
        'baz'  => ['type' => function()  use(&$bazType) { return $bazType;}],
    ]
]

in favor of:

[
    'name' => 'Foo',
    'fields' => function() use (&$barType, &$bazType) {
        return [
            'bar' => ['type' => $barType],
            'baz' => ['type' => $bazType],
        ];
    }
]

[ooflorent]

Is there any reason why? What's the motivation behind this change?

[vladar]

This seems redundant. Also first notation adds some performance overhead because every field type has to be checked with is_callable and then validated against base Type class.

Pretty much everything you can do with first notation can be done with second as well. Also that's how reference js implementation works. It turns out that maintaining this library is way easier when there is little deviation from reference implementation (at least until things settle down with GraphQL itself).

But if you have any use-cases which require this feature - please describe them. That's pretty much the reason why I created these issues - to start a discussion if somebody needs them %)

[mcg-web]

👍 for this change! it will be much more easier to implements dynamics fields without having to used hacks.

[vladar]

Done in c11f257

[decebal]

must compliment you guys for referincing this issue in the actual error message! 🥇

[Alexandru-Dobre]

Is there any chance the first notation could be permitted once again ?
We have a library that uses this and json models in order to build a loopback-like php graphql api factory.
Having to move the callback to the fields rather than the type of each field would greatly complicate things since the field is linked to the middleware-connector pipeline during field construction. Here's a link to the concerned code : https://github.com/ampize/alambic/blob/master/src/Alambic/Alambic.php

[vladar]

@Alexandru-Dobre We can discuss it, but first I would like to figure out why it is impossible to change the code of your library for this notation (as it is also the only way reference js implementation allows you to define fields). I am pretty sure there should be a way to do this.

What will happen if you change these lines to:

$typeArray = [
    'name' => $type['name'],
    'fields' => function() use ($type) {
        $fields = [];
        foreach ($type['fields'] as $fieldKey => $fieldValue) {
            $fields[$fieldKey] = $this->buildField($fieldKey, $fieldValue);
        }
        return $fields;
    }
];
if (!empty($type['description'])) {
    $typeArray['description'] = $type['description'];
}
$this->alambicTypes[$typeName] = new ObjectType($typeArray);

Obviously you will have to remove closure for type option in buildField (it is also quite possible that you will have to wrap remaining code of this method to same closure as well).

Why this will not work?

[Alexandru-Dobre]

@vladar Thank you for the suggestion! It worked perfectly with minimum changes to our library code. The rest of the method doesn't need closures as only the "fields" part was exposed to a cyclic dependency issue (models for SQL databases had foreign keys pointing to each other) since we automatically exclude non-scalar fields as query args.

[vladar]

Nice!