allow granular overriding of sql_mode options

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

This is what I had in mind.

Would need documentation in settings.php and a CR.

Hi,

I altered the patch, so it is compatible with Drupal Core 8.6.4.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Connection.php
@@ -452,16 +452,30 @@
-    // NO_AUTO_CREATE_USER is removed in MySQL 8.0.11
-    // https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-11.html#mysqld-8-0-11-deprecation-removal
-    $version_server = $pdo->getAttribute(\PDO::ATTR_SERVER_VERSION);
-    if (version_compare($version_server, '8.0.11', '<')) {
-      $sql_mode .= ',NO_AUTO_CREATE_USER';

The patch is losing this logic.

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule → and the Allowed changes during the Drupal 8 and 9 release cycles → .

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

I've restored the handling of MySQL 8.0.

This special handling for this one MySQL mode seems to me to add to the necessity for this patch.

> $sql_mode .= ',NO_AUTO_CREATE_USER';

Having to faff with strings like that is ugly.

And with the current functionality, any site that overrides this in its settings.php needs to know for sure if it's going to be run on MySQL 8.0 or earlier, as it must explicitly specify NO_AUTO_CREATE_USER or not. With the patch, handling of NO_AUTO_CREATE_USER can be left safely to core.

Hello,
I am trying to install Drupal 8.7 version on Azure platform with MySQL 8(exact version is 8.0.15) And I get the below error:

Variable 'sql_mode' can't be set to the value of 'NO_AUTO_CREATE_USER'. (See the attached screenshot).

Here the comparison condition "version_compare($version_server, '8.0.11', '<')" returns TRUE. Then I found out that the value of $version_server which is nothing but \PDO::ATTR_SERVER_VERSION returns the value as 5.6.42.0

Now, I am confused on why we get the value as 5.6.42.0? Is that the Microsoft Drivers for PHP for SQL Server needs to be updated? Or anything needs to be done at Azure end?

For now, I have went ahead with the installation of Drupal 8.7 by commenting out this condition check and installed the Drupal 8.7 version in MySQL 8. But would be curious to know the reason behind this. Thanks.

Drupal 9.2.0-alpha1 → will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.1.0-alpha1 → will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule → and the Allowed changes during the Drupal 9 release cycle → .

Drupal 8.9.0-beta1 → was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule → and the Allowed changes during the Drupal 8 and 9 release cycles → .

@rcbcool: That is because there is a problem with running Drupal on Azure. See: #3089902: "Azure Database for MySQL server" reports wrong database version → .

The sql_mode is now less granular. How can we override TRADITIONAL to remove ONLY_FULL_GROUP_BY?

    $connection_options['init_commands'] += [
      'sql_mode' => "SET sql_mode = 'ANSI,TRADITIONAL'",
    ];

Reroll. For concern on #14, isn't the idea that these defaults can be overridden on sql_mode_options, but then one needs to define the modes it is equivalent, e.g something like this:

$databases['drupal']['default'] = [
  'database' => 'drupal',
  'username' => 'drupal',
  'password' => 'hello',
  'init_commands' => [
    'sql_mode_options' => [
      'TRADITIONAL' => FALSE,
      'ONLY_FULL_GROUP_BY' => FALSE,
      // Defining all the modes TRADITIONAL is equivalent on current MySQL version.
    ],
  ],
];

Drupal 9.3.0-rc1 → was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

1. The file default.setting.php needs to be updated with the new option.
2. There needs to be testing for the new functionality.
3. The issue summary needs to be updated with a proposed solution.

> The file default.setting.php needs to be updated with the new option.

There's currently nothing about sql_mode there anyway.

> There needs to be testing for the new functionality.

I'm not sure how. There's no test coverage of sql_mode at the moment AFAICT.

> The issue summary needs to be updated with a proposed solution.

That's already in the IS and I'm not sure what to add to it. I've put it under a heading to make it clearer.

Drupal 9.5.0-beta2 → and Drupal 10.0.0-beta2 → were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.4.0-alpha1 → was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Made a 10.1 MR.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened → , as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Reabsed the MR and fixed the tests by updating the version to support the 10.3 window.

Thanks!

Created a CR: https://www.drupal.org/node/3403416 → . It might need some tweaking though.
MR updated.
Thanks!

Was previously tagged for issue summary update which still seems to be the case. Added missing sections.

Hiding patches for clarity.

The comment that added 'Needs issue summary update' asks for a proposed solution in the IS, which is present.

The requested docs have also been added.

Thanks @joachim looking at the MR 3444 I see there are a number of test failures though.

lcatlett → changed the visibility of the branch 2939760-allow-granular-overriding-of-sqlmode-options-pan to hidden.

lcatlett → changed the visibility of the branch 2939760-sql-mode-override-9 to hidden.

lcatlett → changed the visibility of the branch 9.5.x to hidden.

A question about the implementation. This MR adds a new field, 'sql_mode_options', to the 'init_commands' portion of the $databases array. However, 'init_commands' is defined to be a list of SQL expressions to execute, leading to a bit of awkwardness where we need to un-set 'sql_mode_options' before executing everything else in the 'init_commands' list. On the surface, it seems better to make 'sql_mode_options' a top-level element of the $databases array, since it is unlike everything else inside 'init_commands'. Is there any reason why it would be inconvenient or infeasible to add a new top-level element to the $databases array?

> Is there any reason why it would be inconvenient or infeasible to add a new top-level element to the $databases array?

Just because I thought to group it in 'init_commands' because it's to do with that, but you're right, it would be clearer as a top-level. Let's do that.

Rebased, and changed sql_mode_options to be top-level.

Update summary to show that sql_mode_options is a top-level element of the $databases array.

Done:

- 11.x MR without deprecation: https://git.drupalcode.org/project/drupal/-/merge_requests/1895
- 10.3 MR with deprecation: https://git.drupalcode.org/project/drupal/-/merge_requests/7690

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Changes made.

I don't see how this faillure is related to this MR:

    Drupal\Tests\demo_umami\Functional\DemoUmamiProfileTest::testDemoSpecificFeatures
    Behat\Mink\Exception\ElementNotFoundException: Button with
    id|name|label|value "Log out" not found.

It comes after 45 page requests which all pass! If something was going to be wrong with the DB, it would have failed earlier!

Do you mean whether we should totally ignore it, or throw a warning if it's present and then unset it?

Yes, I think it should be as you did it in the latest pair of MRs: 10.3.x should warn, but still respect $connection_options['init_commands']['sql_mode'], and 11.x should silently unset it.

Regarding the test failures, we have this in the latest 11.x run:

1)
    Drupal\FunctionalTests\Installer\InstallerExistingConfigSyncDirectoryMultilingualTest::testConfigSync
    Behat\Mink\Exception\ElementNotFoundException: Form field with
    id|name|label|value
    "Drupal\mysql\Driver\Database\mysql[sql_mode_options][ANSI]" not found.
    
    /builds/issue/drupal-2939760/vendor/behat/mink/src/WebAssert.php:731
    /builds/issue/drupal-2939760/core/tests/Drupal/Tests/UiHelperTrait.php:85
    /builds/issue/drupal-2939760/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php:265
    /builds/issue/drupal-2939760/core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php:174
    /builds/issue/drupal-2939760/vendor/phpunit/phpunit/src/Framework/TestResult.php:729

