Q	A
Bug report?	no
Feature request?	yes
BC Break report?	no
RFC?	no
Version/Branch	0.14

@overblog/graphql

I strongly believe that we should rework all builders in the bundle and here I'll try to explain why.

This is just a draft with my thoughts on how builders could be changed. Use examples as a starting point to come to a common conclusion of how field/fields/args builders should be used/changed/enhaced.

What is wrong with builders?

1) Currently all builders are working with raw arrays and simply return them. Working with PHP arrays is not the best experience, so I suggest we create new builder classes to get rid of using arrays. This would lead to less errors, more readable code, autosuggestions by IDEs + opens perspectives for some other features in the future, for example using one builder inside another. Examples will be shown below a little later.

2) The configuration flow is pretty messy right now. Take a look at the following example:

    User:
        type: object
        config:
            fields:
                rawId:
                    # Field builder
                    builder: "RawId"
                    builderConfig:
                          name: photoID
                          type: String
                    # Args builder
                    argsBuilder:
                        builder: "Pager"
                        config:
                            defaultLimit: 50

We have `builder`, `builderConfig`, `argsBuilder`, another `builder` one level deeper and `config`, which actually should be also named `builderConfig`. This is not the most readable type configuration and should be also reworked.

3) Lack of flexibility when it comes to the question Which part of my GraphQL Type do I want to be generated by a builder? We have 3 types of builders: args, field and fields builders. So what is the difference? Take a look at this picture:

