Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal of return types for Result interface #6287

Open
wants to merge 10 commits into
base: 4.1.x
Choose a base branch
from

Conversation

devnix
Copy link

@devnix devnix commented Jan 30, 2024

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?

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Author

devnix commented Jan 31, 2024

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

(╯°□°)╯︵ ┻━┻

@devnix devnix force-pushed the better-result-types branch 2 times, most recently from 2b442be to af7fd28 Compare January 31, 2024 07:54
@derrabus
Copy link
Member

derrabus commented Jan 31, 2024

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
Copy link
Author

devnix commented Jan 31, 2024

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
Copy link
Member

derrabus commented Feb 1, 2024

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

@derrabus derrabus changed the base branch from 4.0.x to 4.1.x February 3, 2024 20:39
@devnix devnix marked this pull request as ready for review February 4, 2024 17:27
@devnix
Copy link
Author

devnix commented Feb 4, 2024

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!

src/Driver/FetchUtils.php Outdated Show resolved Hide resolved
src/Driver/PgSQL/Result.php Outdated Show resolved Hide resolved
@devnix
Copy link
Author

devnix commented Feb 5, 2024

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

src/Driver/PgSQL/Result.php Outdated Show resolved Hide resolved
src/Portability/Converter.php Outdated Show resolved Hide resolved
@devnix devnix requested a review from derrabus February 5, 2024 10:01
@devnix
Copy link
Author

devnix commented Feb 13, 2024

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

@derrabus
Copy link
Member

if you want to approve the workflows 😃

🫡

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants