friends-of-reactphp / mysql

Async MySQL database client for ReactPHP.
MIT License
331 stars 66 forks source link

Add missing namespace import in `MysqlClient` #193

Closed bwaidelich closed 3 months ago

bwaidelich commented 4 months ago

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

bwaidelich commented 4 months ago

@SimonFrings you got me

In my opinion, PHPStan will be an important topic

=> #195

devnix commented 3 months ago

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

bwaidelich commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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