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
252 stars 17 forks source link

Dibi - return type extension #474

Open jakubvojacek opened 1 year ago

jakubvojacek commented 1 year ago

Hello @staabm

I created the dibi return extension which returns array{...} for fetch and array<arary{...}> for fetchAll().

However, by default, dibi returns not array but \Dibi\Row (https://github.com/dg/dibi/blob/master/src/Dibi/Row.php#L22) which is just a crate-like class wit magic properties.

I was thinking that in order to allow the dibi static analysis to be usefull for wider range of programmers, both should be supported.

Perhaps new constat DibiConnectionFetchDynamicReturnTypeExtension::RETURN_TYPE = '\Dibi\Row' could be added (which coudl be overwritten in phpstan-dba-bootstra.php to DibiConnectionFetchDynamicReturnTypeExtension::RETURN_TYPE = 'array') and return the return type based on this variable.

I tried it via

// for fetch
return new GenericObjectType(\Dibi\Row::class, [$resultType]);

however that didnt have desired results

$app = $this->database->fetch('SELECT apps_id, apps_name from apps where apps_id = %i', $appsId);
echo $app->unknown;

as the above code didnt throw phpstan exception that unknown isnt property of Row class.

I will be happy to create pull request but I am lost for now as I couldnt force phpstan to return correct Type.

Could you please guide me?

staabm commented 1 year ago

Do the dibi methods really return a array or does it always return a Row object which implement ArrayAccess and therefore can be accessed like a regular array?

In other words: maybe we just need to make it return a generic Row object - and we don't need a config flag?

jakubvojacek commented 1 year ago

Its setupable via setRowFactory. It could return either

    /**
     * Fetches the row at current position, process optional type conversion.
     * and moves the internal cursor to the next position
     * @return Row|array|null
     */
    final public function fetch()
    {
        $row = $this->getResultDriver()->fetch(true);
        if ($row === null) {
            return null;
        }

        $this->fetched = true;
        $this->normalize($row);
        if ($this->rowFactory) {
            return ($this->rowFactory)($row);
        } elseif ($this->rowClass) {
            return new $this->rowClass($row);
        }

        return $row;
    }
jakubvojacek commented 1 year ago

Seems like you already tried to discuss this with ondrejmirtes here https://github.com/phpstan/phpstan/issues/2923 a it is not yet implemented

staabm commented 1 year ago

the mentioned issue is related to how stdClass works. for concrete classes we don't have this problem.

see e.g. https://github.com/staabm/phpstan-dba/blob/dc579c0c10c9522b9a321bd007a5309d7ab9b876/tests/default/data/pdo.php#L18 in which you can see we support a similar thing but for PDOStatement.

AFAIR we need a generic stub for your object in https://github.com/staabm/phpstan-dba/blob/main/config/stubFiles.neon and if we have that it should be possible todo new GenericObjectType(\Dibi\Row::class, [$resultType]);.

if we make the return type extension return a Union of array<...>|Row<...> this might even work without the need for a new config switch

jakubvojacek commented 1 year ago

i tried copying the stub from pdoStatement.stub, changing namespace & classname but still didnt work.

<?php

namespace Dibi;

/**
 * @template rowType
 *
 * @implements Traversable<int, rowType>
 * @implements IteratorAggregate<int, rowType>
 *
 */
class Row implements Traversable, IteratorAggregate
{

}
$app = $this->database->fetch('SELECT apps_id from apps where apps_id = %i', $appsId);

echo $app->test;
echo $app['test'];
echo $app->apps_id;
echo $app['apps_id'];       

there should be reported twice that test does not exists on Dibi\Row. Can you please help me with the stub?

staabm commented 1 year ago

please open a PR with what you have. otherwise its just guesswork on my end

patrickkusebauch commented 8 months ago

Just to chime in with more info.

Dibi returns Dibi\Row by default. Dibi\Row implements ArrayAccess, it is also an UniversalObjectCrate meaning it has dynamic properties whose names correspond to the query result key(most of the time column name) names.

However, the return behavior is configurable.

There are 2 places: