provide transactional interface for EntityManager and Connection #171

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
5 participants
@dfreudenberger

No description provided.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 12, 2012

This pull request fails (merged 6da6b7f into 00ac50c).

This pull request fails (merged 6da6b7f into 00ac50c).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 12, 2012

This pull request fails (merged 6ae5658 into 00ac50c).

This pull request fails (merged 6ae5658 into 00ac50c).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 12, 2012

This pull request fails (merged 5ef5a37 into 00ac50c).

This pull request fails (merged 5ef5a37 into 00ac50c).

@stof

View changes

lib/Doctrine/DBAL/Transactional.php
+ */
+interface Transactional
+{
+ public function transactional(\Closure $func);

This comment has been minimized.

@stof

stof Jul 12, 2012

Member

If we add an interface, I don't think it should typehint \Closure but allow any callable instead (without typehinting them as it is only possible on 5.4). I don't see why other callables should be forbidden.
Removing teh typehint is possible in the Connection class, but it should be done before adding an interface as changing the typehint in an interface is a BC break.

@beberlei thoughts ?

@stof

stof Jul 12, 2012

Member

If we add an interface, I don't think it should typehint \Closure but allow any callable instead (without typehinting them as it is only possible on 5.4). I don't see why other callables should be forbidden.
Removing teh typehint is possible in the Connection class, but it should be done before adding an interface as changing the typehint in an interface is a BC break.

@beberlei thoughts ?

This comment has been minimized.

@beberlei

beberlei Jul 12, 2012

Member

I am +1 on removing \Closure and make a docblock hint for "callable"

@beberlei

beberlei Jul 12, 2012

Member

I am +1 on removing \Closure and make a docblock hint for "callable"

This comment has been minimized.

@dfreudenberger

dfreudenberger Jul 12, 2012

i totally agree with you. i'm just wondering why you typehint \Closure in EntityManager and Connection? Is there a good reason why a concrete class should be more specific than an interface?

@dfreudenberger

dfreudenberger Jul 12, 2012

i totally agree with you. i'm just wondering why you typehint \Closure in EntityManager and Connection? Is there a good reason why a concrete class should be more specific than an interface?

This comment has been minimized.

@stof

stof Jul 12, 2012

Member

@dfreudenberger because of a crappy decision at the beginning (we have the same issue in collections). It can be changed in the EntityManager and the Connection (and will be needed if the interface enforces no typehint) but cannot be changed for collections without a BC break unfortunately.

@stof

stof Jul 12, 2012

Member

@dfreudenberger because of a crappy decision at the beginning (we have the same issue in collections). It can be changed in the EntityManager and the Connection (and will be needed if the interface enforces no typehint) but cannot be changed for collections without a BC break unfortunately.

@stof

View changes

lib/Doctrine/DBAL/Transactional.php
+ *
+ * @license http://www.opensource.org/licenses/lgpl-license.php LGPL
+ * @link www.doctrine-project.com
+ * @version $Revision$

This comment has been minimized.

@stof

stof Jul 12, 2012

Member

this should be removed

@stof

stof Jul 12, 2012

Member

this should be removed

@stof

View changes

lib/Doctrine/DBAL/Transactional.php
+/**
+ * Interface for transactional aware classes
+ *
+ * @license http://www.opensource.org/licenses/lgpl-license.php LGPL

This comment has been minimized.

@stof

stof Jul 12, 2012

Member

this is wrong. The code is MIT

@stof

stof Jul 12, 2012

Member

this is wrong. The code is MIT

This comment has been minimized.

@dfreudenberger

dfreudenberger Jul 12, 2012

actually i copied the license and the $id$ part from Doctrine\DBAL\LockMode - but sure i'll fix it

@dfreudenberger

dfreudenberger Jul 12, 2012

actually i copied the license and the $id$ part from Doctrine\DBAL\LockMode - but sure i'll fix it

@stof

View changes

lib/Doctrine/DBAL/Transactional.php
@@ -0,0 +1,35 @@
+<?php
+/*
+ * $Id$

This comment has been minimized.

@stof

stof Jul 12, 2012

Member

this should be removed. It was used in the SVN area

@stof

stof Jul 12, 2012

Member

this should be removed. It was used in the SVN area

Daniel Freudenberger
droped typehint from signature
Doctrine\DBAL\Connection::transactional() throws exception for invalid argument
added unit test
@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 12, 2012

This pull request fails (merged 345ba96 into 00ac50c).

This pull request fails (merged 345ba96 into 00ac50c).

@dfreudenberger

This comment has been minimized.

Show comment
Hide comment
@dfreudenberger

dfreudenberger Jul 13, 2012

do you need anything else from me? not sure what the current status is

do you need anything else from me? not sure what the current status is

@dfreudenberger

This comment has been minimized.

Show comment
Hide comment
@dfreudenberger

dfreudenberger Jul 28, 2012

@stof ping

+ * wraps the provided callable in a transaction.
+ *
+ * @param Callable $func
+ * @return void

This comment has been minimized.

@beberlei

beberlei Jul 29, 2012

Member

The original methods didnt return void or? At least the EntityManager has a return value here. This should be replicated.

@beberlei

beberlei Jul 29, 2012

Member

The original methods didnt return void or? At least the EntityManager has a return value here. This should be replicated.

@beberlei

This comment has been minimized.

Show comment
Hide comment
@beberlei

beberlei Jul 29, 2012

Member

You need to rebase the PR onto master again, its not mergable anymore.

Member

beberlei commented Jul 29, 2012

You need to rebase the PR onto master again, its not mergable anymore.

@ramonornela

This comment has been minimized.

Show comment
Hide comment
@ramonornela

ramonornela Aug 16, 2013

Contributor

This interface should be to Common.

Contributor

ramonornela commented Aug 16, 2013

This interface should be to Common.

@beberlei

This comment has been minimized.

Show comment
Hide comment
@beberlei

beberlei Dec 20, 2013

Member

I don't think this adds value over not having an interface. There are very seldom situations where you need this API independent of the actual underyling connection.

Member

beberlei commented Dec 20, 2013

I don't think this adds value over not having an interface. There are very seldom situations where you need this API independent of the actual underyling connection.

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