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.
Fixed
@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.
✨ [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.