🐛 Ensure post transaction callbacks are always executed ONLY at the end of the root Drupal transaction Needs review has landed.
My workflow:
cd drupalci_environments/db/mongodb-7
docker build --no-cache .
# get the docker image
docker image ls
docker run -it df1bfbf41a98 bash
# on the cli in the container run
./docker-entrypoint-initdb.d/startup.sh
The last command will:
- create the mongoDB instances;
- create the replica set;
- create the database user for Drupal.
Hopefully this will make it easier for one of the maintainers to fix the problem.
I have tried to create a MR, but I was not allowed to do so.
daffie → created an issue.
The CR is written by developers for developers. Only in this instance it is also very important that operational persons understand it. The reference "see Drupal\mysql\Driver\Database\mysql\SqlMode." is something that is not very helpful for them. Can we copy that documentation to the CR.
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.