Custom types without array configuration

[enumag]

The number one thing that I do not like about this library is the usage of arrays for configuration. From what I've heard from my friends who have more experience with GrpahQL in PHP they have ultimately chosen Youshido for this very reason.

Instead of this from the documentation

class UserType extends ObjectType
{
    public function __construct()
    {
        $config = [
            'fields' => function() {
                return [
                    'email' => MyTypes::string(),
                    'friends' => MyTypes::listOf(MyTypes::user())
                ];
            }
        ];
        parent::__construct($config);
    }
}

I'd much rather define my types like this:

class UserType extends ObjectType
{
    public function getFields(): array
    {
        return [
            'email' => MyTypes::string(),
            'friends' => MyTypes::listOf(MyTypes::user())
        ];
    }

    // getDescription, getInterfaces etc.
}

When I looked into the ObjectType I noticed that it already has methods like these. In my opinion we should add an ObjectTypeInterface and have the ObjectType class implement it. In the code wherever an instance of ObjectType is required it should instead require the interface. Then anyone could extend that interface to define their types without nasty arrays.

There could also be an AbstractObjectType class implementing methods that you dont always need to define yourself such as getInterfaces returning an empty array.

Similar thing should be done for FieldDefinition though it should no longer have public properties like args. There should be a getArguments method instead. This will be a bit more tricky to do and would probably involve deprecating the FieldDefinition class in favor of a completely new FieldInterface with some proxy implementation for the FieldDefinition class.

I saw a Youshido-based application where not just every type but also every field had its own class and it looked very good. I want to achieve the same with webonyx.

This could be a better alternative to #70 and #74.

[vladar]

We considered adding it in #78 but the more I think about it the more I incline not to introduce any base classes or interfaces. There is a good reason why smart people prefer composition over inheritance.

Interfaces are fragile and hard to modify. Say we have an interface in the library and many projects start using it. Then at some point, we decide to add an optional method to this interface or even rename a method. It will introduce an unnecessary breaking change. An alternative here is to add new interfaces for each minor tweak.

With data structures instead of interfaces, we can provide reasonable defaults ourselves and avoid many breaking changes. We can also do much smoother deprecations.

Another problem is that having two ways of defining types makes maintenance much harder. It also introduces different experience for library users and makes it harder to communicate and solve issues.

That's why I am actually re-considering if we need #74 at all (even though it's just a syntax sugar).

The way you want to define your types is quite possible with composition. Just use your own classes and have a separate factory which builds actual definitions. For example:

class UserType
{
    public function getFields(): array
    {
        return [
            'email' => MyTypes::string(),
            'friends' => MyTypes::listOf(MyTypes::user())
        ];
    }

    // getDescription, getInterfaces etc.
}

class MyTypes
{
    private $types = [];

    function user()
    {
        // obviously you can automate this for all types:
        $myUserType = new UserType();
        return $this->types['user'] ?? $this->types['user'] = new ObjectType([
            'name' => $myUserType->getName(),
            'fields' => [$myUserType, 'getFields'],
            'myClass' => $myUserType, // just in case if you'll need a reference later
        ]);
    }
}

Actually some framework wrappers (like laravel graphql) do exactly this.

I do understand that PHP is heavily infected by OOP, but it doesn't mean that it is the only right way to do things %)

[enumag]

Yeah I know I can do it the way you describe but I don't consider it a good solution - it would only add unnecessary transformations everywhere slowing down the application. I've already heard claims that webonyx is in fact much slower than Youshido (but they don't have the benchmark sources anymore so I can't validate that claim) so I really don't want to add more layers to slow it down even more. And the code would look very ugly in some cases where it would not be possible to abstract.

Firstly let me correct my previous comment a bit - when I talked about abstract class implementing the optional methods like getDescription and getInterfaces it should not be an abstract class but actually a trait. So custom types would look like this:

class UserType implements ObjectTypeInterface
{
    use ObjectTypeTrait; // empty getDescription and other optional methods

    public function getFields(): array
    {
        return [
            'email' => MyTypes::string(),
            'friends' => MyTypes::listOf(MyTypes::user())
        ];
    }

    // ...
}

Interfaces are fragile and hard to modify. Say we have an interface in the library and many projects start using it. Then at some point, we decide to add an optional method to this interface or even rename a method. It will introduce an unnecessary breaking change. An alternative here is to add new interfaces for each minor tweak.

