@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.
I have one nitpick on the MR.
I think the CR needs to be updated for the part: "it will need to change in 12.0.0". We are not doing it in 12.0 but in 11.X.
The CI pipeline isn't happy. See: https://git.drupalcode.org/issue/drupal-3489427/-/jobs/3599665
Make Drupal great for sites with a great number of authenticated users.
That is what Drupal Core is missing. In the market there is a shift towards those kind of sites. Site owners want a more personalized experience for their users and they are switching from anonymous users to authenticated users. It would great if we could use Drupal for kind of sites.
To let Drupal be a great solution for complicated sites with a great number of authenticated users we need to do a couple of things:
1. Start using asynchronous PHP in Drupal Core. At least for retrieving data out of the database.
🌱
[meta] Use Fibers for concurrency in core
Active
&
📌
Add revoltphp/event-loop dependency to core
Active
2. Switch to using MongoDB as the database and store all entity instances in their own JSON object in the database. This will lead to a lot less queries on the database, therefore making Drupal faster. The amount of functionality that Drupal requires from the database for supporting entity instances in JSON object is significant. The only database that can do that is MongoDB.
✨
[meta] Add database driver for MongoDB to Core as experimental
Active
&
📌
Drupal on MongoDB (the full PR)
Active
3. Load the Drupal kernel (and other stuff) in memory before the user request comes in.
#2218651: [meta] Make Drupal compatible with persistent app servers like ReactPHP, PHP-PM, PHPFastCGI, FrankenPHP, Swoole →
The combination of those 3 solutions combined with a more carefully use of how data is stored in the database and how it is retrieved will make Drupal great for sites with a huge number authenticated users.
The MongoDB community edition has more benefits:
- Support for regular search. We would no longer need a SOLR server next to your Drupal site. It is just stored in MongoDB. The 2 storage's do not need to be kept in sync.
- Support for vector search. Vectors are the way to store the results from an Artificial Intelligence (AI) in a database. With MongoDB those vectors can be indexed and searched. No Vector database needed and no need to keep data in sync between the two.
- Supports horizontal scaling. With this Drupal can support massive numbers of concurrent authenticated users.
The company MongoDB has a 1000 developers working on making the database a better product. The MongoDB has improved enormously over the last 10 years. When you last looked at MongoDB was more then sat 5 years or more. You need to take another look.
The databases we are using now (MySQL & MariaDB) have almost no improvements over the last 10 years. They are what we call lecacy products. MySQL is owned by the Oracle company and they want every user to switch over to their Oracle database. They are doing the minimum amount of support and development on MySQL.
To me, as the subsystem maintainer of the Database API and the database driver modules, MySQL and MariaDB are for the long term not the right database for Drupal.
For more info about Drupal on MongoDB: https://techblog.finalist.nl/blog/drupal-mongodb
I have updated the feed and there are now 2 Drupal related articles.
All code changes look good to me.
My remark has been addressed.
The IS and the CR are in order.
For me it is RTBC.
I have just a single remark on the MR.
I have updated the IS and the CR.
All changes look good to me.
All my remarks have been addressed.
For me it is RTBC.
All changes look good to me.
All my remarks are addressed.
For me it is RTBC.
My remarks have been addressed.
All code changes look good to me.
For me it is RTBC.
A couple of remarks on the MR.
All the documentation changes look good to me.
The remark of @alexpott has been addressed.
For me it is RTBC.
A couple remarks (nitpicks) on the MR.
A couple of remarks on the MR.
The CIn pipeline is failing for PHPCS.
Change look good to me.
Back to RTBC.
All code changes look good to me.
I have run ddev exec -d /var/www/html "./vendor/bin/phpcbf core --standard=core/phpcs.xml.dist" on my local machine. With and without the MR and with the rule "Drupal.Commenting.DocComment.SpacingBeforeTags" included.
The results are as expected.
For me it is RTBC.
All code changes look good to me.
All points have been addressed.
I leave it to the committer to decide if we can do this in a minor version or we shall have to wait for a new major version.
For me it is RTBC.
@daffie have you checked your local phpunit.xml vs the one in the MR?
Oeps, I did not.
Now that I did do that the test passes on my local machine.
All code changes look good to me.
For me it is RTBC.
@modrake: Thank you for your help.
When I run the Drupal\KernelTests\Core\Test\PhpUnitTestDiscoveryTest on my local machine it fails. THe code dump(array_diff(array_values($internalList), array_values($phpUnitList))); returns:
array:35 [
1439 => "Drupal\Tests\config_test\Functional\Rest\ConfigTestJsonAnonTest"
1440 => "Drupal\Tests\config_test\Functional\Rest\ConfigTestJsonBasicAuthTest"
1441 => "Drupal\Tests\config_test\Functional\Rest\ConfigTestJsonCookieTest"
1442 => "Drupal\Tests\config_test\Functional\Rest\ConfigTestXmlAnonTest"
1443 => "Drupal\Tests\config_test\Functional\Rest\ConfigTestXmlBasicAuthTest"
1444 => "Drupal\Tests\config_test\Functional\Rest\ConfigTestXmlCookieTest"
1635 => "Drupal\Tests\demo_umami_content\Functional\DefaultContentFilesAccessTest"
1636 => "Drupal\Tests\demo_umami_content\Functional\UninstallDefaultContentTest"
1637 => "Drupal\Tests\drupal_system_listing_compatible_test\Kernel\SystemListingCrossProfileCompatibleTest"
1660 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestBundleJsonAnonTest"
1661 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestBundleJsonBasicAuthTest"
1662 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestBundleJsonCookieTest"
1663 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestBundleXmlAnonTest"
1664 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestBundleXmlBasicAuthTest"
1665 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestBundleXmlCookieTest"
1666 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestComputedFieldNormalizerTest"
1667 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestJsonAnonTest"
1668 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestJsonBasicAuthTest"
1669 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestJsonCookieTest"
1670 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestJsonInternalPropertyNormalizerTest"
1671 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestLabelJsonAnonTest"
1672 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestLabelJsonBasicAuthTest"
1673 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestLabelJsonCookieTest"
1674 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestLabelXmlAnonTest"
1675 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestLabelXmlBasicAuthTest"
1676 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestLabelXmlCookieTest"
1677 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestMapFieldJsonAnonTest"
1678 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestTextItemNormalizerTest"
1679 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestXmlAnonTest"
1680 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestXmlBasicAuthTest"
1681 => "Drupal\Tests\entity_test\Functional\Rest\EntityTestXmlCookieTest"
2282 => "Drupal\Tests\layout_builder_expose_all_field_blocks\Functional\GenericTest"
2654 => "Drupal\Tests\navigation_top_bar\Functional\GenericTest"
2816 => "Drupal\Tests\package_manager\Build\PackageInstallTest"
2817 => "Drupal\Tests\package_manager\Build\PackageUpdateTest"
]
The code changes in the MR look good to me.
All the code changes look good to me.
I have run the tests for the new version of dump() on my local machine and it work.
The CR is in order and I should have read it earlier.
My suggestion is to put this requirement at the top of the release notes!!!
When you do not update your local phpunit.xml file, the function just does not do anything. It will take some time to find out that you need to update your local phpunit.xml file. I have updated the IS for this.
For me it is RTBC.
alexpott → credited daffie → .
@mondrake: I am testing the MR on my local machine and there the tests are failing. I am using a DDEV environment.
ddev exec -d /var/www/html "./vendor/bin/phpunit -c ./core/phpunit.xml ./core/tests/Drupal/Tests/UnitTestCaseTest.php" results in:
PHPUnit 10.5.38 by Sebastian Bergmann and contributors.
Runtime: PHP 8.3.10
Configuration: /var/www/html/core/phpunit.xml
^ {#571
+"Aldebaran": "Betelgeuse"
}
^ "Alpheratz"
FF 2 / 2 (100%)
Time: 00:00.147, Memory: 4.00 MB
There were 2 failures:
1) Drupal\Tests\UnitTestCaseTest::testVarDumpSameProcess
Failed asserting that '[]' [ASCII](length: 2) contains "Aldebaran" [ASCII](length: 9).
/var/www/html/core/tests/Drupal/Tests/UnitTestCaseTest.php:29
2) Drupal\Tests\UnitTestCaseTest::testVarDumpSeparateProcess
Failed asserting that '[]' [ASCII](length: 2) contains "Denebola" [ASCII](length: 8).
/var/www/html/core/tests/Drupal/Tests/UnitTestCaseTest.php:49
ddev exec -d /var/www/html "./vendor/bin/phpunit -c ./core/phpunit.xml --filter testVarDump ./core/tests/Drupal/KernelTests/KernelTestBaseTest.php" results in:
PHPUnit 10.5.38 by Sebastian Bergmann and contributors.
Runtime: PHP 8.3.10
Configuration: /var/www/html/core/phpunit.xml
F 1 / 1 (100%)
Time: 00:00.479, Memory: 4.00 MB
There was 1 failure:
1) Drupal\KernelTests\KernelTestBaseTest::testVarDump
Failed asserting that '[]' [ASCII](length: 2) contains "KernelTestBaseTest::testVarDump" [ASCII](length: 31).
/var/www/html/core/tests/Drupal/KernelTests/KernelTestBaseTest.php:316
ddev exec -d /var/www/html "./vendor/bin/phpunit -c ./core/phpunit.xml --filter testVarDump ./core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php" results in:
PHPUnit 10.5.38 by Sebastian Bergmann and contributors.
Runtime: PHP 8.3.10
Configuration: /var/www/html/core/phpunit.xml
F 1 / 1 (100%)
Time: 00:09.816, Memory: 4.00 MB
There was 1 failure:
1) Drupal\FunctionalTests\BrowserTestBaseTest::testVarDump
Failed asserting that '[]' [ASCII](length: 2) contains "BrowserTestBaseTest::testVarDump" [ASCII](length: 32).
/var/www/html/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php:612
Maybe I am doing something wrong. Can you help me?
All code changes look good to me.
Hoping that the change will not result in a BC break on site in the wild.
I leave it to the committer to decide if we can do this in a minor version or we shall have to wait for a new major version.
For me it is RTBC.
All code changes look good to me.
For me it is RTBC.
I have tested both links and they are pointing to the right place.
For me it is RTBC.
Maybe it is possible to add a listener in kernel tests to ensure that the entity schema is installed before creating a field on it?
I have added a listener to the kerneltestbase class and found a new instance were an entity schema was not created: Drupal\Tests\layout_builder\Kernel\ConfigActionsTest. The test was added a week ago in ✨ Allow recipes to enable layout builder Needs work .
@alexpott: Thank you for the review and your suggestion how to make it better.
@catch: I some cases it can be a constant in other cases it cannot. Like with Drupal\Core\Menu\MenuTreeStorage, Drupal\Core\Routing\MatcherDumper and Drupal\Core\KeyValueStore\DatabaseStorage.
@andypost: Thank you for the review.
Changing the class variable to a readonly variable and setting its value is not allowed in PHP. From the PHP documentation:
Specifying an explicit default value on readonly properties is not allowed, because a readonly property with a default value is essentially the same as a constant, and thus not particularly useful.
See: https://www.php.net/manual/en/language.oop5.properties.php
I have changed the variables to be of the string type.
The new message looks good to me.
It should be better for the site owner and it allow us to add checks on JSON functionality from the database at a later point in time.
For me it is RTBC.
Change look good to me.
Back to RTBC.
The Mr is ready for a review.
daffie → changed the visibility of the branch 2371709-move-the-on-demand-table to hidden.
I have updated the IS and created a CR.
Lets skip the test. We can do that in 📌 Replace ban_schema() implementation with ::ensureTableExists() Needs work .
smustgrave → credited daffie → .
I think we should add JSON storage support in ✨ Add "json" as core data type to Schema and Database API Needs work . If that takes too long, we should at least at testing for the change and a comment about why the change is done.
In 📌 [PP-1] Create the database driver for MySQLi Postponed we shall get testing for this problem. The new MySQLi database driver has the old MySQL database driver as a dependency.
Ready for a review.
All looks good to me.
Back to RTBC.
All the code change good to me.
I have updated the CR and the IS.
The CI pipeline is green for all by core supported databases.
All my questions have been answered.
For me it is RTBC.
The CI pipeline is not happy.
The point of @alexpott has been addressed.
Back to RTBC.
Looks good to me.
The CI pipeline is green for all supported databases.
For me it is RTBC.
I did as the subsystem maintainer a review and therefore I have removed the tag.
Changing the status to needs work for the nitpick on the MR.
For the change record and the questions on the MR.
Rebased and back to RTBC.
Back to needs work for my remarks on the PR.
Rebased the PR. Back to RTBC.
The problem is that it's not currently possible within the Entity API to type cast anything.
I think we can get the type casting from the field storage type. We are doing that already in the function _comment_entity_uses_integer_id() and in the method Drupal\Core\Entity\ContentEntityStorageBase::cleanIds(). I have updated the PR to use the field storage type for type casting the return value.
why should we treat getLoadedRevisionId() differently than getRevisionId()?
I do not want to treat them diferently. I also have 📌 The method ContentEntityBase::getRevisionId() should not return string values Needs review for the other method. The solution from this issue should also be used in the other issue. I have learned that smaller issues are easier to lands then bigger issues. That is why I split them up.
the new argument defaults to FALSE but we update every usage of that method to pass TRUE. Wouldn't it be easier to not introduce the argument in the first place and only trigger the deprecation when !ctype_digit($this->loadedRevisionId)?
Yes, that would work for triggering the deprecation. Only that does not allow us to change the type of the return value in core without doing the same in contrib/custom code. Doing it in contrib/custom code might cause a BR break and that is not allowed.
That's not actually true, the @return doxygen for \Drupal\Core\Entity\RevisionableInterface::getRevisionId() is int|null|string.
That change has been made 2 year ago ( #2941148: Fix Drupal.Commenting.FunctionComment.MissingReturnType → ) to fix a PHPCS rule. In Drupal Core are all used revision IDs integers. No string revisions IDs. Maybe a good idea for a followup.
I am not against supporting UUIDs as revision IDs. What I would like for MongoDB is that revision IDs are returned as the correct type. When they are an integer they should be returned as an integer value, not a string value. When they are a string value like with a UUID then they should be returned as a string value. I have updated the title and the IS.
As discused with @larowlan: the loaded revision ID will only return an integer value when all characters of the loaded revision ID are numeric. The CR is updated for this change.
@avpaderno: Is there something that I need to do to get finalist.nl on Drupal Planet? Or do I just have to be patient? If so, any idea when finalist.nl will be on Drupal Planet?
Hi @larowlan,
Using revision ids that are not an integer value is not supported by Drupal Core. The docblock of RevisionableInterface::getLoadedRevisionId() is clearly stating that the method only returns an integer value. What your client project is doing is NOT supported by Drupal core.
The problem for me with the database driver for MongoDB is that MongoDB in conditions is doing === and relational databases do ==. For them the condition revision_id = '4' is not a problem. For MongoDB it will always be FALSE.
Would it be acceptable to only return an integer value when ctype_digit($this->loadedRevisionId) is TRUE? This would work for MongoDB and it would work non-integer revision ids.
Replied to comment on the PR.
Rebased and back to RTBC.
The Pr is ready for a review.
Rebased and back to RTBC.
Rebased and bacj to RTBC.
Rerolled the MR. Back to RTBC.
@quietone: You are right, it is better to use another name for the database then "mongodb". I have changed it to "dummydb". I thought that I had an option to add "mongodb" to the Drupal core code. :)
@andypost: Thank you for your help on this!
@andypost: I am not allowed to create another branch on this issue. Therefore I have added a patch file. User @alcaeus works for MongoDB and he said that you can create a replica set with a single MongoDB instance. The added patch change the replica set to a single MongoDB instance. Could you give @alcaeus issue creates for his advice. I have been using this configuration for a week now and it is working fine.
Removed the return void from the docblock as requested.
BTW: I get over 20 hits when I do a search for @return void in the code base.