RatkoR / laravel-crate.io

Crate.io driver for Laravel
MIT License
36 stars 12 forks source link

Use PDO::FETCH_OBJ #21

Closed olavski closed 3 years ago

olavski commented 6 years ago

Hi,

Crate PDO supports PDO::FETCH_OBJ which allows DB queries to return an array of stdClass objects, same as the default in Laravel.

Since Laravel 5.4: https://laravel.com/docs/5.4/upgrade "Laravel no longer includes the ability to customize the PDO "fetch mode" from your configuration files. Instead, PDO::FETCH_OBJ is always used. "

Is it possible to use PDO::FETCH_OBJ with this library?

Cheers, Olav

RatkoR commented 6 years ago

Hmm, I'm not sure, I'll have to look. Next week I'm ooo, I'll see if I can get some time to look at this until then.

olavski commented 6 years ago

Thank you for the quick response! It is not urgent, but it would be great if you can have a look when you have the time.

I'm just testing this on a small project where I hardcoded:

// in /RatkoR/Crate/Connection.php
protected $fetchMode = PDO::FETCH_OBJ;

and both Model results and DB results seems to be working ok.

Note: Changing to FETCH_OBJ would be a breaking change and cause problems for existing users of this library using DB queries and expecting an array of arrays to be returned.

Thanks!

JulianMar commented 3 years ago

I have to agree. It is really weird to see that the Laravel query builder returns an array of objects and here it's an array of array. It's calling the setFetchMode on the connection parent class, which doesn't even exist anymore...

It would be good to switch to default Laravel way, by just using the default fetch mode

RatkoR commented 3 years ago

I agree, FETCH_OBJ should be the default. What I don't like is breaking all user's code. If we do a major release (version 11) and put a big breaking change notice in readme, will this be enough to warn users?

I know users should check what new major version brings, and I hope they test before releasing, but still.. Any other thing we can do for them? Otherwise I'll just go with v11 and release it after I see it passes our tests.

JulianMar commented 3 years ago

Yeah me neither. It can get quite frustrating for the users.
Apart from Increasing the main version to 11.x I don't see another way. Definitly mention it in the changelog for the release aswell

I hope most users have a testsuite to catch these errors, but I will watch the repo and can help if somebody has a problem

RatkoR commented 3 years ago

It's ready and our tests pass: https://github.com/RatkoR/laravel-crate.io/pull/42

Care to check it @JulianMar ? It's based on your commit + some cosmetics and that readme update.

JulianMar commented 3 years ago

Looks good in my opinion! Thanks for the work @RatkoR

RatkoR commented 3 years ago

Merged into master. Thx.