code refactorings on persister #413

Merged
merged 16 commits into from Nov 25, 2012

5 participants

@FabioBatSilva
Doctrine member

hi

This patch does not add any feature, just small refactorings and code clean up.
It make easier to implement the generation of persisters:
http://www.doctrine-project.org/jira/browse/DDC-1889

Cheers ...

@travisbot

This pull request passes (merged a7f0b26e into 00a5f18).

@guilhermeblanco guilhermeblanco commented on the diff Aug 12, 2012
...ctrine/ORM/Persisters/AbstractCollectionPersister.php
@@ -86,30 +86,30 @@ public function delete(PersistentCollection $coll)
return; // ignore inverse side
}
- $sql = $this->_getDeleteSQL($coll);
- $this->_conn->executeUpdate($sql, $this->_getDeleteSQLParameters($coll));
+ $sql = $this->getDeleteSQL($coll);
@guilhermeblanco
Doctrine member

Missing line break

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on the diff Aug 12, 2012
...ctrine/ORM/Persisters/AbstractCollectionPersister.php
}
}
- //public function updateRows(PersistentCollection $coll)
@guilhermeblanco
Doctrine member

I'd still leave this method here commented.
They are the reference we have when we implement the Concrete Table Inheritance (1 table = 1 class), which we call as "UnionSubclassPersister".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
...ctrine/ORM/Persisters/AbstractCollectionPersister.php
}
}
+ /**
+ * Count the size of this persistent collection
+ *
+ * @param \Doctrine\ORM\PersistentCollection $coll
+ * @return integer
@guilhermeblanco
Doctrine member

Missing line break between param and return

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
...ctrine/ORM/Persisters/AbstractCollectionPersister.php
public function count(PersistentCollection $coll)
{
throw new \BadMethodCallException("Counting the size of this persistent collection is not supported by this CollectionPersister.");
}
+ /**
+ * Slice elements
+ *
+ * @param \Doctrine\ORM\PersistentCollection $coll
+ * @param integer $offset
+ * @param integer $length
+ * @return array
@guilhermeblanco
Doctrine member

Missing line break between param and return

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
...ctrine/ORM/Persisters/AbstractCollectionPersister.php
public function slice(PersistentCollection $coll, $offset, $length = null)
{
throw new \BadMethodCallException("Slicing elements is not supported by this CollectionPersister.");
}
+ /**
+ * Check for existance of an element
+ *
+ * @param \Doctrine\ORM\PersistentCollection $coll
+ * @param mixed \Doctrine\ORM\PersistentCollection
+ * @return boolean
@guilhermeblanco
Doctrine member

Missing line break between param and return

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
...ctrine/ORM/Persisters/AbstractCollectionPersister.php
public function contains(PersistentCollection $coll, $element)
{
throw new \BadMethodCallException("Checking for existance of an element is not supported by this CollectionPersister.");
}
+ /**
+ * Check for existance of a key
+ *
+ * @param \Doctrine\ORM\PersistentCollection $coll
+ * @param mixed $key
+ * @return boolean
@guilhermeblanco
Doctrine member

Missing line break between param and return

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
...ctrine/ORM/Persisters/AbstractCollectionPersister.php
public function containsKey(PersistentCollection $coll, $key)
{
throw new \BadMethodCallException("Checking for existance of a key is not supported by this CollectionPersister.");
}
+ /**
+ * Remove an element
+ *
+ * @param \Doctrine\ORM\PersistentCollection $coll
+ * @param object $element
+ * @return mixed
@guilhermeblanco
Doctrine member

Missing line break between param and return

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
...ctrine/ORM/Persisters/AbstractCollectionPersister.php
public function removeKey(PersistentCollection $coll, $key)
{
throw new \BadMethodCallException("Removing a key is not supported by this CollectionPersister.");
}
+ /**
+ * Get an element by key
+ *
+ * @param \Doctrine\ORM\PersistentCollection $coll
+ * @param mixed $index
+ * @return object
@guilhermeblanco
Doctrine member

Missing line break between param and return

@guilhermeblanco
Doctrine member

You still missed this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
@@ -304,8 +301,8 @@ public function executeInserts()
*/
protected function assignDefaultVersionValue($entity, $id)
{
- $value = $this->fetchVersionValue($this->_class, $id);
- $this->_class->setFieldValue($entity, $this->_class->versionField, $value);
+ $value = $this->fetchVersionValue($this->class, $id);
+ $this->class->setFieldValue($entity, $this->class->versionField, $value);
@guilhermeblanco
Doctrine member

Missing line break

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
- return Type::getType($versionedClass->fieldMappings[$versionField]['type'])->convertToPHPValue($value, $this->_platform);
+ $value = $this->conn->fetchColumn($sql, array_values((array)$id));
+ $value = Type::getType($versionedClass->fieldMappings[$versionField]['type'])
@guilhermeblanco
Doctrine member

You can inline this type conversion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
- return Type::getType($versionedClass->fieldMappings[$versionField]['type'])->convertToPHPValue($value, $this->_platform);
+ $value = $this->conn->fetchColumn($sql, array_values((array)$id));
@guilhermeblanco
Doctrine member

Missing space between type conversion and variable name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
- $set[] = $versionColumn . ' = ' . $versionColumn . ' + 1';
- } else if ($versionFieldType == Type::DATETIME) {
- $set[] = $versionColumn . ' = CURRENT_TIMESTAMP';
+ $versionField = $this->class->versionField;
+ $versionFieldType = $this->class->fieldMappings[$versionField]['type'];
+ $versionColumn = $this->quoteStrategy->getColumnName($versionField, $this->class, $this->platform);
+
+ $where[] = $versionColumn;
+ $types[] = $this->class->fieldMappings[$versionField]['type'];
+ $params[] = $this->class->reflFields[$versionField]->getValue($entity);
+
+ switch ($versionFieldType) {
+ case Type::INTEGER:
+ $set[] = $versionColumn . ' = ' . $versionColumn . ' + 1';
+ break;
+ case Type::DATETIME:
@guilhermeblanco
Doctrine member

Missing line break

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on the diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
- if ($newVal !== null) {
- $newValId = $uow->getEntityIdentifier($newVal);
+ if ($newVal !== null) {
+ $oid = spl_object_hash($newVal);
+
+ if (isset($this->queuedInserts[$oid]) || $uow->isScheduledForInsert($newVal)) {
+ // The associated entity $newVal is not yet persisted, so we must
+ // set $newVal = null, in order to insert a null value and schedule an
+ // extra update on the UnitOfWork.
+ $uow->scheduleExtraUpdate($entity, array(
@guilhermeblanco
Doctrine member

You can inline this call

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on the diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
}
- $hydrator = $this->_em->newHydrator(Query::HYDRATE_OBJECT);
-
- return $hydrator->hydrateAll($stmt, $rsm, $hints);
+ return $this->em->newHydrator(Query::HYDRATE_OBJECT)
@guilhermeblanco
Doctrine member

You can inline this call

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on the diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
}
- $hydrator = $this->_em->newHydrator(Query::HYDRATE_OBJECT);
-
- return $hydrator->hydrateAll($stmt, $rsm, $hints);
+ return $this->em->newHydrator(Query::HYDRATE_OBJECT)
@guilhermeblanco
Doctrine member

You can inline this call

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on the diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
- if ($filterSql = $this->generateFilterConditionSQL($this->_class, $alias)) {
- if ($conditionSql) {
- $conditionSql .= ' AND ';
- }
+ switch ($lockMode) {
+ case LockMode::PESSIMISTIC_READ:
+ $lockSql = ' ' . $this->platform->getReadLockSql();
+
@guilhermeblanco
Doctrine member

Your line breaks here are flipped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
- if ($filterSql = $this->generateFilterConditionSQL($this->_class, $alias)) {
- if ($conditionSql) {
- $conditionSql .= ' AND ';
- }
+ switch ($lockMode) {
+ case LockMode::PESSIMISTIC_READ:
+ $lockSql = ' ' . $this->platform->getReadLockSql();
+
+ break;
+ case LockMode::PESSIMISTIC_WRITE:
+ $lockSql = ' ' . $this->platform->getWriteLockSql();
+
@guilhermeblanco
Doctrine member

Remove extra line break

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
- $conditionSql .= $filterSql;
+ if ('' !== $filterSql) {
@guilhermeblanco
Doctrine member

Yoda conditions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
}
- return $this->_platform->modifyLimitQuery('SELECT ' . $this->_getSelectColumnListSQL()
- . $this->_platform->appendLockHint(' FROM ' . $this->quoteStrategy->getTableName($this->_class, $this->_platform) . ' '
- . $alias, $lockMode)
- . $this->_selectJoinSql . $joinSql
- . ($conditionSql ? ' WHERE ' . $conditionSql : '')
+ $select = 'SELECT ' . $columnList;
+ $from = ' FROM ' . $tableName . ' '. $tableAlias;
+ $join = $this->selectJoinSql . $joinSql;
+ $where = ($conditionSql ? ' WHERE ' . $conditionSql : '');
+
+ return $this->platform->modifyLimitQuery($select
@guilhermeblanco
Doctrine member

Create the query as a variable and pass it to method call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
}
- $tableAlias = isset($this->_class->fieldMappings[$fieldName]['inherited']) ?
- $this->_getSQLTableAlias($this->_class->fieldMappings[$fieldName]['inherited'])
+ $tableAlias = isset($this->class->fieldMappings[$fieldName]['inherited']) ?
@guilhermeblanco
Doctrine member

Wrong ternary construction:

$tableAlias = isset($this->class->fieldMappings[$fieldName]['inherited'])
    ? $this->getSQLTableAlias($this->class->fieldMappings[$fieldName]['inherited'])
    : $baseTableAlias;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on the diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
- if ($assoc2ColumnSQL) {
- if ($columnList) $columnList .= ', ';
- $columnList .= $assoc2ColumnSQL;
- }
+ if ( ! $assoc['isOwningSide']) {
+ $eagerEntity = $this->em->getClassMetadata($assoc['targetEntity']);
+ $association = $eagerEntity->getAssociationMapping($assoc['mappedBy']);
+ }
+
+ $joinTableAlias = $this->getSQLTableAlias($eagerEntity->name, $assocAlias);
+ $joinTableName = $this->quoteStrategy->getTableName($eagerEntity, $this->platform);
+
+ if ($assoc['isOwningSide']) {
+
@guilhermeblanco
Doctrine member

Remove extra line break

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
}
- return $this->_insertSql;
+ $this->insertSql = 'INSERT INTO ' . $this->quoteStrategy->getTableName($this->class, $this->platform)
@guilhermeblanco
Doctrine member

Use sprintf to build these type of strings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on the diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
}
}
- } else if ($this->_class->generatorType != ClassMetadata::GENERATOR_TYPE_IDENTITY || $this->_class->identifier[0] != $name) {
- $columns[] = $this->quoteStrategy->getColumnName($name, $this->_class, $this->_platform);
- $this->_columnTypes[$name] = $this->_class->fieldMappings[$name]['type'];
+
+ continue;
+ }
+
+ if ($this->class->generatorType != ClassMetadata::GENERATOR_TYPE_IDENTITY || $this->class->identifier[0] != $name) {
+ $columns[] = $this->quoteStrategy->getColumnName($name, $this->class, $this->platform);
+ $this->columnTypes[$name] = $this->class->fieldMappings[$name]['type'];
@guilhermeblanco
Doctrine member

Missing line break

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
@@ -1376,21 +1428,30 @@ protected function _getSQLTableAlias($className, $assocName = '')
*/
public function lock(array $criteria, $lockMode)
{
- $conditionSql = $this->_getSelectConditionSQL($criteria);
+ $lockSql = '';
+ $conditionSql = $this->getSelectConditionSQL($criteria);
+
+ switch ($lockMode) {
+ case LockMode::PESSIMISTIC_READ:
+ $lockSql = $this->platform->getReadLockSql();
+
@guilhermeblanco
Doctrine member

Line breaks are flipped

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

This pull request passes (merged f97f7b42 into 72ce9a7).

@fprochazka

You rock :)

@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
foreach ($updateData as $columnName => $value) {
- $column = $columnName;
+
@guilhermeblanco
Doctrine member

Remove extra line break

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@guilhermeblanco guilhermeblanco commented on an outdated diff Aug 12, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
- if ( ! isset($mapping['isOnDeleteCascade'])) {
+ if ($selfReferential) {
+ $otherColumns = ! $mapping['isOwningSide']
@guilhermeblanco
Doctrine member

Missing parenthesis around the condition

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

This pull request passes (merged cd7becd8 into 72ce9a7).

@FabioBatSilva
Doctrine member

Hi guys :)

Anything else pending here ?

@sstok sstok commented on an outdated diff Oct 19, 2012
...ctrine/ORM/Persisters/AbstractCollectionPersister.php
public function slice(PersistentCollection $coll, $offset, $length = null)
{
throw new \BadMethodCallException("Slicing elements is not supported by this CollectionPersister.");
}
+ /**
+ * Check for existance of an element
+ *
+ * @param \Doctrine\ORM\PersistentCollection $coll
+ * @param mixed \Doctrine\ORM\PersistentCollection
@sstok
sstok added a note Oct 19, 2012

@ param mixed \Doctrine\ORM\PersistentCollection ?

this seems wrong

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

I'm fine to merge this. Anyone else?

@stof stof commented on the diff Oct 22, 2012
...ctrine/ORM/Persisters/AbstractCollectionPersister.php
/**
* @var \Doctrine\ORM\UnitOfWork
*/
- protected $_uow;
+ protected $uow;
@stof
Doctrine member
stof added a note Oct 22, 2012

This would be a BC break for people extending the persisters as they are protected properties

@stof
Doctrine member
stof added a note Oct 22, 2012

and the same is true for protected methods

@guilhermeblanco
Doctrine member

Persisters are currently unable to be extended anyway. It cannot be classified as a BC break. =)

@stof
Doctrine member
stof added a note Oct 22, 2012

then it is fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Oct 22, 2012
...ctrine/ORM/Persisters/AbstractCollectionPersister.php
public function removeKey(PersistentCollection $coll, $key)
{
throw new \BadMethodCallException("Removing a key is not supported by this CollectionPersister.");
}
+ /**
+ * Get an element by key
+ *
+ * @param \Doctrine\ORM\PersistentCollection $coll
+ * @param mixed $index
+ *
+ * @return object
@stof
Doctrine member
stof added a note Oct 22, 2012

this should be mixed as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Oct 22, 2012
...ctrine/ORM/Persisters/AbstractCollectionPersister.php
public function slice(PersistentCollection $coll, $offset, $length = null)
{
throw new \BadMethodCallException("Slicing elements is not supported by this CollectionPersister.");
}
+ /**
+ * Check for existance of an element
+ *
+ * @param \Doctrine\ORM\PersistentCollection $coll
+ * @param \Doctrine\ORM\PersistentCollection
@stof
Doctrine member
stof added a note Oct 22, 2012

this is wrong

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof commented on an outdated diff Oct 22, 2012
lib/Doctrine/ORM/Persisters/BasicEntityPersister.php
}
+
@stof
Doctrine member
stof added a note Oct 22, 2012

you should remove this extra empty line

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

Done,

Thanks @guilhermeblanco and @stof

@guilhermeblanco
Doctrine member

I'm fine with the code. Please rebase.
Sorry @FabioBatSilva, but can you enable the Performance tests and run them before and after your patch?
Place the response here.

@FabioBatSilva
Doctrine member

Hi @guilhermeblanco, the performance tests are almost the same ..

doctrine/master :
phpunit --group performance
PHPUnit 3.6.7 by Sebastian Bergmann.

Configuration read from /Users/fabio/backup/workspace/php/doctrine2/phpunit.xml

.testSimpleQueryScalarHydrationPerformance10000Rows - 0.35219120979309 seconds
.testSimpleQueryArrayHydrationPerformance10000Rows - 0.52367997169495 seconds
.testMixedQueryFetchJoinArrayHydrationPerformance10000Rows - 1.1706650257111 seconds
.testSimpleQueryPartialObjectHydrationPerformance10000Rows - 1.0675690174103 seconds
.testSimpleQueryFullObjectHydrationPerformance10000Rows - 4.3161261081696 seconds
.testMixedQueryFetchJoinPartialObjectHydrationPerformance2000Rows - 0.41715288162231 seconds
.testMixedQueryFetchJoinFullObjectHydrationPerformance2000Rows - 0.91599106788635 seconds
.99 CompanyContract: 0.024647
99 CompanyContract: 0.019772
. Inserted 10000 objects in 5.8436779975891 seconds
.100 CmsArticle findAll(): 0.018096
100 CmsArticle findAll(): 0.016093
100 CmsArticle find(): 0.052408
100 CmsArticle find(): 0.046749
.100 CmsGroup: 0.010710
100 CmsGroup: 0.010186
.100 CmsUser: 0.138691
100 CmsUser: 0.025820
. Compute ChangeSet 100 objects in 0.038508176803589 seconds


Time: 19 seconds, Memory: 265.75Mb
My branch :
phpunit --group performance
PHPUnit 3.6.7 by Sebastian Bergmann.

Configuration read from /Users/fabio/backup/workspace/php/doctrine2/phpunit.xml

.testSimpleQueryScalarHydrationPerformance10000Rows - 0.35408091545105 seconds
.testSimpleQueryArrayHydrationPerformance10000Rows - 0.51799082756042 seconds
.testMixedQueryFetchJoinArrayHydrationPerformance10000Rows - 1.1625339984894 seconds
.testSimpleQueryPartialObjectHydrationPerformance10000Rows - 1.062805891037 seconds
.testSimpleQueryFullObjectHydrationPerformance10000Rows - 4.3324000835419 seconds
.testMixedQueryFetchJoinPartialObjectHydrationPerformance2000Rows - 0.41576409339905 seconds
.testMixedQueryFetchJoinFullObjectHydrationPerformance2000Rows - 0.88009285926819 seconds
.99 CompanyContract: 0.024710
99 CompanyContract: 0.019921
. Inserted 10000 objects in 5.839998960495 seconds
.100 CmsArticle findAll(): 0.018246
100 CmsArticle findAll(): 0.022888
100 CmsArticle find(): 0.057067
100 CmsArticle find(): 0.054318
.100 CmsGroup: 0.009762
100 CmsGroup: 0.016044
.100 CmsUser: 0.139612
100 CmsUser: 0.026701
. Compute ChangeSet 100 objects in 0.03905200958252 seconds


Time: 19 seconds, Memory: 265.75Mb
@beberlei beberlei merged commit 05e5ae8 into doctrine:master Nov 25, 2012

1 check passed

Details default The Travis build passed
@FabioBatSilva FabioBatSilva deleted the FabioBatSilva:refactory-persisters branch Jan 18, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment