Proposal: Generic types for DTO casting

[michaeldyrynda]

I've added annotations for the dto and dtoOrFail methods, enabling passing of a class-string to each method, in order to get proper type suppose when working with the response.

The new $dto parameter is optional to enable backwards-compatibility, but means that you can call the method with a FQCN passed as an argument, in order to get type completion.

Omitting the argument continues as currently.

I was inspired by @ash-jc-allen's blog post, which passed the FQCN to this method, which initially looked unnecessary to me.

public function getRepo(string $owner, string $repo): Repo
{
    return $this->client()
        ->send(new GetRepo($owner, $repo))
+       ->dtoOrFail(Repo::class);
}

This implementation conforms to PHPStan requirements, so I believe it to be technically correct, but I wasn't actually able to verify it in my editor (Neovim).

For it to function (for me), I had to tweak the annotation slightly:

  * @template T of object
+ * @param class-string<T>|null $dto
- * @return ($dto of class-string<T> ? T : object)
+ * @return T

[juse-less]

@michaeldyrynda Have you tried with both Intelephense and Phpactor?

My experience is that Phpactor works far better.

It shouldn't be the reason, but I also realise that the return type of the DTO methods is mixed rather than object.
Have you tried change it to object, as well as your conditional fallback return type to mixed?

In terms of changing this type, static analysis tools will start failing for those of us who use custom responses and override the DTO methods directly.
I'm not sure if that strictly qualifies as a breaking chance, though.

I have actually managed to solve DTOs, by defining them in the request, and have them propagate to the DTO methods in the response.
But PhpStorm (that I primarily tested in) doesn't work with cross-file generics like that.
So I gave up rather than propose it.
PHPStan was satisfied, though, and I could perfectly fine use \PHPStan\dumpType() the correct type from the DTO methods.

I can commit and push it if someone is interested anyway.

[ash-jc-allen]

Hey all! I was just looking through my blog post and agree that it was unecessary for me to add that parameter. I think it was a typo on my end that slipped through my proof reading haha 🤦

Although, I do like the idea of getting some kind of autocomplete and static analysis in my IDE, so I'd be a fan of this addition 🙂

[michaeldyrynda]

I have actually managed to solve DTOs, by defining them in the request, and have them propagate to the DTO methods in the response.

Agree; I typically use resource-based SDK builds like the docs suggest, so do my typed returns on the connector, rather than on the request itself.

I've tried with phpactor and it makes no difference sadly (though I'll be making the switch to that permanently regardless, it provides better diagnostics than intelephense!), which is a shame.

Wish there was a better way of handling this, as having to annotate things by hand is grim.

[juse-less]

I did some tests with the proposed solution, and it works with Phpactor in Neovim, albeit with some adjustments.
I haven't tried Intelephense, though.

I set up a fresh Neovim using NvChad, and then just installed Phpactor.

Interestingly enough, PhpStorm doesn't handle it at all.
But it's also in-line with my experience of the two - Phpactor being far more PHP-intelligent than PhpStorm.

Screenshots of relevant code in Neovim and PhpStorm

Neovim:

PhpStorm:

[michaeldyrynda]

Well I'll be a monkey's uncle; I tried this again today and it seems to be working as per the PR.

Unfortunately, making the change as @juse-less suggested raises the ire of PHPStan.

So it would seem that the annotation is correct, and functions with both PHPStan and Intelephense. I believe there's a PHPStan plugin for PhpStorm that should enable intelligence for this?

As it stands, the PR is correct, appeases static analysis, as well as completion for the two intelligence tools. Given that the annotations are PHPStan ones, I think it's reasonable for it to only work in that context.

Whether or not this is considered breaking, however, is another story.

[juse-less]

Ohh. Right.
I believe it's complaining when passing null (or nothing), because T doesn't exist without the class string.
So the check should be to check if $dto is null, and, if not, then return T.

/** @return ($dto is null ? object : T) */

I've always gotten complaints that the template has to be used on the param.
So interesting that it works.
It's entirely possible it's changed more recently and I just haven't checked.

In terms of PHPStan in PhpStorm, it's bundled and didn't make a difference in my screenshot.
But that plugin seems quirky. For things like class-string, you need the bundled Psalm plugin to be active, f.ex.
I do have both active, but no dice.

If it works as proposed, then I don't really see any issues with it per se.
Just that PhpStorm won't have autocompletion.

[Sammyjo20]

Hey @michaeldyrynda Thank you so much for this PR, I really like this idea and thank you for contributing too @juse-less ❤️

How do we feel about implementing a type check so if someone accidentally puts a different class in, it throws an error? Or even passing the class as a second argument like:

createDtoFromResponse(Response $response, string $classString)

The latter could open up cool DTO casting like match statements based on the class string 🤔 What do you think? I'm happy to keep it simple though