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 1 commit into
base: 4.3.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 😃

🫡

Copy link

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.

@github-actions github-actions bot added the Stale label May 14, 2024
@devnix
Copy link
Author

devnix commented May 14, 2024

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

Yes, please rebase.

@greg0ire
Copy link
Member

greg0ire commented May 14, 2024

@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.

SenseException
SenseException previously approved these changes May 14, 2024
src/Driver/Mysqli/Result.php Outdated Show resolved Hide resolved
@@ -28,6 +28,7 @@ public function __construct(private readonly mixed $statement)

public function fetchNumeric(): array|false
{
/** @var non-empty-list<mixed>|false $row */
$row = @db2_fetch_array($this->statement);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@devnix devnix May 15, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, thank you!

@github-actions github-actions bot removed the Stale label May 15, 2024
Copy link

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.

@github-actions github-actions bot added the Stale label Aug 14, 2024
@devnix
Copy link
Author

devnix commented Aug 14, 2024

Yup, I'll want to finish a couple of contributions to PHPStan and Psalm first related to this pull request 😄

@derrabus derrabus changed the base branch from 4.1.x to 4.2.x August 15, 2024 09:19
@derrabus derrabus changed the base branch from 4.2.x to 4.3.x October 10, 2024 13:11
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.

6 participants