Connection refactoring #39

Merged
merged 13 commits into from Nov 8, 2011

Conversation

Projects
None yet
3 participants
Member

fzaninotto commented Nov 6, 2011

To accomodate the possibility to use non-PDO connections, I refactored the connections from classes extending PDO and PDOStatement to classes wrapping around the actual connection classes. After turning the problem in my head for quite some time, I came to the conclusion that wrapper classes - a concept already used by Doctrine2 for connections - is the best way to go.

I took the opportunity to tweak interfaces for Connection and Statement, and cleaned up the full query logging system. In theory, all the Propel code could work with only ConnectionPdo and StatementPdo, the wrapper classes providing only additional features:

  • nested transactions
  • statement cache
  • logging

I made this PR so that the code to merge can be kept to a reasonable size. The refactoring isn't finished, so don't pay attention to the ConnectionPdo and StatementPdo class names for now. Also, I will add more tests as soon as the naming is final. For now, all the existing tests pass (at least the ones that pass in master).

The next step will be to move ConnectionPdo and StatementPdo under 'Runtime/Adapter/Pdo/, as well as all the current Adapter classes (which are, in fact PDO adapter above all). That way, an oci8/db2 adapter can contain both anAdaptersubclass, and aConnectionandStatement` subclass.

fzaninotto added some commits Nov 1, 2011

@fzaninotto fzaninotto Refactor PropelPDO to wrap arounf a PDO instance instead of extending…
… it.

Refs #37
13ea7c8
@fzaninotto fzaninotto Refactor connections (WIP)
 - Split `PropelPDO` into two classes, one to proxy PDO, the other to provide nested transactions and logging
 - Move the actual PDO connection from Propel class to DBAdapter class (which is, in fact, a PDOAdapter class)
 - [BC break] Change the names of the logging methods, e.g. `Propel\Runtime\Connection\PropelPDO::exec` => `exec` (these start to look like events, it is intentional)
 - Remove a few methods from ConnectionInterface, as it may be used for non-PDO connections
a35dab3
@fzaninotto fzaninotto Continuing connection refactoring.
Changing the logic from adapter to wrapper for the actual PDO
connection.
44e911b
@fzaninotto fzaninotto Continuing connection refactoring - this time with PDOStatement.
Replace PDOStatement by a StatementWrapper. Also introduced a
StatementInterface to remove dependency on PDOStatement:
782563a

It's an interface, you can (have to) avoid visibility keywords as all methods will be public and it's the default visibility in PHP.

Owner

fzaninotto replied Nov 6, 2011

Fixed in 00249b8

you can avoid the visibility keyword here as well

Owner

fzaninotto replied Nov 6, 2011

Check.

Owner

fzaninotto replied Nov 6, 2011

It's column. Fixed in the next commit.

Owner

fzaninotto replied Nov 6, 2011

Fixed.

ConnectionWrapper implements ConnectionInterface so how about to switch the type hint here ?

Owner

fzaninotto replied Nov 6, 2011

No, not here. It's on purpose. You see, a StatementWrapper only works if built by a ConnectionWrapper, because it will later need abilities that only ConnectionWrapper (and not any class implementing ConnectionInterface) provides. See for instance L83.

So there is no mistake here.

I think PdoConnection was ok as it's the kind of rule we use for all classes (ArrayFormatter, GeneratorConfig, ...)

Owner

fzaninotto replied Nov 6, 2011

The name will change anyway...

@willdurand willdurand commented on the diff Nov 6, 2011

