Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
On sites where there are several admins, problems can arise from enabling or disabling modules. I would love to see a message that gets logged when a module is enabled, disabled, or uninstalled, and I'd like the uid included.
Comment | File | Size | Author |
---|---|---|---|
#57 | 210876_log_module_install-D6-2.patch | 1.73 KB | c960657 |
#47 | unistall_patch.txt | 378 bytes | anujahiniduma |
#39 | 210876_log_module_install-D6-1.patch | 4.07 KB | c960657 |
#36 | 210876_log_module_install_4.patch | 2.84 KB | c960657 |
#34 | 210876_log_module_install_v3.patch | 1.62 KB | deekayen |
Comments
Comment #1
deekayen CreditAttribution: deekayen commentedAdds 3 simple watchdog calls. I didn't put a uid in the string because the user that performed the action is automatically linked with the watchdog entry and displays as a link in the User column of the Recent log entries page.
Comment #2
Freso CreditAttribution: Freso commentedHm. Do we need a test for this?
Comment #3
deekayen CreditAttribution: deekayen commentedSame patch with tests.
Comment #4
deekayen CreditAttribution: deekayen commentedI'm not used to tagging yet.
Comment #5
deekayen CreditAttribution: deekayen commented...and for some variety, this patch tests to make sure both the t() string are there and the module name in the variables field. This one is more precise; #3 is more flexible.
Comment #6
Dries CreditAttribution: Dries commentedI support logging these events -- they are quite important. However, a couple of comments based on the last patch:
1.
Assert there was a log entry is generated for module status change.
sounds like it is broken English.2. In
assertModuleLogMessage($status, $module)
it feels more natural to flip the order of the parameters.3. I wonder if there needs to be a more generic
assertLogMessage()
(instead ofassertModuleLogMessage()
).Thanks for your work!
Comment #8
deekayen CreditAttribution: deekayen commentedThis version builds on item 3 in #6 to make the test behave more generically, so it accepts parameters like watchdog()'s.
Comment #9
BerdirA "db_select() object" acts the same way as a "db_and() object", so you don't need that, you can simply call condition multiple times. You only need db_and() if you want to add them to db_or() or another DatabaseCondition object.
Comment #10
deekayen CreditAttribution: deekayen commentedI'm glad that's the case. Revised to remove the extra condition junk.
Comment #11
Dries CreditAttribution: Dries commentedThis looks like a drastic improvement compared to the initial incarnation of this patch. Two small things though:
1. I noticed that you where missing a trailing point in one of the sentences in the phpdoc.
2. "Keep $message translatable by not concatenating dynamic values into it!" seems a little cryptic. I think it would be good to elaborate a bit on that in the docs.
Almost RTBC, IMO.
Comment #12
deekayen CreditAttribution: deekayen commentedI ripped off the docblock from
watchdog()
, so I've updated it as well in this revision.Comment #13
Dries CreditAttribution: Dries commentedLooks great. Committed to CVS HEAD. Thanks deekayen.
Comment #14
Dave ReidWhy did this not use hook_modules_installed/enabled/disabled/uninstalled? Let's keep this logging out of the api functions.
Comment #15
sunHm. Not sure. Invoking watchdog() after an action was performed is usually done by the API function itself.
For example: http://api.drupal.org/api/function/node_form_submit/7
Comment #16
BerdirFirst that and second, who whould do it then? system.module? We don't really gain anything then. I would agree if there would be a watchdog module though, but that would mean that all watchdog calls need to be moved to hooks.
Comment #17
deekayen CreditAttribution: deekayen commented@Dave Reid: Mostly because I wasn't aware of the new hooks. I could move them to new hook functions in system.module. I'm mostly willing to consider this because I noticed I created the only calls to watchdog in all of install.inc and module.inc. Otherwise, moving them to system.module, which is also required anyway, seems to just spread around the code and seems like more to keep track of and more function overhead.
Being serious though, because I really want to get the whole picture, what is gained by moving them to the new hooks in system.module as opposed to having them called deeper in the .inc?
Comment #18
sunSorry, Dave, I've thought quite a bit about this, but moving those watchdog() invocations elsewhere does not make sense. watchdog() itself is a layer of abstraction (or "hook") already, and currently, all of core invokes it directly to log an event.
It might be different if watchdog was a core-event-logging-API module of its own. But then again, we'd probably have to rewrite quite a bit of core to invoke hooks all over the place and allow that watchdog.module to catch them.
Reverting status.
Comment #19
agentrickard@sun
Respectfully. Dave's point makes great sense from a DX, code readability perspective. Putting _some_ install/uninstall function directly inside the .inc file is an inconsistency that, if allowed once, will continue, leading to difficulties in finding where certain actions take place.
The proper place for this is in
system_modules_(un)installed()
. That's why the abstraction hooks are there.Not changing status. This is in. We should open a new issue to fix.
Comment #20
BerdirWe are not executing an action, we just log the action we've already done. With your argument, we need to move *every* watchdog call to a hook, I don't think that makes sense, atleast not if we would do it in system.module.
Comment #21
agentrickardNo. You totally misunderstand.
We aren't moving the watchdog() part _per se_. We are saying "Hey, when modules are installed/uninstalled, system module wants to act." The fact that all system.module does is call watchdog is not the point. (And, yes, calling watchdog() is an action.)
In effect, we are demonstrating 'best practice' code for use of the new hooks.
Think of it this way -- which is the intended use of the new hooks -- what if, on module uninstall, Drupal core _always_ wanted to execute 4 functions instead of just one. Where should that code go and why?
The answer is system_modules_uninstalled, for easier readability and to leverage the API.
Comment #22
deekayen CreditAttribution: deekayen commentedDave and Ken are swaying me to their side, so I'll put this back on my todo list (through #461990: use new install/enable/disable/uninstall hooks for logging module changes).
Comment #23
sun@Dries: Your thoughts highly appreciated.
My take still is: As long as watchdog() doesn't belong to a dedicated watchdog.module, the "core" function always invokes watchdog() to log an event. "core" does not mean Drupal core, but refers to the main function that takes some action. Hooks are invoked to execute additional actions, depending on the main action. But logging the event itself is always done by the main function/action.
Comment #24
NancyDruIndeed, I must absolutely agree with Daniel on this one. As a matter of fact, the "watchdog" function may, at least in this case, be poorly named since logging is replaceable. Logging must be contemporaneously done by the function taking the action, whether that is the "core" or a hook function. Anything else runs an increased risk of losing the logged message. While the request that spawned this would 99% of the time not be critical if lost, there is always that one time when a lost message is a back-breaker.
Comment #25
agentrickard@sun
That is a good point about logging within the function. Perhaps that should be a documented Drupal standard, if it isn't already.
@NancyDru
I don't see how moving the watchdog call could cause an error; besides, don't we need to ensure that these new hook work? And therefore a test based on this function makes sense.
Comment #26
webchickMy general rule of thumb for issues like this is "What does core do?" And what core does is routinely fire the watchdog message wherever the actions it wishes to log are made. Often, this is in form submit handlers, but it can also be in CRUD API functions when those are present.
But nowhere do we implement hooks in order to log messages. For example, comment watchdogs are comment_save(), not hook_comment_update/insert().
There are definitely legitimate use cases for hook_modules_X(), but this isn't one of them.
Comment #27
Dave ReidOk thank you webchick, sun, and NancyDru for helping convince me. Just wanted to make sure we weren't adding crud to the module api that the new hooks were trying to prevent.
Comment #28
deekayen CreditAttribution: deekayen commentedJust to document it for later passer-bys, I did work some on logging the install. The problem with logging installs happens especially during the Drupal core installation because not all tables exist yet (like watchdog), so you get ugly looking things like
I'm attaching the method that I tried, but I left it out of the final patch because I don't think it's a simple thing to add, and even if it could be added, I think it would require more code than the log entry would be worth (since we can still easily log the enable event) to check for whether tables exist or not before inserting the log entry.
Comment #29
deekayen CreditAttribution: deekayen commentedNevermind, it dawned on me how to log the install.
Comment #30
NancyDru@agentrickard: the farther apart the action and the logging are, the greater the chance for a problem (such as a server crash) to cause the logging to not occur.
Comment #31
agentrickardI'm on board, too. Previous objections overruled.
Comment #32
deekayen CreditAttribution: deekayen commentedJust to clarify what's going on here, #12 that was committed, only logs enable, disable, and uninstall events. #29 adds an additional watchdog call to log install as well.
Comment #34
deekayen CreditAttribution: deekayen commentedJust freshening it up for the change in line numbers.
Comment #36
c960657 CreditAttribution: c960657 commentedComment #37
sunThanks! Will most probably pass, but of course, let's wait for the bot. ;)
Comment #38
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #39
c960657 CreditAttribution: c960657 commentedA candidate for D6? Here is a backport.
Comment #40
otisjs01 CreditAttribution: otisjs01 commented#3: 210876_module_logging_w_tests.patch queued for re-testing.
Comment #41
rCharles CreditAttribution: rCharles commentedMyself, after a year plus of learning Drupal from scratch with many attempts to build a feature rich (many modules) site and many restarts due to unresolvable module errors, i would classify this priority as major. While "Backup and Migrate" is effective to a degree I had to discovery SSH/Putty on my own for server folder backups to match the database backups and I am dreading more time trying to learn CVS or BASH. Put simply, if module logging with notice of version changes is implemented it should be much easier to restore a Drupal site ironically unusable as a result of trying to be improved.
Comment #42
deekayen CreditAttribution: deekayen commented@rCharles: this issue doesn't really have anything to do with logging of version changes to modules, database backups, or BASH. It's simply to make a log entry when a module is installed, activated, disabled, or uninstalled.
Comment #44
c960657 CreditAttribution: c960657 commented#39: 210876_log_module_install-D6-1.patch queued for re-testing.
Comment #46
c960657 CreditAttribution: c960657 commentedThe patch still applies. The patch is rejected by the test bot due to #961172: All D6 Core patches are failing.
Comment #47
anujahiniduma CreditAttribution: anujahiniduma commentedTry this patch. this can be customized to log the fields what ever we wants.
Comment #48
NancyDruWhy not use
watchdog()
? http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/watchd...Comment #49
rCharles CreditAttribution: rCharles commentedBeen some time and a few back and forth battles since i reviewed "my issues" posted but from the looks of it, the install reporting question was not the simplest matter. Guess I should have looked to watchdog and reporting more in the first place. Thanks.
Still, for Drupal to be a little more newbie proof, listing timelines for feature upgrades events (module installs) would seem to help pin point key changes when clusters of activity, especially when multiple users or admin groups make changes on top of the new modules and there is a weakness in backup strategies, not uncommon for many businesses. Anyway, maybe more amateur analysis than assistance to this issue so I'll eave it at that.
Comment #50
rCharles CreditAttribution: rCharles commentedWhy not indeed. Makes sense but seems like yet another potential module. Looking it over i get a glimmer of the results and tracking opportunities although I dare say I'd rather be working on graphics.
Comment #51
c960657 CreditAttribution: c960657 commentedStatus: The patch in #39 is a straight D6 backport of the patch that was committed to HEAD for D7.
If you want to have further logging of upgrades etc., please file a separate ticket for that. I think it is a good idea.
Comment #52
beyond67 CreditAttribution: beyond67 commentedGreat concept. Is there any plans to create a module for this? Im leery of patching core.
Comment #53
agentrickardBumping to 8.x
Comment #54
c960657 CreditAttribution: c960657 commentedThe fix was committed to HEAD last year. The latest patch is a backport for D6.
Comment #55
c960657 CreditAttribution: c960657 commented#39: 210876_log_module_install-D6-1.patch queued for re-testing.
Comment #56
agentrickardoops. sorry.
Comment #57
c960657 CreditAttribution: c960657 commentedRerolled with Git.
Comment #58
mdupontAFAIK this is too late for D6 now. Closing.