Skip to content

Allow simulation of DateTime objects #339

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

Merged
merged 7 commits into from
May 18, 2022
Merged

Allow simulation of DateTime objects #339

merged 7 commits into from
May 18, 2022

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