Done
+1 for ADR
@vadim.jin: Support for Drupal on PostgreSQL in another schema then public is something that is at the moment not supported. Would be great if we can make it work.
All use of the method yield() has been replaced with commitOrRelease().
The IS and the CR are updated.
Back to RTBC.
Let’s do #44
I like the change to only using Transaction::commit() or Transaction::releaseSavepoint() and having no Transaction::yield(). Developer need to learn the difference between to two.
The proposed changes look good to me.
Good that the label "Experimental" was added to the database driver description.
I have updated the CR.
Back to RTBC.
All code change look good to me.
The test has been improved.
For me it is RTBC.
It all looks good to me.
The only thing I am missing is the deprecation message testing.
Everything else is RTBC for me.
When a file is included, the code it contains inherits the variable scope of the line on which the include occurs. Any variables available at that line in the calling file will be available within the called file, from that point forward. However, all functions and classes defined in the included file have the global scope.
Now I get it. Thanks for the explanation. Could you remove the assert(isset($databases)); from the PR and add a comment with your explanation? After that it is RTBC for me.
When the module is not set in the connection array, Drupal will use the database driver name as the module name. Therefor we can mark this issue as outdated.
All code changes look good.
The deprecation message testing has been added.
For me it is RTBC.
We can as an alternative solution add a new index on the delta column. Not sure if a multi-column index would be better.
The change looks good.
Back to RTBC.
All code changes look good to me.
The new database driver is marked as "hidden" and therefore does not show up on in the install process.
The new database driver is marked as "experimental" and therefore not yet fully supported.
The CI pipeline is green for the mysql and the new mysqli drivers.
The IS and the CR are in order.
For me it is RTBC.
For the committer: As the database driver will be added to Drupal 11.2, all update test are skipped. There are no Drupal sites <11.2 with the new MySQLi database driver. The problem is that the first person who want to add an update test to Drupal 11.3 will have to create new fixtures which need to include the mysqli module.
@mondrake: Skipping the exiting update tests is not a problem. Only we need those fixtures for all the future update tests. Do you want me to do it?
I am fine with skipping Drupal\Tests\mysql\Functional\RequirementsTest.
#134 I looked at tests in that namespace, but still I miss what such a test should be written for, here.
Yes, you are right. The mysqli driver is hidden and therefore does not show up in the install process. My bad.
I have updated the CR.
Now we just need to update the fixtures and the CI pipeline to be green, then it is RTBC for me.
I am not familiar with such tests; can anybody point me to an example to work from, in case?
The test in the namespace Drupal\FunctionalTests\Installer
Added a mock CR... please help filling it in :)
I will do that.
For #131: For example core/modules/system/tests/src/Functional/System/DatabaseDriverProvidedByModuleTest.php
📌 [PP-1] Introduce a StatementBase abstract class Postponed has landed.
@benjifisher: Thank you for reviewing the issue. It is very much appreciated.
I am sorry to be so slow in responding.
@benjifisher: No worries, I am very happy that you review.
I have addressed all remarks from @benjifisher.
+1 for the reply from @bradjones1 in comment #18.
Let do ✨ Add "json" as core data type Active first and build the solution for this issue on top of JSON in DBAL.
I support the basic solution. Great work!
-
The DX can be improved. Do we really need to deprecate methods like Connection::escapeTable() and Connection::getPrefix(). Can we keep them? Changes like:
- $this->connection->getPrefix() + $this->connection->identifiers->tablePrefixor
- $this->connection->escapeTable($this->table) + $this->connection->identifiers->table($this->table)->forMachine()Those changes do not make for an improved developer experience. Lets keep those methods as a wrapper for the new implementation. Maybe we need to add some more wrapper methods to improve the DX. It is great that we have an improved identifier management solution. Now lets hide it from them as much as possible. It works just a little bit better then before. For most of them that is all they need to know and maybe they do not need to know at all.
-
The default database driver for Drupal is MySQL/MariaDB. Let the default implementation in the namespace
Drupal\Core\Database\Identifierbe for just MySQL/MariaDB. The specific changes that are needed for PostgreSQL and SQLite need to be done in their database driver modules. Like no "schema" stuff in the namespaceDrupal\Core\Database\Identifier. Other database drivers that are not supported by Drupal core, have their own specific stuff they need to override. Let use the database drivers for PostgreSQL and SQLite as an example for those contrib database drivers. The current solution is limited to making it work for the by Drupal core supported database drivers.
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.
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 .
@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.
Fixed.
@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.
@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 → 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.
quietone → credited daffie → .
Done
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
The database driver has been updated for Drupal 11.1.1
@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.