Create the database driver for MySQLi

IMHO we should also do #3217531: Deprecate usage of Connection::getDriverClass for some classes, and use standard autoloading instead to reduce bolierplate before starting this

I think we need to verify that this will actually be useful, by working on issues for some other parts of #3257726: [meta] Use Fibers for concurrency in core and then checking it does what we think it will do. But in principle adding a new driver as a new module sounds good.

Not sure why we'd do this though?

Add the PHP MySQLi extension as a requirement to the core MySQL and MySQLi database drivers.

Don’t you need product manager review?

Yep I think so for a new module.

Started something at https://github.com/mondrake/mysqli, copying from mysql. We would also need #3124382: Remove per-table prefixing and #3231950: Split Database tests in 'core' ones and 'driver specific' ones done before, though.

#8 is more or less working. The driver installs, most Database tests pass. There are issues with the fact that mysqli does not support inTransaction(), still to work out as much as proper statement rowCount().

I will spin off a separate issue to address couple of things that if core addresses will make the mysqli module code simpler.

Filed #3260007: Decouple Connection from the wrapped PDO connection to allow alternative clients

FTR: there’s also a PHP Async client fo PHP, https://github.com/amphp/mysql

The most interesting is sqlite client as it could use "bundled bootstrap database" so core could run out of box

So, after a fierce fight with mysqli's limping transaction management functions, now all the group=Database tests are passing for the mysqli module on GitHub. As of next Drush release (>11.0.5), it will also be possible to install Drupal via CLI.

See know limitations, and patches required, in the README.

As I see https://github.com/php/php-src/pull/7889 also mostly ready, so using mysqli will benefit decoupling from libmysql client libs

@mondrake great work! I think it makes sense to file issue to php-src (at Github) about savepoints, maybe it's a supposed behavior

@mondrake as I see it depends on which kind of library used for mysqli https://github.com/php/php-src/blob/PHP-8.1.2/ext/mysqli/mysqli_nonapi.c...

@andypost mind opening the issue to php-src? I'm not familiar.

#16 that seems related to savepoints, not rollbacks?

.

Nice writeup re differences between PDOMYSql and mysqli, https://websitebeaver.com/php-pdo-vs-mysqli

From the github repo, it looks like any core inclusion is currently blocked on #3110546: Allow contributed modules (mostly database drivers) to override tests in core and #3256642: Introduce database driver extensions and autoload database drivers' dependencies, so I'm going to mark this PP-2.

None of this would stop us trying out some proof of concept stuff with PHP Fibers though.

Actually, #3110546: Allow contributed modules (mostly database drivers) to override tests in core is only necessary on GitHub to skip running the ApcuBackendTest on GHA - I couldn't find a away to enable APCu properly there, so I am still skipping it.

However, I would say that to be on a preferrable scenario, we would need two more issues in first:

#3217699: Convert select query extenders to backend-overrideable services - this left harbour one year ago, then sank couple of weeks later, with only limited SAR activities afterwards. It's tagged for framework manager review.

#3217531: Deprecate usage of Connection::getDriverClass for some classes, and use standard autoloading instead - to reduce boilerplate empty classes, which is blocked on the above

Actually, #3265086: Implement statement classes using \Iterator to fix memory usage regression and prevent rewinding too.

Having committed #3217531: Deprecate usage of Connection::getDriverClass for some classes, and use standard autoloading instead, now #3217699: Convert select query extenders to backend-overrideable services is not strictly necessary to get this moving on.

Last blocker is in.

From the issue summary:

Create the MySQLi database driver which should extend the MySQL database driver.
Add the PHP MySQLi extension as a requirement to the core MySQL and MySQLi database drivers.

Why would we need the MySQLi extension for the current PDO-based MySQL driver?

Very probably it means mysqlnd extension which is primary driver for Mysql and it's reused by PDO_mysql at least

I'm still doing some changes on the mysqli module on GitHub - I'd like to have it pass tests there before moving the code here. Also, I think we should try to refactor there the parser code that was taken from doctrine/dbal to manage converting SQL statements from having named placeholders (':placeholder') to have positional placeholders ('?') - it's overkill for the specific purpose here.

Question: do we want to introduce a new 'mysqli' module, or shall we try to have a single 'mysql' module with two drivers, 'mysql' and 'mysqli'?

@lauriii posted some question in slack, added them, and the answers to the issue summary.

Question: do we want to introduce a new 'mysqli' module, or shall we try to have a single 'mysql' module with two drivers, 'mysql' and 'mysqli'?

Two drivers in the existing module might be simpler, but:

1. Can we make it work where only one option is available due to installed extensions? And also not show drivers that won't work to people?
2. Would there be any need for the new driver to be experimental?

btw depending on the answers to this, there might be issue summary updates needed now.

Lauri pinged me to provide feedback as a product manager. These two statements are contradictory and are critical to asses the impact of the proposal I think :)

We need to continue to support PDO since not all hosting has php_mysqli enabled, so it has to be a new driver. However it might be fine to have both drivers in the same module.

vs

Add the PHP MySQLi extension as a requirement to the core MySQL and MySQLi database drivers.

IMHO there's no need to add the PHP MySQLi extension as a requirement to the core (PDO, existing) MySQL database driver.

That will obviously have to be a requirement for the NEW MySQLi driver, though.

The installability of a driver is driven by the installable() method in the Install/Tasks class of each driver.

For the existing PDO/MySQL driver it is inheriting from the base class

  public function installable() {
    return $this->hasPdoDriver() && empty($this->error);
  }

For the new driver it will be something like

  public function installable() {
    return extension_loaded('mysqli');
  }

I think in case if both drivers are available some description should explain benefits using mysqli
So on the most of hostings user could contact support to ask more performant driver

If benefits is obvious then we can add some logic to set "preferred driver" at install screen

I'll start adding the code from GitHub as a separate module, since I understand the driver may start as experimental, and I do not think we can have a module with a released driver and an experimental one in the same base.

OK, now code checks pass. I suppose the next step is to get tests run using the new driver, but I have no clue how this can happen. I assume infra need to do infra things?

I filed #3384661: Upgrade PHP 8.2 image to 8.2.10 and as the new image will pass tests we can and mysqli extension

The extension already in the CI image

$ docker run --rm -it drupalci/php-8.2-apache:production php --ri mysqli

mysqli

MysqlI Support => enabled
Client API library version => mysqlnd 8.2.0
Active Persistent Links => 0
Inactive Persistent Links => 0
Active Links => 0

Directive => Local Value => Master Value
mysqli.max_links => Unlimited => Unlimited
mysqli.max_persistent => Unlimited => Unlimited
mysqli.allow_persistent => On => On
mysqli.rollback_on_cached_plink => Off => Off
mysqli.default_host => no value => no value
mysqli.default_user => no value => no value
mysqli.default_pw => no value => no value
mysqli.default_port => 3306 => 3306
mysqli.default_socket => no value => no value
mysqli.allow_local_infile => Off => Off
mysqli.local_infile_directory => no value => no value

EDIT from container log - PHP settings are
./configure --with-config-file-path=/usr/local/etc/php --with-config-file-scan-dir=/usr/local/etc/php/conf.d --enable-ftp --enable-mbstring --enable-mysqlnd --with-curl --with-libedit --with-zlib --with-kerberos --with-openssl --with-mysql=mysqlnd --with-mysqli=mysqlnd --with-pdo-mysql=mysqlnd --with-pdo-sqlite --with-pdo-pgsql --enable-phpdbg --with-readline --with-freetype --with-zlib-dir --with-jpeg --with-mcrypt --with-xsl --with-tidy --with-xmlrpc --with-gettext=shared --enable-gd --with-webp --with-avif --with-pear --enable-sockets --enable-exif --with-zip --enable-soap --enable-sysvsem --enable-cgi --enable-sysvshm --enable-shmop --enable-pcntl --enable-bcmath --enable-xmlreader --enable-intl --enable-opcache --with-apxs

