🇳🇱Netherlands @daffie

I have read the IS and most of the comments on this issue.
We are only adding the CachePreWarming service, but we are not going to use it.
The solution in the MR looks like the right one. Only as we are not going to use it we cannot be sure that it is the right solution.
When we are going to use the service we might find that this solution is not the right one and we need to change it.
Therefore it would be sensible to mark the new service as @internal. Changing the status to NW for this.

Everything else is to me RTBC.

Fixed

daffie → created an issue.

The CR looks good to me.
All code changes look good to me.
The CI pipeline is green for all 3 databases.
For me it is RTBC.

The Drupal\Core\EventSubscriber\ConfigImportSubscriber can be fixed with the following code change:

diff --git a/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
index bc85f19ecaf..5b34360f9da 100644
--- a/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
+++ b/core/lib/Drupal/Core/EventSubscriber/ConfigImportSubscriber.php
@@ -102,6 +103,7 @@ protected function validateModules(ConfigImporter $config_importer) {
     // Get the install profile from the site's configuration.
     $current_core_extension = $config_importer->getStorageComparer()->getTargetStorage()->read('core.extension');
     $install_profile = $current_core_extension['profile'] ?? NULL;
+    $database_driver_module = \Drupal::database()->getProvider();
     $new_install_profile = $core_extension['profile'] ?? NULL;
 
     // Ensure the profile is not changing.
@@ -159,7 +161,7 @@ protected function validateModules(ConfigImporter $config_importer) {
     $uninstalls = $config_importer->getExtensionChangelist('module', 'uninstall');
     foreach ($uninstalls as $module) {
       foreach (array_keys($module_data[$module]->required_by) as $dependent_module) {
-        if ($module_data[$dependent_module]->status && !in_array($dependent_module, $uninstalls, TRUE) && $dependent_module !== $install_profile) {
+        if ($module_data[$dependent_module]->status && !in_array($dependent_module, $uninstalls, TRUE) && !in_array($dependent_module, [$install_profile, $database_driver_module], TRUE)) {
           $module_name = $module_data[$module]->info['name'];
           $dependent_module_name = $module_data[$dependent_module]->info['name'];
           $config_importer->logError($this->t('Unable to uninstall the %module module since the %dependent_module module is installed.', ['%module' => $module_name, '%dependent_module' => $dependent_module_name]));

A lot of update tests are failing because they use core/modules/system/tests/fixtures/update/drupal-10.3.0.bare.standard.php.gz or core/modules/system/tests/fixtures/update/drupal-10.3.0.filled.standard.php.gz and the module mysqli has not been included in those fixtures.

The NamedPlaceholderConverterTest has been added. Therefore removing the tag "Needs tests".

We need the following code change to fix the failing Drupal\Tests\help\Functional\HelpTest.

diff --git a/core/modules/mysqli/src/Hook/MysqliHooks.php b/core/modules/mysqli/src/Hook/MysqliHooks.php
index 3f7e6683704..5fae187d16c 100644
--- a/core/modules/mysqli/src/Hook/MysqliHooks.php
+++ b/core/modules/mysqli/src/Hook/MysqliHooks.php
@@ -17,7 +17,7 @@ class MysqliHooks {
    * Implements hook_help().
    */
   #[Hook('help')]
-  public function help($route_name, RouteMatchInterface $route_match): array {
+  public function help($route_name, RouteMatchInterface $route_match): ?string {
     switch ($route_name) {
       case 'help.page.mysqli':
         $output = '';
@@ -26,7 +26,7 @@ public function help($route_name, RouteMatchInterface $route_match): array {
         return $output;
 
     }
-    return [];
+    return NULL;
   }
 
 }

All the code changes look good to me.
The CI pipeline is green for all 3 databases.
For the new method we shall need a CR.

I have removed the by reference part of the $query parameter. The CI pipeline stays green.

@benjifisher: Could you maybe explain why the by reference is not needed the $query parameter. We are changing the query in the method and we need those changes. Only it works without the by reference part. ???

@benjifisher: I think we have 1 open remark left and that is the one for not passing the database query by reference. As you are one of the subsystem maintainer of the migrate system, you may decide how you want this solved. Could you give a code example how you would like it solved, then I will implement it that way. I have now: $this->addFilterToQuery($request, $query);

✨ Additional blob field sizes in schema definitions Needs work needs to land before we can work on this issue.

Do we need to trigger a "PHP 8.3 PostgreSQL 16" test run?

I have tried, but I am not allowed to do that.

Everything is green.

@alexpott: Moving the deprecation to the constructor was the solution. Thank you for your help. It is clear to me why you are a core framework manager and I am not. ;-)

All code changes look good to me.
All deprecations have testing.
We have a change record.
For me it is RTBC.

@benjifisher: Thank you for the great review. I think I have addressed all your remarks.

The CI pipeline is failing with a PHPStan error.
Do we need a deprecation message test?

@dagmar: There is already testing for this, see: Drupal\Tests\migrate\Functional\MigrateMessageFormTest.

But what's the algorithm for deciding what counts as the 'middle'?

Take the maximum allowed length and subtract the strlen($this->getPrefix()).
From that subtract 14 characters for the hash value and 2 underscore characters before and after the hash value.
Now you got a number characters left. Half is for the first part of the table/alias name and the rest is for the last part of the table/alias name.

For instance: QWERTYUIOPASDFGHJKLZXCVBNM1234567890qwertyuiopasdfghjklzxcvbnm
Becomes something like: QWERTYUIOPASDFGHJK__10charhash__iopasdfghjklzxcvbnm

I have also made in the PR a suggestion for the method for how to do it..

To make the change as useful as possible for the views module, the last part of the original name/alias is very important. For others it is the first part. Therefore my suggestion is to replace the 'middle' part. As little as possible and replace it with a short 10 character hash. In this way we can preserve as much as possible from the original name/alias.
The problem with the views module is that we have a lot of aliases that are being abbreviated in the way you want and that results in aliases like:
- QWERTYUIOPASDFGHJKLZXCVBNM1234567890qwertyuiopasdfgh__DFSGHeabdfag
- QWERTYUIOPASDFGHJKLZXCVBNM1234567890qwertyuiopasdfgh__bdfhbEWRGEA

I have 2 aliases and that are different and know idea what the originals are. Yes, they are from 2 different joins, only which is which. The first part is important and the last part is important. That is why I would like to remove the "middle" part.

We can ask a framework manager/committer for some input/ideas?

I had another look at this issue and I found out that we have fixed this problem in the past. We are doing that in the added ExceptionHandler class. See: https://git.drupalcode.org/project/drupal/-/blame/11.x/core/modules/mysq...

Changed the migration message filter to a condition object.

I have reviewed the code changes and they look good.
My question has been answered.
There is already a lot of testing in core.
The change for a BC break is very small. Very unlikely.
For me it is RTBC.

daffie → made their first commit to this issue’s fork.

Could I get a review from one of the committers, I have rebased this PR now for the seventh time.

I am ok with adding the functionality for abbreviated table/alias names and the new methods createAbbreviatedTable() and getAbbreviatedAlias(). The last method shall be used by the views module.

I have added the by @alexpott requested deprecation to the service in core.services.yml.

In which case, can't it just carry on doing that if it wants to? Using this new API is optional.

We shall need to be very careful that we do not break any core/contrib/custom code.

A follow-up could take care of renaming those tables and switching the entity field storage system to using the API.

That will be a very big update for site owners. Are you sure that this improvement is worth the update for site owners?

We have similar issues like: 🐛 migrate mapping & messages table names are truncated, can lead to incorrect mapping lookups Needs work and 🐛 Identifiers longer than 63 characters are truncated, causing Views to break on Postgres Needs work .

Fixed

daffie → created an issue.

@quietone: It is for me RTBC with or without the nitpicks.

The big problem for me is what are we going to do with, for instance, tables from entity field storage system that have long tables names that have been abbreviating with a hash? Do we still want to support that or do you want to rename them? To me that is the big problem. If you have a better idea on how to solve this, then please speak up.

Fixed.

daffie → created an issue.

Fixed.

daffie → created an issue.

Fixed.

daffie → created an issue.

@faheemakhtar, Could you tell me which version of Drupal you are using? The module currently only supports Drupal 11.1. My second question is: did you follow the instructions in the readme file? Did you apply the Drupal core patch? Without it it does not work.

BTW Thank you for trying out this module. The easiest way to reach me is on Drupal Slack.

Fixed

daffie → created an issue.

@nod_: I did not know that I should have added the "[ignore]" to the title.

@quietone: I should have added "[ignore]" and I know you cleaning up the core issue queue. Thank you for all of your work on that. :)

daffie → created an issue.

daffie → changed the visibility of the branch 3503946-drupal-on-mongodb-11-1.x to hidden.

@quietone: Could you be so kind and not changed the Drupal core version. I am using the issue to create and to keep a Drupal core patch for my contrib module for MongoDB. Thanks.

daffie → created an issue.

quietone → credited daffie → .

Done

daffie → created an issue.

daffie → changed the visibility of the branch 3500849-allow-the-default to hidden.

daffie → created an issue.

Back to needs work for my remark on the PR.

The fields key needs to be set in mysql/Schema::getNormalizedIndexes(), because we are want the resulting index name to be less than 191 characters. When we use utf8mb4 the resulting index name will be below 756 bytes. That is the maximum name length for an index in InnoDB when the row format is REDUNDANT or COMPACT. See: https://dev.mysql.com/doc/refman/8.4/en/innodb-limits.html.

If it is up to me we would not be doing index changes in the method Schema::addField(). For the better and more stable solution would be the add a field first and then add an index in a seperate call with Schema::AddIndex(). That would be the better solution for all databases, including MongoDB.

As requested by @alexpott, the extending of the deprecated classes to the new ones in the pgsql module have been removed and the original code has been restored..

Fixed

daffie → created an issue.

The database driver has been updated for Drupal 11.1.1

daffie → created an issue.

@dpi: I get why you need to have a database connection that reconnects itself automatically. Doing an extra database call every time will kill performance and most of the time it is unnecessary. Most of the time we are done with the database connection long before the database connection times out. As an alternative we could add a new method called Connection::getConnectionWithReconnect(). Let the new method do the same as Connection::getConnection() only add the reconnect functionality.

✨ [PP-1] Introduce a Connection::executeDdlStatement method Active has landed.

The MR looks good to me.
The IS and the CRs need to be updated.

The CI pipeline is not happy. It fails for Drupal\KernelTests\Core\Database\DatabaseEventTest

Maybe we should do 📌 Allow parsing and writing PHP constants and enums in YAML files Needs work first?

The dummydb database driver is now dependent on the one for MySQL.
Back to NR.

How about adding a driver_test driver to core/modules/system/tests/modules/driver_test can be a copy of system/tests/modules/driver_test/src/Driver/Database/DriverTestMysql with the driver name changed to driver_test... less to maintain but the same amount of code coverage.

That sounds like a good idea, only it does not work. For that to work the $url need to change to: 'driver_test://test_user:test_pass@test_host/test_database'. That url gets parsed into components and the result will be:

array (
  "path" => "driver_test://test_user:test_pass@test_host/test_database"
);

No schema and host values are set and a \InvalidArgumentException wiil be thrown with the message: "The database connection URL 'driver_test://test_user:test_pass@test_host/test_database' is invalid. The minimum requirement is: 'driver://host/database'" The function parse_url does not accept schema's with an underscore character in it. To me, we have 2 options: the first is to change the module name "driver_test" to "drivertest". That will result in a lot of code changes. The other is to leave it like it is and go with the new test module called "dummydb". Putting the issue back to RTBC for the core committers decision.

We cannot use Drupal\core_fake\Driver\Database\CoreFakeWithAllCustomClasses for testing with this MR, because of the fact that the module name and the driver name are not the same.
Back to RTBC.

Removed the added style guide violations from the base line file.
Back to NR.

Addressed the remarks and suggestions of @alexpott.
Back to NR.

The requested deprecation test has been added.
Back to NR.

@smustgrave: Thank you for the review.

The requested warning has been added for non-core tests. Back to NR.

I think we should add a CR.

Applied the requested changes.

@mondrake: Thanks for the review.

All remarks on the MR by @mondrake have been addressed.
A test has been added for the new field storage create check subscriber.
Back to NR.

@mondrake: Thank you for the review.

My question has been answered.
All code changes look good to me.
For me it is RTBC.

This is a problem in the field system. Moving the issue to the field system queue.

This is a bug in the entity system. Moving the issue to the entity system queue.

I think we should add testing for this change. Lets add testing for:
- as a field of a new table
- add a field to an existing table
- change an existing field to use the new field type

For each way to create a field add a check that the field is of the correct type and do that for each new blob type.

Please add a change record

Changed the issue to a feature request.

All changes look good to me.
Back to RTBC.

Wrongly assigned to issue to @acbramley

@mondrake: It looks like a beautiful solution. My problem is that the solution will not work with MongoDB. The solution relies on that the database will throw an exception when the table does not exists. However that is not what MongoDB does. MongoDB just creates a table when one does not exists on an insert. A table in MongoDB is called a collection. It will be a table with no validation added to make sure that it only allows data in the right form being added. See: https://www.mongodb.com/docs/manual/reference/method/db.collection.inser....

The reason I started to work on this issue was to add a hook to allow the database driver to change the table schema. The database driver for MongoDB stores timestamps as MongoDB\BSON\UTCDateTime instead of an integer value as is done by the by core supported databases.

If we could make it to work for MongoDB that would be great. I will need some time to think how I can make it to work with MongoDB. If you have some ideas or suggestions, then please speak up.

My questions have been answered.
All code changes look good to me.
The IS and the CR are in order.
For me it is RTBC.

Back to NW for the 2 remarks on the MR.

I have one nitpick on the MR.
I think the CR needs to be updated for the part: "it will need to change in 12.0.0". We are not doing it in 12.0 but in 11.X.

The CI pipeline isn't happy. See: https://git.drupalcode.org/issue/drupal-3489427/-/jobs/3599665

Make Drupal great for sites with a great number of authenticated users.

That is what Drupal Core is missing. In the market there is a shift towards those kind of sites. Site owners want a more personalized experience for their users and they are switching from anonymous users to authenticated users. It would great if we could use Drupal for kind of sites.
To let Drupal be a great solution for complicated sites with a great number of authenticated users we need to do a couple of things:
1. Start using asynchronous PHP in Drupal Core. At least for retrieving data out of the database. 🌱 [meta] Use Fibers for concurrency in core Active & 📌 Add revoltphp/event-loop dependency to core Active
2. Switch to using MongoDB as the database and store all entity instances in their own JSON object in the database. This will lead to a lot less queries on the database, therefore making Drupal faster. The amount of functionality that Drupal requires from the database for supporting entity instances in JSON object is significant. The only database that can do that is MongoDB. ✨ [meta] Add database driver for MongoDB to Core as experimental Active & 📌 Drupal on MongoDB (the full PR) Active
3. Load the Drupal kernel (and other stuff) in memory before the user request comes in. #2218651: [meta] Make Drupal compatible with persistent app servers like ReactPHP, PHP-PM, PHPFastCGI, FrankenPHP, Swoole →

The combination of those 3 solutions combined with a more carefully use of how data is stored in the database and how it is retrieved will make Drupal great for sites with a huge number authenticated users.

The MongoDB community edition has more benefits:
- Support for regular search. We would no longer need a SOLR server next to your Drupal site. It is just stored in MongoDB. The 2 storage's do not need to be kept in sync.
- Support for vector search. Vectors are the way to store the results from an Artificial Intelligence (AI) in a database. With MongoDB those vectors can be indexed and searched. No Vector database needed and no need to keep data in sync between the two.
- Supports horizontal scaling. With this Drupal can support massive numbers of concurrent authenticated users.

The company MongoDB has a 1000 developers working on making the database a better product. The MongoDB has improved enormously over the last 10 years. When you last looked at MongoDB was more then sat 5 years or more. You need to take another look.
The databases we are using now (MySQL & MariaDB) have almost no improvements over the last 10 years. They are what we call lecacy products. MySQL is owned by the Oracle company and they want every user to switch over to their Oracle database. They are doing the minimum amount of support and development on MySQL.
To me, as the subsystem maintainer of the Database API and the database driver modules, MySQL and MariaDB are for the long term not the right database for Drupal.

For more info about Drupal on MongoDB: https://techblog.finalist.nl/blog/drupal-mongodb

I have updated the feed and there are now 2 Drupal related articles.

catch → credited daffie → .

All code changes look good to me.
My remark has been addressed.
The IS and the CR are in order.
For me it is RTBC.

I have just a single remark on the MR.
I have updated the IS and the CR.

All changes look good to me.
All my remarks have been addressed.
For me it is RTBC.

All changes look good to me.
All my remarks are addressed.
For me it is RTBC.