Skip to content
Snippets Groups Projects

Issue #2939760: allow granular overriding of sql_mode options

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Looks like the current code is written to target the 10.3.x branch, but the MR is targeting the 11.x branch. Ideally we'd have both branches available.

  • Joachim Noreiko added 2 commits

    added 2 commits

    • 2cdf5362 - Fixed handling of sqlmode_ansi_quotes.
    • 0ceb4da4 - Removed deprecation handling.

    Compare with previous version

  • added 1 commit

    • 089bdfb7 - Unset sql_mode init command.

    Compare with previous version

  • Greg Anderson
  • Should we ignore $connection_options\['init_commands'\]\['sql_mode'\], since we advertised in 10.3.x that it will be obsolete in 11.x?

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

  • Greg Anderson added 1 commit

    added 1 commit

    • 64d5f065 - Ignore new top-level driver setting "sql_mode_options" when setting up functional tests.

    Compare with previous version

  • Greg Anderson added 159 commits

    added 159 commits

    • 64d5f065...966fd63a - 150 commits from branch project:11.x
    • 7d2430f7 - Changed sql_mode to be set with an array of options rather than a string.
    • 43841a9c - Fixed sql_mode getting written into connection options init_commands in migrate tests.
    • 603690e3 - Removed use of deprecated sql_mode option in tests.
    • 3c321188 - Changed sql_mode_options to be at the top level of the connection options array.
    • 2fdc03cb - Updated deprecation message.
    • f777a4fb - Fixed handling of sqlmode_ansi_quotes.
    • e3d297e1 - Removed deprecation handling.
    • ee2097da - Unset sql_mode init command.
    • 6a888965 - Ignore new top-level driver setting "sql_mode_options" when setting up functional tests.

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • 28f88f74 - Add example of sql_mode_options usage to default.settings.php

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • a1f806d5 - Use TRUE rather than true in default.settings.php comment.

    Compare with previous version

  • Greg Anderson added 14 commits

    added 14 commits

    • 4a4a78b5 - 1 commit from branch project:11.x
    • 4a4a78b5...7bb61c6c - 3 earlier commits
    • bd1e078f - Changed sql_mode_options to be at the top level of the connection options array.
    • 13c8480a - Updated deprecation message.
    • 4247fa0c - Fixed handling of sqlmode_ansi_quotes.
    • 38ebe044 - Removed deprecation handling.
    • 0a50bff8 - Unset sql_mode init command.
    • 405cecb3 - Ignore new top-level driver setting "sql_mode_options" when setting up functional tests.
    • d92e9214 - Add example of sql_mode_options usage to default.settings.php
    • d1dacfc3 - Use TRUE rather than true in default.settings.php comment.
    • fbe988dc - Revert "Use TRUE rather than true in default.settings.php comment."
    • 4f555b5f - Revert "Add example of sql_mode_options usage to default.settings.php"

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • e4b80c30 - Copy default.settings.php to scaffold asset files.

    Compare with previous version

  • Greg Anderson added 15 commits

    added 15 commits

    • e4b80c30...152b7f06 - 3 commits from branch project:11.x
    • 152b7f06...3337783f - 2 earlier commits
    • 22e08821 - Removed use of deprecated sql_mode option in tests.
    • 9df20fa9 - Changed sql_mode_options to be at the top level of the connection options array.
    • 50d21b79 - Updated deprecation message.
    • a5b66235 - Fixed handling of sqlmode_ansi_quotes.
    • 727c90c7 - Removed deprecation handling.
    • 2b65a300 - Unset sql_mode init command.
    • cf2667c5 - Ignore new top-level driver setting "sql_mode_options" when setting up functional tests.
    • 7211c5e7 - Add example of sql_mode_options usage to default.settings.php
    • a40f08d3 - Use TRUE rather than true in default.settings.php comment.
    • 6ec83a6f - Copy default.settings.php to scaffold asset files.

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • daffie
  • 34 34 // based on databaseType() rather than 'driver', but here all we have to go
    35 35 // on is 'driver'.
    36 36 if ($info['default']['driver'] === 'mysql') {
    37 $info['default']['init_commands']['sql_mode'] = "SET sql_mode = ''";
    37 $info['default']['sql_mode_options']['ANSI'] = FALSE;
    • Whye is this change necessary? Do we also need to set "TRADITIONAL" to false. If not why?

    • The existing test clears all of the SQL modes, but 'ANSI' is the only one that the test actually depends on. It would do no harm to set 'TRADITIONAL' to false, but it is not necessary. If we wanted the test to be literally the same as the existing line, then we would use $info['default']['sql_mode_options'] = [];. I will consider that option when I am working on enhancing the tests later today.

    • Oh, forgot to follow up on the above. Setting sql_mode_options to empty means "make no changes to the SQL mode options", not "disable all options". We could set "TRADITIONAL" to false if we wanted to be literally the same as the existing test code.

    • Greg Anderson changed this line in version 42 of the diff

      changed this line in version 42 of the diff

    • Please register or sign in to reply
  • I think we also need testing for the different combinations we can set with sql modes. We are adding a lot of code and I would very much like to make sure we are not breaking something. Specially for edge cases.

  • Greg Anderson added 1 commit

    added 1 commit

    • 59f3062f - Make SqlModeTest literally the same as it was when it used sql_mode instead of sql_mode_options.

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • e84e52ab - Add some ONLY_FULL_GROUP_BY tests (preliminary, will fail).

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • b572122f - Placate spell checker and coding standards.

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • cf498c84 - qwerqwer was supposed to be a word

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • 96e526d4 - Use groupBy queries copied from another test.

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • c59790a8 - Fix typo, need to fix field name.

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • cf022ab6 - What I intended to do last time

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    Compare with previous version

  • Greg Anderson added 28 commits

    added 28 commits

    • 56525ba1...8d512c2a - 14 commits from branch project:11.x
    • 8d512c2a...559a6a40 - 4 earlier commits
    • 09fbe066 - Updated deprecation message.
    • 1d75cf81 - Fixed handling of sqlmode_ansi_quotes.
    • 1ea8be49 - Removed deprecation handling.
    • 0e8e3707 - Unset sql_mode init command.
    • 8baf2e41 - Ignore new top-level driver setting "sql_mode_options" when setting up functional tests.
    • 68449658 - Add example of sql_mode_options usage to default.settings.php
    • cec6bf2d - Use TRUE rather than true in default.settings.php comment.
    • 25d8b889 - Copy default.settings.php to scaffold asset files.
    • e6e1c899 - Removed obsolete comment.
    • 821364a3 - Add tests to demonstrate that individual sql modes can be set.

    Compare with previous version

  • Greg Anderson added 16 commits

    added 16 commits

    • 821364a3...6293c8e0 - 2 commits from branch project:11.x
    • 6293c8e0...bcdb58f3 - 4 earlier commits
    • fc637b42 - Updated deprecation message.
    • 1f8c6648 - Fixed handling of sqlmode_ansi_quotes.
    • 55aba116 - Removed deprecation handling.
    • 5ce38711 - Unset sql_mode init command.
    • ef1c29b3 - Ignore new top-level driver setting "sql_mode_options" when setting up functional tests.
    • ec462aa9 - Add example of sql_mode_options usage to default.settings.php
    • 10f965de - Use TRUE rather than true in default.settings.php comment.
    • 627e7ad4 - Copy default.settings.php to scaffold asset files.
    • 9447e6e2 - Removed obsolete comment.
    • 761af4dd - Add tests to demonstrate that individual sql modes can be set.

    Compare with previous version

  • Greg Anderson added 96 commits

    added 96 commits

    • 5ca2d2ef...6fc1e6f2 - 81 commits from branch project:11.x
    • 6fc1e6f2...cc491bbe - 5 earlier commits
    • 22f6f8c7 - Fixed handling of sqlmode_ansi_quotes.
    • 04237b38 - Removed deprecation handling.
    • f5963ee0 - Unset sql_mode init command.
    • c8f46420 - Ignore new top-level driver setting "sql_mode_options" when setting up functional tests.
    • 7f24323a - Add example of sql_mode_options usage to default.settings.php
    • d255f4b4 - Use TRUE rather than true in default.settings.php comment.
    • 7d81048c - Copy default.settings.php to scaffold asset files.
    • 70b68738 - Removed obsolete comment.
    • 2bccfa63 - Add tests to demonstrate that individual sql modes can be set.
    • 37ad08b1 - Deprecate sql_mode in 11.x for removal in 12.x.

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • 95e3963c - Deprecate in 11.1.0 instead of 11.0.0.

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • 8756253c - Be more careful to behave correctly when sql_mode is still in use.

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • 2090351f - isolation_level already pollutes the $connection_options array in the existing...

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • 2c8da601 - Move sql_mode_options calculation to the end

    Compare with previous version

  • Greg Anderson added 29 commits

    added 29 commits

    • 2c8da601...cdb29302 - 10 commits from branch project:11.x
    • cdb29302...fce1bcb0 - 9 earlier commits
    • 23fdcea8 - Add example of sql_mode_options usage to default.settings.php
    • 2068edbc - Use TRUE rather than true in default.settings.php comment.
    • 643b91f9 - Copy default.settings.php to scaffold asset files.
    • 1697c33e - Removed obsolete comment.
    • bec6d165 - Add tests to demonstrate that individual sql modes can be set.
    • 7a021fb6 - Deprecate sql_mode in 11.x for removal in 12.x.
    • cb1ac676 - Remove unused use statement
    • 2341aa29 - Deprecate in 11.1.0 instead of 11.0.0.
    • 53b622ef - Be more careful to behave correctly when sql_mode is still in use.
    • cc78bcd0 - Move sql_mode_options calculation to the end

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • 6de110ac - Introduce an SqlMode constant

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    Compare with previous version

  • Greg Anderson added 24 commits

    added 24 commits

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    Compare with previous version

  • Greg Anderson added 26 commits

    added 26 commits

    Compare with previous version

  • Greg Anderson added 29 commits

    added 29 commits

    • 0a258119...20a080c2 - 5 commits from branch project:11.x
    • 20a080c2...bc4169b4 - 14 earlier commits
    • c709fa92 - Deprecate sql_mode in 11.x for removal in 12.x.
    • 39694211 - Remove unused use statement
    • 9f64165b - Deprecate in 11.1.0 instead of 11.0.0.
    • 33980aa3 - Be more careful to behave correctly when sql_mode is still in use.
    • fa47f9e8 - Move sql_mode_options calculation to the end
    • 0ce781fb - Introduce an SqlMode constant
    • 9480fb86 - Code style
    • 9caa88ca - Moar code style
    • e6075703 - Touch up SqlMode comments.
    • 29284493 - Add a few more Sql Mode docs, and reference them from default.settings.php.

    Compare with previous version

  • daffie
    daffie @daffie started a thread on the diff
  • 195 197 'init_commands' => [],
    196 198 ];
    197 199
    198 $connection_options['init_commands'] += [
    199 'sql_mode' => "SET sql_mode = 'ANSI,TRADITIONAL'",
    200 // Emit a deprecation warning if sql_mode is in the init commands.
    201 if (isset($connection_options['init_commands']['sql_mode'])) {
    202 @trigger_error("The 'sql_mode' database command is deprecated in drupal:11.1.0 and will be removed in drupal:12.0.0. Use an array of options in 'sql_mode_options' instead. See https://www.drupal.org/node/3403416", E_USER_DEPRECATED);
    • We should have testing for this deprecation.

    • How strongly do you feel about having a test for this? If I leverage DriverSpecificDatabaseTestBase, then this method is called during setup(), so that won't work. If I extend KernelTestBase, then I could duplicate the setup() method in my test class, and add an expectDeprecation() before calling Database::getConnection(), which calls this open() method. Do you think that's worth doing, or can you think of a better way?

    • After mulling it over a bit, I think that the code duplication path isn't too bad. I'll work on doing it that way tomorrow.

    • I added SqlModeDeprecationTest, that confirms that this deprecation error is thrown. The duplicate code turned out to not be a concern, as most of it was optimized away.

    • Please register or sign in to reply
  • daffie
    daffie @daffie started a thread on the diff
  • 195 197 'init_commands' => [],
    196 198 ];
    197 199
    198 $connection_options['init_commands'] += [
    199 'sql_mode' => "SET sql_mode = 'ANSI,TRADITIONAL'",
    200 // Emit a deprecation warning if sql_mode is in the init commands.
    201 if (isset($connection_options['init_commands']['sql_mode'])) {
    202 @trigger_error("The 'sql_mode' database command is deprecated in drupal:11.1.0 and will be removed in drupal:12.0.0. Use an array of options in 'sql_mode_options' instead. See https://www.drupal.org/node/3403416", E_USER_DEPRECATED);
    203 }
    204
    205 // If the user has set sql_mode_options, then ignore sql_mode.
    206 if (isset($connection_options['sql_mode_options'])) {
    207 unset($connection_options['init_commands']['sql_mode']);
    • We should document in the CR that when you use the new setting "sql_mode_options", the old setting "init_commands sql_mode" will be skipped.

    • I updated the CR to include this information, and also removed the reference to the source code in favor of making the CR itself more complete.

    • Please register or sign in to reply
  • I think we also need testing for the different combinations we can set with sql modes. Specially for edge cases.

  • Greg Anderson added 1 commit

    added 1 commit

    • a871bba1 - Add a test that demonstrates that we can add and remove specific SQL modes.

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    Compare with previous version

  • Greg Anderson added 31 commits

    added 31 commits

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • 26b73e25 - We must use group "legacy" to use expectDeprecation

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • 3387c3e5 - Put in the actual deprecation message

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • 81ecfc7b - Improve comments in SqlModeSelectionTest, and also illustrate that it is not...

    Compare with previous version

  • Greg Anderson added 1 commit

    added 1 commit

    • 8ace4947 - Add forgotten "use" statement

    Compare with previous version

  • 167 167 * Advanced users can add or override initial commands to execute when
    168 168 * connecting to the database server, as well as PDO connection settings. For
    169 169 * example, to enable MySQL SELECT queries to exceed the max_join_size system
    170 * variable, and to reduce the database connection timeout to 5 seconds:
    170 * variable, and configure MariaDB to interpret GROUP BY clauses the same way
    171 * that MySQL does, and to reduce the database connection timeout to 5 seconds:
    171 172 * @code
    172 173 * $databases['default']['default'] = [
    173 174 * 'init_commands' => [
    174 175 * 'big_selects' => 'SET SQL_BIG_SELECTS=1',
    175 176 * ],
    177 * 'sql_mode_options' => [
    178 * 'ONLY_FULL_GROUP_BY' => TRUE,
    • I wonder if we should create an Enum for all of the allowed values here. We could then validate the keys passed are valid. We can use an Enum interface if the expectation is that other DB engines may define unknown values, but at least that way its a deliberate value rather than a typo.

    • Please register or sign in to reply
  • One question about using enums, and the fact that backed enums can implement an interface

  • Please register or sign in to reply
    Loading