![enter image description here](https://sun9-41.userapi.com/c857728/v857728717/9e470/ii7qaLN-TQk.jpg)

As you can see the main difference between builders is that they are responsible for different parts of GraphQL types. Important is that some builders are supersets of other builders, which means, that they can do the same as their subsets, for example we can replace multiple field and args builders with one fields builder, becase field and args builders are its subsets.

    Fields Builder > Field Builder > Args Builder

The problem here is, that a user can't use one builder in which he could decide what he wants to be generated. Instead he needs to choose between 3 predefined builders with static limited responsibility (green, blue and red areas on the pic).  Maybe we can combine all 3 builders into 1 (Type Builder)?

These are 3 points on why we should rework builders.

1. Getting rid of arrays

FieldBuilder

Current implementation:

The method gets a single argument with all configs and returns an array.

class RawIdField implements MappingInterface
{
    public function toMappingDefinition(array $config): array
    {
        $name = $config['name'] ?? 'ID';
        $type = $config['type'] ?? 'Int!';

        return [
            'description' => 'The raw ID of an object',
            'type' => $type,
            'resolve' => "@=value.$name",
        ];
    }
}

Suggestion:

The method gets all configs separately and doesn't return anything. The class extends a base class instead of implementing an interface.

class RawIdField extends FieldBuilder
{
    public function build(string $name, string $type): void
    {
        $this
            ->setType($type)
            ->setDescription('The raw ID of an object')
            ->setResolverExpression("value.$name");

        # if without expression
        $this->setResolver('my_resolver');
    }
}

Adding validation:

Passing constraint objects to the builder is not possible:

$this->setConstraints([
    new Assert\Uuid()
    new Assert\Length(['min' => 1, 'max' => 15]),
]);

because eventually a type class will be generated where the constraints will be actually instantiated. So we should pass class names instead of objects and optionally their params one by one:

$this->addConstraint(Uuid::class);
$this->addConstraint(Length::class, ['min' => 1, 'max' => 15]);

There might be different approaches to add constraints all at once:

Passing array pairs

->setConstraints([
[Uuid::class]
[Length::class, ['min' => 1, 'max' => 15]],
]);

Using an associative array, which has it's drawback, that constraints cannot be added more than once:
```
->setConstraints([
Uuid::class => null,
Length::class => ['min' => 1, 'max' => 15],
]);
```
This is a pretty rare case when someone needs to add a constraint more than once, but in case it's required it could be done by using addConstraint().

Entries without params could omit null:

->setConstraints([
    Uuid::class,
    Length::class => ['min' => 1, 'max' => 15],
]);

Example:

class RawIdField extends FieldBuilder
{
    public function build(): void
    {
        $this
            ->setType(Type::ID)
            ->isNullable(false)
            ->setResolveExpression("value.$name")
            ->setConstraints([
                Assert\Uuid::class,
                Assert\Length::class => ['min' => 1, 'max' => 15],
            ]);

        # or set a link
        $this->setLinkToProperty(User::class, 'id');

        # or set cascade validation
        $this->setCascade(true);       
    }
}

Args Builder

Pretty much the same as field builder.

Current Implementation:

class Pager implements MappingInterface
{
     public function toMappingDefinition(array $config): array
     {
         $defaultLimit = isset($config['defaultLimit']) ? (int)$config['defaultLimit'] : 20;
         return [
             'limit' => [
                 'type' => 'Int!',
                 'defaultValue' => $defaultLimit,
             ],
             'offset' => [
                 'type' => 'Int!',
                 'defaultValue' => 0,
             ],
         ];
     }
}

Suggestion:

class Pager extends ArgumentsBuilder
{
     public function build(int $defaultLimit = 20): void
     {
        $limit = $this->addArgument('limit', Type::INT, $defaultLimit)->isNullable(true);
        $offset = $this->addArgument('offset', 'Int!', 0);
     }
 }

Some extra examples:

class Pager extends ArgumentsBuilder
{
    public function build(): void
    {
        // ...

        # Change some options
        $limit->isNullable(false);
        $limit->setType(Type::STRING);

        # Add validation
        $limit->addConstraint(Length::class, ['min' => 1, 'max' => 10]);

        $this->removeArgument($offset);
        # or
        $this->removeArgument('offset');
    }
}

Fields Builder

The same approach as in field and args builders: each part of the type should be buildable with predefined methods.

2. Changing the configuration

I don't have any idea to drastically change the current approach to insert builders into GraphQL types. It just feels somehow messy. Here is an example using all 3 builders:

User:
    type: object
    config:
        # Fields builder
        builders:
            - builder: Timestamped
              builderConfig:
                  propertyCreated: dateCreated
        fields:
            rawId:
                # Field builder
                builder: "RawId"
                builderConfig:
                      name: photoID
                      type: String
                # Args builder
                argsBuilder:
                    builder: "Pager"
                    config:
                        defaultLimit: 50

The first thing I don't like about this approach is mixing of concepts of yaml and php configurations. The configuration gets sparsed, because part of the type lies in a yaml file and another part in a php file.

The second thing I don't like is that builders can be defined at many places, which looks kind of messy and confusing.

Maybe we can fix both problems with a Type Builder (new builder type), which I will talk about later.

And even if we don't change the approach above, we could at least make it a little bit nicer. Here is the same example changed:

User:
    type: object
    config:
        # Fields builder
        builders:
            - name: Timestamped
              params: [dateCreated]
        fields:
            rawId:
                # Field builder
                builder: 
                    name: "RawId"
                    params: [photoId, String]
                # Args builder
                argsBuilder:
                    name: "Pager"
                    params: [50]

builderConfig and config are renamed to params and moved under the builder option, because builderConfig makes no sense without builder.
params can be defined without names (collection) and they will be injected into builder separately. If user wants to use named params he can do it and in this case params will be injected into the builder using same argument names in the PHP.

short syntax (without params):

User:
    type: object
    config:
        builders: [Timestamped]
        fields:
            rawId:
                builder: RawId
                argsBuilder: Pager

Btw I don't know if field builder and args builder can be mixed.

3. Type Builder

A Type Builder is the new type of builders and it's responsible for generating of any part of a type:

enter image description here

Here is an example:

class UserBuilder extends TypeBuilder  
{  
    public function build(string $name, string $type, ?string $resolver = null): void  
    {  
        $this->setName($name);  
        $this->setType($type);  

        $usernameField = $this->addField("username", "String!")->setResolver($resolver);  

        if("User" === $type) {
            $usernameField->addArgument('limit', 'Int');
        }

        // ...
    }  

    public function configProvider()  
    {  
        yield ['User', 'object', 'user_resolver'];  
        yield ['UserCreateInput', 'input-object'];  
        yield ['UserUpdateInput', 'input-object'];  
        yield ['UserPayload', 'object'];  
    }  
}

This builder is used to create 4 different GraphQL types. The build method will be called 4 times with different arguments received from configProvider (like in PHPUnit tests). Technically a single type builder would be enough to build the entire GraphQL schema, but it would make sense to create separate builders for groups of related types.

With this we don't need yaml types at all, the entire schema could be defined with type builders. It could even be possible to use arg and field builders inside the type builder (don't know if it could be useful).

