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.
The wrong return value and type hint for the method DbLogController::addFilterToQuery() has been adressed.
@dagmar & @rajneeshb: Thanks!
@tobiasb: Thank you for your concern. This issue is a preparation issue for getting Drupal to work with MongoDB. Drupal on MongoDB is needed for creating Drupal sites with a huge number of authenticated users. Adding a type hint with every call to this method will result that you as a programmer will sometimes forget to add the type hint. MongoDB is very strict in variables being of the correct type. With a relational database like MySQL, MariaDB and PostgreSQL they are very forgiving. Fro them there is no difference between the string '4' and the integer 4. For MongoDB they are totally 2 different things. The added parameter is temporary and will be removed in Drupal 12.0. We do not want to break peoples existing sites, therefore we are adding the deprecation in that way.
The requested deprecation has been added.
The requested deprecation has been added.
But also why would the test database need a replica set?
The test database needs for MongoDB be a replica set, because Drupal needs transaction support from its database and with MongoDB support for transactions is only available in a replica set. You can run MongoDB as standalone, only than you have no transactions support. For more info click here
Rebased as requested.
The MR is changed to 11.x as requested.
Added 📌 Allow config items to have database driver overrides Active as an alternative solution for ✨ Entity Query views backend Active .
The framework manager need to review the suggested solution.
Added example of dblog module with override directory,
Added 📌 Change the boolean constants to have boolean values in NodeInterface, CommentInterface and FileInterface Active as a not hard requirement.
Added 📌 Change the filter in the overview page of the dblog module to a condition object Active to the list of child issues.
daffie → created an issue.
@smustgrave: I have thought about any possible BC problems and I do not see them. I could be wrong. If you see any, than please explain.
Added 📌 The method ContentEntityBase::getRevisionId() should not return string values Needs review as a very nice to have child issue
Added 🐛 The method ContentEntityBase::getLoadedRevisionId() should not return string values Needs review as a child issue.
daffie → created an issue.
@catch: We have changed a lot of hardcoded SQL queries to dynamic queries in the past. If you want, I will go and search for those issues. These are the hardcoded queries that are left. The remaining ones are mostly in Drupal's critical path.
I am at the moment still in the phase of getting Drupal on MongoDB to support all the features. Drupal on MongoDB now is at a 99% pass rate for the CI
pipeline
📌
Drupal on MongoDB (the full PR)
Active
. Sooner or later I will get the phase where MongoDB specific improvements will be added. I am still not at the point where I have an official go for Drupal on MongoDB from the Drupal Core committer team. :)
Addressed the remark of @catch. Back to needs review.
Added 📌 Set entity schema installation, module configuration installation and content creation in the right order in kerneltests for MongoDB Needs review as a child issue
daffie → created an issue.
Added the CR.
Added 📌 Replace hardcoded database queries with dynamic queries Needs review .
This problem is fixed for MongoDB.
Removed 🐛 Install the user module in the site install process Active as it is no longer necessary.
Removed 📌 Add a number addExpression specific functions Active it once from the list. It was there twice.
📌 Add a number addExpression specific functions Active is added to the list.
@johnwebdev: Thank you for your question. When I worked on the database driver in the past, I had a bug when running the install process. I could fix the problem by requiring the user module be installed early in the install process. I have removed the early installing the user module and the CI pipeline came back green. I have run the install process on my local machine and that gave no problems. Therefore I have removed the early installing of the user module. Again that you for asking the question.
Rerolled for 11.x. Back to RTBC.
LocaleTranslationUiTest fails on 11.x with PostgreSQL!
Setting the priority to critical.
Added 🐛 CommentNonNodeTest::testCommentFunctionality() is failing for MongoDB Needs review as a child issue
Added 📌 Make the nightwatch tests work with MongoDB Needs review as a child issue.
The patch is not relevant any more.
It is not clear to me which row format options Drupal is going to support. Are we are going to support any other one than the default one. Could we update the CR with this information. Including which is the default row format option? With these kind of changes the CR becomes very important to me. There should be information on which row format options there are and what they do. If you are a site owner, it should be clear what the options are, what they do and when to use them. Now the CR is missing that information.
It would be great if we could add an installer test to see if we can install Drupal with another row format option other then the default one.
The PR should pass the testbot for MySQL and MariaDB.
As we are no longer going to do Brad Jones' "Drupal core's out-of-the-box JSON data storage", I have no problems with lowering the minimal supported version of SQLite.
Updated the IS and the patch.
🐛 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.