@mondrake I think you can edit drupalci.yml to run only new module's testgroup

#50 no I think we need first to be able to install using the new driver. I.e. a new test environment option like Test PHP 8.1 & MySQL 5.7 via MySQLi. Editing drupalci.yml we can only get the new module test to run with the other PDO drivers under the hood.

Attention - the driver requires minimum PHP 8.1.2 where this was fixed.

@mondrake as I see last CI run https://dispatcher.drupalci.org/job/drupal_patches/201116/testReport/Dat...
there's mysqli tests running fine)

Posgres also supports async queries
https://www.php.net/manual/en/function.pg-send-query.php

Is there a chance that Drupal some day would have a driver for it?

I've updated the issue summary to remove the bit about the mysqli requirement on the PDO driver and to say we'll start with an experimental module but eventually merge the stable driver into the main mysql module.

@Chi a pgsql driver should be able to build on the work done here, so that seems possible if someone's motivated to work on it.

I think mysqli module should implement hook_requirements() and check if PHP MySQLI extension is available.

#56 maybe, but I do not think that's relevant. During early installation there's no module concept yet, and installability of a driver is checked as described in #43.

We already recommend PHP 8.1.6 or above. Easy to add a hard requirement on PHP 8.1.6 to the experimental module, but we could probably only add a hook_requirements() warning checking the database settings + PHP version once it's incorporated into the main mysql module.

I wonder if we should try to do the mysqli testing directly on gitlab CI rather than getting a new environment on DrupalCI since it's getting a lot closer to the point where core can switch over entirely - not sure exactly how close though and wouldn't want to artificially hold this up on that either.

I did some googling about PDO support for async and found the PDO brainstorming page: https://wiki.php.net/internals/pdo/brainstorming

If that's up-to-date, then it's been discussed, but nothing has happened for years. It's possible that PHP Fibers support will increase demand and might spur some activity, but I think we're looking at more years before it would be supported, if ever.

Should we open a follow-up on how to handle the actual async support? For the BigPipe/Renderer implementations of fibers the main use case is to fire off a single async query, suspend the fiber so the calling code can do something else, then when we're resumed see if it came back. It looks like this might require multiple database connections (since 'something else' could also be executing database queries) so could be a bit tricky.

I can't see us needing or wanting to execute multiple queries async in core although support for that would be good to add at some point, maybe its own issue.

edit: opened the follow-up #3384763: Async database query + fiber support

##6 Yeah I think better have that as a follow-up, the issue here I think it's to get a mysqli driver that is like-for-like the pdo_mysql one.

Probably it will be easier to fix credentials in gitlabci as drupalci_testbot has a lot of things hardcoded

• https://git.drupalcode.org/project/drupalci_testbot/-/blob/dev/src/Drupa...
• https://git.drupalcode.org/project/drupalci_testbot/-/blob/dev/src/Drupa...

Replied via slack, copying here too:

This is the way we are defining the database and variables for GitlabCI for core: https://git.drupalcode.org/project/gitlab_ci_testbed_for_drupal_core/-/b...

Note that this uses the images generated from https://git.drupalcode.org/project/drupalci_environments but does not use https://git.drupalcode.org/project/drupalci_testbot to run the tests.

We connect the images via the .gitlab-ci.yml file and just call the core/scripts/run-tests.sh file to run the tests, so everything you need should be able to be set here: https://git.drupalcode.org/project/gitlab_ci_testbed_for_drupal_core/-/b...

I *think* now on GitlabCI we can run tests on the new driver by only changing the database URL. Just for the sake of getting it run I changed in the MR from the PDO MySQL driver to mysqli, obviously the change would have to be structured moving forward.

