Add logging of who installs/uninstalls a module

Adds 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.

Hm. Do we need a test for this?

Same patch with tests.

I'm not used to tagging yet.

...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.

I 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 of assertModuleLogMessage()).

Thanks for your work!

This version builds on item 3 in #6 to make the test behave more generically, so it accepts parameters like watchdog()'s.

+    $count = db_select('watchdog', 'w')
+      ->condition(db_and()
+        ->condition('type', $type)
+        ->condition('message', $message)
+        ->condition('variables', serialize($variables))
+        ->condition('severity', $severity)
+        ->condition('link', $link)
+      )

A "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.

I'm glad that's the case. Revised to remove the extra condition junk.

This 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.

I ripped off the docblock from watchdog(), so I've updated it as well in this revision.

Looks great. Committed to CVS HEAD. Thanks deekayen.

Why did this not use hook_modules_installed/enabled/disabled/uninstalled? Let's keep this logging out of the api functions.

Hm. 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

First 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.

@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?

Sorry, 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.

@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.

We 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.

No. 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.

Dave 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).

@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.

@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.

My 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.

Ok 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.

Just 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

SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal-head.simpletest882494watchdog' doesn't exist in dblog_watchdog() (line 148 of /var/www/drupalhead/modules/dblog/dblog.module).

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.

Nevermind, it dawned on me how to log the install.

I'm on board, too. Previous objections overruled.

Just 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.

Just freshening it up for the change in line numbers.

Thanks! Will most probably pass, but of course, let's wait for the bot. ;)

Committed to CVS HEAD. Thanks.

A candidate for D6? Here is a backport.

#3: 210876_module_logging_w_tests.patch queued for re-testing.

Myself, 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.

@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.

#39: 210876_log_module_install-D6-1.patch queued for re-testing.

The patch still applies. The patch is rejected by the test bot due to #961172: All D6 Core patches are failing.

Try this patch. this can be customized to log the fields what ever we wants.

Been 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.

Why 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.

Status: 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.

Great concept. Is there any plans to create a module for this? Im leery of patching core.

Bumping to 8.x

The fix was committed to HEAD last year. The latest patch is a backport for D6.

#39: 210876_log_module_install-D6-1.patch queued for re-testing.

oops. sorry.

Rerolled with Git.

AFAIK this is too late for D6 now. Closing.