The first part of the database driver files have been uploaded to the 3.x branch.
All the code changes look good to me.
The added code comments are clear.
For the different situations is testing added.
The testbot is green for all supported databases.
For me it is RTBC.
The testbot is failing on PostgreSQL on your added code.
The CR needs examples on how the use the added GIN and GIST indexes.
Forgot to change the status to NW.
The PR looks good to me.
All the minimum database versions have been correctly updated.
For me it is RTBC.
Changed the status by accident.
The change record looks good to me.
The change record looks good to me.
@rfay Is it a possibility to move SQLite support in DDEV from default included to must be added with an addon command? Just like for Oracle and MongoDB.
The change record looks good to me.
Looks good to me.
@mondrake: Could you explain to me how with this change, it would help with 📌 Adding GIN and GIST indexes to PostgreSQL databases RTBC . Also with DB's that do not support transactional DDL. Why does this change make it better for those DB's? I missing why with this change it will be better.
Could we set the minimum required version for SQLite in core/modules/sqlite/src/Driver/Database/sqlite/Install/Tasks.php to 3.45 for D11 and see that it passes the testbot.
@bbrala: I have created a patchfile for Drupal 10.1.x. It should pass the tests in the namespace Drupal\KernelTests\Core\Database.
A MongoDB replicaset is needed, because a single server of MongoDB does not support multi document transactions.
After you have enabled the mongodb PHP extension, the style guide checker will run and fail. I will fix hose violations.
Again: thank you for helping me.
As 🌱 [11.x] [meta] Set Drupal 11 platform and browser requirements at least six months before the release Active just got in and given the 6 months period, can I conclude that we have missed the release window for mid-June and late July?
Let's add Swoole then also for searchability.
All the code changes look good to me.
For things like this I wonder if the database driver can hint whether we can do raw SQL updates, and we have both paths in the update hook, falling back to entity save in non-SQL cases?
Sounds like a good idea to me. Raw queries are great for performance, only you need to know exactly how the entity data is stored in the database. Creating a fallback for non-sql databases is a great idea. It will be needed after we have a non-sql database driver like the one for MongoDB. It is not in core yet and a fallback will only needed for changes after there are sites running on MongoDB. For now, yust not needed. The database driver for MongoDB can mark the update test as skipped.
Could we just check if the entity storage is SqlContentEntityStorage?
Sorry, but no. The database driver for MongoDB uses SqlContentEntityStorage for its entity storage.
+1 for RTBC.
This really cleans up the Database layer and the drivers a LOT!!!
@smustgrave: Thank for the review.
I have added the return type hint to the new method Condition::compare().
Anyone knows where is the documentation about introducing a new method inside a public interface
I think in this issue we do it without adding the new method to the interface. In a followup we than can add the method on the interface in a new major version. We can do BC breaks in a new major version.
For me the created developer experience could be better than with adding a second parameter to the method groupBy().
Personally I think that create another method called groupByExpression() would be much better in the developer experience. Now we can do $query->groupByExpression('ROLLUP ("' . $field1 . '", "' . $field2 . '")'); instead of $query->groupBy('ROLLUP ("' . $field1 . '", "' . $field2 . '")', TRUE);. Every Drupal developer knows the method addExpression(). The new method does the same only in combination with group by.
As I am removing the tag "Needs subsystem maintainer review" as I the subsystem maintainer for the Database API.
We also need a change record.
@andypost: Thanks!
The deprecation of the use of string conditions in joins in dynamic queries has been removed from the MR and a followup has been created for the deprecation ( 📌 [PP-2] Deprecate the use of string conditions in joins in dynamic queries Postponed ).
In the MR are the changes for the join and relationship plugins of the views module removed. I have created 📌 [PP-1] Make the conditions in joins in dynamic queries use Condition objects in the Views module Postponed as a followup to make the changes to the views module.
daffie → created an issue. See original summary → .
@Wim Leers: Ok, than it is fine. Just want to make sure that Drupal does not get slower. :)
I think that we should make sure that we have an database index for every query we add orderby().
if you look at the top level Postgres build you can see that the normal test runs pass and only the test-only run fails, as we would hope
I did not know we had a "tests only" run. That is really great!
@lendude: Thank you for the explanation!
The testbot for PostgreSQL is still failing. See: https://git.drupalcode.org/issue/drupal-3415118/-/jobs/653536
This issue has not landed yet and SQLite 3.45 has been released. Lets set the minimum required version to SQLite 3.45.
Back to NR for the Core release managers.
Reroll for D10.2 looks good to me.
The requested TODO has been added to the correct issue.
Back to RTBC.
@mondrake said in #3405976-84: Transaction autocommit during shutdown relies on unreliable object destruction order → :
We are currently blocked by ✨ Introduce a Connection::executeDdlStatement method RTBC
As that issue is critical and this issue is a blocker for that issue, this issue should also be critical.
I see a lot of changes in the MR from @mondrake, which adds code that relies on deprecated code. When we do that we cannot remove the deprecated code. So, can we please not do that. :)
If I understand the problem correctly is that it only a problem when the database connection desctucted, before the Transaction object is destructed. The logical solution for me would be to have Connection->__destruct() check through the transaction manager if there is still an active transaction(s) and if so then to commit that transaction(s). If it is more complicated or if my solution does not work, can you explain why I am wrong.
@catch I know that we have used distribution as a basis essentially forever. Just like you have said. The big change for me is that we are now do a new major release with there being a previous major release that is almost the same as the new major version and the previous major release has 2 more years of full support. For me that is a big change. Lets agree that we have a different opinion. :)
However you are the release manager and let go with your opinion. We have little choice than to go with MySQL 8.0 as a minimum version. For MariaDB we have a couple of options:
- We have MariaDB 10.5 which is released with RHEL and has an EOL in 2025;
- We have MariaDB 10.6 which is not in RHEL and has an EOL in 2026;
- We have MariaDB 10.11 which is not in RHEL and has an EOL in 2028.
If we look at the distributions we should go with MariaDB 10.5. That version has an EOL in 2025, which is only 1 year after the release of D11. D11 will be supported by us until 2028.
If we do not take RHEL in account we can go with MariaDB 10.11. That version has an EOL in 2028, which is the same year as we offer support for D11.
@catch: I will let you make the choice for which the minimum version for MariaDB should be for D11. My preference would be MariaDB 10.11. If you however will select MariaDB 10.5, that is fine by me too.
Maybe I am missing the point in why distributions are so important. In 🌱 [11.x] [policy] Require PHP 8.3 for Drupal 11 RTBC we set the minimum version for PHP to 8.3 in spite of no distribution supportion PHP 8.3. Yet, for MySQL and MariaDB it is somehow the deciding factor. We are now in a situation were every new major version of Drupal has an older major Drupal version that has full support. When D11 is released you do not have to migrate anytime soon. For at least 2 years after the release of D11 you can still use D10 and have full support. This gives us the possibility to set the minimum requirements for MySQL and MariaDB as high as possible. The newer versions of MySQL and MariaDB have an improved performance and some new features. When people do a performance comparison between D10 with MySQL 5.7 and D11 with MySQL 8.2 the performance improvement will be much better than with D11 on the oldest version of MySQL 8.0.
I agree that core being able to use 3.45's jsonb_*() functions would be nice. Given that I'll create a contrib sqlite driver for old sqlite versions, there might be a pathway to us choosing to be this aggressive with core's requirement.
@effulgentsia: That would be great!
Finally, if any of the work in #3343634: [PP-1] Add "json" as core data type to Schema and Database API and related issues actually wants to use sqlite 3.45 syntax (jsonb functions) or even 3.38 syntax (-> and ->>) operators, are we then saying that that work will only be committed to 11.0 and not to 10.3?
I would rather go for a great solution that is only D11+ instead of a lesser option that can be committed to 10.2 or 10.3.
I don't think so, and I think there's some flexibility built in - we've never actually done it a proper six months before - it's mainly intended to give hosting platforms time to prepare, and that's not really an issue for sqlite so I think it's reasonable to exclude it.
I agree with @catch. Back to RTBC.
@bradjones1: The solution looks good to me.
We are missing some CRUD operations with the GIN/GIST indexes. We should also add functionality and testing to:
- to check that the GIN/GIST index exists;
- to add GIN/GIST index is created on an existing field;
- to remove a GIN/GIST index;
- when we change an existing field we can also add/remove a GIN/GIST index.
Add some testing that when we create a new table with a GIN/GIST index that the index exists and is a GIN/GIST index. Please create a query with the explain to check that we have created a GIN/GIST index and that the query is using it.
All the code changes look good to me.
For me it is RTBC.
All the code changes look good to me.
For me it is RTBC.
+1 for SQLite 3.42
@catch Would it be possible to go for SQLite 3.42 for now. And when SQLite 3.45 comes out, check if it is possible to upgrade the mimimum required version to SQLite 3.45. If for some reason it is not possible, than no. If it is, than change it to SQLite 3.45. It that something we could do?
@poker10 Thanks you for the info. You are right a LTS would be preferable. MySQL 8.0 with a EOL of April 2026, would mean that it would be EOL before the release of D12. Both are not great options.
+1 for PHP 8.3
If it would be fully up to me we would go for MySQL 8.2 and MariaDB 10.11. Both are the newest versions for both databases. MariaDB 10.6 is by the time we release D11 already 3 years old and MySQL 8.0 is by than 6 years old. If somebody does not have hosting that is compatible with MariaDB 10.11 or MySQL 8.2, they should keep using D10. D10 will be supported until at least the summer of 2026.
MySQL 8.2 has been released on 25th October 2023.
Brad Jones is working on JSON storage project and the developers from SQLite have added support for JSONB in SQLite. There is no release from SQLite with support for JSONB. If have asked him on Slack what his opinion on this.
If it is not important to him than we should go with SQLite 3.42. I did not see anything important in SQLite 3.43 or 3.44 that is interesting for Drupal.
+1 for PostgreSQL 16
Back to RTBC.
All the changes look good to me.
For me it is RTBC.
+1 for RTBC
All remarks of @xjm have been addressed.
Back to RTBC.
The MR needs to be updated for that this will go in 10.3 instead of 10.2
Minor remark on the MR.
Could we run the pipeline for PostgreSQL for this MR?
Back to needs work.
Going forward, we should set the minimum PHP requirement to the highest stable PHP version available at the time beta1 of the major is released. That means PHP 8.3 for D11.
+1 This sounds to me as the best (most sensible) solution.
Hopefully D11 will not have a December release. But if that happens, it should have a PHP 8.4 minimum version. AFAIK PHP 8.4 will be released in October/November 2024. Lets for now go for a earlier release than December. :)
Again if for some reason you have a hosting environment with older software you should stay on the previous major Drupal version (D10). D10 is still supported for another 2 years after the release of D11.
A new major version of Drupal should be released with all the latest stable versions for its dependencies. The same for the minimum required versions for its databases. For me as a developer on Drupal core/contrib I want to work with the newest software. I do not want to wait to 2 extra years until Drupal ups its minimum supported versions. With D10 we could have set the minimum required version for PostgreSQL to 15. PostgreSQL 15 was released on October 13, 2022. Drupal 10 was released on December 14, 2022. We did not do that. Only in PostgreSQL 15 was support added for native MERGE. Now I have to wait for D11, before I can use it in Drupal. That just does not make me happy. See: 📌 Improve support of native MERGE on postgresql 15 Active .
I would like to set the minimum required PHP version for D11 to the newest PHP version available at the moment that D11 is released. We need to support D11 for at least 4 years. If for some reason you do not have an environment with a older/lower PHP version, than stay on D10. D10 will also be supported for at least 4 years. Let the developers that work on Drupal core/contrib work with all the newest that PHP has to offer. As such a developer I like to work with the newest stuff.
@mondrake: I like your idea and I have 2 variants I would like to propose.
We create a helper method that would return the Condition object:
public function joinCondition(string $conjunction = 'AND') {
return $this->connection->condition($conjunction);
}
The query before
$query->leftJoin($this->dataTable, 'data', $this->database->condition('AND')->compare("revision.$this->idKey", "data.$this->idKey")->compare("revision.$this->langcodeKey", "data.$this->langcodeKey"));
The query after
$query->leftJoin($this->dataTable, 'data',
$query->joinCondition()
->compare("revision.$this->idKey", "data.$this->idKey")
->compare("revision.$this->langcodeKey", "data.$this->langcodeKey")
);
We allow the join condition is an array of arrays with each inner array having two values that must be equal. When it is not a simple join than the Condition object must be used.
The query before
$query->leftJoin($this->dataTable, 'data', $this->database->condition('AND')->compare("revision.$this->idKey", "data.$this->idKey")->compare("revision.$this->langcodeKey", "data.$this->langcodeKey"));
The query after
$query->leftJoin($this->dataTable, 'data', [
["revision.$this->idKey", "data.$this->idKey"],
["revision.$this->langcodeKey", "data.$this->langcodeKey"]
]);
For 11.2 I personally like compare() more, because it is closer to condition(). But I can live with compareFields().
For 11.3 If you think that is better, then lets do that.
@mondrake: Thank you for the reviewing and helping to get this issue to land.
If we are deprecating passing a string as $conditions to Select::addJoin(), I think also ::join(), ::leftJoin() and ::innerJoin() should do the same.
The methods ::join(), ::leftJoin() and ::innerJoin() are wrapper methods on top of Select::addJoin(). If you deprecate those on their own, you will get 2 deprecations. One from the wrapper method and one from Select::addJoin().
Hide the patch files.
+1 for me.
I really dislike unset($transaction); for the implicit commit.
The patch is for D10.3
Maybe the module should not be hidden and removed from the database selection during the install process. We want to allow site owners to experiment with the mysqli database driver and therefor the module should be visible on the module list.
Changed DBAL to DBTNG.
@fgm: Thanks!
In 🐛 Performance issues with path alias generated queries on PostgreSQL Needs work there was some discussion about how to add GIN/GIST index support in core. I have created a separate issue for it ( 📌 Adding GIN and GIST indexes to PostgreSQL databases RTBC ). I am postponing this issue on getting the other is done.
For the bikeshedding part see 🐛 Performance issues with path alias generated queries on PostgreSQL Needs work
quietone → credited daffie → .