Yes, a failure like

    PHPUnit Test failed to complete; Error: PHPUnit 9.6.8 by Sebastian Bergmann
    and contributors.
    
    Testing Drupal\Tests\migrate_drupal\Kernel\MigrateMissingDatabaseTest
    E                                                                   1 / 1
    (100%)R
    
    Time: 00:01.211, Memory: 4.00 MB
    
    There was 1 error:
    
    1)
    Drupal\Tests\migrate_drupal\Kernel\MigrateMissingDatabaseTest::testMissingDatabase
    mysqli_sql_exception: php_network_getaddresses: getaddrinfo for
    does_not_exist failed: Name or service not known
    

tells us that the pipeline used the new driver for testing.

There are quite a few things to fix looking at the pipeline failures.

Found a blocker. Filed #3389202: migrate\Plugin\migrate\id_map\Sql assumes a PDO db driver.

There are update tests failing because the database dumps contain the 'mysql' module installed, but since the test are run with 'mysqli', when updating we get a requirement error 'install the mysqli module that contains the database driver'.

Not sure how to tackle that.

it could use update hook to pass upgrade tests, or it will need common fixture to apply to all this tests

There are update tests failing because the database dumps contain the 'mysql' module installed, but since the test are run with 'mysqli', when updating we get a requirement error 'install the mysqli module that contains the database driver'.

Not sure how to tackle that.

I think we've had the same problem when the database driver modules were introduced in the first place, the way to fix it is to re-import the database dump into a real site, install the mysqli module, then re-export the database dump. Then it gets picked up when you try to update from the dump and everything works (or hopefully). You have to rewrite history to make it seem as if the mysqli module existed at the time of the dump - so it might mean manually copying the module folder back to the old commit hash that you import the database dump to, or potentially manually editing the uncompressed dump to add the correct lines etc.

Thanks for #71 (that's what I was afraid of :))

Tried manually updating the dump files, just to add 'mysqli' to the system.schema key_value collection (should suffice? module has no config), then compressing them with
$ tar -czf drupal-9.4.0.bare.standard.php.gz drupal-9.4.0.bare.standard.php

But now the import fails during the update test with
ParseError: syntax error, unexpected character 0x00, expecting end of file

It could be some wrong way of calling tar?

Just throwing here if anyone has faced this earlier and has a ready-made answer

#72 actually found #3305003: Create standard steps for creating database dumps.

FTR, the changes to the database dumps:

1. gunzip core/modules/system/tests/fixtures/update/drupal-9.4.0.[type].standard.php.gz ([type] three of them, bare, filled and phpass)

2. Edited and manually added the module: add 'mysqli' to the system.schema key_value collection and to the extension.core collection

3. gzip core/modules/system/tests/fixtures/update/drupal-9.4.0.[type].standard.php

Note - do not use tar for the purpose as it fills files with null characters at the end

We are almost there. There are still a handful of installer test failures, again because an attempted uninstall of the 'mysql' module fails since it is a dependency of the new module, and 2 kernel tests that fail on MySql 8 but not on MySql 5.7.

So, this is passing on MySQL 5.7 now, but not on MySQL 8 because of this test failing:

PHPUnit 9.6.8 by Sebastian Bergmann
    and contributors.
    
    Testing Drupal\KernelTests\Core\Database\BasicSyntaxTest
    ...E...                                                             7 / 7
    (100%)
    
    Time: 00:05.293, Memory: 4.00 MB
    
    There was 1 error:
    
    1) Drupal\KernelTests\Core\Database\BasicSyntaxTest::testConcatWsFields
    Drupal\Core\Database\DatabaseExceptionWrapper: Illegal mix of collations
    for operation 'concat_ws': SELECT CONCAT_WS('-', ?, "name", ?, "age") FROM
    "test48178452test" WHERE "age" = ?; Array
    (
        [:a1] => name
        [:a2] => age
        [:age] => 25
    )
    
    
    /builds/issue/drupal-3259709/core/modules/mysqli/src/Driver/Database/mysqli/ExceptionHandler.php:56
    /builds/issue/drupal-3259709/core/lib/Drupal/Core/Database/Connection.php:858
    /builds/issue/drupal-3259709/core/tests/Drupal/KernelTests/Core/Database/BasicSyntaxTest.php:67
    /builds/issue/drupal-3259709/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    
    Caused by
    mysqli_sql_exception: Illegal mix of collations for operation 'concat_ws'
    
    /builds/issue/drupal-3259709/core/modules/mysqli/src/Driver/Database/mysqli/Statement.php:84
    /builds/issue/drupal-3259709/core/lib/Drupal/Core/Database/Connection.php:826
    /builds/issue/drupal-3259709/core/tests/Drupal/KernelTests/Core/Database/BasicSyntaxTest.php:67
    /builds/issue/drupal-3259709/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    
    ERRORS!
    Tests: 7, Assertions: 8, Errors: 1.

