Zend\Db transaction api unification #5001

corentin-larose · 2013-08-21T21:05:33Z

Added an abstract to avoid repetiting code between adapters.
Fixed contracts (for example dockblock said @return self for fluent interface but method didn't actually return $this).
Added more consistancy in the methods between adapters
Added unit tests

This work is an introduction to another PR to come.

samsonasik · 2013-09-02T17:31:49Z

library/Zend/Db/Adapter/Driver/ConnectionAbstract.php

+    /**
+     * Disconnect
+     *
+     * @return Connection


@return self

corentin-larose · 2013-09-03T07:30:31Z

Ok, gonna fix that, thanks!

corentin-larose · 2013-09-03T07:40:04Z

Done

corentin-larose · 2013-09-19T14:03:43Z

@samsonasik should be ok/compliant if you want to merge.

samsonasik · 2013-09-19T14:47:36Z

👍 I have no access for that :D

mwillbanks · 2013-10-09T18:50:16Z

@ralphschindler you'll want to review this one.

Maks3w · 2013-10-21T09:28:51Z

library/Zend/Db/Adapter/Driver/IbmDb2/Connection.php

     */
    public function beginTransaction()
    {
        // TODO: Implement beginTransaction() method.
+
+        return $this;


Should this methods throw a BadMethodCallException until implement the code?

Talked about that with @weierophinney @zendcon today, would be a BC break. So, I propose a first "non breaking" PR followed by another with these fixes.

corentin-larose · 2013-10-21T13:53:44Z

Hello @Maks3w, just let me know for your BadMethodCallException suggestion, since there were no exception there, I didn't want to change this for BC.

Maks3w · 2013-10-21T19:39:34Z

library/Zend/Db/Adapter/Driver/Pgsql/Connection.php

-        if (!$this->inTransaction) {
-            return;
+        if (!$this->isConnected()) {
+            throw new Exception\RuntimeException('Must be connected before you can rollback');


These exceptions are a BC break and a break in the Interface contract

@Maks3w In fact, depending on the adapter your are looking at, some throwed before my unification, other don't, since this is an adapter pattern, I think that adapters should work the same way. I agree for the BC break, but shouldn't we unify the way these adapters work ?

I don't like the idea of throw exceptions in rollback

By the moment undo this because is not related with the content of the PR. If u want add a new PR later unifying the behavior of this method.

Hello @Maks3w

I don't get your point, my PR title is "Zend\Db transaction api unification", some of the adapters and not the least (pdo, mysqli, oci8) already throw in rollback which is a part of the transaction system, it seems to me that an unification should make them work the same way.

If you don't like the throw in rollback, we also could remove it from the three most used adapters, but I think this is far beyond my PR in term of BC break.

I don't refuse to rollback this change, I just want to be sure you get the entire point.

If this is only a BC problem, I can make a separate PR for the rollback unification.

Let me know

corentin-larose · 2013-11-19T20:05:59Z

Hello @weierophinney, @Maks3w, @ralphschindler,

after our ZendCon talk I fixed the points we discussed.

Let me know if something is missing for you.

ralphschindler · 2013-11-19T20:38:01Z

library/Zend/Db/Adapter/Driver/AbstractConnection.php

+
+use Zend\Db\Adapter\Profiler;
+
+abstract class AbstractConnection implements ConnectionInterface, Profiler\ProfilerAwareInterface


I'm sure there will be developers that will disagree with me and who's frame of reference is to always use inheritance even for trivial things, but this abstraction seems unnecessary to me. The guts of this class do nothing algorithmically interesting and since that is the case, I see little need to introduce a base type that all driver connection classes inherit from.

As I told before, this abstract was a basis for another PR, so, I'm going to push it either in order to show you my final goal

weierophinney · 2014-03-03T16:35:31Z

@ralphschindler Where does this stand in regards to review and milestone?

weierophinney · 2014-05-12T21:38:43Z

@corentin-larose Can you rebase off of latest develop for me, please? Personally, I'm on board with creating an abstract implementation -- it's less code for us to maintain over the long run, and it clearly provides benefits with regards to unification of behaviors (which is the goal of a DBAL as far as I'm concerned). I think we can merge this for 2.4.

corentin-larose · 2014-05-13T10:06:56Z

Just rebased #5001 on develop then rebased #5505 on #5001.

Ocramius · 2014-10-10T21:09:55Z

@corentin-larose this PR needs a rebase

corentin-larose · 2014-10-11T14:47:05Z

I have to review once again this is only a tmp commit

corentin-larose · 2014-10-11T15:05:05Z

Should be ok for 2.4

Close #6699

… `Zend\Db\Adapter\Driver\IbmDb2\Connection`

… `Zend\Db\Adapter\Driver\Mysqli\Connection`

… `Zend\Db\Adapter\Driver\Oci8\Connection`

… `Zend\Db\Adapter\Driver\Pdo\Connection`

… `Zend\Db\Adapter\Driver\Pgsql\Connection`

… `Zend\Db\Adapter\Driver\Sqlsrv\Connection`

…\AbstractConnection`

…filerAwareInterface`

Close #5001

Ocramius · 2014-11-12T11:31:48Z

Manually merged, thanks :-)

Ocramius · 2014-11-12T11:33:17Z

@corentin-larose please note that I had to remove some commits/squash them due to the rebase not being handled cleanly upfront, so no panic if the commit message is not the original one.

corentin-larose · 2014-11-13T21:21:27Z

Hmm this is what happens when letting a PR hanging around for 18 months :D, Went back an dforth from dev to master, conflicts and so...

Ocramius · 2014-11-13T22:37:05Z

@corentin-larose no big deal, I just filtered out the correct commits :-)

samsonasik reviewed Sep 2, 2013
View reviewed changes

library/Zend/Db/Adapter/Driver/ConnectionAbstract.php

/**

* Disconnect

*

* @return Connection

Copy link

Contributor

samsonasik Sep 2, 2013

Choose a reason for hiding this comment

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

@return self

Maks3w reviewed Oct 21, 2013
View reviewed changes

ralphschindler reviewed Nov 19, 2013
View reviewed changes

This was referenced Nov 19, 2013

Zend db nested transactions #5504

Closed

Zend db nested transactions #5505

Closed

ralphschindler added this to the 3.0.0 milestone Mar 4, 2014

ralphschindler self-assigned this Mar 4, 2014

weierophinney modified the milestones: 2.4.0, 3.0.0 May 12, 2014

weierophinney mentioned this pull request May 12, 2014

fix Zend\Db\Adapter\Platform\PlatformInterface::quoteIdentifierInFragment #5683

Closed

EvanDotPro force-pushed the develop branch from 8f4d208 to a2c77c3 Compare September 28, 2014 00:34

Rebase for 2.4

3493bbc

Ocramius and others added 2 commits October 13, 2014 13:10

Merge branch 'cs/#6699-zend-dom-psr-2-compat'

3e6936d

Close #6699

Merge branch 'master' into zend-db-transac-api-unification

6e8a1d9

Ocramius assigned Ocramius and unassigned weierophinney Nov 12, 2014

Ocramius pushed a commit that referenced this pull request Nov 12, 2014

PR #5001 - transaction API unification

02c0bda

Ocramius added a commit that referenced this pull request Nov 12, 2014

#5001 - avoiding overriding docs for inherited/implemented methods in…

077afad

… `Zend\Db\Adapter\Driver\IbmDb2\Connection`

Ocramius added a commit that referenced this pull request Nov 12, 2014

#5001 - avoiding overriding docs for inherited/implemented methods in…

0672926

… `Zend\Db\Adapter\Driver\Mysqli\Connection`

Ocramius added a commit that referenced this pull request Nov 12, 2014

#5001 - avoiding overriding docs for inherited/implemented methods in…

600e59f

… `Zend\Db\Adapter\Driver\Oci8\Connection`

Ocramius added a commit that referenced this pull request Nov 12, 2014

#5001 - avoiding overriding docs for inherited/implemented methods in…

3df1e40

… `Zend\Db\Adapter\Driver\Pdo\Connection`

Ocramius added a commit that referenced this pull request Nov 12, 2014

#5001 - avoiding overriding docs for inherited/implemented methods in…

c645165

… `Zend\Db\Adapter\Driver\Pgsql\Connection`

Ocramius added a commit that referenced this pull request Nov 12, 2014

#5001 - avoiding overriding docs for inherited/implemented methods in…

f442d03

… `Zend\Db\Adapter\Driver\Sqlsrv\Connection`

Ocramius added a commit that referenced this pull request Nov 12, 2014

#5001 - Fixed default values and docblocks in `Zend\Db\Adapter\Driver…

5a4e3fb

…\AbstractConnection`

Ocramius added a commit that referenced this pull request Nov 12, 2014

#5001 - Implemented missing docblock in `Zend\Db\Adapter\Profiler\Pro…

2e85a4b

…filerAwareInterface`

Ocramius added a commit that referenced this pull request Nov 12, 2014

#5001 - cs fixes and cleanups in the transaction PDO connection tests

ef7246e

Ocramius added a commit that referenced this pull request Nov 12, 2014

#5001 - license headers and PSR-2 compliance for PdoMock

6a96270

Ocramius added a commit that referenced this pull request Nov 12, 2014

#5001 - license headers and PSR-2 compliance for PdoMock

54e9b66

Ocramius added a commit that referenced this pull request Nov 12, 2014

Merge branch 'feature/#5001-transaction-api-unification' into develop

527908b

Close #5001

Ocramius closed this Nov 12, 2014

Ocramius removed the awaiting author updates label Nov 12, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zend\Db transaction api unification #5001

Zend\Db transaction api unification #5001

corentin-larose commented Aug 21, 2013

samsonasik Sep 2, 2013

corentin-larose commented Sep 3, 2013

corentin-larose commented Sep 3, 2013

corentin-larose commented Sep 19, 2013

samsonasik commented Sep 19, 2013

mwillbanks commented Oct 9, 2013

Maks3w Oct 21, 2013

corentin-larose Nov 19, 2013

corentin-larose commented Oct 21, 2013

Maks3w Oct 21, 2013

corentin-larose Oct 22, 2013

Maks3w Oct 22, 2013

corentin-larose Oct 22, 2013

corentin-larose commented Nov 19, 2013

ralphschindler Nov 19, 2013

corentin-larose Nov 19, 2013

weierophinney commented Mar 3, 2014

weierophinney commented May 12, 2014

corentin-larose commented May 13, 2014

Ocramius commented Oct 10, 2014

corentin-larose commented Oct 11, 2014

corentin-larose commented Oct 11, 2014

Ocramius commented Nov 12, 2014

Ocramius commented Nov 12, 2014

corentin-larose commented Nov 13, 2014

Ocramius commented Nov 13, 2014


		use Zend\Db\Adapter\Profiler;

		abstract class AbstractConnection implements ConnectionInterface, Profiler\ProfilerAwareInterface

Zend\Db transaction api unification #5001

Zend\Db transaction api unification #5001

Conversation

corentin-larose commented Aug 21, 2013

Choose a reason for hiding this comment

corentin-larose commented Sep 3, 2013

corentin-larose commented Sep 3, 2013

corentin-larose commented Sep 19, 2013

samsonasik commented Sep 19, 2013

mwillbanks commented Oct 9, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corentin-larose commented Oct 21, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corentin-larose commented Nov 19, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Mar 3, 2014

weierophinney commented May 12, 2014

corentin-larose commented May 13, 2014

Ocramius commented Oct 10, 2014

corentin-larose commented Oct 11, 2014

corentin-larose commented Oct 11, 2014

Ocramius commented Nov 12, 2014

Ocramius commented Nov 12, 2014

corentin-larose commented Nov 13, 2014

Ocramius commented Nov 13, 2014