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!

The last submitted patch failed testing.

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

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.

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

@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

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.

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

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.

The last submitted patch failed testing.

Just freshening it up for the change in line numbers.

The last submitted patch failed testing.