I cannot reproduce this locally; can it be it's some setting of MySQL8 on the runner?

Also, Drupal\Tests\mysqli\Kernel\mysqli\LargeQueryTest behaves erratically, if I remove the dump() in it then the statement errors under destruction (?).

Apart from these two, I think this can go through more heavy-lifting reviewing.

Some help would be appreciated on the GitlabCI set-up to make this run independently from the PDO-Mysql tests; the current changes are only a shortcut to make test run with mysqli instead of pdo_mysql.

I added the async job as a separate job and left the others as they were. Note that this issue contains #3392739: _TARGET_DB_TYPE does not exist that will be committed shortly, so we might get a conflict on the rebase. I can take care of it.

We might want to call it 'MYSQLI' instead of 'ASYNC' but otherwise that looks great.

Now I understand the logic, thanks a lot @fjgarlin.

How about generalizing more, and instead of DB_ASYNC, add two more variables

_TARGET_DB_DRIVER
_TARGET_DB_DRIVER_MODULE

and use them too in building the database URL?

@mondrake - we initially had some more variables around the database type and version, but wanted to simplify the job definition as much as possible, so it's easy to understand and to create new jobs. Having said that, it's an option.

Right now, we only added an optional variable, which I think keeps the simplicity of new jobs. I renamed it as suggested in #78 but feel free to rename it again if there is a better name.

I had some misgivings on user space parsing of SQL but there's nothing better we can do: I hoped we could reuse the parser in PDO. Alas, this is not doable. If you need the grisly details, see below.

1. When PDO::ATTR_EMULATE_PREPARES is set to true, the query gets parsed on execute and the parsed query is displayed by PDOStatement::debugDumpParams however the parameters are replaced with their values. This happens in ext/pdo/pdo_stmt.c, search for stmt->active_query_string.
2. Otherwise, the MySQL driver replaces the named parameters with positional ones (yay?). However, within mysql_handle_preparer the only time this string is used it is passed outside of PHP, to mysql_stmt_prepare within the MySQL driver. Sad. Maybe the caller could help? This preparer is called here and it does pass statement as a pointer to the variable, so the caller could in theory do something with it but it does not, the variable is never used again. So unless the MySQL API has a way to read the query out of a prepared statement and PDO exposes this we can not use this parsed result despite it is exactly what we need. Looking at the MySQL API, the documentation says "The MYSQL_STMT structure has no members intended for application use" so the only hope is a function but there is none.

We could write a small patch to be included in PHP-next which would allow us to drop the userspace parser in D11 or D12. I will try.

working on a unit test for NamedPlaceholderConverter

Added tests for NamedPlaceholderConverter.

@chx thanks for #81.

Clearly, NamedPlaceholderConverter is not optimal: it destroys and rebuilds a SQL statement that was built already.

A solution to try and retrieve from pdo_mysql something to be used by mysqli is for me ... weird. It'd keep PDO as a required PHP extension, and would just move the parsing from our class to PHP internals. If (if, I dunno) that would also require establishing a client connection it would be even more weird.

On the other side, IIUC mysqli is meant to be a very thin client. Expecting it to implement positional placeholders would be against its concept.

Personally, I think we should aim at trying to embed alternative placeholder generation (positional/named) into dynamic queries instead - and avoid reparsing post-factum. It would definitely increase performance. Then, we should be able to still do the placeholder conversion for static queries - allowing an option to be passed to ::query(). But that would be D11 material IMHO.

