allow granular overriding of sql_mode options

Created on 26 January 2018, over 6 years ago
Updated 10 June 2024, about 1 month ago

Problem/Motivation

It's possible to override the sql_mode options by doing this in your database definition in settings.php:

$databases['drupal6_csp']['default'] = array (
  'database' => 'drupal8',
  'username' => 'drupal',
  'password' => 'hello',
  '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'",
  ],
);

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.

Steps to reproduce

To see the modes that Drupal is currently setting, run:

drush ev '$database = \Drupal::database(); $query = $database->query("SELECT @@SESSION.sql_mode"); $result = $query->fetchAll(); var_export($result);'

Note that meta-modes such as ANSI and TRADITIONAL will not be reported, but instead, the actual modes that the database server set will be shown.

Proposed resolution

We could allow an 'sql_mode_options' array to override each one, eg:

$databases['drupal6_csp']['default'] = array (
  'database' => 'drupal8',
  'username' => 'drupal',
  'password' => 'hello',
  'sql_mode_options' => [
    // I don't want this one.
    'ONLY_FULL_GROUP_BY' => FALSE,
    // I do want this one.
    'CAKE' => TRUE,
    // Anything I don't specify, I'm happy with core's default.
  ],
);

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

The Drupal Mysql database driver now has an SQL Mode options configuration setting โœจ allow granular overriding of sql_mode options Needs work available in the $databases array that allows individual SQL Modes to be set or cleared in settings.php.

โœจ Feature request
Status

Needs review

Version

11.0 ๐Ÿ”ฅ

Component
Databaseย  โ†’

Last updated 1 day ago

  • Maintained by
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands @daffie
Created by

๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

Live updates comments and jobs are added and updated live.

Missing content requested by

๐Ÿ‡ฆ๐Ÿ‡บAustralia dpi
8 months ago
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @joachim
  • 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 โ†’ .

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    This is what I had in mind.

    Would need documentation in settings.php and a CR.

  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands hako74

    Hi,

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

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim
    +++ 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 โ†’ .

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    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.

  • ๐Ÿ‡จ๐Ÿ‡ณChina rcbcool

    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 โ†’ .

  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands daffie

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

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia imclean Tasmania

    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'",
        ];
    
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands daffie
  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland sokru

    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 โ†’ .

  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands daffie
    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.
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    > 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.

  • Status changed to Needs review over 1 year ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    Made a 10.1 MR.

  • Status changed to Needs work over 1 year ago
  • 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.

  • Status changed to Needs review over 1 year ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim
  • Status changed to Needs work over 1 year ago
  • 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.

  • Status changed to Needs review 8 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim
  • Pipeline finished with Failed
    8 months ago
    Total: 170s
    #53412
  • Pipeline finished with Failed
    8 months ago
    Total: 184s
    #53414
  • Status changed to Needs work 8 months ago
  • 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.

  • First commit to issue fork.
  • Status changed to Needs review 8 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia ankithashetty Karnataka, India

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

    Thanks!

  • Pipeline finished with Failed
    8 months ago
    Total: 507s
    #53843
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia ankithashetty Karnataka, India

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

  • Pipeline finished with Failed
    8 months ago
    Total: 755s
    #53905
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

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

  • Status changed to Needs work 8 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Hiding patches for clarity.

  • Status changed to Needs review 8 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    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.

  • Status changed to Needs work 8 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

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

  • First commit to issue fork.
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States lcatlett

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

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States lcatlett

    lcatlett โ†’ changed the visibility of the branch 2939760-sql-mode-override-9 to hidden.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States lcatlett

    lcatlett โ†’ changed the visibility of the branch 9.5.x to hidden.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    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?

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    > 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.

  • Pipeline finished with Failed
    3 months ago
    Total: 178s
    #154525
  • Pipeline finished with Failed
    3 months ago
    Total: 195s
    #154526
  • Status changed to Needs review 3 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    Rebased, and changed sql_mode_options to be top-level.

  • Pipeline finished with Failed
    3 months ago
    Total: 187s
    #154530
  • Status changed to Needs work 3 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

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

  • Pipeline finished with Failed
    3 months ago
    Total: 193s
    #155209
  • Status changed to Needs review 3 months ago
  • Pipeline finished with Canceled
    3 months ago
    Total: 35s
    #155261
  • Pipeline finished with Failed
    3 months ago
    Total: 177s
    #155262
  • Pipeline finished with Failed
    3 months ago
    Total: 992s
    #155255
  • Status changed to Needs work 3 months ago
  • 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.

  • Status changed to Needs review 3 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim
  • Pipeline finished with Failed
    3 months ago
    Total: 993s
    #155391
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    Changes made.

  • Pipeline finished with Failed
    3 months ago
    #155503
  • Pipeline finished with Failed
    3 months ago
    #155505
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    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!

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    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.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    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.

  • Pipeline finished with Success
    2 months ago
    Total: 1080s
    #165103
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    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?

  • Pipeline finished with Failed
    2 months ago
    Total: 962s
    #165123
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    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.

  • Pipeline finished with Success
    2 months ago
    Total: 602s
    #165148
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    Rebasing fixed the 10.3.x branch.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

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

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

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

  • Pipeline finished with Success
    2 months ago
    Total: 575s
    #165557
  • Pipeline finished with Failed
    2 months ago
    Total: 536s
    #165600
  • Pipeline finished with Failed
    2 months ago
    Total: 627s
    #165601
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    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.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    > * '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.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

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

  • Pipeline finished with Failed
    2 months ago
    Total: 649s
    #165684
  • Pipeline finished with Failed
    2 months ago
    #165685
  • Pipeline finished with Failed
    2 months ago
    Total: 504s
    #165889
  • Pipeline finished with Success
    2 months ago
    #165904
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    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.

  • Pipeline finished with Success
    2 months ago
    Total: 599s
    #165926
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    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.

  • Pipeline finished with Success
    2 months ago
    Total: 528s
    #166770
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    Update release notes snippet.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    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.

  • Status changed to RTBC 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    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.

  • Status changed to Needs work 2 months ago
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia darvanen Sydney, Australia

    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.

  • Status changed to Needs review 2 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    Well spotted!

    Fixed.

  • Pipeline finished with Success
    2 months ago
    Total: 576s
    #167239
  • Status changed to Needs work 2 months ago
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia darvanen Sydney, Australia

    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?

  • Status changed to Needs review 2 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    @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);
    
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    @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?

  • Status changed to Needs work 2 months ago
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia darvanen Sydney, Australia

    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.

  • Status changed to Needs review 2 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia darvanen Sydney, Australia

    @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.

  • Status changed to Needs work 2 months ago
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands daffie

    Forgot to change the status to NW.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    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.

  • Pipeline finished with Success
    2 months ago
    Total: 577s
    #167987
  • Pipeline finished with Failed
    2 months ago
    Total: 180s
    #168064
  • Pipeline finished with Failed
    2 months ago
    Total: 175s
    #168086
  • Pipeline finished with Failed
    2 months ago
    Total: 527s
    #168121
  • Pipeline finished with Failed
    2 months ago
    Total: 190s
    #168154
  • Pipeline finished with Success
    2 months ago
    #168177
  • Pipeline finished with Running
    2 months ago
    #168206
  • Pipeline finished with Failed
    2 months ago
    Total: 576s
    #168222
  • Pipeline finished with Failed
    2 months ago
    Total: 633s
    #168379
  • Pipeline finished with Failed
    2 months ago
    Total: 574s
    #168387
  • Pipeline finished with Failed
    2 months ago
    Total: 171s
    #168394
  • Pipeline finished with Failed
    2 months ago
    Total: 195s
    #168397
  • Pipeline finished with Failed
    2 months ago
    Total: 575s
    #168404
  • Pipeline finished with Failed
    2 months ago
    Total: 175s
    #168411
  • Pipeline finished with Success
    2 months ago
    Total: 691s
    #168414
  • Pipeline finished with Failed
    2 months ago
    Total: 633s
    #168427
  • Pipeline finished with Success
    2 months ago
    Total: 571s
    #168744
  • Pipeline finished with Success
    2 months ago
    Total: 578s
    #168745
  • Status changed to Needs review 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    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.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    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.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    Yeah, #81 sounds reasonable to me.

  • Status changed to Needs work 2 months ago
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia darvanen Sydney, Australia

    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

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    Thanks!

  • Pipeline finished with Failed
    about 1 month ago
    Total: 233s
    #186617
  • Pipeline finished with Success
    about 1 month ago
    Total: 949s
    #186620
  • Status changed to Needs review about 1 month ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    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?

  • Pipeline finished with Success
    about 1 month ago
    Total: 509s
    #187241
  • Pipeline finished with Success
    about 1 month ago
    Total: 816s
    #187343
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    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.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 642s
    #187707
  • Status changed to Needs work about 1 month ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    Looking into test failures.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

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

  • Pipeline finished with Failed
    about 1 month ago
    Total: 325s
    #187795
  • Pipeline finished with Success
    about 1 month ago
    Total: 540s
    #187811
  • Pipeline finished with Failed
    about 1 month ago
    Total: 534s
    #188237
  • Pipeline finished with Success
    about 1 month ago
    Total: 522s
    #188252
  • Status changed to Needs review about 1 month ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    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.

  • Status changed to Needs work about 1 month ago
  • 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.

  • Status changed to Needs review about 1 month ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

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

  • Pipeline finished with Success
    about 1 month ago
    Total: 660s
    #188481
  • Status changed to Needs work about 1 month ago
  • 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.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

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

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    Updated the change record โ†’ to reflect current targeted Drupal core versions.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 168s
    #189156
  • Pipeline finished with Failed
    about 1 month ago
    Total: 184s
    #189178
  • Pipeline finished with Success
    about 1 month ago
    Total: 625s
    #189221
  • Pipeline finished with Success
    about 1 month ago
    Total: 613s
    #189257
  • Pipeline finished with Success
    about 1 month ago
    #189263
  • Status changed to Needs review about 1 month ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    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.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave
  • Pipeline finished with Failed
    about 1 month ago
    Total: 677s
    #189327
  • Pipeline finished with Failed
    about 1 month ago
    Total: 721s
    #189328
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    > 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.

  • Pipeline finished with Success
    about 1 month ago
    Total: 539s
    #189961
  • Pipeline finished with Failed
    about 1 month ago
    Total: 517s
    #190002
  • Pipeline finished with Success
    about 1 month ago
    Total: 565s
    #190103
  • Status changed to RTBC about 1 month ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim
  • Status changed to Needs review about 1 month ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States 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=...)

  • Pipeline finished with Success
    about 1 month ago
    Total: 609s
    #191064
  • Pipeline finished with Success
    about 1 month ago
    #191065
  • Status changed to RTBC about 1 month ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    Looks good!

  • Status changed to Needs work about 1 month ago
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands daffie

    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.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 211s
    #192155
  • Pipeline finished with Failed
    about 1 month ago
    Total: 179s
    #192159
  • Pipeline finished with Success
    about 1 month ago
    Total: 690s
    #192169
  • Pipeline finished with Failed
    about 1 month ago
    Total: 195s
    #193032
  • Pipeline finished with Failed
    about 1 month ago
    Total: 190s
    #193050
  • Pipeline finished with Failed
    about 1 month ago
    Total: 509s
    #193058
  • Pipeline finished with Failed
    about 1 month ago
    Total: 789s
    #193097
  • Pipeline finished with Success
    about 1 month ago
    #193117
  • Pipeline finished with Success
    about 1 month ago
    Total: 5059s
    #193124
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 87s
    #193176
  • Pipeline finished with Running
    about 1 month ago
    #193177
  • Status changed to Needs review about 1 month ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    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.

  • Pipeline finished with Success
    about 1 month ago
    Total: 561s
    #193181
  • Status changed to RTBC about 1 month ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim
  • Status changed to Needs review about 1 month ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexpott ๐Ÿ‡ช๐Ÿ‡บ๐ŸŒ

    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.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States greg.1.anderson

    @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.

Production build 0.69.0 2024