Make dblog entities

Still very interested in this.

Wonder if you could provide a good way for one to test this?

Added review comments

The failing test with postgres is not generated by this patch, it already fails with the 10.1.x-dex branch https://www.drupal.org/node/3060/qa →

NW for #75

1. Are chosen names for the DblogStorageInterface methods are good enough?

No opinion.

2. Are we ok with forbidding individual deletion of entries?

Any reason for this? -1.

3. Are we ok with preventing saving an DblogEntry using save() and suggests instead use the Dblog logger?

Make sense.

Per the test result of DbLogEntryTest::testSerialization() #75 may not be an issue anymore. Probably, it's not an issue in newer versions of PostgreSQL.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Left some comments on the MR. More than profiling I am wondering if the indexes are correct (i.e. an EXPLAIN on some common queries would be useful) and whether the schema matches between existing sites and a new install.

I'm back to work on this.

I updated the issue summary to answer @catch concerns regarding indexes. I ran a `DESCRIBE watchdog;` command before and after installing the site using this patch, and the table structure remains the same.

I guess this also is valid for the issue 📌 Make dblog entities Postponed , if we fix this in the dblog.install module, the changes will also apply to dblog as entities.

I also addressed the comments about dblog exception in ContentUninstallValidator. Based on my current understanding of the module_installer logic, there is no way for dblog to by pass this content validator exception, so I had to modify it any way. I tried to make it as simple and clear possible, and not only for dblog.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I updated the issue summary based on the latest reviews assuming the proposed method names are ok. Removing needs profiling as I did the profiling tests in #61.

dagmar → closed merge request !1159

I went over all the pending open threads of the merge request and closed most of them. I left a few open to make sure the provided answer is enough for the original reviewers.

I also checked some concerns I had related caching. It seems entities are not cached but routes are (example route:[language]=en:/admin/reports/dblog/event/10) in cache_data table. This is also happening for the current non entity implementation so I will not attempt to fix this as part of this issue. Maybe is not even an issue.

Most of the concerns regarding performance, indexing and future compatibility with 🐛 Database primary keys can exceed maximum integer storeable (has actually occurred, in watchdog) Fixed are addressed as the underlying system stills uses the same database scheme and insert queries.

My general feeling about the code (as the current dblog maintainer) is: this is pretty much ready to be in. There is some room of improvement in the DblogEntryViewBuilder, but probably qualifies for a follow up. And comparing this with the current code of the dblog module I would say this is a big improvement in code quality and alignment of Drupal standards. And will also allow to close several postponed issues for more than a few years now.

The remaining steps in my opinion are:

1. Reviewing the latest changes to ContentUninstallValidator::validate

as this is calling a new API method: ContentEntityTypeInterface;:requireDeleteContentBeforeUninstall which needs some grammar and common sense checking.

2. Review if all annotations defined in DblogEntry are correct, or if something is missing or redundant.
3. Get some last code review round paying specially attention to the high level picture of this. I already did this a few times, but I wrote most of the code, and doesn't feel right to assume it is right.

I had more time to think about item 1. I think we should not make an exception for dblog entries. After researching a bit why the content validator was created it seems a bit dangerous to bypass the checks.

Concerns about disabling the dblog module in a production environment are still valid though. I created this to take care of this functionality: ✨ Provide a way to disable creation of database log entries Needs work

Hmm, idk why it doesn't show my picture or link to my profile. That's weird.

I found a way to not have to pause the creation of logs and remove the dblog module transparently as usual.

The approach is not elegant, but it works. When dblog module is asked if has data from the ContentUninstallValidator it replies no... And the module can be uninstalled. Logs are deleted anyways by the dblog_module_preuninstall hook. There is no other way to implement that I'm aware of as the ModuleInstaller::uninstall method does not offer any other opportunity to modify the validations.

This means this issue is not longer postponed on ✨ Provide a way to disable creation of database log entries Needs work

dagmar → changed the visibility of the branch 2401463-dblog-entities-D10 to hidden.

dagmar → changed the visibility of the branch 2401463-dblog-entities to hidden.

Added ✨ Allow kernel tests to fail or expect logged errors Needs work to the issue summary to, to proper fix the problem with tests. Thanks @joachim

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Added some comments to the MR. Majority small stuff.