Fix on FetchMode getting lost between instances#183
Fix on FetchMode getting lost between instances#183TCB13 merged 5 commits intousmanhalalit:masterfrom
Conversation
this would fix the issues on fallback fetchMode if set
vikkio88
changed the title
|
hi @TCB13 this Pr started as a change on a readme, now is doing that plus the FetchMode changes. |
|
@vikkio88 forget my comment. |
|
@vikkio88 do you have any comments on the reviews above? |
|
@TCB13 I dont see any review |
|
@vikkio88 you're replaced |
|
@TCB13 sure thing, let me fix that, is most likely something due to phpstorm autofixing the coding style. |
| /** | ||
| * @param null|\Pixie\Connection $connection | ||
| * | ||
| * @throws \Pixie\Exception |
There was a problem hiding this comment.
you mean this @TCB13 ?
this is because you are using use Pixie\Exception, this will be automatically inferred to be a Pixie one
| * @var array | ||
| */ | ||
| protected $fetchParameters = array(\PDO::FETCH_OBJ); | ||
| protected $fetchParameters = array(PDO::FETCH_OBJ); |
There was a problem hiding this comment.
@TCB13 and this is the same thing, as you are using use PDO the trailing \ is not required
|
@TCB13 as I commented those changes were done automatically by phpstorm as it realizes that |
adding required changes on the PR
|
hi @vikkio88 you've a point there. You can keep it without the |
|
sure thing I will add some tests too. |
| * | ||
| * @throws \Pixie\Exception | ||
| * @param int $fetchMode | ||
| * @throws Exception |
There was a problem hiding this comment.
Why replace \Pixie\Exception by Exception?
There was a problem hiding this comment.
use is already specifying that the Exception in here is \Pixie\
| * @var array | ||
| */ | ||
| protected $fetchParameters = array(\PDO::FETCH_OBJ); | ||
| protected $fetchParameters = array(PDO::FETCH_OBJ); |
There was a problem hiding this comment.
@usmanhalalit need your view in there. This can potentially break some setups.
There was a problem hiding this comment.
it will not, as use PDO; on this file make \PDO and PDO the same thing
adding regression tests
fix on regression test
|
Everything looks great, many thanks for your time. 🥇 |
|
thank you for this project, let me know when you are planning to do a release |
|
@vikkio88 that is something only @usmanhalalit can answer. I use myself |
3 participants
this should fix #185