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

Add missing namespace import in MysqlClient #193

Merged
merged 1 commit into from
Apr 28, 2024

Conversation

bwaidelich
Copy link
Contributor

@bwaidelich bwaidelich commented Mar 20, 2024

Adds a missing use statement for the PromiseInterface type to MysqlClient. Without that change, using the API leads to type warnings:

function someMethod(): PromiseInterface {
  return $this->mysql->query(...);
}
// Return value is expected to be '\React\Promise\PromiseInterface', '\React\Mysql\PromiseInterface' returned

image

Adds a missing `use` statement for the `PromiseInterface` type to `MysqlClient`.
Without that change, using the API leads to type warnings:

```php
function someMethod(): PromiseInterface {
  return $this->mysql->query(...);
}
// Return value is expected to be '\React\Promise\PromiseInterface', '\React\Mysql\PromiseInterface' returned
Copy link
Contributor

@SimonFrings SimonFrings left a comment

Choose a reason for hiding this comment

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

Hey @bwaidelich, thanks for bringing this up 👍

Actually an interesting one. We would normally check these types by running our test suite with an static analysis tool like PHPStan, similar to reactphp/promise#246, reactphp/async#77 and others.

In my opinion, PHPStan will be an important topic once we're looking into a major version bump where we remove support for part of the legacy PHP versions (like we're currently doing with ReactPHP v3, see https://github.com/orgs/reactphp/discussions/481) We're still supporting legacy PHP versions (< PHP 7.1) in this project, so we have to resort to type definitions in docblocks instead of using native PHP types for now and thus need some workarounds for old PHP.

Reviewing your description, I think your changes are an improvement to this project and this is why I'd approve this pull request. I would really like to see some tests for this, but like I said above, I'm not sure if this is something to look into right now or a topic for another day 👍

Interested in what @WyriHaximus and @clue think about this.

@bwaidelich
Copy link
Contributor Author

@SimonFrings you got me

In my opinion, PHPStan will be an important topic

=> #195

@devnix
Copy link

devnix commented Apr 21, 2024

I was about to do this same PR: the documented returned class does not exist.

@bwaidelich
Copy link
Contributor Author

FYI: I'm in the process of cleaning up my pending PRs. I'll go ahead and close this one in the next couple of days if there is no more feedback

@WyriHaximus
Copy link
Member

Hey @bwaidelich, thanks for bringing this up 👍

Actually an interesting one. We would normally check these types by running our test suite with an static analysis tool like PHPStan, similar to reactphp/promise#246, reactphp/async#77 and others.

In my opinion, PHPStan will be an important topic once we're looking into a major version bump where we remove support for part of the legacy PHP versions (like we're currently doing with ReactPHP v3, see https://github.com/orgs/reactphp/discussions/481) We're still supporting legacy PHP versions (< PHP 7.1) in this project, so we have to resort to type definitions in docblocks instead of using native PHP types for now and thus need some workarounds for old PHP.

Reviewing your description, I think your changes are an improvement to this project and this is why I'd approve this pull request. I would really like to see some tests for this, but like I said above, I'm not sure if this is something to look into right now or a topic for another day 👍

Interested in what @WyriHaximus and @clue think about this.

+9001, PHPStan is already helping our packages a lot. It is only natural we expand that effort beyond to our non-core packages once v3 is out.

@WyriHaximus
Copy link
Member

FYI: I'm in the process of cleaning up my pending PRs. I'll go ahead and close this one in the next couple of days if there is no more feedback

@bwaidelich I'll ping @clue, this should be ready to merge at this point

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@clue clue left a comment

Choose a reason for hiding this comment

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

@bwaidelich Thanks for looking into this, changes LGTM! :shipit:

@clue clue added this to the v0.7.0 milestone Apr 28, 2024
@clue clue merged commit b01e1ce into friends-of-reactphp:0.7.x Apr 28, 2024
13 checks passed
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.

5 participants