I haven't looked at the code from the stack trace yet, but I'll see if I can figure out what is going on. Maybe it's only in the tests, or maybe it's in the config code, but it looks like something might be building forms based on the values in the $databases array, which is to say, this might be related to moving sql_mode_options to the top level of the database info array.

My hypothesis seems to have held out; the 11.x branch tests are now green. I have pushed the same fix for the tests to the 10.3.x branch, which is running now. There isn't any need to backport this to 10.1.x or 9.x, is there? Shall we simply hide those branches?

Now I am seeing the same result as #53 on the 10.3.x branch. Didn't see that at all on the 11.x branch. Bringing us up to date with HEAD of 10.3.x, to see if that helps.

Rebasing fixed the 10.3.x branch.

joachim → changed the visibility of the branch 2939760-10.1-allow-granular-overriding-of-sqlmode-options to hidden.

joachim → changed the visibility of the branch 2939760-9-allow-granular-overriding-of-sqlmode-options to hidden.

I added a comment-only change, and the phpunit tests are now marked as failed with exit code 1, even though there are no error lines in the job output. Not sure if a warning is triggering the bad exit code, or if it's just a job runner failure.

> * 'ONLY_FULL_GROUP_BY' => true,

Should be TRUE.

Could that be the failure? I don't know if the PHP linting checks @code blocks in docblocks.

Changed it for good measure (and correctness of the comment); if nothing else, this will give the testbot another chance to run it again.

Ironically, the failing test that was confusing me was one that I wrote during the composer initiative, to ensure that no one ever forgot to change both the scaffold assets and the original file, which of course I did. Not sure why the failure wasn't visible until I looked at the raw output, but once I did that, the fix was clear, and the 11.x branch is green again. I just pushed the same fix to the 10.3.x branch, which is now running.

