Skip to content

Portability wrapper _defaultFetchMode #348

Merged
merged 3 commits into from Aug 10, 2013

5 participants

@mdriessen

The Portability wrapper did not properly use the _defaultFetchMode when the method setFetchMode() was used on the Connection. Since the defaultFetchMode for the portability wrapper is set to PDO::FETCH_BOTH, I couldn't change this to the desired format.

Matty Driessen Portability wrapper did not properly use the _defaultFetchMode when t…
…he method setFetchMode() was used on the Connection
513228d
@doctrinebot

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-568

We use Jira to track the state of pull requests and the versions they got
included in.

@Ocramius
Doctrine member

@mdriessen can you provide a test that prevents regressions on this?

@stof stof and 1 other commented on an outdated diff Jul 24, 2013
lib/Doctrine/DBAL/Connection.php
@@ -177,7 +177,7 @@ class Connection implements DriverConnection
/**
* @var integer
*/
- private $_defaultFetchMode = PDO::FETCH_ASSOC;
+ protected $_defaultFetchMode = PDO::FETCH_ASSOC;
@stof
Doctrine member
stof added a note Jul 24, 2013

I think it would be a good idea to drop the leading underscore before changing the visibility.

Renaming a protected property is a BC break while it is not an issue for private ones. So if we keep the underscore when switching to protected, we will not be able to remove it anymore. What do you think @Ocramius ?

@Ocramius
Doctrine member
Ocramius added a note Jul 24, 2013

@stof yes, no BC here, so +1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Matty Driessen added some commits Jul 24, 2013
Matty Driessen Removed leading underscore of private property _defaultFetchMode beca…
…use of changed visibility
3e0ec23
Matty Driessen Added regression test for the Portability Wrapper when setting the de…
…faultFetchMode on the Connection instead of the Statement
5c305a9
@mdriessen

I removed the leading underscore and added some tests to prevent regression.

@stof stof commented on the diff Jul 24, 2013
tests/Doctrine/Tests/DBAL/Functional/PortabilityTest.php
@@ -67,18 +72,42 @@ public function testFullFetchMode()
}
$stmt = $this->getPortableConnection()->query('SELECT * FROM portability_table');
- while ($row = $stmt->fetch(\PDO::FETCH_ASSOC)) {
+ while (($row = $stmt->fetch(\PDO::FETCH_ASSOC))) {
@stof
Doctrine member
stof added a note Jul 24, 2013

the extra braces you added are not needed

@mdriessen
mdriessen added a note Jul 24, 2013

I know but it highlights that it's intended to evaluate the result instead of being a typo ( = instead of ==). Most code editors also see the 'Assignment in condition' as a code smell. With the added braces the editor knows it's intended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mdriessen

Any chance this PR will be included in the 2.4 release?

@beberlei beberlei merged commit ace5645 into doctrine:master Aug 10, 2013

1 check passed

Details default The Travis CI build passed
@mdriessen mdriessen deleted the mdriessen:fetch_portability branch Aug 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.