Account created on 15 February 2007, about 17 years ago
#

Merge Requests

Recent comments

🇳🇱Netherlands daffie

+1 for RTBC.

This really cleans up the Database layer and the drivers a LOT!!!

🇳🇱Netherlands daffie

@smustgrave: Thank for the review.

I have added the return type hint to the new method Condition::compare().

🇳🇱Netherlands daffie

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.

🇳🇱Netherlands daffie

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.

🇳🇱Netherlands daffie

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 ).

🇳🇱Netherlands daffie

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.

🇳🇱Netherlands daffie

@Wim Leers: Ok, than it is fine. Just want to make sure that Drupal does not get slower. :)

🇳🇱Netherlands daffie

I think that we should make sure that we have an database index for every query we add orderby().

🇳🇱Netherlands daffie

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!

🇳🇱Netherlands daffie

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.

🇳🇱Netherlands daffie

The requested TODO has been added to the correct issue.
Back to RTBC.

🇳🇱Netherlands daffie

@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.

🇳🇱Netherlands daffie

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.

🇳🇱Netherlands daffie

@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.

🇳🇱Netherlands daffie

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.

🇳🇱Netherlands daffie

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.

🇳🇱Netherlands daffie

@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.

🇳🇱Netherlands daffie

All the code changes look good to me.
For me it is RTBC.

🇳🇱Netherlands daffie

All the code changes look good to me.
For me it is RTBC.

🇳🇱Netherlands daffie

@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?

🇳🇱Netherlands daffie

@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.

🇳🇱Netherlands daffie

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.

🇳🇱Netherlands daffie

MySQL 8.2 has been released on 25th October 2023.

🇳🇱Netherlands daffie

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.

🇳🇱Netherlands daffie

All the changes look good to me.
For me it is RTBC.

🇳🇱Netherlands daffie

+1 for RTBC

All remarks of @xjm have been addressed.
Back to RTBC.

🇳🇱Netherlands daffie

The MR needs to be updated for that this will go in 10.3 instead of 10.2

🇳🇱Netherlands daffie

Minor remark on the MR.

Could we run the pipeline for PostgreSQL for this MR?

🇳🇱Netherlands daffie

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.

🇳🇱Netherlands daffie

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 .

🇳🇱Netherlands daffie

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.

🇳🇱Netherlands daffie

@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.

🇳🇱Netherlands daffie

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().

🇳🇱Netherlands daffie

+1 for me.

I really dislike unset($transaction); for the implicit commit.

🇳🇱Netherlands daffie

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.

🇳🇱Netherlands daffie

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 Needs review ). I am postponing this issue on getting the other is done.

🇳🇱Netherlands daffie

I have talked to @laurii in person and as long as the module is hidden for new site install as long as the module is experimental, am I allowed to remove the "Needs product manager review". Updated comment #85.

🇳🇱Netherlands daffie

I have talked to @longwave, @alexpott and @catch about the new MySQLi database driver. The plan that we came up with is the following:

  1. The MySQLi module goes into core as experimental. The is important because we need the freedom to change anything in this module that we would like to change without triggering a BC break. Also when users install a new Drupal site they need to see this module as experimental instead of it being a regular module.
  2. The next phase for the module is to add all the parallel query functionality to make the PHP Fibers part work for the database queries. Maybe add other improvements? After this is done we are going to ask expert Drupal users to test out the database driver.
  3. When a number of expert Drupal users have been using this database driver without any problems and all other bugs have been fixed that are the result of using the MySQLi database driver and the PHP mysqli extension, the module can be marked as stable.
  4. When the module is marked as stable, we can start informing site owners that we have a new database driver and what is improved compared to the existing MySQL driver. The only thing that existing site owners need to do start using the MySQLi driver is to update their database driver settings in settings.php. Change 'driver' => 'mysql to 'driver' => 'mysqli'. Just adding the i to mysql. For new site installs the MySQLi database driver becomes the default option.
  5. When the MySQLi database driver is ready for becoming the default driver for MySQL, the are going to remove all code from the MySQL module that is overridden in the MySQLi module and all code from the MySQLi module is copied to the MySQL module. All override code in MySQLi module will be removed. The MySQL and the MySQLi module are now the same. The MySQL module will stay the default module for a MySQL database. The MySQLi module will now be marked as deprecated. Now all site owners who are using a MySQL or equivalent are now using the code from the original MySQLi module. Existing site owners who did not change to the MySQLi driver do not have to do anything and they are now using the code from the original MySQLi driver. Site owners who changed their site to switch to the MySQLi module shall need to change back to the default MySQL driver. We are now finished!

To do in this patch:
- Make it is experimental. Add testing for this.
- Add in the autoloading of database driver module dependencies an override to when the driver is "mysqli" to also autoload the "mysql" module. This has the advantage that existing site owners can switch to the MySQLi driver without adding the autoload dependency to the database driver settings to the settings.php file. Do the same for the "autoload" and "namespace" settings. Lets make it easy for site owners to try out the MySQLi driver and revert back to MySQL driver. Add testing for this.

🇳🇱Netherlands daffie

Status update for the full database driver for MongoDB:

1. The is a proof-of-concept site running Drupal on MongoDB at: https://gitlab.com/daffie/mongodb867. It is a Drupal 8.6.7 site with PHP 7.2 and an older MongoDB version. It works in that you can do a Drupal site install and you can do some basic stuff.
2. I have worked for the last 3 years on getting contrib database driver support in Drupal core much better. It is now at a level that it is good enough for a contrib database driver for MongoDB.

My plan for the coming 6-12 months is to update the MongoDB database driver to a point that is good enough to run a simple Drupal on MongoDB site. That will be a Drupal site that will be ONLY using MongoDB as its database. No relational database is necessary. All entity data will be stored in MongoDB's document storage. All entity data for a single entity instance, including translations, revisons and field data will be stored in a single document/JSON object. In MongoDB JSON objects are called documents.

After that I am looking for a project that is much larger to show of that with Drupal on MongoDB you can do much larger projects that are not possible on a relational database like MySQL, MariaDB or PostgreSQL.

Production build https://api.contrib.social 0.61.6-2-g546bc20