staabm / phpstan-dba

PHPStan based SQL static analysis and type inference for the database access layer
https://staabm.github.io/archive.html#phpstan-dba
MIT License
248 stars 18 forks source link

Nette/dibi driver support #462

Open jakubvojacek opened 1 year ago

jakubvojacek commented 1 year ago

Hello

I tried modifying phpstan-dba to add support to either https://github.com/dg/dibi or https://github.com/nette/database (both are very very similar in terms of methods, almost interchangeable).

The syntax is mainly

$connection = new \Dibi\Connection([...]);
// $connection->getDriver()->getResource() returns instance of \mysqli
$connectoin->query('SELECT * from users where users_name = ?', $username)->fetch();

i tried copying stuff from mysqli/pdo driver but they are different, they execute the query in 2 steps (prepare, execute) while dibi/nette does that in one method that accepts N-parameters.

Could you please guide me a bit how to add basic support for this?

Thanks a lot Jakub

staabm commented 1 year ago

Hi Jakub,

great to see people interessting in adding support for new db apis.

to add a new api to phpstan-dba you need 2 things: 1) adding the query-string taking methods to the syntax error detection rule by adding full qualitfied method in https://github.com/staabm/phpstan-dba/blob/0f46ee86370633d8a69eaf5cdb5319af496df2ed/config/dba.neon#L15-L91

2) adding type inference via PHPStan ReturnTypeExtensions. you can take inspiration from already existing extensions. I am not at all familiar with any of the proposed apis, but I think they look similar to the already existing Doctrine DBAL stuff (which also separates the connection and the result object). see

I would suggest doing these 2 things in separate PRs. starting with 1) which is the easier step.

feel free to open a PR with your attemps, so I can give you more concrete hints

jakubvojacek commented 1 year ago

Well 1) seems easy (I already kind of did that when I added those methods in services in the neon file) when I was testing, afaik the it would be this

🐞  jakubvojacek@~/Sites/phpstan-dba $ git diff
diff --git a/config/dba.neon b/config/dba.neon
index 2542a3d..250bdda 100644
--- a/config/dba.neon
+++ b/config/dba.neon
@@ -49,6 +49,7 @@ services:
                 - 'mysqli::execute_query#0'
                 - 'Doctrine\DBAL\Connection::query#0' # deprecated in doctrine
                 - 'Doctrine\DBAL\Connection::exec#0' # deprecated in doctrine
+                - 'Dibi\Connection::query#0'

     -
         class: staabm\PHPStanDba\Rules\SyntaxErrorInQueryFunctionRule
@@ -89,3 +90,4 @@ services:
                 - 'mysqli::execute_query#0'
                 - 'Doctrine\DBAL\Connection::query#0' # deprecated in doctrine
                 - 'Doctrine\DBAL\Connection::exec#0' # deprecated in doctrine
+                - 'Dibi\Connection::query#0'

but its hard for me to say that I didnt miss anything since I cannot test it properly without 2).

I will try looking at the samples that you sent and in case I manage somehow to make it work, I will open the PRs

Thanks again

jakubvojacek commented 1 year ago

nevermind my previous post. The problem with Dibi\Connection::query is that it does not return any result, it requires another method to be called upon the query result

$connection->query('...')->fetchAll();
$connection->query('...')->fetchSingle();
$connection->query('...')->fetchAssoc('xxx');

and to write support for that does not seem as easy. But dibi also supports

$connection->fetchAll();
$connection->fetchSingle();
$connection->fetchAssoc();

which returns the desired results already. in that case 1) would be something like

🐞  jakubvojacek@~/Sites/phpstan-dba $ git diff
diff --git a/config/dba.neon b/config/dba.neon
index 2542a3d..f15bcae 100644
--- a/config/dba.neon
+++ b/config/dba.neon
@@ -49,6 +49,11 @@ services:
                 - 'mysqli::execute_query#0'
                 - 'Doctrine\DBAL\Connection::query#0' # deprecated in doctrine
                 - 'Doctrine\DBAL\Connection::exec#0' # deprecated in doctrine
+                - 'Dibi\Connection::fetch#0'
+                - 'Dibi\Connection::fetchSingle#0'
+                - 'Dibi\Connection::fetchAssoc#0'
+                - 'Dibi\Connection::fetchAll#0'
+                - 'Dibi\Connection::fetchPairs#0'

     -
         class: staabm\PHPStanDba\Rules\SyntaxErrorInQueryFunctionRule
@@ -89,3 +94,8 @@ services:
                 - 'mysqli::execute_query#0'
                 - 'Doctrine\DBAL\Connection::query#0' # deprecated in doctrine
                 - 'Doctrine\DBAL\Connection::exec#0' # deprecated in doctrine
+                - 'Dibi\Connection::fetch#0'
+                - 'Dibi\Connection::fetchSingle#0'
+                - 'Dibi\Connection::fetchAssoc#0'
+                - 'Dibi\Connection::fetchAll#0'
+                - 'Dibi\Connection::fetchPairs#0'
🐞  jakubvojacek@~/Sites/phpstan-dba $ 

and then I'd have to create rules for each of those methods, something like

final class DibiConnectionFetchDynamicReturnTypeExtension implements DynamicMethodReturnTypeExtension
{
    public function getClass(): string
    {
        return \Dibi\Connection::class;
    }

    public function isMethodSupported(MethodReflection $methodReflection): bool
    {
        return \in_array(strtolower($methodReflection->getName()), ['fetch'], true);
    }

    public function getTypeFromMethodCall(MethodReflection $methodReflection, MethodCall $methodCall, Scope $scope): Type
    {
        // some logic that will return the type of the single column (via something like inferType) that the user selected (nullable because the query might return zero rows)
    }

}

right?

Please excuse my "stupidness", I've never contributed to phpstan before so it might take some time before I'm up to speed.

jakubvojacek commented 1 year ago

I am getting somewhat close to finishing stage two (will have to write some tests) but I got the feeling you won't like some parts of that :) but works like a charm so far

staabm commented 1 year ago

I am getting somewhat close to finishing stage two

great news.

you won't like some parts of that :) but works like a charm so far

thats totally fine. I am looking forward to it.

patrickkusebauch commented 5 months ago

Just a thought.

You could implement Dibi\Connection::query by templating Dibi\Result. Something like:

namespace Dibi

class Connection
{
  /**
   * @template args
   * @param args $args
   * @return Result<args, Dibi\Row::class>
   */
  public function query(mixed ...$args): Result {}

}

/**
 * @template args
 * @template rowType
 */
class Result
{
  /**
   * @return self<args, $class>
   */
  public function setRowClass(?string $class): self {}
}

Having the template on the Dibi\Result class with the arguments would allow you to get the template values in the DynamicMethodReturnTypeProvider to do the same analysis as is done on Dibi\Connection with the added benefit of setting the correct result class.