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

Dibi - return type extension #474

Open
jakubvojacek opened this issue Nov 17, 2022 · 7 comments
Open

Dibi - return type extension #474

jakubvojacek opened this issue Nov 17, 2022 · 7 comments

Comments

@jakubvojacek
Copy link
Contributor

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

staabm commented Nov 17, 2022

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
Copy link
Contributor Author

jakubvojacek commented Nov 17, 2022

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
Copy link
Contributor Author

Seems like you already tried to discuss this with ondrejmirtes here phpstan/phpstan#2923 a it is not yet implemented

@staabm
Copy link
Owner

staabm commented Nov 18, 2022

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

see e.g.

assertType('PDOStatement<array{email: string, adaid: int<-32768, 32767>}>', $stmt);
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
Copy link
Contributor Author

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

staabm commented Nov 18, 2022

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

@patrickkusebauch
Copy link
Contributor

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:

  • Dibi\Connection - there is no option for configuration there, it always returns DibiRow
  • Dibi\Result - you can call setRowClass(class-string|null) or setRowFactory(callable(Dibi\Row):mixed) to define what you should return, where setRowClass(null) means return array{column-name: column-value, ...}

How is this possible?

Dibi\Connection->fetch*() method are just shortcuts to Dibi\Connection->query("THE SQL QUERY")->fetch*(). Dibi\Connection->query("THE SQL QUERY") returns Dibi\Result that has the methods to define the return type.

So when using just Dibi\Connection, you have no control over defining the return type and it is always Dibi\Row.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants