Don't use double quotes in MySQL queries #8391
Conversation
MorrisJobke
requested review from
rullzer,
nickvergessen,
icewind1991,
ChristophWurst and
blizzz
| @@ -70,6 +70,7 @@ protected function setUp() { | |||
|
|
|||
| $dbPrefix = $this->config->getSystemValue("dbtableprefix"); | |||
| $this->tableName = $this->getUniqueID($dbPrefix . "_collation_test"); | |||
| $this->connection->exec("SET SESSION sql_mode = CONCAT(@@SESSION.sql_mode, ',ANSI_QUOTES')"); | |||
There was a problem hiding this comment.
This is the only way to ensure that the MySQL server has enabled the ANSI_QUOTES mode for the test that I've found. If there is a better solution to configure the MySQL server for the tests just let me know.
There was a problem hiding this comment.
But is it necessary to make the tests pass? If not, I would leave it out, to make sure it works even without it.
Because now you force it on the unit tests, but in production we work without the option, so we can not be sure they behave the same
There was a problem hiding this comment.
The test will pass also without this line. I just added the line after reading the contribution guide. This guide "forces" tests for changed code. If it's OK for you that there isn't a test for this change I'll remove this line. Can I do this with a force push or will this break the history of the PR?
There was a problem hiding this comment.
Yeah remove it and do a force push
There was a problem hiding this comment.
Maybe we could have both cases covered as well?
There was a problem hiding this comment.
I could clone the method testCollationConvert and enable the ANSI_QUOTES mode just for the new method to cover both cases. But I think the better way of testing this would be either to run all MySQL tests for a MySQL server that has enabled the ANSI_QUOTES mode and for one that hasn't or to enable the mode at all. This would prevent any further issues in the future where double quotes are used within MySQL queries. An extra test for the collation convert would just prevent that someone will add again double quotes within this which should be very unlikely.
There was a problem hiding this comment.
True point. Would you mind to create this in a separate PR?
There was a problem hiding this comment.
I'd definitively do this in a separate PR. But at the moment I haven't got the time for doing this. So I'll remove this line for the moment. Maybe there is some time next week to have a deeper look into the testing stuff.
Codecov Report
@@ Coverage Diff @@
## master #8391 +/- ##
============================================
+ Coverage 51.71% 51.77% +0.06%
Complexity 25404 25404
============================================
Files 1600 1600
Lines 95135 95135
Branches 1377 1377
============================================
+ Hits 49199 49258 +59
+ Misses 45936 45877 -59
|
MySQL databases with the ANSI_QUOTES mode enabled treat " as an identifier quote (see https://dev.mysql.com/doc/refman/5.7/en/sql-mode.html#sqlmode_ansi_quotes). So for such databases the 'occ upgrade' fails with an error message like this: ... unknown column 'oc_*' in where clause. This fix replaces the doulbe quotes with single quotes that should be always used in MySQL queries to quote literal strings. Signed-off-by: Robin Müller <robin.mueller@1und1.de>
coder-hugo
force-pushed
the
feature/mysql_ansi_quotes
branch
from
5c4aa17 to
6aa8169
Compare
MySQL databases with the ANSI_QUOTES mode enabled treat
"as an identifier quote. So for such databases the 'occ upgrade' fails with an error message like this:... unknown column 'oc_*' in where clause.
This fix replaces the doulbe quotes with single quotes that should be always used in MySQL queries to quote literal strings.