Revert "Merge pull request #888 from goetas/update-query" #1071
Conversation
|
@greg0ire this commit message generated by git itself 😬. I understand the message is confusing, but it talks about the parent |
|
Oh right, I misread, didn't notice it was changes applied to b5d7611 . Anyway, an automated message cannot be as good as the one you could write as suggested in #888 (comment) , please edit the message to include the information to understand why that revert is necessary. |
|
I'm sorry that this created so many issue, but i'm ok for reverting it 👍 |
from: https://dev.mysql.com/doc/refman/8.0/en/repair-table.html > REPAIR TABLE returns a result set with the columns shown in the following table. That's why doctrine/migrations should not use `executeUpdate`, because results are not read. So the user can end with the following errors ``` SQLSTATE[HY000]: General error: 2014 Cannot execute queries while other unbuffered queries are active. Consider using PDOStatement::fetchAll(). Alternatively, if your code is only ever going to run against mysql, you may enable query buffering by setting the PDO::MYSQL_ATTR_USE_BUFFERED_QUERY attribute. ``` More over, `executeUpdate` is deprecated
|
@greg0ire is it OK now ? |
Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>
|
There is a new failure I didn't see before in Appveyor, but since this is a revert, it's fine to merge, let's proceed, hopefully we will be able to fix the build soon 😬 |
|
Thanks @lyrixx ! |
|
Hello, May I ask for a release of 2.3.1? Thanks |
| @@ -345,10 +345,11 @@ private function executeVersionExecutionResult( | |||
|
|
|||
| $this->outputSqlQuery($key, $query); | |||
|
|
|||
| // executeQuery() must be used here because $query might return a result set, for instance REPAIR does | |||
There was a problem hiding this comment.
then, to be sure to be compatible with PrimaryReadReplicaConnection (which allows executeQuery to run on the read replicas), we should add a check to call ensureConnectedToPrimary when the connection is a PrimaryReadReplicaConnection (for transactional migrations, there is a good chance to be connected on the primary due to starting the transaction, but it is possible to disable all usages transactions in doctrine/migrations AFAIK)
There was a problem hiding this comment.
That wouldn't qualify as a bugfix, so in 3.1.x, right? That being said, your remark makes me notice that we have instanceof MasterSlaveConnection that should be replaced with instanceof PrimaryReadReplicaConnection
|
hello @greg0ire ; Now I'm trying to update to 3.x, and I'm facing the very same issue :) |
|
I was on holidays, but it looks like you have been heard: https://github.com/doctrine/migrations/releases/tag/3.0.2 :) |
This reverts commit b13df49, reversing
changes made to b5d7611.