Dev/Core#90 Disable Only Full Group By sql mode on specific queries t… #12043
Conversation
|
@seamuslee001 can we leave the reports out of the first iteration of this? I think we can refine it a bit - I would be happy to merge on pass the rest |
|
ok i'll move that to another PR |
…o get tests to pass so we can switch to using MySQL 5.7 for PR Tests
seamuslee001
force-pushed
the
dev_core_90
branch
from
29d5e16 to
84cb7d1
Compare
|
@eileenmcnaughton done now |
|
I'm happy if jenkins is |
|
merging as per merge on pass |
|
Will be interesting to see test run overnight |
|
@eileenmcnaughton there are 3 groups of test failures these which = about 2 tests failures, then there are the reports that = about 4 test failures and then there are the 2 international strings API call failures |
| $dao->query($query); | ||
|
|
||
| CRM_Core_DAO::enableFullGroupByMode(); |
There was a problem hiding this comment.
I'm happy if jenkins is
Agree and disagree with this comment.
The idea of temporarily disabling ONLY_FULL_GROUP_BY seems good. (I originally voted to disable it across all of Civi... so doing it in a narrower way is also OK.)
The implementation in this PR feels a little a leaky -- e.g. if the system starts in
ONLY_FULL_GROUP_BY=on, then this correctly restores it. However, if the system starts without ONLY_FULL_GROUP_BY=off, then this incorrectly switches us to ONLY_FULL_GROUP_BY=on. A leak like this can influence other code.
In practice, TrackableURLOpen is generally called on its own (without a lot of other business logic), so a leak isn't likely to be symptomatic. The main time we care about leaks is in running unit-tests. Thus, I mostly agree with @Eileen's point -- if it works in the tests, then it's OK.
However, TrackableURLOpen is a bit special in how very isolated the use-case is. If you were to reproduce this coding pattern in other contexts, then there'd be more risk of a leak producing symptoms.
Which is a long-winded way of saying... this passes and it's OK, but when making a temporary change to global state in the future, let's try to be a little more strict about cleanup.
There was a problem hiding this comment.
@totten will it turn it back on tho? i think by the inclusion of the checking of this function https://github.com/civicrm/civicrm-core/blob/master/CRM/Utils/SQL.php#L114 in the turning back on function will mean that ONLY if its in the global defaults and its not present at the moment will the ONLY_FULL_GROUP_BY mode be added back to the sessions sql_modes
There was a problem hiding this comment.
@seamuslee001 @totten should the function name be changed to reenableFullGroupByModeIfConfigured or something to better match what it does?
There was a problem hiding this comment.
I would be fine with that
There was a problem hiding this comment.
It's worth getting this right because I believe this approach should be our first line of defence against breakage in full group by mode & once we stop seeing new ones we should start to slowly & with many tests tackle the root cause of the queries. We have had too many resulting breakages up until now from full group by fixes for me to be comfortable with fixing the queries largely in isolation.
There was a problem hiding this comment.
@seamuslee001 but don't you hate the way re-enable doesn't camel well?
There was a problem hiding this comment.
Oh, good call in pointing circa line 114. I didn't realize that it was based on the default (aka @@global.sql_mode). Agree it's not a functional problem and just a misinterpretation of the function's intent -- reenableFullGroupByModeIfConfigured() does seem more astute description.
…o get tests to pass so we can switch to using MySQL 5.7 for PR Tests
Overview
This disables the
ONLY_FULL_GROUP_BYsql mode on specific queries to get the failed tests here https://test.civicrm.org/job/CiviCRM-Core-Matrix/CIVIVER=master,label=ubuntu1604/lastCompletedBuild/testReport/ to passBefore
Tests fail on MySQL 5.7
After
Tests Pass
ping @eileenmcnaughton @monishdeb @totten