Proposal of return types for Result interface

[devnix]

Q	A
Type	improvement
Fixed issues

Summary

Taking advantage that the next major version is very near, I thought this could be a good addition (maybe a little too late but I just realized about this part of the API.

This draft contains modifications only on the interface just to get your feedback about the correctness of the types, and if it could be a nice addition for 4.0. Also, there could be a couple more of places that could be better typed, WDYT?

[Ocramius]

@devnix one thing I realized here: you may get resource objects, in case of LOBs? 🤔

That said, this is a good move forward, and the time is good to introduce it now (rather than when the major release is finalized)

[devnix]

@devnix one thing I realized here: you may get resource objects, in case of LOBs? 🤔

(╯°□°)╯︵ ┻━┻

[derrabus]

I'm not sure, I want to make any guarantees on possible data types returned by our drivers or middleware, tbh. Database-related PHP extensions have a history of getting more type-aware release after release. And if someone decided to write a piece of middleware that let's say decodes JSON on the fly, why should we forbid that?

mixed still feels like our best option here.

I like the proposed non-empty- types though.

[devnix]

I've added another non-empty- and I think that should be all.

Now I can take a look at each implementation. In the cases where the types are not guaranteed but we assume that they are correct, should we force the type with @var, or should we implement some kind of runtime type check @derrabus?

[derrabus]

If rearranging code without runtime impact (!) makes the static analysis happy, let's do that. Otherwise annotations are fine.

[devnix]

I think these changes are correct, if there is any other API I can improve in this PR let me know! I observed some Result classes that with another internal implementation could be type safer, but I preferred not to alter the current implementation that much!

[devnix]

Not sure what's up with those Invalid inline documentation comment format "@var (non-empty-list<mixed> | false)", expected "@var type $variableName Optional description". 🤔

[devnix]

Little ping here @derrabus, I think all the requested changes are done, if you want to approve the workflows 😃

[derrabus]

if you want to approve the workflows 😃

🫡

[github-actions]

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

[devnix]

I can rebase and solve conflicts, but I don't know if there is interest in merging this pull request, feedback would be welcome 🙂

[derrabus]

Yes, please rebase.

[greg0ire]

@devnix the changes look good. Assuming this is no longer a draft, I think there should be fewer commits with better messages… for instance, a short explanation making it more obvious why using non-empty- is the way to go would be nice. There are commits that should be squashed into other commits, such as the one that amends the CS config file, or the one that removes debugging leftovers. In case you intend us to squash merge this, please do the squash yourself, and give the final commit a good message.

Side note: I opened #6395, it should make the weird Codecov comments disappear.

[derrabus]

Can we contribute this upstream to the function stubs of PHPStan and Psalm? Overriding all those return types doesn't feel right.

[devnix]

I guess you are referring also to db2_fetch_assoc(), pg_fetch_row(), pg_fetch_assoc(), pg_fetch_all(),SQLite3Result::fetchArray(), right? I can open a pull request in both projects. I'll link them below.

[derrabus]

Yes, thank you!