Allow setting sql_mode via config

[blueo]

MYSQL >=5.7.5 introduced a change to the ANSI alias for the sql_mode setting. Namely, it now includes ONLY_FULL_GROUP_BY. While this may be more correct SQL it can cause problems eg:

• Test failure in MySQL 5.7 #5451
• Fluent extension causes Errors with MySQL version >= 5.7.5 tractorcow-farm/silverstripe-fluent#257

Projects built to 5.6 will have a few surprises when upgrading to 5.7 as there is no warning on the current behaviour.

And there have been a few attempts to fix it outside of framework eg:

• BUG Fix compatibility issues with mysql 5.7 and above tractorcow-farm/silverstripe-fluent#617
• https://github.com/SpliffSplendor/silverstripe-mysqlfixer
• https://github.com/sunnysideup/silverstripe-mysql-5-7-fix/

So i'm proposing we make this a config option and those who need to/know what they're after can change the connection mode.

[dhensby]

This seems fair enough - though I'm not sure that changing the mode is the right way to fix issues where more specific SQL is required.

[sminnee]

SQL mode ANSI was necessary for double-quoted column names to work when it was first introduced.

If SQL mode isn't ANSI does the ORM still work properly? We should probably run the test suite with this setting.

If this is basically the "5.7 support" flag then it might make sense to document as such and include a 5.7 build in our CI matrix.

[dhensby]

@sminnee I think the issue is that ANSI mode is short hand for quite a few different modes to be enabled (including ANSI_QUOTES) and in 5.7 ONLY_FULL_GROUP_BY was bundled into ANSI "mode". (see #5451 (comment))

For modules/code that doesn't provider a fully qualified GROUP BY clause the fix is either:

1. Fix code to provide full GROUP BY clause (high effort)
2. Change SQL mode to cherry-pick the modes required to conform with ANSI except ONLY_FULL_GROUP_BY (low effort)

As with issue #5451 - it's really an issue with query construction (and maybe we could have a bit more "magic" in our query builders to add the table prefix to the GROUP BY clause) - this just allows a bit of developer freedom to be lazy whilst our query builder remains a little less magical in this area than in others

[sminnee]

My main concern here is that the ORM relies on the SQL mode, so on the face of it proving an open-ended SQL mode selector seems like a "maybe this breaks the ORM" button.

So if there's a different SQL mode that is necessary to get 5.7 to work, we should focus on defining and allowing that.

[sminnee]

Alternatively we could find a SQL mode that works in both 5.6
and 5.7 and hard code that in, with representation of each in our build matrix.

My concern with this config option is that we'll end up with many different SQL modes in the wild and effectively sign up for supporting each of them.

I naively suspect that there is, in fact, "one right answer" here. ;-)

[blueo]

As far as I'm aware I don't think this a requirement for 5.7 - as this is mainly in the scenario that you're using custom queries (eg the fluent issue above) but I could be wrong.

As far as having the option - we allow configuring almost all the other options for the connection but this one is hard-coded. It feels a little prescriptive to hard code this as we can't really foresee how things might change or others' use cases. I don't think this implies support of any old sql mode. Allowing people to set options is the kind of flexibility we encourage with other parts of the system why is this one different?

Defaulting to having FULL_GROUP_BY seems reasonable and encourages more deterministic SQL so would seem a good thing to keep. But given 5.6 is EOL in feb, I can foresee a bunch of people running into issues with this when they upgrade. The option to dial it back would likely reduce support issues.

[sminnee]

As far as I'm aware I don't think this a requirement for 5.7 - as this is mainly in the scenario that you're using custom queries (eg the fluent issue above) but I could be wrong.

Improving the build matrix will help with that.

As far as having the option - we allow configuring almost all the other options for the connection but this one is hard-coded.

Perhaps. But even if we do have this option I wouldn't want it to be a necessary part of solving the problem you're experiencing right now ;-) key modules shouldn't require hacks to deal with 5.6's EOL.

So while we may add set mode as an @internal "danger zone" API, I think we do need to reconsider the default value we have in place.

[sminnee]

As far as having the option - we allow configuring almost all the other options for the connection but this one is hard-coded

I think my concern is that I want to make it impossible to turn off ANSI_QUOTES, and maybe some other setting without which the ORM would be bricked.

So perhaps this config setting is more like an array argument of flags and some minimal flags are enforced by the connector.

Then I'd have in the CI matrix a config that includes only those minimal flags to confirm that framework does not require any others.

[sminnee]

OK so having had a look at this:

• ANSI implies REAL_AS_FLOAT, PIPES_AS_CONCAT, ANSI_QUOTES, IGNORE_SPACE, and (as of MySQL 5.7.5) ONLY_FULL_GROUP_BY.

• Only ANSI_QUOTES is necessary for the functioning of the ORM.

So I think we should:

• Update framework to only set REAL_AS_FLOAT, PIPES_AS_CONCAT, ANSI_QUOTES, IGNORE_SPACE by default instead of ANSI. This means that ONLY_FULL_GROUP_BY is excluded by default, retaining consistent behaviour in 5.6 and 5.7 which I think is most appropriate for the framework
• Amend this PR so that sql_mode is an array
• If ANSI_QUOTES is excluded from the list, throw an error that ANSI_QUOTES is necessary for the functioning of the ORM (probably less confusing than silently adding it to the list)