I upgraded a site to 10.3.x-dev and applied the patch to it. After updating setting.php to include:

$databases['default']['default']['sql_mode_options']['ONLY_FULL_GROUP_BY'] = TRUE;

Running drush ev '$database = \Drupal::database(); $query = $database->query("SELECT @@SESSION.sql_mode"); $result = $query->fetchAll(); var_export($result);' before and after applying the settings.php change showed that the mode ONLY_FULL_GROUP_BY was, in fact, added to the SQL Mode after adding that modification.

Update release notes snippet.

I did the same test as #65 on a Drupal 11.x-dev site, after adding the patch, and got consistent results there as well.

I have reviewed and tested this MR per #67 and #65, and believe this is now ready for review by a core committer. I did make a couple of commits; a one-line change (#56), to make the tests pass, and added some documentation to default.settings.php. Hopefully this involvement is minor enough that it is okay for me to still RTBC here.

Regarding #17, the first and third requests have been done. As for testing, the existing functional tests wouldn't pass if the logic here was faulty, so I feel like existing coverage is adequate. If there's anything else desired here, though, please let me know.

Responding to a callout on Slack to review this, I have only looked over the code. Overall

Raised one tiny nit that would probably see it sent back by a committer. Once that's in I think I would be happy to RTBC based on #68.

Well spotted!

Fixed.

I just read the change record, it says this is "deprecated" but there's no BC layer or deprecation error messages, I think we need those?

@darvanen are you looking at the right branch?

In refs/heads/2939760-10.3-allow-granular-overriding-of-sqlmode-options:

      @trigger_error("The 'sql_mode' database command is deprecated in drupal:10.3.0 and is removed from drupal:11.0.0. Use an array of options in 'sql_mode_options' instead. See https://www.drupal.org/node/3403416", E_USER_DEPRECATED);

@darvanen yes, please, as @joachim mentioned, please review both the 11.x and the 10.3.x branches. The change record is for the 10.3.x branch. Do we need a second change record for the 11.x branch, describing the removal of the deprecation checks?

Oh of course, I'm not accustomed to looking for two MRs. Just gotta remove that same comment in the other MR and then I reckon we're good.

@joachim noted re stripos still being relevant in 10.x. I agree that's resolved.

I'm going to defer to @daffie now, my timezone is about to pass midnight.

Forgot to change the status to NW.

I realize that I was not correct above; sql_mode_options is merged with the default options, so there is no way to blanket-reset the Drupal defaults. In order to eliminate a setting that Drupal applies, you must explicitly set the default modes to FALSE. This leads us to two alternatives:

1. We could un-deprecate setting sql_mode in init_commands, as this setting provides a capability that sql_mode_options does not.
2. We could address this edge case in #3261236, and simply not add any modes related to ANSI or TRADITIONAL if sql_mode_options sets ANSI or TRADITIONAL to FALSE, respectively.

I added a good test that demonstrates that we are able to change SQL modes. Two tests are done:

1. There is a test that checks the SQL mode and fails if the new mode added in the setup function is not listed.
2. There is a test that confirms the behavior of ONLY_FULL_GROUP_BY. This test will fail if that mode is not correctly applied in the setup function. This is essentially the test suggested in #3261236-3 → . (It differs in that we explicitly set ONLY_FULL_GROUP_BY in the test; we could/should copy this test to core/tests/Drupal/KernelTests/Core/Database/SelectComplexTest when updating #3261236, to ensure that default behavior includes ONLY_FULL_GROUP_BY for both Mysql and Mariadb).

An existing test confirms that the default SQL modes are not set when removed in the setup function.

Remaining questions:

1. Are there more edge cases we should be testing? I can not think of any that are needed, but if anyone can think of something that should be covered, let me know and I will add more tests as needed.
2. Do we need to worry about making it easy to remove Drupal default options? I am inclined to go with option 2 from #78, above ✨ allow granular overriding of sql_mode options Needs work
3. Right now, we are deprecating SQL mode in 10.3.x, and removing it in 11.x. Do we need to deprecate it in 10.3.x and 11.x, for removal in 12.x?

I think that the current MR satisfies all of the remaining questions, at least for the resolution of each that I suspect. If there is general agreement on that, then we're ready to go back to RTBC.

I am not entirely sure about silently ignoring an 'sql_mode' in 'init_commands'. The documentation for the whole of this is pretty sparse, so someone might reasonably see 'init_commands' and try to use 'sql_mode' in that since it is an actual MySQL init command. Might be better DX to throw an exception so a developer is immediately told they've done something wrong, rather than leave them headscratching over why it doesn't work.

