Fixed #6167 - Forced SELECT NEXTVAL to be run on master #6168
Conversation
|
Requires a test case |
|
And it seems it broke support for other databases. |
|
Ok, so this does not break any DB support, it just does not fit the tests written. I will provide a fix and a new test for this scenario. |
|
@Ocramius I have fixed the existing test, but I cannot think about a good test that could be run in CI. Travis does not support master-slave setup, and the whole logic is inside MasterSlaveConnection that I did not touch at all. I would have to write a custom test that wouldn't be run on Travis, or I would have to mock a lot of things just to check that MasterSlaveConnection::query() is run on the master server. Please let me know if I'm missing something and there is a better way to test this case. |
|
@mkurzeja what you could do is writing a test that asserts that Can be done via a mock/spy, and indeed needs some documentation around it. |
…equence nextval
| for ($i=0; $i < 42; ++$i) { | ||
| if ($i % 10 == 0) { | ||
| $this->_em->getConnection()->setFetchOneResult((int)($i / 10) * 10); | ||
| $nextId = array(array((int)($i / 10) * 10)); |
There was a problem hiding this comment.
@mkurzeja please use short-array syntax here ;)
| @@ -86,12 +97,24 @@ public function lastInsertId($seqName = null) | |||
| */ | |||
| public function fetchColumn($statement, array $params = array(), $colnum = 0, array $types = array()) | |||
| { | |||
| if ($this->_fetchOneException != null) { | |||
There was a problem hiding this comment.
Please, use Yoda condition and strict comparison.
| @@ -113,6 +136,16 @@ public function setFetchOneResult($fetchOneResult) | |||
| } | |||
|
|
|||
| /** | |||
| * @param \Exception $exception | |||
There was a problem hiding this comment.
Please, update this docblock in order to reflect that this param also accepts null.
| @@ -133,6 +166,14 @@ public function setLastInsertId($id) | |||
| } | |||
|
|
|||
| /** | |||
| * @param Statement $result | |||
| */ | |||
| public function setQueryResult($result) | |||
There was a problem hiding this comment.
Can $result argument be declared as Statement?
| @@ -23,17 +24,20 @@ protected function setUp() | |||
|
|
|||
| public function testGeneration() | |||
| { | |||
| $this->_em->getConnection()->setFetchOneException( | |||
| new \Exception('Fetch* method used. Query method should be used instead, as NEXTVAL should be run on a master server in master-slave setup.') | |||
There was a problem hiding this comment.
SPL \LogicException or \RuntimeException could be less generic.
| @@ -86,12 +97,24 @@ public function lastInsertId($seqName = null) | |||
| */ | |||
| public function fetchColumn($statement, array $params = [], $colnum = 0, array $types = []) | |||
| { | |||
| if (null !== $this->_fetchOneException) { | |||
There was a problem hiding this comment.
This makes sense, but I'll probably have to change it to use the mocking framework to do this (on merge)
|
Hi, Any reason why is this not merged into 2.5 branch? Also is there a workaround if I want to use it in v2.5? Thanks. |
|
Hi! |
|
@wlzch @gigi I've just tried to backport this and unfortunately it cannot be done easily, since it has dependencies on things that only exists on As stated on every single |
|
@lcobucci |
|
@gigi not really, we need to get the tests as well to make sure things work and that's the tricky part |
… is used instead of `fetchColumn` (cherry picked from commit d2be4a2)
…cblocks/return-types (cherry picked from commit 462481e)
…r readability and error messages (cherry picked from commit a97c265)
A possible solution for #6167 - Doctrine ORM failing to insert to a master-slave PostgreSQL setup because of running NEXTVAL on a slave.