Maybe with type builders the need in other builders will disappear?

Hi @murtukov ! I agree that PHP arrays are not really sexy.... but no matter what you do, at the end of the day, it's what you get. I don't really like to extend class, I prefer to implement interfaces, because my "builder" can be anything (a service or whatever) already extending something. The builders are just config generator. You could add a object to it, like, instead of returning an array in the toMappingDefinition you could return objects. But your objects will be turn into PHP array just after. So adding objects into all of this, would just be a new way to express configuration array.

A PHP like that, you already know that it's a GraphQL config :

return [
    'description' => 'The raw ID of an object',
    'type' => $type,
    'resolve' => "@=value.$name",
];

With this, you need to learn something new, and understand that it's the same as the above:

$this
    ->setType($type)
    ->setDescription('The raw ID of an object')
    ->setResolverExpression("value.$name");

# if without expression
$this->setResolver('my_resolver');

So, for me, it's just "adding" a new step with new stuff to learn. Not really necessary I think. And I don't really understand the problem with the constraints as you just need to do as usual, like:

return [
    'description' => 'The raw ID of an object',
    'type' => $type,
    'resolve' => "@=value.$name",
    'validation' => ...
];

I let the others express themselves on the subject.

Hi @Vincz

What do you mean by that?

but no matter what you do, at the end of the day, it's what you get.

If we implement the builders I suggested, we would have no arrays at all, so at the end of the day we would have objects 😀.

I don't really like to extend class, I prefer to implement interfaces, because my "builder" can be anything (a service or whatever) already extending something.

Currently we have just one interface MappingInterface for all 3 builder types, that means when you open any builder you can't say of which type it is unless you read the code inside the toMappingDefinition method.

But with object builders on other hand we would have 3 different classes: ArgumentBuilder, FieldBuilder and FieldsBuilder predefined by the bundle, so you would always know of which type it is and what $this means. Your builder couldn't be anything else, but only one of those 3 classes.

But your objects will be turn into PHP array just after. So adding objects into all of this, would just be a new way to express configuration array.

It doesn't matter what happens afterwards, we are talking now about a convenient API for users of this bundle, not for developers. The users don't have to think about what happens after. From their point of view they will never get into touch with arrays.

A PHP like that, you already know that it's a GraphQL config :
return [
    'description' => 'The raw ID of an object',
   'type' => $type,
   'resolve' => "@=value.$name",
];

How do you know it's a GraphQL config? It's just a regular PHP array, it doesn't contain any additional information.

But if it were an object: 1) You could know what options you can add to the builder just by typing CTRL+SPACE (in PHPStorm, could be different in other IDEs) 2) You could read the description of every method again with the help of your IDE.

With this, you need to learn something new, and understand that it's the same as the above:

Let's be honest, when you use this bundle for the first time do you know what a builder is? Surely not. So you have to read the documentation and learn this feature anyways to know, what it is. But with objects it's easier to use them, because of the autosuggestions and descriptions of all methods. You don't have to return to the docs every time to look, what keys your array is allowed to contain. And what if you make a mistake? It would be hard to find it, especially if you have something like that (from documentation):

class MutationField implements MappingInterface
{
    public function toMappingDefinition(array $config): array
    {
        $name = $config['name'] ?? null;
        $resolver = $config['resolver'] ?? null;
        $inputFields = $config['inputFields'] ?? [];

        $successPayloadFields = $config['payloadFields'] ?? null;
        $failurePayloadFields = [
            '_error' => ['type' => 'String'],
        ];

        foreach (\array_keys($inputFields) as $fieldName) {
            $failurePayloadFields[$fieldName] = ['type' => 'String'];
        }

        $payloadTypeName = $name.'Payload';
        $payloadSuccessTypeName = $name.'SuccessPayload';
        $payloadFailureTypeName = $name.'FailurePayload';
        $inputTypeName = $name.'Input';

        $field = [
            'type' => $payloadTypeName.'!',
            'resolve' => \sprintf('@=mutation("%s", [args["input"]])', $resolver),
            'args' => [
                'input' => $inputTypeName.'!',
            ],
        ];

        $types = [
            $inputTypeName => [
                'type' => 'input-object',
                'config' => [
                    'fields' => $inputFields,
                ],
            ],
            $payloadTypeName => [
                'type' => 'union',
                'config' => [
                    'types' => [$payloadSuccessTypeName, $payloadFailureTypeName],
                    'resolveType' => \sprintf(
                        '@=resolver("PayloadTypeResolver", [value, "%s", "%s"])',
                        $payloadSuccessTypeName,
                        $payloadFailureTypeName
                    ),
                ],
            ],
            $payloadSuccessTypeName => [
                'type' => 'object',
                'config' => [
                    'fields' => $successPayloadFields,
                ],
            ],
            $payloadFailureTypeName => [
                'type' => 'object',
                'config' => [
                    'fields' => $failurePayloadFields,
                ],
            ],
        ];

        return ['field' => $field, 'types' => $types];
    }
}

