- 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 removeONLY_FULL_GROUP_BY
?$connection_options['init_commands'] += [ 'sql_mode' => "SET sql_mode = 'ANSI,TRADITIONAL'", ];
- 🇫🇮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
- The file default.setting.php needs to be updated with the new option.
- There needs to be testing for the new functionality.
- 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.
- Merge request !1895Issue #2939760: allow granular overriding of sql_mode options → (Open) created by joachim
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.
- Merge request !3444Issue #2939760: allow granular overriding of sql_mode options → (Open) created by joachim
- Status changed to Needs review
almost 2 years ago 6:02pm 11 February 2023 - Status changed to Needs work
almost 2 years ago 6:57pm 11 February 2023 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
almost 2 years ago 9:36pm 11 February 2023 - Status changed to Needs work
almost 2 years ago 10:07pm 11 February 2023 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 the11.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
about 1 year ago 3:56pm 21 November 2023 - Status changed to Needs work
about 1 year ago 6:15am 22 November 2023 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
about 1 year ago 12:51pm 22 November 2023 - 🇮🇳India ankithashetty Karnataka, India
Reabsed the MR and fixed the tests by updating the version to support the 10.3 window.
Thanks!
- 🇮🇳India ankithashetty Karnataka, India
Created a CR: https://www.drupal.org/node/3403416 → . It might need some tweaking though.
MR updated.
Thanks! - 🇺🇸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
about 1 year ago 12:40am 26 November 2023 - Status changed to Needs review
about 1 year ago 11:16am 26 November 2023 - 🇬🇧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
about 1 year ago 6:01pm 26 November 2023 - 🇺🇸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.
- Status changed to Needs review
8 months ago 3:26pm 23 April 2024 - 🇬🇧United Kingdom joachim
Rebased, and changed sql_mode_options to be top-level.
- Status changed to Needs work
8 months ago 9:12pm 23 April 2024 - 🇺🇸United States greg.1.anderson
Update summary to show that
sql_mode_options
is a top-level element of the $databases array. - Merge request !7690Resolve #2939760 "10.3 allow granular overriding of sqlmode options" → (Open) created by joachim
- Status changed to Needs review
8 months ago 10:35am 24 April 2024 - 🇬🇧United Kingdom joachim
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 - Status changed to Needs work
8 months ago 12:39pm 24 April 2024 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
8 months ago 1:15pm 24 April 2024 - 🇬🇧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.
- 🇺🇸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?
- 🇺🇸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.
- 🇺🇸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.
- 🇺🇸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.
- 🇺🇸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 modeONLY_FULL_GROUP_BY
was, in fact, added to the SQL Mode after adding that modification. - 🇺🇸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
7 months ago 12:25am 8 May 2024 - 🇺🇸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
7 months ago 4:29am 8 May 2024 - Status changed to Needs review
7 months ago 8:53am 8 May 2024 - Status changed to Needs work
7 months ago 11:00am 8 May 2024 - 🇦🇺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
7 months ago 11:08am 8 May 2024 - 🇬🇧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
7 months ago 1:04pm 8 May 2024 - 🇦🇺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
7 months ago 1:15pm 8 May 2024 - 🇦🇺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
7 months ago 1:27pm 8 May 2024 - 🇺🇸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. - Status changed to Needs review
7 months ago 2:08pm 9 May 2024 - 🇺🇸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:
- 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.
- 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
- 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 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.
- Status changed to Needs work
7 months ago 11:21pm 9 May 2024 - 🇦🇺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 beta1Setting to NW as we'll need to keep the deprecation code in the 11.x MR
- Status changed to Needs review
7 months ago 11:46pm 30 May 2024 - 🇺🇸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?
- 🇺🇸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.
- Status changed to Needs work
6 months ago 11:53pm 31 May 2024 - Merge request !8254Changed sql_mode to be set with an array of options rather than a string (10.4.x) → (Open) created by greg.1.anderson
- 🇺🇸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.
- Status changed to Needs review
6 months ago 2:48pm 1 June 2024 - 🇺🇸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
6 months ago 8:27pm 1 June 2024 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
6 months ago 9:27pm 1 June 2024 - 🇺🇸United States greg.1.anderson
Thanks, Needs Review Queue Bot! Rebased to HEAD of 11.x branch.
- Status changed to Needs work
6 months ago 10:27pm 1 June 2024 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.
- Status changed to Needs review
6 months ago 7:26pm 2 June 2024 - 🇺🇸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 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.
- Status changed to RTBC
6 months ago 5:58pm 3 June 2024 - Status changed to Needs review
6 months ago 7:52pm 4 June 2024 - 🇺🇸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=...)
- Status changed to RTBC
6 months ago 8:35pm 4 June 2024 - Status changed to Needs work
6 months ago 1:11pm 5 June 2024 - 🇳🇱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.
- Status changed to Needs review
6 months ago 11:28pm 6 June 2024 - 🇺🇸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.
- Status changed to RTBC
6 months ago 7:10am 7 June 2024 - Status changed to Needs review
6 months ago 8:27am 10 June 2024 - 🇬🇧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.
- 🇺🇸United States smustgrave
@alexpott is this something you can see being accepted? If not should it be closed.
- Status changed to Needs work
about 2 months ago 4:40am 21 October 2024 - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
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.