While true at first glance this is not actually the case. There is an approach to make such additions very smooth. When you need to add an optional method you add it but comment it out, implement it in the trait with default value and add a note to the change log that implementations of that interface that do not implement such method are deprecated. In the code you would use method_exist for now before calling it. Then you just uncomment the method in the next major release and remove the method_exist checks. This could break some applications of course, however since 99% of all implementations would use the trait anyway they would be uneffected because the trait would define it for them. This is how Symfony does BC breaks by the way. It works very well.

Or you can add a new interface. Which way is better depends on the case.

Besides the cases where you actually want to add such optional method are quite rare. It would certainly not be necessary for "every minor tweak" as you put it. In the lifetime of this library I don't expect more than one or two such methods to be added.

Also I should note that this library is still in 0.x.y versions which is a pre-release stage and BC breaks can occur according to semver. Still it's a good practice to make them smooth as I described above.

There is a good reason why smart people prefer composition over inheritance.

That's exactly why I'm proposing this. To use composition to its full potential you need interfaces defining the contract between classes so that you can replace any class with your own implementation if necessary. This library is very bad in this regard.

I want to achieve a state where a Type definition uses composition for the fields instead of defining them by itself. Yes I could compose it using some factories and automate it as you described but its not a clean solution that I can recommend to others. If this library had some interfaces then I could write an article with recommended usage of this library and the code would look very good.

Another problem is that having two ways of defining types makes maintenance much harder. It also introduces different experience for library users and makes it harder to communicate and solve issues.

That's why this library should ultimately deprecate the current array configuration in favor of the interfaces and completely remove arrays in some future version.

[enumag]

I see now that #78 is exactly the same proposal. I don't understand why you're against it now since you originally proposed it yourself. It is the right direction, just interfaces and traits should be used instead of abstract classes as suggested by @tpetry. Your worry about extending the interfaces in the future is baseless - I guarantee you it would be needed very very rarely and can be perfectly smooth as described above.

[vladar]

I don't understand why you're against it now since you originally proposed it yourself

Because I had quite bad experience since then maintaining interfaces. Commenting methods in them is a nice workaround but it just proves the point. Also, you haven't covered method renamings and signature changes.

I don't consider it a good solution - it would only add unnecessary transformations everywhere slowing down the application.

How much will it slow down the application? It may have some memory overhead, but in general, with type lazy loading, I do not think it will be significant (claiming otherwise requires some evidences for rational decision making, otherwise it sounds like premature optimization).

This solution is actually good enough. Yes, it is more of a functional approach than OOP, but it is OK (even if you don't like it).

I've already heard claims that webonyx is in fact much slower than Youshido

Claims, really? If you have doubts - do your benchmarks. But keep in mind that there are at least 3 stages - parsing, validation, and execution. In order to compare apples to apples, you must have all 3 stages feature-complete in both libs, properly configured (lazy type loading?) which is not the case. Otherwise, you would probably pick the other option, right?

And the code would look very ugly in some cases where it would not be possible to abstract.

Seeing real-world examples would be nice?

Also I should note that this library is still in 0.x.y versions which is a pre-release stage

It just mirrors versioning of the reference implementation to simplify understanding of compatibility between them.

To use composition to its full potential you need interfaces defining the contract between classes so that you can replace any class with your own implementation if necessary.

Except that providing such contracts we also establish a fragile public API which we'll have to respect for BC. We do not provide such interfaces intentionally.

Instead, we split the processing into steps of data transformations and if you need something - you can do it yourself between those steps. Other things you can solve with composition (like resolver composition). It actually allows us moving without major breaks.

I want to achieve a state where a Type definition uses composition for the fields instead of defining them by itself

I would really prefer an example of what you are trying to do which is impossible now. That would help much more than such an abstract discussion. For now, I see only your personal preferences.

One thing you should understand is that this lib is a direct port of graphql-js. Moving too far away from it means it will be much, much harder to follow. And that's one of the main reasons why we were able to port most of their features (and there are lots of them). Deviating significantly means making lib maintenance orders of magnitude harder.

We are quite open to helping solve real problems, but in order to do so, please start by providing real examples of what you couldn't do with the lib. The fact that you don't like the solution proposed is just not good enough reason for a change (doesn't pass cost-benefit analysis).

Also, note that the issue #78 is still open because I am still waiting for a real-world data of benefits from it. If we see significant performance benefits on real-world projects or something major that really can't be done with current implementation and can be done with interfaces - we will move forward, but I haven't seen enough evidence yet that we really need it.

To summarize, give us some numbers/examples.

[enumag]

I'l take a step back, reconsider my opinions and respond properly later. Thank you for taking your time to write such detailed response.

[vladar]

Closing as dupe of #78 We'll continue this discussion there.