src/Propel/Generator/Builder/Om/PHP5PeerBuilder.php
@@ -1123,7 +1123,7 @@ abstract class ".$this->getClassname(). $extendingPeerClass . " {
* @throws PropelException Any exceptions caught during processing will be
* rethrown wrapped into a PropelException.
*/
- public static function populateObjects(PDOStatement \$stmt)
+ public static function populateObjects(StatementInterface \$stmt)
@willdurand

willdurand Nov 6, 2011

Owner

Well, I read the whole PR and I found some CS mistakes. Actually it's not caused by your changes but, I guess we should track all CS issues during this refactoring/improvement.
So here is the first issue. We have to put the static keyword before the visibility one.

@fzaninotto

fzaninotto Nov 7, 2011

Member

we'll move all static keywords first in a CS refactoring commit, it doesn't make sense to change just this one in the entire builder.

@willdurand willdurand commented on an outdated diff Nov 6, 2011

src/Propel/Runtime/Adapter/AbstractAdapter.php
@@ -16,6 +16,8 @@ use Propel\Runtime\Map\DatabaseMap;
use Propel\Runtime\Util\PropelColumnTypes;
use Propel\Runtime\Util\PropelDateTime;
use Propel\Runtime\Query\Criteria;
+use Propel\Runtime\Connection\ConnectionPdo;
+use Propel\Runtime\Connection\StatementInterface;
use \PDO;
use \PDOStatement;
@willdurand

willdurand Nov 6, 2011

Owner

Is this always used ?

@willdurand willdurand commented on an outdated diff Nov 6, 2011

src/Propel/Runtime/Adapter/AbstractAdapter.php
+ public function getConnection($conparams)
+ {
+ $conparams = $this->prepareParams($conparams);
+
+ if (!isset($conparams['dsn'])) {
+ throw new PropelException(sprintf('No dsn specified in your connection parameters for datasource "%s"', $name));
+ }
+
+ $dsn = $conparams['dsn'];
+ $user = isset($conparams['user']) ? $conparams['user'] : null;
+ $password = isset($conparams['password']) ? $conparams['password'] : null;
+
+ // load any driver options from the config file
+ // driver options are those PDO settings that have to be passed during the connection construction
+ $driver_options = array();
+ if ( isset($conparams['options']) && is_array($conparams['options']) ) {
@willdurand

willdurand Nov 6, 2011

Owner

Extra spaces before isset and after is_array

@willdurand willdurand commented on an outdated diff Nov 6, 2011

src/Propel/Runtime/Adapter/AbstractAdapter.php
*
- * @param array $params
- * @return array
+ * @param array $conparams connection parameters
+ *
+ * @return PDO A PDO instance
@willdurand

willdurand Nov 6, 2011

Owner

Shouldn't be ConnectionPdo ? Or the corresponding interface ?

@willdurand willdurand and 1 other commented on an outdated diff Nov 6, 2011

src/Propel/Runtime/Adapter/AbstractAdapter.php
+
+ if (!isset($conparams['dsn'])) {
+ throw new PropelException(sprintf('No dsn specified in your connection parameters for datasource "%s"', $name));
+ }
+
+ $dsn = $conparams['dsn'];
+ $user = isset($conparams['user']) ? $conparams['user'] : null;
+ $password = isset($conparams['password']) ? $conparams['password'] : null;
+
+ // load any driver options from the config file
+ // driver options are those PDO settings that have to be passed during the connection construction
+ $driver_options = array();
+ if ( isset($conparams['options']) && is_array($conparams['options']) ) {
+ foreach ($conparams['options'] as $option => $optiondata) {
+ $value = $optiondata['value'];
+ if (is_string($value) && strpos($value, '::') !== false) {
@willdurand

willdurand Nov 6, 2011

Owner

the value should be before the comparison sign.

(I know, it's quite strict but if I don't do that now, it will be a mess)

@fzaninotto

fzaninotto Nov 7, 2011

Member

Let's not focus too much on that kind of thing, otherwise any major code change will be hell. If someone has time to spend on swapping the arguments of comparisons, let him do it. I don't.

@willdurand willdurand commented on an outdated diff Nov 6, 2011

src/Propel/Runtime/Adapter/AbstractAdapter.php
@@ -110,7 +154,7 @@ abstract class AbstractAdapter
* @param PDO $con A PDO connection instance.
* @param array $settings An array of settings.
*/
- public function initConnection(PDO $con, array $settings)

@willdurand willdurand commented on an outdated diff Nov 6, 2011

src/Propel/Runtime/Adapter/AbstractAdapter.php
@@ -135,7 +179,7 @@ abstract class AbstractAdapter
* @param PDO $con A $PDO PDO connection instance.
* @param string $charset The $string charset encoding.
*/
- public function setCharset(PDO $con, $charset)
@willdurand

willdurand Nov 6, 2011

Owner

same here

@willdurand willdurand commented on the diff Nov 6, 2011

src/Propel/Runtime/Adapter/AbstractAdapter.php
* @param PDO $con
* @param string $name
*
* @return mixed
*/
- public function getId(PDO $con, $name = null)
+ public function getId($con, $name = null)
@willdurand

willdurand Nov 6, 2011

Owner

same here

@willdurand willdurand commented on the diff Nov 6, 2011

src/Propel/Runtime/Adapter/MysqlAdapter.php
@@ -97,7 +98,7 @@ class MysqlAdapter extends AbstractAdapter
*
* @throws PDOException No Statement could be created or executed.
*/
- public function lockTable(PDO $con, $table)
+ public function lockTable($con, $table)
@willdurand

willdurand Nov 6, 2011

Owner

Is it a ConnectionInterface ?

@willdurand willdurand commented on the diff Nov 6, 2011

src/Propel/Runtime/Collection/OnDemandCollection.php
*/
- public function initIterator(AbstractFormatter $formatter, PDOStatement $stmt)
+ public function initIterator(AbstractFormatter $formatter, StatementInterface $stmt)
@willdurand

willdurand Nov 6, 2011

Owner

Maybe we could create an interface for formatters..

@fzaninotto

fzaninotto Nov 7, 2011

Member

yeah, let's do that. Later.

@willdurand willdurand commented on the diff Nov 6, 2011

src/Propel/Runtime/Connection/ConnectionInterface.php
@@ -20,16 +20,6 @@ namespace Propel\Runtime\Connection;
Interface ConnectionInterface
@willdurand

willdurand Nov 6, 2011

Owner

interface, not Interface.

@willdurand willdurand commented on an outdated diff Nov 6, 2011

src/Propel/Runtime/Connection/ConnectionPdo.php
+use Propel\Runtime\Exception\PropelException;
+
+use \PDO;
+
+/**
+ * PDO extension that implements ConnectionInterface and builds statements implementting StatementInterface.
+ */
+class ConnectionPdo extends PDO implements ConnectionInterface
+{
+ /**
+ * Creates a PDO instance representing a connection to a database.
+ */
+ public function __construct($dsn, $user = null, $password = null, array $options = null)
+ {
+ parent::__construct($dsn, $user, $password, $options);
+ $this->setAttribute(PDO::ATTR_STATEMENT_CLASS, array('\Propel\Runtime\Connection\StatementPDO', array()));
@willdurand

willdurand Nov 6, 2011

Owner

Should be StatementPdo, right ?

@willdurand willdurand commented on an outdated diff Nov 6, 2011

src/Propel/Runtime/Connection/ConnectionPdo.php
+ parent::__construct($dsn, $user, $password, $options);
+ $this->setAttribute(PDO::ATTR_STATEMENT_CLASS, array('\Propel\Runtime\Connection\StatementPDO', array()));
+ $this->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
+ }
+
+ /**
+ * Sets a connection attribute.
+ *
+ * This is overridden here to allow names corresponding to PDO constant names.
+ *
+ * @param integer $attribute The attribute to set (e.g. 'PDO::ATTR_CASE', or more simply 'ATTR_CASE').
+ * @param mixed $value The attribute value.
+ */
+ public function setAttribute($attribute, $value)
+ {
+ if (is_string($attribute) && strpos($attribute, '::') === false) {
@willdurand

willdurand Nov 6, 2011

Owner

the value should be placed before the comparison sign

@willdurand willdurand commented on the diff Nov 6, 2011

src/Propel/Runtime/Connection/ConnectionWrapper.php
+ */
+ protected $cachedPreparedStatements = array();
+
+ /**
+ * Whether to cache prepared statements.
+ *
+ * @var boolean
+ */
+ protected $isCachePreparedStatements = false;
+
+ /**
+ * Whether or not the debug is enabled
+ *
+ * @var boolean
+ */
+ public $useDebug = false;
@willdurand

willdurand Nov 6, 2011

Owner

This attribute should be moved before as the order is: const, public, protected, private.
It's the same for methods btw

@willdurand willdurand commented on an outdated diff Nov 6, 2011

src/Propel/Runtime/Connection/ConnectionWrapper.php
+ */
+ public $useDebug = false;
+
+ /**
+ * Configured BasicLogger (or compatible) logger.
+ *
+ * @var BasicLogger
+ */
+ protected $logger;
+
+ /**
+ * The log level to use for logging.
+ *
+ * @var integer
+ */
+ private $logLevel = Propel::LOG_DEBUG;
@willdurand

willdurand Nov 6, 2011

Owner

This attribute should be moved after.

@willdurand willdurand commented on the diff Nov 6, 2011

src/Propel/Runtime/Connection/ConnectionWrapper.php
+ }
+
+ /**
+ * @return ConnectionInterface
+ */
+ public function getWrappedConnection()
+ {
+ return $this->connection;
+ }
+
+ /**
+ * Inject the runtime configuration
+ *
+ * @param Configuration $configuration
+ */
+ public function setConfiguration($configuration)
@willdurand

willdurand Nov 6, 2011

Owner

Can we type hint here ? With ConfigurationInterface for instance ?

@fzaninotto

fzaninotto Nov 7, 2011

Member

Once there is a ConfigurationInterface, why not. It's not the purpose of this refactoring.

@willdurand willdurand commented on the diff Nov 6, 2011

src/Propel/Runtime/Connection/ConnectionWrapper.php
+ }
+
+ /**
+ * Inject the runtime configuration
+ *
+ * @param Configuration $configuration
+ */
+ public function setConfiguration($configuration)
+ {
+ $this->configuration = $configuration;
+ }
+
+ /**
+ * Get the runtime configuration
+ *
+ * @return Configuration
@willdurand

willdurand Nov 6, 2011

Owner

same here

@willdurand willdurand commented on an outdated diff Nov 6, 2011

src/Propel/Runtime/Propel.php
// load any connection options from the config file
// connection attributes are those PDO flags that have to be set on the initialized connection
if (isset($conparams['attributes']) && is_array($conparams['attributes'])) {
- $attributes = array();
- try {
- self::processDriverOptions( $conparams['attributes'], $attributes );
- } catch (PropelException $e) {
- throw new PropelException('Error processing connection attributes for datasource ['.$name.']', $e);
- }
- foreach ($attributes as $key => $value) {
- $con->setAttribute($key, $value);
+ foreach ($conparams['attributes'] as $option => $optiondata) {
+ $value = $optiondata['value'];
+ if (is_string($value) && strpos($value, '::') !== false) {
@willdurand

willdurand Nov 6, 2011

Owner

the value should be placed before the comparison sign

@willdurand willdurand commented on the diff Nov 6, 2011

tests/Propel/Tests/Runtime/Adapter/MysqlAdapterTest.php
@@ -111,3 +111,11 @@ class MockPDO extends \PDO
{
}
}
+
+class TestableMysqlAdapter extends MysqlAdapter
@willdurand

willdurand Nov 6, 2011

Owner

Be careful with that, autoloading issues can appear.

@fzaninotto

fzaninotto Nov 7, 2011

Member

Don't see why. There is no autoloading here, since the testable class is in the same file as the test class.

@pminnieur pminnieur commented on an outdated diff Nov 7, 2011

src/Propel/Generator/Builder/Om/PHP5PeerBuilder.php
@@ -138,10 +138,10 @@ abstract class ".$this->getClassname(). $extendingPeerClass . " {
'\Propel\Runtime\Propel',
'\Propel\Runtime\Exception\PropelException',
'\Propel\Runtime\Connection\ConnectionInterface',
+ 'Propel\Runtime\Connection\StatementInterface',
@pminnieur

pminnieur Nov 7, 2011

\ missing in front of Propel?

Member

fzaninotto commented Nov 7, 2011

Fixed all that had to be fixed.

willdurand merged commit aed24bf into propelorm:master Nov 8, 2011

Owner

willdurand commented Nov 8, 2011

Many thanks

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