search

laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.51k stars 11.02k forks source link

MSSQL PDO cannot connect - Argument 1 passed ... must be an instance of PDO, array given #35557

Closed TomHAnderson closed 3 years ago

TomHAnderson commented 3 years ago

Description:

Using a MSSQL database in a Docker container and trying to run migrations I get an exception. This is a real easy one.

https://github.com/laravel/framework/blob/8.x/src/Illuminate/Database/PDO/SqlServerDriver.php types for an array which is then directly passed to https://github.com/laravel/framework/blob/8.x/src/Illuminate/Database/PDO/Connection.php#L31 which types for a PDO object.

The correct fix is https://github.com/laravel/framework/blob/8.x/src/Illuminate/Database/PDO/SqlServerDriver.php#L12 to

new Connection($params['pdo'])

Steps To Reproduce:

Try to run a migration on MSSQL.

taylorotwell commented 3 years ago

If you've already diagnosed it, why not send in a PR? 😄

driesvints commented 3 years ago

Thanks, I've sent in a PR here: https://github.com/laravel/framework/pull/35564

TomHAnderson commented 3 years ago

I didn't send a PR because I'm not ready to dive into your framework for unit testing MSSQL.

driesvints commented 3 years ago

@TomHAnderson understandable. I've also just sent in the PR without tests since we don't have a CI setup for SqlServer atm.

TomHAnderson commented 3 years ago

You should use Docker for CI. No joke, that's going to be the best way to test all your supported databases. I'm taken aback that you're not testing your MSSQL support right now.

See https://github.com/doctrine/DoctrineMongoODMModule#development for an implementation I did for a Mongo module. See https://github.com/team-telnyx/telnyx-php#development for another Docker for unit testing I did.

I think Laravel is far behind since you're not already using this tech for your unit tests. This just doesn't cut the mustard: https://github.com/laravel/framework/blob/8.x/docker-compose.yml

If you agree LMK and I'll open a new PR issue for discussion.

afraca commented 3 years ago

I know this is closed already but I wholeheartedly agree with @TomHAnderson . If you provide drivers in the framework they should be tested. (Though perhaps part of it is already tested with extensive mocking?) This is a really embarrassing bug with something like Docker these days, although I will admit setting it up would be no small task!

driesvints commented 3 years ago

@TomHAnderson @afraca the entire MSSQL implementation has been largely a community contribution since none of us at Laravel use it anywhere in production or for our own products. We have no experience with it ourselves therefor we rely on the community to contribute here. This is still open source and we very much welcome any help in improving its implementation as well as additional tests.

This is a really embarrassing bug

The fact that no one stumbled on this before says a lot more about the adaptation of the driver than anything else really.

This just doesn't cut the mustard:

Btw Tom, that docker compose file is only meant for us to quickly run a part of the test suite locally and nothing more.