Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

code refactorings on persister #413

Merged
merged 16 commits into from Nov 25, 2012

Conversation

FabioBatSilva
Copy link
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
Copy link

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

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing line break

@travisbot
Copy link

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

@fprochazka
Copy link
Contributor

You rock :)


foreach ($updateData as $columnName => $value) {
$column = $columnName;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove extra line break

@travisbot
Copy link

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

@FabioBatSilva
Copy link
Member Author

Hi guys :)

Anything else pending here ?

* Check for existance of an element
*
* @param \Doctrine\ORM\PersistentCollection $coll
* @param mixed \Doctrine\ORM\PersistentCollection
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ param mixed \Doctrine\ORM\PersistentCollection ?

this seems wrong

@guilhermeblanco
Copy link
Member

I'm fine to merge this. Anyone else?


/**
* @var \Doctrine\ORM\UnitOfWork
*/
protected $_uow;
protected $uow;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and the same is true for protected methods

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then it is fine

@FabioBatSilva
Copy link
Member Author

Done,

Thanks @guilhermeblanco and @stof

@guilhermeblanco
Copy link
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
Copy link
Member Author

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 added a commit that referenced this pull request Nov 25, 2012
@beberlei beberlei merged commit 05e5ae8 into doctrine:master Nov 25, 2012
@FabioBatSilva FabioBatSilva deleted the refactory-persisters branch January 18, 2013 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants