Clarify in the dblog_help that the dblog module should not be used for forensic log

Applying patch

Thanks!!

Hi dagmar,

I have explained why the db_log should not be used.

Please review.

Thanks!!

Done changes as per said.

Thanks!!

Adding Patch

Thanks!!

I have done the changes.

Thanks!!

Adding this information seems like a good idea.

A few things to fix in this documentation:

a) You've introduced the term "forensic log" without explaining what that means. I can guess, but I had never heard this term before. And I'm a fairly experienced Drupal user. This documentation is in hook_help(), which is meant for novices, so terms really need to be explained.

b) The HTML needs some attention. You can't drop an H3 into the middle of a DL list. It needs to be a DT. And the LI list items are not good, as they aren't part of a UL list.

c) The heading also needs attention. You're in the middle of the "Uses" section, and our standard is that the headers in the Uses section need to start with -ing verbs.

d) When referring to a module, use a format like "the Foo Bar module", where "Foo Bar" is the name of the module that appears in the .info.yml file. So don't say "the syslog module".

e) There are a lot of grammatical problems, but let's get the text right before addressing those.

I did some re-wording to correct the output. I think it reads better.

I am going to look at this as part of SprintWeekend2017

I checked /admin/help/dblog in 8.4.0-dev before and after applying the patch. Looks good.

Before:

After:

1. +++ b/core/modules/dblog/dblog.module
   @@ -28,6 +28,9 @@ function dblog_help($route_name, RouteMatchInterface $route_match) {
   +      $output .= '<p>' . t('Users with <em>administer site reports</em> permissions can clear all the logs, and the cron can <a href =":logging_settings" title="Go to logging settings page.">regularly clear</a> old entries, this module should not be used to keep logs for forensic purposes.', [':logging_settings' => \Drupal::url('system.logging_settings')]) . '</p>';
   

   It is "cron", not "the cron".

2. +++ b/core/modules/dblog/dblog.module
   @@ -28,6 +28,9 @@ function dblog_help($route_name, RouteMatchInterface $route_match) {
   +      $output .= '<p>' . t('Forensic logs are intended to persist over time to allow researchers to detect problems after they have occured. This type of log cannot be deleted or altered. The Syslog module is recomended for this scenario.') . '</p>';
   

   "This type of log" is confusing here. I would rather it read "Forensic logs cannot be..." and "If forensic logging is needed, the Syslog module is recommended."

Updated text, uploaded patch, and interdiff for the first time :)

needed to re-run test

1. +++ b/core/modules/dblog/dblog.module
   @@ -28,6 +28,9 @@ function dblog_help($route_name, RouteMatchInterface $route_match) {
   +      $output .= '<p>' . t('Users with <em>administer site reports</em> permissions can clear all the logs, and cron can <a href =":logging_settings" title="Go to logging settings page.">regularly clear</a> old entries, this module should not be used to keep logs for forensic purposes.', [':logging_settings' => \Drupal::url('system.logging_settings')]) . '</p>';
   

   This is a run-on sentence.

2. +++ b/core/modules/dblog/dblog.module
   @@ -28,6 +28,9 @@ function dblog_help($route_name, RouteMatchInterface $route_match) {
   +      $output .= '<p>' . t('Forensic logs are intended to persist over time to allow researchers to detect problems after they have occured. Forensic logs cannot be deleted or altered. If forensic logging is needed, the Syslog module is recommended.') . '</p>';
   

   Sorry, I should have reviewed this a little deeper. Only "researchers" can detect problems in forensic logs? What about site admins, developers, or sysadmins?

   Forensic logs cannot be deleted or altered. If forensic logging is needed, the Syslog module is recommended.

   If you use the Syslog module, that does not mean logs at the destination cannot be deleted or altered. The entire second sentence is problematic and maybe misleading. I would cut the sentence, and add to the first sentence something like "It is recommended to use the Syslog module to build a forensic logging system".

The run-on sentence can be fixed by changing "This module should not be used to keep logs for forensic purposes." to its own sentence.

I think "researchers" is a term being used to describe people that need to solve a problem, and those could be any type of person. How about:

Users with administer site reports permissions can clear all the logs, and cron can regularly clear old entries. This module should not be used to keep logs for forensic purposes.

Forensic logs are intended to persist over time to detect and research problems after they have occurred. It is recommended to use the Syslog module to build a forensic logging system.

Note: occurred was spelled incorrectly.

Let me know any changes (or not) and I will re-do the patch.

I think: "Because users with the administer site reports permission can clear all the logs, and because cron can regularly clear old entries, this module should not be used to keep logs for forensic purposes. Instead, it is recommended to use the Syslog module to build a forensic logging system."

Ok. That's better.

Forgot the period at the end of the sentence. Re-did the patch.

Review of latest patch shows the requested changes to the forensic section of the help text.

Great work refining this help text!

There's one more change I would make. Starting sentences with "Because..." and following with many commas increases the burden on the user, because they have to remember more things to read the sentence. How about starting with "Users with..., and cron..." Then in a separate sentence, something like:

For these reasons, the Database Logging module should not be used for forensic purposes. (The Syslog module can be used forensically instead.)

Also, it might be good to either find a reliable resource on what forensic logging is, and link it, or to include a brief clarification in a few words on what that means.

Something like this?

Users with administer site reports permissions can clear all the logs, and cron can regularly clear old entries.
For these reasons, the Database Logging module should not be used for forensic purposes. Instead, it is recommended to use the Syslog module to build a forensic logging system.

Any recommendations for a reliable resource for the text/link?

Added the wikipedia reference with interdiff. Please review! :)

Converted to new array syntax.

"see here" is not good anchor text.

How about read more about "Audit trail" on wikipedia

1. +++ b/core/modules/dblog/dblog.module
   @@ -28,6 +28,8 @@ function dblog_help($route_name, RouteMatchInterface $route_match) {
          $output .= '<dd>' . t('The Database Logging module allows you to view an event log on the <a href=":dblog">Recent log messages</a> page. The log is a chronological list of recorded events containing usage data, performance data, errors, warnings and operational information. Administrators should check the log on a regular basis to ensure their site is working properly.', [':dblog' => \Drupal::url('dblog.overview')]) . '</dd>';
   

   You could say "...containing errors, warnings, operational information, as well as usage and performance data". This is to avoid repeating and too many times in the same sentence.

2. +++ b/core/modules/dblog/dblog.module
   @@ -28,6 +28,8 @@ function dblog_help($route_name, RouteMatchInterface $route_match) {
          $output .= '<dd>' . t('In case of errors or problems with the site, the <a href=":dblog">Recent log messages</a> page can be useful for debugging, since it shows the sequence of events. The log messages include usage information, warnings, and errors.', [':dblog' => \Drupal::url('dblog.overview')]) . '</dd>';
   

   There is no "," before and. It should be "...usage information, warnings and errors"

Changed from #53

Added space before "Read more"

1. The scope of this patch seems to be expanding. We need to be careful about adding one piece of documentation to changing all the documentation, because it means patches never get done.

2. +++ b/core/modules/dblog/dblog.module
   @@ -25,9 +25,11 @@ function dblog_help($route_name, RouteMatchInterface $route_match) {
   +      $output .= '<dd>' . t('The Database Logging module allows you to view an event log on the <a href=":dblog">Recent log messages</a> page. The log is a chronological list of recorded events containing errors, warnings, operational information, as well as usage and performance data. Administrators should check the log on a regular basis to ensure their site is working properly.', [':dblog' => \Drupal::url('dblog.overview')]) . '</dd>';
   

   There's an "and" missing before "operational information". However, this change is out of scope. The scope of this issue is to add one point of documentation, not improve the existing documentation. We should file a separate followup for @abi_cima's suggested improvements.

3. +++ b/core/modules/dblog/dblog.module
   @@ -25,9 +25,11 @@ function dblog_help($route_name, RouteMatchInterface $route_match) {
   +      $output .= '<p>' . t('Users with <em>administer site reports</em> permissions can clear all the logs, and cron can <a href =":logging_settings" title="Go to logging settings page.">regularly clear</a> old entries. For these reasons, the Database Logging module should not be used for forensic purposes. Instead, it is recommended to use the Syslog module to build a forensic logging system. <a href=":audit_trail_wiki">Read more </a> about forensic logging.', [':logging_settings' => \Drupal::url('system.logging_settings'), ':audit_trail_wiki' => 'https://en.wikipedia.org/wiki/Audit_trail']) . '</p>';
   

   I still think there are several problems with this paragraph.

   • "Regularly clear" and "Read more" are also not good link texts. Link texts should make sense and be specific out of context. Adding titles to the links does not really make up for just selecting better text, e.g. <a href="">Read more about forensic logging</a>.
   • I don't think we need to provide so much detail on what can change the logs. I'd simply say "Administrators and automated cron tasks can clear the database logs."
   • The sentence "It is recommended to use the Syslog module to build a forensic log" is also misleading. It sounds like we're telling the user they need to go and make a forensic log. That's not the goal. The goal is only to provide people who already want a forensic log information that this is the wrong tool.
   • We also don't need to distract them so much about forensic logging.

Putting it together:

Administrators and automated cron tasks can clear the Database Logging module's logs, so they should not be used for <a href="https://en.wikipedia.org/wiki/Audit_trail">forensic logging</a>. For forensic purposes, use the Syslog module instead.

Much shorter. Less to read. Fewer misdirections.

The other two changes should be removed from the patch. @abi_cima, maybe you could file the new issue for them since they were your suggested improvements?

Thanks everyone!

Thanks for review!

Enclosed is a patch reducing the scope and changing the wording as outlined in #58.

@abi_cima I have not filed the new issues as suggested.

Regards

@JacobSanford please note, that per https://www.drupal.org/docs/develop/standards/coding-standards#quotes,
single quote escaping should be avoided when possible.

@valthebald Thanks for quick review!

My bad - reversed the quotes in that line.

Hmmmm I don't think we use single quotes around HTML attributes, like, anywhere. (Well, there are three total instances in core with an href attribute.) OTOH there are plenty of examples of escaping an apostrophe in a string.

So let's go back to the previous patch. Sorry for the confusion! We need to reupload it as the final file attachment to the issue.

Actually, we can also just remove the 's entirely: "can clear the Database Logging module logs". :)

Added change from #63 on patch from #59

Picking this up to quickly test during Baltimore 2017 - I think this can RTBC'd looks good and concise.

Patch applies and looks good.
+1 RTBC

Just been curious why the test is getting failed.

1. Markup should be consistent with other message, i.e. '<h3>' should be '<dt>', '<p>' => '<dd>'
2. Second message doesn't contain apostrophe, so single quotes should be used.
3. word "instead" is unnecessary in the end, can be just "For forensic purposes, use the Syslog module."

As non-native English speaker, I am not sure in the last item, safe to ignore if you disagree.

Made changes according to comment #71.1 and #71.3. Not sure about #71.2. Because earlier, using single quotes was the reason for tests getting failed.

Correct me if I'm wrong.

Thank you.

I thinks #71.2 should be followed. Let's give it a try.

Thank you @dhruveshdtripathi! Additions look good, and consistent with previous output of

dblog_help()

Retested patch #73

+++ b/core/modules/dblog/dblog.module
@@ -29,6 +29,8 @@ function dblog_help($route_name, RouteMatchInterface $route_match) {
+      $output .= '<dd>' . t('Administrators and automated cron tasks can clear the Database Logging module logs, so they should not be used for <a href=":audit_trail_wiki">forensic logging</a>. For forensic purposes, use the Syslog module.', [':audit_trail_wiki' => 'https://en.wikipedia.org/wiki/Audit_trail']) . '</dd>';

This is very picky, but the 'they' feels ambivalent here between'Database Logging module logs' and 'Administrators and automated cron tasks'.

I wonder if just removing the 'so' would help with that. Or reversing so "The Database Logging module logs may be cleared by administrators and automated cron tasks, so they should [...]'.

I have implemented the changes mentioned in #77.

#78 looks good to me.
+1 to RTBC.

What about:

These logs are not useful for forensic logging because they can be cleared by administrators and cron tasks. For forensic purposes, use the Syslog module.

@yoroy I'd still vote for #78 because it's more neutral (i.e. does not contain evaluation "not useful")

These logs cannot be used for forensic logging because they could be cleared by administrators and cron tasks. For forensic purposes, use the Syslog module.

There isn't any evaluation, and I think it's slightly better.

The text first says This log is not persistent, and then The Database Logging module logs. The use of singular/plural should be consistent.

This log cannot be used for forensic logging because it could be cleared by administrators and cron tasks. For forensic purposes, use the Syslog module.

Re-rolled for 9.2

I'm going to work on this issue during Drupalcon NA 2021 for the next hour or two

Novice contributor, DrupalCon2021. I'm working on this as part of mentoring for 90 minutes...

Per "The text first says This log is not persistent, and then The Database Logging module logs. The use of singular/plural should be consistent.

This log cannot be used for forensic logging because it could be cleared by administrators and cron tasks. For forensic purposes, use the Syslog module.
"

Helping out with mentoring on this issue with selwynpolit and ijf8090

Forked issue, created merge request #MR !549
Updated the text to more clearly reflect what the Irish would say. Note. They have more nobel prize winners for the english language per capita than anyone else. The text was provided by the amazing Ian Finlay

and thanks to Aimee Hannaford for the cheerleading and James Gilliland for the Gitlab wisdom. I'd like to thank the academy *sniff*

See new screenshot below:

+      $output .= '<dt>' . t('This log is not persistent') . '</dt>';
+      $output .= '<dd>' . t('Database Logging will not allow you to see logs if your site is not accessible. Also, the Database logging module writes logs to the database which can slow down the website. By using Syslog you can improve the performance of the site. Syslog, helps you to see the logs and troubleshoot even if site is not accessible', [':audit_trail_wiki' => 'https://en.wikipedia.org/wiki/Audit_trail']) . '</dd>';

There should not be any comma between the subject (Syslog) and the verb (helps).
The module name is Database Logging, but the description once uses that correctly, and once uses Database logging.

I think the following text is preferable, as it avoids repeating the same words.

The Database Logging module writes logs to the database, which can slow down the website. It won't allow you to see logs if your site isn't accessible. By using the Syslog module, you can improve the performance of the site. The module helps you to see the logs and troubleshoot even if your site is not accessible.

I am not sure the system log is always accessible when the website isn't accessible; it seems to exclude the website isn't accessible because the server isn't accessible. Given the description is read from users who access a remote server (in most of the cases), those sentences seem misleading.

Addressed #98.

I still think that the sentence used in the previous patches is preferable.

The Database Logging module logs may be cleared by administrators and automated cron tasks, so they should not be used for forensic logging. For forensic purposes, use the Syslog module.

The part about performance is irrelevant. The point is that the database table used from the Database Logging module can be cleared at any time, from cron tasks or from administrator users; for that reason, it cannot used for forensic logging.
The part about the system log being always available is probably not relevant for the forensic purposes, and it isn't completely true; on a remote server, both the system log and the site could be inaccessible at the same time.

I am going to update my merge request to reflect the new text requested.

The merge request changes the code as suggested.

#99 looks good to me.
+1 to RTBC.

RTBC+1, the new text is succinct and explains the issue at hand.

Hiding all patches as this issue is using the merge workflow.

Saving issue credits.

Will leave this for @catch or @xjm to have the final say as they've reviewed it a few times.

Looked back over mine and @xjm's comments (from 2017 (!)) and the latest version addresses both.

Committed 383360e and pushed to 9.2.x. Thanks!