With this, you need to learn something new, and understand that it's the same as the above:

With this you don't have do understand it's the same as the above, because the above would not exist.

And I don't really understand the problem with the constraints as you just need to do as usual, like

When you add constraint into a yaml file you write string names of the constraints:

username:
    type: String!
    validation:
        - Json: ~
        - Length: 
              min: 6
              max: 32

If you want to repeat the above in the builder with arrays we will get the following:

'username' => [
    'type' => 'String!',
    'validation' => [
        [
            'Json' => null
        ],
        [
            'Length' => ['min' => 6, 'max' => 32]
        ]
    ]
]

As you can see I don't use objects there.

I understand your point, and you have solid arguments, but what I meant is that... no matter what kind of parser you use (Annotations, Yaml, GraphQL, ...), they all end up dumping a config array for Webonyx GraphQL lib (check the ParserInterface). And yes, even with your implementation, your objects will be turned and merge into a PHP array (check the BuilderProcessor). Currently, there is no object anywhere else in the bundle to describe GraphQL types/fields and this is what you want to implement with your objects and it would be very specific to builders. With the current builders implementation, we are low level. I agree, it's less sexy than using objects with a fluent interface but there is no extra step. And about the learning curve, if you use builders, it means that you have already understand the type configuration (as there is anyway only 3 or 4 keys to know). About the configuration part, I agree we could improve it.

with your objects and it would be very specific to builders

Why would it be "very specific"? It will generate the same webonyx objects at the end, there is no difference, what you use (Annotations, Yaml etc.).

With the current builders implementation, we are low level. I agree, it's less sexy than using objects with a fluent interface but there is no extra step.

Low level doesn't mean good, actually I suggest to introduce object builders to avoid low level interactions. And why is an extra step even an issue if it happens in the background and only once in the generation time?

And about the learning curve, if you use builders, it means that you have already understand the type configuration (as there is anyway only 3 or 4 keys to know).

Learning curve? These needs max 5-10 minutes to understand. As you said, if you are working with builders, you already know types and their keys, therefore you will understand builders too. It is not something big so that you need to sit and learn it all day long.

I don't see any serious reason to NOT to implement this. But I do see reasons to DO it.

I agree, that replacing deep arrays is a good thing. I also see the point of @Vincz . What if we could achieve both things and stay BC?

@murtukov What if your builders (except type, which is a new thing) would be simply implementations of the MappingInterface? I see there one fixable issue though: If you use $this to provide API for building, it means you either have to instantiate a new builder instance each build operation or the builder must track state and reset it. My proposal would be instead of using the builder as API, provide some object injected into the build method, which has the desired API.

I agree with you, that things feel partially redundant, especially args were never useful to me. And the field builder only became useful after I have added the type emission feature

The types emission features above is in some way related to the type builder you propose:

Actually, the initial Idea I had way back in version 0.7, was to have a type building system, where having type: object actually means you use a builder object, so that the builder is kind of first-class concept. In fact the bundle uses internal MappingInterface exactly like that for those built-in relay types. You use the type: relay-connection and it goes through a MappingInterface to generate the corresponding relay types (connection, edge).

Quote:


friendConnection:
    type: relay-connection # this is the relevant part
    config:
        nodeType: User
        resolveNode: '@=resolver("node", [value])'
        edgeFields:
            friendshipTime:
                type: String
                resolve: "Yesterday"
        connectionFields:
            totalCount:
                type: Int
                resolve: '@=resolver("connection")'

@akomm

First of all, can we agree that the performance is not so important here, as builders are called only in the generation time and only once?

