Problem/Motivation
Based on #1635646: Admin users should know site report permission allows for more than viewing it is clear that the dblog module cannot be used as a forensic log.
Here is a screen shot of the current help screen at /admin/help/dblog
Proposed resolution
Add to the dblog_help hook the reasons why dblog cannot be used for forensic log. These reasons are:
- The user with administer site reports can delete them.
- Cron automatically removes older logs.
Also recommend alternatives like syslog.
Remaining tasks
Write a patch. Review the wording.
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|
Issue fork drupal-2732113
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Blisstering Solutions commentedComment #3
rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Blisstering Solutions commentedApplying patch
Thanks!!
Comment #4
dagmarThanks! However it feels a bit out of context copy & paste a comment from a thread into the hook_help. Try to change the second line of your patch to something that explain why the dblog is not a good solution.
Comment #5
rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Blisstering Solutions commentedHi dagmar,
I have explained why the db_log should not be used.
Please review.
Thanks!!
Comment #6
dagmarBetter :)
Syslog is not the only alternative to keep persistent logs. There are other logging mechanisms like monolog. So recommend syslog as the final solution may not be accurate.
I think we should provide a positive feedback here, it seems dblog is not useful at all. Maybe a bullet list that says something like:
The dblog module may not work on the following scenarios ...
-
-
-
Comment #7
rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Blisstering Solutions commentedDone changes as per said.
Thanks!!
Comment #9
rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Blisstering Solutions commentedAdding Patch
Thanks!!
Comment #10
dagmarThanks! Although there are some new issues with this patch.
double ;;
extra space there before the semicolon.
I think we should mention also that cron clears automatically old logs.
Comment #11
rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Blisstering Solutions commentedI have done the changes.
Thanks!!
Comment #12
dagmarComment #13
dagmarMoving temporarily to documentation to get feedback about this from @jhodgdon
Comment #14
jhodgdonAdding 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.
Comment #16
dagmarThanks. I took another approach this time.
Comment #18
arkiii CreditAttribution: arkiii at Multimedia Partners commentedComment #19
arkiii CreditAttribution: arkiii at Multimedia Partners commentedI did some re-wording to correct the output. I think it reads better.
Comment #20
dagmarThanks, I think you probably modified by hand the patch and now it didn't apply.
Please follow this instructions https://www.drupal.org/patch
Comment #21
dagmarComment #22
rakesh.gectcrComment #24
ronchica CreditAttribution: ronchica as a volunteer commentedI am going to look at this as part of SprintWeekend2017
Comment #25
ronchica CreditAttribution: ronchica as a volunteer commentedI checked /admin/help/dblog in 8.4.0-dev before and after applying the patch. Looks good.
Before:
After:
Comment #26
ronchica CreditAttribution: ronchica as a volunteer commentedComment #27
cilefen CreditAttribution: cilefen commentedIt is "cron", not "the cron".
"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."
Comment #28
ronchica CreditAttribution: ronchica as a volunteer commentedUpdated text, uploaded patch, and interdiff for the first time :)
Comment #30
ronchica CreditAttribution: ronchica as a volunteer commentedneeded to re-run test
Comment #31
dagmarThanks everyone.
Comment #32
cilefen CreditAttribution: cilefen commentedThis is a run-on sentence.
Sorry, I should have reviewed this a little deeper. Only "researchers" can detect problems in forensic logs? What about site admins, developers, or sysadmins?
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".
Comment #33
ronchica CreditAttribution: ronchica as a volunteer commentedThe 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:
Note: occurred was spelled incorrectly.
Let me know any changes (or not) and I will re-do the patch.
Comment #34
cilefen CreditAttribution: cilefen commentedI 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."
Comment #35
ronchica CreditAttribution: ronchica as a volunteer commentedOk. That's better.
Comment #36
ronchica CreditAttribution: ronchica as a volunteer commentedForgot the period at the end of the sentence. Re-did the patch.
Comment #37
ronchica CreditAttribution: ronchica as a volunteer commentedComment #38
leslieg CreditAttribution: leslieg as a volunteer commentedReview of latest patch shows the requested changes to the forensic section of the help text.
Comment #39
xjmGreat 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:
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.
Comment #40
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedSomething like this?
Comment #41
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedComment #42
dagmarThanks @tameeshb. Stills needs to fix this:
Comment #43
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedAny recommendations for a reliable resource for the text/link?
Comment #44
dagmarMaybe this one: https://en.wikipedia.org/wiki/Audit_trail
Comment #45
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedAdded the wikipedia reference with interdiff. Please review! :)
Comment #46
dagmarThanks. This needs to be converted to the new
arrays syntax []
. The patch doesn't apply anymore.Comment #47
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedConverted to new array syntax.
Comment #48
cilefen CreditAttribution: cilefen commented"see here" is not good anchor text.
Comment #49
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedHow about read more about "Audit trail" on wikipedia
Comment #50
dagmarMaybe "Read more about forensic logging."
Also, please remove the
<br>
Comment #51
dagmarComment #52
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #53
abi_cima CreditAttribution: abi_cima as a volunteer and at Globant commentedYou 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.
There is no "," before and. It should be "...usage information, warnings and errors"
Comment #54
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedChanged from #53
Comment #55
dagmarWe need an extra space here:
Comment #56
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedAdded space before "Read more"
Comment #57
dagmarThanks everyone.
Comment #58
xjmThere'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.
I still think there are several problems with this paragraph.
<a href="">Read more about forensic logging</a>.
Putting it together:
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!
Comment #59
JacobSanfordThanks 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
Comment #60
valthebald@JacobSanford please note, that per https://www.drupal.org/docs/develop/standards/coding-standards#quotes,
single quote escaping should be avoided when possible.
Comment #61
JacobSanford@valthebald Thanks for quick review!
My bad - reversed the quotes in that line.
Comment #62
xjmHmmmm 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.
Comment #63
xjmActually, we can also just remove the
's
entirely: "can clear the Database Logging module logs". :)Comment #64
tameeshb CreditAttribution: tameeshb at Google Summer of Code commentedAdded change from #63 on patch from #59
Comment #65
mikeoharaPicking this up to quickly test during Baltimore 2017 - I think this can RTBC'd looks good and concise.
Comment #66
mikeoharaComment #68
c.nish2k3 CreditAttribution: c.nish2k3 as a volunteer commentedPatch applies and looks good.
+1 RTBC
Comment #70
rakesh.gectcrJust been curious why the test is getting failed.
Comment #71
valthebald1. 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.
Comment #72
dhruveshdtripathi CreditAttribution: dhruveshdtripathi at DevsAdda commentedMade 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.
Comment #73
dhruveshdtripathi CreditAttribution: dhruveshdtripathi at DevsAdda commentedI thinks #71.2 should be followed. Let's give it a try.
Comment #74
valthebaldThank you @dhruveshdtripathi! Additions look good, and consistent with previous output of
Comment #76
dhruveshdtripathi CreditAttribution: dhruveshdtripathi at DevsAdda commentedRetested patch #73
Comment #77
catchThis 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 [...]'.
Comment #78
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedI have implemented the changes mentioned in #77.
Comment #79
vaibhav29 CreditAttribution: vaibhav29 commented#78 looks good to me.
+1 to RTBC.
Comment #80
yoroy CreditAttribution: yoroy at Roy Scholten commentedWhat 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.
Comment #81
valthebald@yoroy I'd still vote for #78 because it's more neutral (i.e. does not contain evaluation "not useful")
Comment #82
apadernoThere isn't any evaluation, and I think it's slightly better.
Comment #83
apadernoThe text first says This log is not persistent, and then The Database Logging module logs. The use of singular/plural should be consistent.
Comment #91
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedRe-rolled for 9.2
Comment #92
selwynpolit CreditAttribution: selwynpolit commentedI'm going to work on this issue during Drupalcon NA 2021 for the next hour or two
Comment #93
ijf8090 CreditAttribution: ijf8090 as a volunteer commentedNovice 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.
"
Comment #94
ChrisDarke CreditAttribution: ChrisDarke as a volunteer commentedHelping out with mentoring on this issue with selwynpolit and ijf8090
Comment #95
selwynpolit CreditAttribution: selwynpolit commentedComment #97
selwynpolit CreditAttribution: selwynpolit commentedForked 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:
Comment #98
apadernoThere 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.
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.
Comment #99
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Care, Drupal Association commentedAddressed #98.
Comment #100
apadernoI still think that the sentence used in the previous patches is preferable.
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.
Comment #101
selwynpolit CreditAttribution: selwynpolit commentedI am going to update my merge request to reflect the new text requested.
Comment #102
apadernoThe merge request changes the code as suggested.
Comment #103
kleiton_rodrigues CreditAttribution: kleiton_rodrigues at CI&T commented#99 looks good to me.
+1 to RTBC.
Comment #104
longwaveRTBC+1, the new text is succinct and explains the issue at hand.
Hiding all patches as this issue is using the merge workflow.
Comment #105
larowlanSaving issue credits.
Will leave this for @catch or @xjm to have the final say as they've reviewed it a few times.
Comment #106
catchLooked back over mine and @xjm's comments (from 2017 (!)) and the latest version addresses both.
Committed 383360e and pushed to 9.2.x. Thanks!