Yeah, #81 sounds reasonable to me.

Regarding deprecations I checked and the answer I got was a link to https://drupal.slack.com/archives/C03L6441E1W/p1713811922586309?thread_t...

Repeated here for convenience:

catch
17 days ago
Any actual API deprecations should be for removal in Drupal 12 already unless it's blocking Drupal 11. Internal and dead code sort of things until 10.3.0 beta1

Setting to NW as we'll need to keep the deprecation code in the 11.x MR

Thanks!

Updated the MR (11.x branch only) to deprecate the feature in 11.x, and remove in 12.x. Question: for the deprecation message, should it be version 11.1.0, or is 11.0.0 ok?

Regarding the last change I pushed, while it would be possible to simplify the logic slightly, I wanted to point out that I deliberately ordered things the way I did to minimize the number of lines that had to be touched when the deprecated sql_mode support is removed.

Looking into test failures.

greg.1.anderson → changed the visibility of the branch 2939760-10.3-allow-granular-overriding-of-sqlmode-options to hidden.

Switched this to deprecate sql_mode in 11.1.0 for removal in 12.x, and backported sql_mode_options to the 10.4.x branch without deprecation of sql_mode.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Thanks, Needs Review Queue Bot! Rebased to HEAD of 11.x branch.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Hum. Seems to still be up-to-date to me. :shrug:

Updated the change record → to reflect current targeted Drupal core versions.

Ok, "needs review queue bot", come at me. I actually expect it is going to "needs work" me, because the "MR" badge is grey instead of green, even though the branch is up-to-date with HEAD of 11.x. Setting back to "needs review" for the record, though.

> The reason I sort of favor duplicating the docs, as I currently have it, though, is that these modes can change over time, so it seems good to snapshot the expectations at the time the commit was made in Drupal core.

+1 to this point you made in Slack, @greg.1.anderson.

Updated Sql Mode docs to include a reference from default.settings.php per suggestion by @chx. (https://drupal.slack.com/archives/C1BMUQ9U6/p1717457117657049?thread_ts=...)

Looks good!

The CR is written by developers for developers. Only in this instance it is also very important that operational persons understand it. The reference "see Drupal\mysql\Driver\Database\mysql\SqlMode." is something that is not very helpful for them. Can we copy that documentation to the CR.

I think we also need testing for the different combinations we can set with sql modes. Specially for edge cases.

I added SqlModeSelectionTest, which tests every condition and edge case I could think of related to selecting or de-selecting Sql modes. I think all of the review comments have been responded to.

The issue summary states:

However, it's an all or nothing override. Connection::open does this:

    $connection_options['init_commands'] += [
      'sql_mode' => "SET sql_mode = 'ANSI,STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,ONLY_FULL_GROUP_BY'",
    ];

so either I override the whole of that string, or none of it.

But given we're not merging the defaults I don't understand how the new code is better - according to the way the issue summary lays out the improvement. Given the lack of times that changing this is necessary are we sure that all this extra code and processing on every bootstrap is really worth the additional maintenance and effort to change existing set ups?

@greg.1.anderson @joachim are there any other benefits here?

Another concern I have is that providing an API for this means that we have to track changes and do deprecations for values when the underlying modes change in the database API.

@alexpott: you are correct, the summary is out of date. It should say ANSI and TRADITIONAL rather than listing all of the individual modes that Drupal used to set. If we did https://www.drupal.org/project/drupal/issues/3261236 → , then Drupal would use individual mode values again. I don't know if that issue is a good idea or not, but if it is, it wold be better to do this one first.

There is very little motivation for individual users to set these mode values in most cases. Right now, Pantheon is tracking an issue were sites migrated from other systems where they used to run on Mysql sometimes suffer performance penalties when the same SQL queries run on Mariadb. Allowing control over individual modes would give ISPs more flexibility, e.g. we could set a mode for a group of sites, and individual sites could later alter individual modes further if necessary.

Another concern I have is that providing an API for this means that we have to track changes and do deprecations for values when the underlying modes change in the database API.

That's fair. Maybe it's better to go back to strings of values instead of providing a class with constants.

@alexpott is this something you can see being accepted? If not should it be closed.

Left a comment that might help with the API question in #106, or might make it worse depending on how you perceive it 😂
In work projects we've been replacing a lot of 'random strings' with enums that helps avoid typos and allows type-hinting and validation. And in instances where the enum is supposed to be a deliberate extension point we're adding an interface for the enum and type-hinting that instead. It allows a type-safe approach with a degree of flexibility for extension.