If you use $this to provide API for building, it means you either have to instantiate a new builder instance each build operation or the builder must track state and reset it.

The second option would do. No need to track the state, just need to call the reset method on the builder before system uses it's build.

My proposal would be instead of using the builder as API, provide some object injected into the build method, which has the desired API.

My first idea was to inject a builder object as you said, but I found some disadvantages of this approach:

If we inject a builder object, then how we inject the params? There are 2 options (neither of which I like):
- We injects params as a second argument: build(Builder $builder, $params) The problem is, that you can't see which params do you have and don't know their types. Plus you need to extract them first.
- We can inject params separately after the builder: build(Builder $builder, $name, $type) This is not intuitive for the users and just doesn't feel right, because it's not typical for PHP.
It also feels wrong to inject a builder into a builder:
```
class Pager extends ArgumentsBuilder # this class is a builder
{
    public function build(Builder $builder): void # and we inject a builder
    { }
}
```
You can't use dependency injection. My idea is to make all builders as services, so that you can inject any dependency you want. You can even build your entire schema depending on DB data (I already saw an issue with such request).

And the field builder only became useful after I have added the type emission feature

I read about this feature and found it weird. Why would you generate new GraphQL types in a Field Builder? The name speaks for itself, it should build fields, not new types. Of course all this unless I misunderstood something in this featur.

First of all, can we agree that the performance is not so important here, as builders are called only in the generation time and only once?

Basically yes. In general compile speed is less important than runtime speed. I think once we reach very long compile times it might become a problem. I think we are far away from this case, so I agree :)

I read about this feature and found it weird. Why would you generate new GraphQL types in a Field Builder?

It perfectly makes sense. Every field has a type. When you use a field builder, you do so to abstract a repeating concept/pattern. If the type of the field is specific to the field and only used by this field (which is often the case), you can also abstract the types related to the field without repeating yourself. Because graphql has no namespace/visibility concept, such composite types are often followed by repeating naming patterns. This can also be eliminated using type emitting field builder.

As a side effect it provides you a powerful version of generic types as an abstraction over the graphql schema language for types used once on the field (for reusable, you need type builder):

Query:
  type: object
  config:
    fields:
      users:
        # = type: Pager<User>
        builder: Pager
        builderConfig:
          item: User

User:
  type: object
  config:
    fields:
      friends:
        # = type: Pager<User> 
        builder: Pager
        builderConfig:
          item: User
          name: UserFriends

Could output:

Query:
  type: object
  config:
    fields:
      users:
        type: UserPager!

User:
  type: object
  config:
    fields:
      friends:
        type: UserFriendsPager!

# PageInfo is emitted once, if all usages of Pager are removed, it will be auto-removed from type system
PageInfo:
  type: object
  # ...

UserPager:
  type: object
  config:
    fields:
      pageInfo: PageInfo!
      items: [User!]!

UserFriendsPager:
  type: object
  config:
    fields:
      pageInfo: PageInfo!
      items: [User!]!

Its actually more powerful, than simply generics, because you can for example add pagerFields config to the Pager and make different pager (User.friends vs. Query.users) using same abstraction and not repeating yourself.

This concept could also be used for the type builder, so that you can do same abstraction over reusable types. Look into the relay-connection code in the bundle and you will see, it is actually already used internally like that.

Now to the first-class concept of builders, imagine the above builder usage changed to this:

Query:
  type: object
  config:
    fields:
      users:
        # = type: Pager<User>
        type: Pager
        config:
          item: User

User:
  type: object
  config:
    fields:
      friends:
        # = type: Pager<User> 
        type: Pager
        config:
          item: User
          name: UserFriends

If type = builder, you could also do inline like this:

User:
  type: object
  config:
    fields:
      friends:
        type: object
        config:
          name: UserFriends
          fields:
            pageInfo: PageInfo!
            items: [User!]!

IIRC, @ooflorent has asked for this type of inline function in another issue.

Btw., this is where the $this usage and context resetting might get into your way.

If the API of the builder is improved, similar to what you proposed and you have access to 1. the parent type and 2. the field, generics like Pager could also generate the name by TypeName . uppercase(fieldname), taking as example User.friends with the above config would result in UserFriendsPager. But this is only an example of what you could do, nobody has to pick exactly this convention or even by default use the semantic to produce type name. You could simply leave it an builder option like in my initial example.

