Skip to content

Conversation

@Seldaek
Copy link
Contributor

@Seldaek Seldaek commented May 4, 2022

Partially addresses #278 - but the way I see it, in QuerySimulation::simulateParamValueType you don't have access to the param types argument, only to the parameter themselves?

I mean PDO::exec($query, $params, $paramTypes) <- this param types arg

Without access to that we can serialize the datetime to a Y-m-d H:i:s format, but it won't be always correct, as if you pass Doctrine\DBAL\Types\Types::DATE for ex then it should use Y-m-d only. And strictly speaking we should only simulate/serialize DateTime objects if the appropriate Type is passed in $paramTypes. Otherwise it's unlikely to be a bug if you have a DateTime in there as it will fail at runtime.

This patch does fix the issue I have though so it's a improvement as it allows me to run phpstan-dba on this project of mine, but it's hardly a comprehensive fix.

@staabm
Copy link
Owner

staabm commented May 13, 2022

Could you point me to a code example where this feature is needed?

@Seldaek
Copy link
Contributor Author

Seldaek commented May 13, 2022

There is an example in #278 - I don't have anything public sorry. But essentially doctrine allows binding advanced data types, so it'd be great if we could support that here.

@Seldaek
Copy link
Contributor Author

Seldaek commented May 17, 2022

@staabm any idea what I can do to help move this forward? :)

@staabm
Copy link
Owner

staabm commented May 17, 2022

could you provide a failling unit test?

@staabm
Copy link
Owner

staabm commented May 18, 2022

I mean PDO::exec($query, $params, $paramTypes) <- this param types arg

hmm are you sure there is more then 1 arg to PDO::exec?

https://www.php.net/manual/en/pdo.exec.php

@staabm
Copy link
Owner

staabm commented May 18, 2022

ohh I see, you actually meant doctrine $connection->executeUpdate and friends

@Seldaek
Copy link
Contributor Author

Seldaek commented May 18, 2022

Yeah sorry PDO::exec was a wrong example, DBAL's connection has them and PDOStatement too has a 3rd param $type on bindParam https://www.php.net/manual/en/pdostatement.bindparam.php that takes one of the PDO::PARAM_* consts https://www.php.net/manual/en/pdo.constants.php

So IMO taking this $type into consideration would be good anyway for PDO too (i.e. error if the type is provided but does not match the real type given as value), but it is even more essential for doctrine because it supports additional types like the DATETIME_* ones.

staabm pushed a commit to Seldaek/phpstan-dba that referenced this pull request May 18, 2022
@staabm
Copy link
Owner

staabm commented May 18, 2022

@Seldaek just to confirm - this PR as is would unblock you?

(I am willing to merge it and handle it as a kind of beta-feature) until we got #342 merged

@Seldaek
Copy link
Contributor Author

Seldaek commented May 18, 2022

Yes I'm running on my fork for now, which works for me, so I can wait a bit longer for sure. Up to you.

@staabm staabm merged commit cadf7e4 into staabm:main May 18, 2022
@staabm
Copy link
Owner

staabm commented May 18, 2022

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

Successfully merging this pull request may close these issues.

3 participants