When setting the default value note that an array without keys is a pain to modify in yml config so I'd probably set it up like this:

/**
 * @var array
 * Set the SQL modes to apply to the connection.
 * ANSI_QUOTES is mandatory
 * The keys are not used; they are to provide support for clearing values in yml config.
 */
private static $sql_mode = [
    'ANSI_QUOTES' => 'ANSI_QUOTES',
    'REAL_AS_FLOAT' => 'REAL_AS_FLOAT', 
    'PIPES_AS_CONCAT' =>  'PIPES_AS_CONCAT',
    'IGNORE_SPACE' => 'IGNORE_SPACE', 
];

And then use this to get the value:

array_filter(array_values(static::config()->get(sql_mode));

Which lets you do this to clear a default mode

Bla\MySQLDatabase:
  sql_mode:
    REAL_AS_FLOAT: null

[sminnee]

@silverstripe/core-team but particularly @dhensby keen to get your view on whether removing ONLY_FULL_GROUP_BY to preserve 5.6/5.7 continuity is appropriate. Notably, there are no SQL queries that require ONLY_FULL_GROUP_BY - it's a flag that adds strictness rather than taking it away.

[dhensby]

I don't really mind making this change, but I do think it's unnecessary because there's nothing in the ORM breaking by having these settings. As I understand it, developers need to be implementing their own SQLQuery to have this problem and if they are going to be using MySQL 5.7 then they should probably be writing good SQL and SQL that doesn't get rejected by the DB server. I'd probably just change the documentation around DB compatibility pointing out that MySQL 5.7 is a bit stricter about query construction. If there's a developer out there that really hates having to write full group bys, they can override the DB driver (as some of the linked issues in the description do).

It just feels a bit like PHP magic_quotes_gpc config setting - it's just a config setting to allow developers to be lazy - and I'm not sure we should be encouraging laziness around SQL query construction because it could just be an even bigger pain going forward (and who knows what kind of compatibility issues it could have later).

My order of preference is probably:

1. Update docs to make it clear that by default MySQL 5.7 requires proper GROUP BY clauses
2. Allow it to be configurable but maintain our current default
3. @sminnee's proposal

Having said that, I won't object too strongly because I do believe flexibility and configure-ability is a good thing and if developers want to make changes that break the ORM, that's not really our problem; if there are modules having to implement brittle "hacks" (like fluent) then I'd rather provide them an official API surface to do it for the cases it's needed.

---

PS: I understand this is a bit contradictory - I've approved this change but I'm saying I'd prefer just to tell people to live with it as is - that's because whilst I might have an idealistic preference, I accept that's not much comfort to people who feel they need this change

[chillu]

I'm with Dan on this. Unless our highlevel ORM API produces this issue, I don't think there's a good reason why we should invite this laziness in custom queries by default, and further increase the complexity of our API through Sam's proposal.

But I'm also with Bernie. Making sql_mode configurable unblocks his use case. We can flag it as an advanced/internal configuration setting, and strongly encourage devs using it to fix the root problems (modules producing bad queries) instead.

In terms of build matrix, we've been testing exclusively on MySQL 5.7 for a while (since xenial). At least in Travis, it looks like you can't mix different dists, which would be required to test on different MySQL versions. We could test different mariadb versions though.

[sminnee]

5.6 is going EOL which means we're going to need to force 5.7 on a bunch of users. I'm not sure why "small API change to reduce the pain there" is a bad idea.

This is not about "encouraging laziness" this is about "retaining consistent behaviour".

I could see why we might not want to change the default, but at a minimum, I'd introduce the new API and provide instructions for changing the setting for those who upgrade to MySQL 5.7 and hit this issue.

[sminnee]

If you want to make sql_mode configured as a string as this patch provides, can we please ensure that the docblock clearly says "you must includes ANSI_QUOTES or your site will be completely non-functional".

It would also be worth including in the docblock the recommendation of "REAL_AS_FLOAT, PIPES_AS_CONCAT, ANSI_QUOTES, IGNORE_SPACE" if you have upgraded from 5.6 to 5.7 and are hitting issues.

[Cheddam]

Since there seems to be agreement that this PR should proceed (retaining the current default but making it configurable), let's get this merged in time for 4.7.

Per the discussion above, I've suggested an expanded description for the config itself. I'd recommend adding a note to the documentation - perhaps in the DB section of Server Requirements - about the situational adjustments when running 5.7, and linking to this in the provisional 4.7.0 changelog (and possibly within the docblock too.)

@blueo Will you have time to make these additions within the next few days? If not, we can organise this within the CMS Squad.

[blueo]

Since there seems to be agreement that this PR should proceed (retaining the current default but making it configurable), let's get this merged in time for 4.7.

Per the discussion above, I've suggested an expanded description for the config itself. I'd recommend adding a note to the documentation - perhaps in the DB section of Server Requirements - about the situational adjustments when running 5.7, and linking to this in the provisional 4.7.0 changelog (and possibly within the docblock too.)

@blueo Will you have time to make these additions within the next few days? If not, we can organise this within the CMS Squad.

👍 Should be able to get onto this in the next couple of days @Cheddam

[blueo]

@Cheddam I've added some doc updates in addition to your suggestions.

[Cheddam]

Thanks @blueo! A couple of minor language tweaks, otherwise I'm happy with this 👍