Making builders services sounds good in first place. But your database example I can not see a real usage, because this would require runtime compilation first, and secondly it would make the schema for the client unpredictable. The client would have to make introspection all the time to spot schema changes caused by database updates and even that would not be perfectly safe. Maybe you have some other examples that would make sense?

@akomm

The Field Builder takes the role of a Type Builder, which it shouldn't. Powerful here is not the type emissions feature as a whole, but the type building part of it.

There is no right separation of concerns, because Field Builder makes two things: builds fields and generates completely new types. Powerful doesn't necessarily mean good designed or right designed.

With great power comes great responsibility 😃

If a Field Builder makes more, than just building fields, than don't call it a Field Builder. Right now it has side effects.

I don't say it should be completely removed. It just needs a better design.

You are arguing using current naming, nobody seem to disagree, is not good. How things are named currently does not matter, it matters how they will. And my whole explanation was based on the future, not now. Of this future, one part is there, with the old naming still in place. Just ignore it. If you do not disregard the first-class idea, there is no concept separation issue at all. I explained why type emission on the field perfectly makes sense. Could you please elaborate more deeply why it its a bad concept separation, not disregarding the whole picture I explained?

To clarify:

I am for a better typed builder system.
I am for a type builder.
I am however also for those things being an enhancement over the MappingInterface, so it is still possible to make it low level (agree with @Vincz its good to have), so you can still use it.

The builders you propose could be implementation of MappingInterface that provide API to build the types/config.

Injecting the args typed like you posted is anyway only half of a solution, because as config you can pass nested assoc arrays to the builder, which you will have to type check anyway. So I would propose to simply have an API with a builder object and the config array passed to the builder, instead of each argument separate.

Let me also address this:

You could know what options you can add to the builder just by typing CTRL+SPACE (in PHPStorm, could be different in other IDEs)

Big fan of this, try to type as much as possible, use phpstorm myself :)

You could read the description of every method again with the help of your IDE.

The issue here, and the reason we still need the abstraction to be on top of the low level thing is, that you make a specific API, but the config is more flexible. Having 3 builder type APIs constraint would mean that any extension to it is hard-coded into the bundle. You want validation? Hard-coded, yes its another topic, but the validation solution is now basically hard coded into the typing configuration - even if optional to use. Next step is, hard code the API for the configuration? I don't know if that is the correct direction at this place. And I am telling it to you as someone who can't get enough typing ;)

@akomm

You are arguing using current naming, nobody seem to disagree, is not good. How things are named currently does not matter, it matters how they will. And my whole explanation was based on the future, not now. Of this future, one part is there, with the old naming still in place. Just ignore it. If you do not disregard the first-class idea, there is no concept separation issue at all.

Didn't understand this part.

I explained why type emission on the field perfectly makes sense. Could you please elaborate more deeply why it its a bad concept separation, not disregarding the whole picture I explained?

I meant separation of concerns, not concepts. My mistake, corrected. But I guess you understood what I meant.

Any program should strive to modularity and therefore to a better separation of concerns. This is one of fundamental principles of software engineering. Field builders currently have 2 concerns: building fields and creating new types. "Creating new types" is a side effect here, something what a user wouldn't intuitively expect from a field builder, because it doesn't follow SoC.

1) Good SoC, the module has only 1 concern - creating a field:

2) Bad SoC, the module has 2 concerns - creating a field and generating new type(s) (side effect in this example):

Don't know if I can explain my point more deeply.

Injecting the args typed like you posted is anyway only half of a solution, because as config you can pass nested assoc arrays to the builder, which you will have to type check anyway.

When you create a php function you can either define a single argument and then users are forced to inject everything in this single argument as an assoc array or you can give users the possibility to define arguments on their own. This is what I propose. If a user can define arguments himself but still injects params as assoc array, then it's his problem.

Creating new GraphQL types in field builders also makes the schema more scattered

@akomm

You are arguing using current naming, nobody seem to disagree, is not good. How things are named currently does not matter, it matters how they will. And my whole explanation was based on the future, not now. Of this future, one part is there, with the old naming still in place. Just ignore it. If you do not disregard the first-class idea, there is no concept separation issue at all.

Didn't understand this part.

You are arguing using wrong naming, ignoring new naming and viewpoint I described. That does not lead to anything.