From a release management point of view the plan in #85 looks good to me. I suggest also opening a plan issue for the mysqli driver with the steps from #85 so we have them documented as what is left after this issue is committed.

Thanks for #85! Looks clear to me. Re. #85.2, I suggest to add #84

embed alternative placeholder generation (positional/named) into dynamic queries

to the meta plan suggested in #86.

Now, we have a failure in the recently fixed SqlContentEntityStorageRevisionDataCleanupTest.

This is due to this code

    $result = $connection->query('SELECT nid, vid, langcode FROM {node_field_revision} WHERE nid = :nid', [
      'nid' => 8,
    ])->fetchAll();

The arguments in the queries of this test DO NOT HAVE the colon in front, i.e. it should be ':nid' => 8.

Apparently the PDO-based drivers do not care, even if https://www.drupal.org/docs/drupal-apis/database-api/static-queries#plac... examples clearly indicate placeholders should have the colon.

I'm not sure whether we should fix the test, or relax NamedPlaceholderConverter to also allow placeholder's array keys not to have a colon.

Done the two points from #85 that are for this issue,

• Make it is experimental.
• Add in the autoloading of database driver module dependencies an override to when the driver is "mysqli" to also autoload the "mysql" module.

but tests are missing. Any idea/reference on how to do those?

Just a note I have #3384763: Async database query + fiber support open for the next step (or next two steps, depending on how much async support we add in one issue vs. two).

Updated IS with a list of todos remaining here; feel free to add what is necessary.

Also help on the points would be appreciated.

Re #89 I eventually thought that even if SqlContentEntityStorageRevisionDataCleanupTest should be probably fixed, nevertheless it's better allow parameter arrays with keys missing the initial colon, given that PDO seems to be supporting both. So we can support contrib code that might have missed this detail.

Fixed the collation bug affecting BasicSyntaxTest in MySQL 8. I had to move the test to driver-specific, and for mysqli force conversion of the parameters' values to utf8mb4. Not sure it's the right thing to do but could not find other options. It's an edge case anyway, and could not replicate the issue locally either.

Tests are now green on mysqli for both MySQL 5.7 and 8.

Added test to ensure module is experimental and hidden.

Postponing on the referenced issues, that will make the management of the lack of transactional DDL support consistent across the two drivers.

Uhm... I don't remember how it works. Definitely, a 'module' extension is different from a 'database_driver' extension. The list of database to be presented for installation is independent from the modules, simply because there's is even no notion of the 'module' at that stage of the installation process. So, it could be the database driver 'mysqli' is presented for (a grassroot) installation if the PHP mysqli extension is available, even if the module itself is hidden. Then, during the (grassroot) installation, the mysqli module is installed automatically (even if hidden). But since it's hidden, there will be no way to uninstall/install it from the UI (that, when it's accessed, presumes all the installation, config and bootstrap to be complete).

Worth giving a manual test to all this process, I guess.

Then, if the expectation is to just allow changing 'driver' => 'mysql', to 'driver' => 'mysqli', in settings.php to play around, then we have more to do - i.e. disallow reading the namespace from settings.php in this case (because that will be still the mysql one, and an existing 'namespace' key in the settings takes priority on the 'driver' key), and allow manually installing the mysqli module (so removing the hidden: true from the module yaml).

Thanks for your reviews and input, @daffie. Would you mind checking back the inline comments and replies and not which ones we can mark as solved from your POV, so we can just keep the open ones to work on?

Filed #3399150: [PP-2] Enable dynamic queries to produce SQL with positional placeholders for follow up.

Rebased, with the exception of database dumps which is a pain in the neck to do; that update IMHO should be done just before commit.

The mysqli construction approach has limited ability to define an SSL connection, but leveraging the procedural approach allows you to establish options prior to making a connection.

For example:

      $mysqli = \mysqli_init();
      // Reusing PDO keys for simplicity and commonality with the `mysql` driver
      $mysqli->ssl_set(
        $connection_options['pdo'][\PDO::MYSQL_ATTR_SSL_KEY],
        $connection_options['pdo'][\PDO::MYSQL_ATTR_SSL_CERT],
        $connection_options['pdo'][\PDO::MYSQL_ATTR_SSL_CA],
        $connection_options['pdo'][\PDO::MYSQL_ATTR_SSL_CAPATH],
        $connection_options['pdo'][\PDO::MYSQL_ATTR_SSL_CIPHER]
      );

      $mysqli->real_connect(
        $connection_options['host'],
        $connection_options['username'],
        $connection_options['password'],
        $connection_options['database'] ?? '',
        !empty($connection_options['port']) ? (int) $connection_options['port'] : 3306,
        $connection_options['unix_socket'] ?? '',
        MYSQLI_CLIENT_SSL // This can likely be made through a connection option
      );

Which issue is this postponed on? I can't see anything listed in the issue summary.

It's currently postponed on #3384999: Introduce a Schema::executeDdlStatement method per #97.

Thanks added a note to the issue summary.

Looks it still blocked on #3487851: Replace \PDO::FETCH_* constants to indicate fetch mode with an enumeration

I am working on #3488467: Introduce a StatementBase abstract class which I believe would be simplifying the Statement class here.

MR!1135 incorporates the changes of #3488467: Introduce a StatementBase abstract class and makes evident the simplification possible on the Statement class.

Made some progress. This is now postponed on #3488467: Introduce a StatementBase abstract class.

For review. Fixing fixtures for update tests is a real PITA, so please let's focus on other items for now and let's address them when we are all comfortable with all the rest. Thanks.

Filed #3516489: @phpstan-ignore-next-line annotation collides with PHPCS in the Coder issue queue.

There's a smarter way than adding a Schema class override. Working on it.

Introduced a SchemaPrimaryKeyMustBeDroppedException to abstract the error and drop the Schema class.

Pre-8.0 MySql ANSI modes were removed in #3421175: Update INSTALL.txt and hook_requirements() etc. with remaining Drupal 11 platform requirements, adjusted Connection::__construct() accordingly.

Charset toggling was removed in #3437786: Remove MYSQLND_MINIMUM_VERSION and LIBMYSQLCLIENT_MINIMUM_VERSION checks, adjusted Connection::open() accordingly.

Filed https://github.com/drush-ops/drush/issues/6266 in drush issue queue; drush will need some adjustment to make it work with this module when it will be committed to core.

@daffie

A number of tests only are run for certain database drivers. When such a test is run for the "mysql" database driver, the "mysqli" database driver needs to be added.

Are you talking about the kernel driver-specific tests? If so, they are run for mysqli based on inheritance from the mysql ones, we need not do anything. If something else, can you elaborate?

Added a mock CR... please help filling it in :)

I know we have made this module "hidden", but should we have installertests?

I am not familiar with such tests; can anybody point me to an example to work from, in case?

#134 I looked at tests in that namespace, but still I miss what such a test should be written for, here.

Thanks @daffie

Sanity check on #136, before I get hands dirty with db fixtures: apart from Drupal\Tests\mysql\Functional\RequirementsTest, all the other failing tests are update tests that rely on the 10.3 db dump fixtures.

Are we really sure we want to rewrite history and pretend a mysqli module was available in 10.3? In real life no one will ever update from a 10.3 install that already has mysqli installed. So is it right to rewrite history to make the tests happy, or should we just skip somehow the tests?

The situation in #71 was different - there we had the 'modulification' of db drivers that were already present beforehand, here we are introducing a totally new module.

I'll ping @catch for an opinion here too

#138 well we are missing a c/p of mysql.install in this module so that the requirements can be rendered... that file was added to core 2 years ago in #2733675: Warning when mysql is not set to READ-COMMITTED but did not get its way into the mysqli module. So the test failure is legitimate I think and we should fix it.

