Database dumps are no longer driver-agnostic

Looks good to me.

Since this is blocking #3261486: Remove core updates added prior to 9.3.0 and adjust test coverage which blocks lots of others, tagging to be smashed. Hopefully promptly. 😉

The patch is tiny and looks good to me, too. $RTBC++;

Thanks y'all!
-Derek

+++ b/core/lib/Drupal/Core/Command/DbDumpCommand.php
@@ -415,13 +415,17 @@ protected function getTemplate() {
 // Ensure any tables with a serial column with a value of 0 are created as
 // expected.
...
+if ($connection->databaseType() === 'mysql') {

Would this also be needed for @mondrake's "dbal" driver? I don't know if that matters.

#5 hopefully noone is using that driver in production, let alone making dumps with it :) And if they do, well, they're on their own anyway.

But the point is valid: driver() !== databaseType(). With the change here, any driver that will declare serving a 'mysql' database will be executing this statement.

IMHO we should aim a more permanent solution like #3222127: Add the SQL mode NO_AUTO_VALUE_ON_ZERO to the MySQL database connection or even better, avoid to rely on hardcoded ID on insert like #3225350: Avoid passing arbitrary values to the serial column on INSERT but that's far away AFAICS

Committed and pushed affdb3e70a7 to 10.0.x and e157b9c05e1 to 9.4.x and 0d1783be0a1 to 9.3.x. Thanks!

One thing I'm wondering is that we only change the mode during user insert so that's what the dump should do - but we change the mode for all tables. That means our dumps are essentially behaving like #3222127: Add the SQL mode NO_AUTO_VALUE_ON_ZERO to the MySQL database connection is implemented. I think we should open a follow-up to move the sql mode setting into \Drupal\Core\Command\DbDumpCommand::getTableScript() and hardcode the user table.

I think we should open a follow-up to move the sql mode setting into \Drupal\Core\Command\DbDumpCommand::getTableScript() and hardcode the user table.

Actually I'm not sure about that. If a contrib module has managed to insert a 0 into a serial field by doing something similar to the user entity then the current code is going to make it work for db dumps. So yeah I don't think we need a follow-up. Hopefully #3222127: Add the SQL mode NO_AUTO_VALUE_ON_ZERO to the MySQL database connection will land and we can remove this code because it will be set on the connection.

Does this have implications for migration tests? In core? In contrib?

Thanks, @catch, that helps!

And … not sure yet if this is directly related to #3260111: Regression AND bug: SQLite source and id map tables aren't joinable anymore, but SqlBase::mapJoinable() is unaware of this, but it sure seems related.

Yay, glad to see this in. Thanks!

Are we not crediting folks for reviews? Even if it's a small patch? 😉

Adding credit for @Wim Leers, @mradcliffe, and @mondrake for substantive reviews under our core credit guidelines. Thanks @dww for raising the issue. Once in awhile issue credits get reverted by a crosspost when the committer saves the issue.

Edit: Also some of the helpful discussion here was post-commit. The "looks good to me, +1" reviews on the initial patch are helpful but do not qualify for credit. :)

Removing tag per #11