I explained why type emission on the field perfectly makes sense. Could you please elaborate more deeply why it its a bad concept separation, not disregarding the whole picture I explained?

I meant separation of concerns, not concepts. My mistake, corrected. But I guess you understood what I meant.

Yes I did and my answer was based on this understanding, so what is the point?

Any program should strive to modularity and therefore to a better separation of concerns. This is one of fundamental principles of software engineering. Field builders currently have 2 concerns: building fields and creating new types. "Creating new types" is a side effect here, something what a user wouldn't intuitively expect from a field builder, because it doesn't follow SoC.

Can you stop teaching? This is poison to discussion. Thanks.

1. Good SoC, the module has only 1 concern - creating a field:
   ![](https://camo.githubusercontent.com/3482277efabfb1e276b007524fa7568950660557/68747470733a2f2f73756e392d33362e757365726170692e636f6d2f633835333532382f763835333532383336362f3135376165642f43785478655138657058672e6a7067)

2. Bad SoC, the module has 2 concerns - creating a field and generating new type(s) (side effect in this example):
   ![](https://camo.githubusercontent.com/4df65075f51bfc9cf73dc250d03ffcf12868732e/68747470733a2f2f73756e392d33382e757365726170692e636f6d2f633835333532382f763835333532383336362f3135376166342f635048736e4d77514a4e672e6a7067)

This does not relate to my post in any way as you still disregard most of it, starting at the NAMING of things.

Injecting the args typed like you posted is anyway only half of a solution, because as config you can pass nested assoc arrays to the builder, which you will have to type check anyway.

When you create a php function you can either define a single argument and then users are forced to inject everything in this single argument as an assoc array or you can give users the possibility to define arguments on their own. This is what I propose. If a user can define arguments himself but still injects params as assoc array, then it's his problem.

So you tell the user should not use array configuration but pass everything as a single parameter? Did you try to reflect this on real configuration? This is good depending on whether you are moving in the real or fantasy world. Also your major argument was that its better for typing. I said that in a real world then you would have arrays passed in untyped, so to be consistant, you should also add that you need a hydrator so that the configuration can be passed into the builder as a hydrated object instead of an array. Because the reality of configuration does not exist in the form of plain string/integer/float configuration data. Hell, even your own examples for validation and pager contain array data (for a good reason), so its not really like the user has a choice. So if the point is typing, then it should be followed consistent and complex values should be hydrated. That means you need object types just for the configuration. You see where this goes? This is why I said, the variadic argument spreading for typing is only half of an solution to the problem, but also introduces other problems, as you have noticed yourself. So what is the point of doing it?

Creating new GraphQL types in field builders also makes the schema more scattered

The types and schema you build you actually never directly see, this are the php classes from the graphql library generated. What you do, also now, is giving configuration for generated types. If you stop disregarding my posts, you will remember be posting the object type builder having exactly the same configuration shape as it has now? So where is type configuration scattered?

@mcg-web @murtukov This is maybe something for the 1.0 milestone? I think this is important, but I can't imagine it to fit some feature version. What do you think?

Yes this should be treated in 1.0 indeed. To make millestones easier to follow I also added a deadline.

@mcg-web

The current MappintInterface is internally already used to not only generate fields, but whole types. Example: RelayProcessor.

Does something speak against exposing this type of builder to user?

In the RelayProcessor case, the builder is used via type configuration field, not mutation. And also the builderConfig dos not have to be defined explicitly. The sibling config fields and children of them are the config for the builder.

All the built-in types, could be those builders (object, enum, ...). And user-created builders could be treated the same way. Duplication is now allowed, like it is now already.

The MappingInterface would need to provide additional info, what graphql base-type the new type is based on for the code generator (to generate proper graphql library types). With this approach, you could use low level MappingInterface, but you could also create a new object type by extending the bundles object-builder.

The validation would not be hard coded via extension, but part of the builder. For example @murtukov's symfony validation would not have to be hard coded into extension config, but could be an builder extension for object, that allows additional configs and validates them. One can use this extended, validated builder, or not. Its totally optional.

overblog / GraphQLBundle

Rework all builders #587

What is wrong with builders?

These are 3 points on why we should rework builders.

1. Getting rid of arrays

FieldBuilder

Current implementation:

Suggestion:

Adding validation:

Args Builder

Current Implementation:

Suggestion:

Fields Builder

2. Changing the configuration

3. Type Builder