In https://git.drupalcode.org/project/drupal/-/merge_requests/11355/diffs?c..., we are skipping the update tests if the driver is mysqli and the database dump is from drupal-10.3.0.

I think this is more realistic and avoids tampering with database dump fixtures.

The future update tests, relevant for mysqli, will be those introduced to update from 11.2 to above, right? Because those introduced before 11.2 are not relevant since the module is not released yet.

Then the task will be to add a new database dump for 11.2.0, at that moment including the install of myqli.

But that's a separate issue from here if we agree to proceed as per #140. A new dump will get its own 11.2.0 version name and this check

      // Determine the version of the database dump if specified.
      $matches = [];
      $dumpVersion = preg_match('/drupal-(\d+.\d+.\d+)/', $file, $matches) === 1 ? $matches[1] : NULL;

      // If the db driver is mysqli, we do not need to run the update tests for
      // db dumps prior to 11.2 when the module was introduced.
      if (Database::getConnection()->getProvider === 'mysqli' && $dumpVersion && version_compare($dumpVersion, '11.2.0', '<')) {
        $this->markTestSkipped("The mysqli driver was introduced in Drupal 11.2, skip update tests from database at version {$dumpVersion}");
      }

will no longer skip the test, if the test requires that dump. But it will keep skipping tests using the 10.3.0 dump.

In other words: if we do this, we do not need to change the fixtures now. We can wait for 11.2 to be released.

Pipelines are green, yay.

Rtbc++ for db dumps there's follow-up #3305003: Create standard steps for creating database dumps

Drush problem reported in #129 is actually due to a bug in Connection::convertDbUrlToConnectionInfo() that is checking existence of a class before the dependent classes can be autoloaded. Fixed here, but this needs to go back to NR for the fix now, sorry.

done, tks

Found that mysqli::exec() does not exist while testing manually the GUI installation and database is not yet present.

Creating a database is a nice nitpick, back to RTBC

Testing manually, I can see the new driver showing up at installation time. The CR says differently, so it needs update. Besides, there is no indication that the driver is experimental or hidden - those are 'module' notions, but at the stage of database selection in the installer modules are not yet a thing. So if you select the mysqli driver for installation, its 'experimentality' is a consequence as the driver is included in the mysqli module that is enabled as part of the install process and is marked experimental.

I will add explicitly '(experimental)' in the driver string for now. In a follow up, we could add an additional Connection method indicating and reporting the db client in use (it being PDO, mysqli, oci8, sqlite3, etc).

Done #152. Added pre/post screenshots drush and GUI.

One question inline related to copyright

Found #3524656: Fix Drupal\Core\Database\Fetch* types as they are wrong while reviewing this one.

Updating issue credit.

Looking into latest feedback.

Ok, I think we are safe assuming if anyone is using the mysqli driver, we will not support doing magic to fix malformed settings in settings.php like we do for the drivers that were present in Drupal before database drivers became modules.

Thank you! Let's get it in and discover perf issues in follow-up as jobs on mysqli looks little bit slower

#161 well for sure NamedPlaceholderConverter has its own overhead. But yea... later.

Adjusting new tests to use attributes.

back to RTBC as changes reflect latest core commits

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.

Postponed on #3497431: Deprecate TestDiscovery test file scanning, use PHPUnit API instead

MR !12367 was a mistake.

Unblocked. Rebased and fixed new CS errors reported.

Did not land in 11.2, so updated the CR draft to say 11.3. Also tagging for release highlights for 11.3.

Let's get this in 11.x nice and early to allow us to work on the follow-ups and get them in 11.3.0 too.

Committed 42b7481 and pushed to 11.x. Thanks!

helpful follow-up for contributors #3531022: Let run-tests.sh print db type and version in the initial output

Would it be worth a separate issue to simplify the implementation logic so the extra $databases[..]['dependencies'] logic could be automatic based upon the class hierarchy rather than needing to be explicitly defined?

Unsure if this is truly related and might be caused by this change, but I'd like to draw attention to #3582827: FiberError: Cannot switch fibers in current execution context (Backup Migrate)