Allow constructor fields to use middleware

[oprypkhantc]

Hey @oojacoboo. This is a draft, but I wanted you to take a look first before I get to fix the tests and polish this. There are no new tests as of now.

This is the minimum fix needed to close #565 and allow constructor fields to have middleware, allowing this kind of code:

#[Input]
class SomeType {
    public function __construct(
        #[Field]
        #[SomeMiddleware]
        public readonly string $field,
    ) {}
}

What I did is I refactored resolvers a bit to make sure a single input middleware could be used for both constructor and "set" properties. This is done by requiring resolvers to always return the value that will be set if a parameter is fed into a constructor, as opposed to being set directly through the property.

[oojacoboo]

@oprypkhantc I'm wondering why you've gone with a middleware on a field? Why would we not define a global hydration middleware and let that be it? If there is a need to have field level middleware, that's something that can be implemented via the global middleware in userland. Does SomeMiddleware implement an interface?

I haven't had time to look through the code in detail, as I'm currently out of town. But, I'm assuming it's strictly using reflection now? Can you detail the supported variations for hydration? Is the constructor actually called?

[oprypkhantc]

@oprypkhantc I'm wondering why you've gone with a middleware on a field? Why would we not define a global hydration middleware and let that be it? If there is a need to have field level middleware, that's something that can be implemented via the global middleware in userland. Does SomeMiddleware implement an interface?

I haven't had time to look through the code in detail, as I'm currently out of town. But, I'm assuming it's strictly using reflection now? Can you detail the supported variations for hydration? Is the constructor actually called?

With "middleware" I was referring to the existing InputFieldMiddlewareInterface that we have for modifying input fields. SomeMiddleware is just a regular field middleware annotation implementing MiddlewareAnnotationInterface. Previously, you could not use middleware for fields hydrated through the constructor; now you can.

The hydration is unchanged: any fields that are also defined as constructor parameters are passed into the constructor, the rest is set directly by setting the property publicly, no reflection.

[oojacoboo]

Okay, I see. How does this address the rest of the concerns discussed on #565?

[oprypkhantc]

It doesn't address most of them. I tried to approach all of the concerns raised in the issue, but so far I've decided that some points probably don't make sense, while others are harder to implement than initially anticipated.

Points from that discussion:

• hydrating readonly properties (except for constructor promoted) is unchanged and results in an error. This change is non-controversial and relatively trivial to implement, but will need some work on resolvers. Not part of this PR.
• marking property private (again except for constructor) won't work. There are comments in the code saying this is intentionally not supported, so I believe we should leave it at that, at least for now.
• constructor hydration will now work correctly with all the middleware. It still won't work with the "update: true" flag of input types, but there's nothing that can be done here
• treating null value and "optional" field (i.e. missing key in the request) as separate concepts again I didn't do as we've decided it was out of scope. I implemented that separately in a different library.
• lastly, the custom hydration. I attempted to implement that, but quickly realized I don't actually know why we even need this :) One of the cases that custom hydration middleware was meant to solve was proper constructor hydration, and it is now solved with this PR. I'm not aware of any other use cases, hence it's hard to predict what needs (or doesn't need) to be built here. It sounds good in theory, but I'd rather wait and gather other people's concerns and only then implement something

[oojacoboo]

@oprypkhantc this PR seems fine. I was confused though, because it doesn't close #565, and really doesn't address the main concerns of that ticket.

Custom hydration middleware offers the ability for someone to implement any logic deemed necessary, prior to, or as part of, hydration of an input type. Validation is a clear example of why someone might want to do this. But surely there are other possible reasons.

A custom middleware isn't really that difficult. Basically we'd ship the default hydrator that'd be sufficient for most use cases, only check the config to see if a custom one has been provided and use that instead.

If we move to using reflection for constructor properties, to get around the private and readonly limitations, and don't call the constructor directly, if someone does want to execute the constructor as part of hydration, they'd have to resort to a custom hydrator. This allows for us to solve the issues presented in #565, while still providing some solution for BC support.

[oprypkhantc]

@oprypkhantc this PR seems fine. I was confused though, because it doesn't close #565, and really doesn't address the main concerns of that ticket.

Sorry for the confusion 🥲

A custom middleware isn't really that difficult. Basically we'd ship the default hydrator that'd be sufficient for most use cases, only check the config to see if a custom one has been provided and use that instead.

Unfortunately it's not as easy as that. As of right now, input field resolvers set the property directly or call a setter method. This means that if we do ship some sort of hydration customization, it won't be possible to overwrite literally half of the process - you either call a resolver and the property gets set by graphqlite, or you don't call it and then you don't get the value you need.

To me that's not a good or a complete solution. And to fix that, input resolvers shouldn't be setting the property directly, but rather just returning the value so it can be passed through to the hydrator, which in turn can do anything with it. But the problem here is that now the hydrator must also know which fields are set directly through properties, which fields have setters associated with them and which fields are magic properties.

All this complicates the hydration a lot, so I didn't want to deal with all that as well.

If we move to using reflection for constructor properties, to get around the private and readonly limitations, and don't call the constructor directly, if someone does want to execute the constructor as part of hydration, they'd have to resort to a custom hydrator. This allows for us to solve the issues presented in #565, while still providing some solution for BC support.

Since creating that issue I actually changed my mind regarding constructor hydration. I no longer want hydration to omit the constructor, which is why I'm PRing these changes - to allow using the constructor at all. Of course having custom hydration support is always better than not having it; but I believe the existing hydration (given this PR and fixes to setting readonly/private properties) is sufficient to satisfy 99% users, including me.