| Project: | Mongodb |
| Version: | 7.x-1.x-dev |
| Component: | Watchdog |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Currently mongodb_watchdog uses not one but two variables to defined the watchdog collection name: at some places, it uses mongodb_collectionname, while at others it uses mongodb_watchdog_collectionname. This does not break by default as both are defaulted to watchdog, and any user who customized their collectionname is likely to have noticed the two variables and set both accordingly, but should be fixed.
I think it would be best to standardize on the latter, to let other modules customize their own collectionname.
The mongodb_session module also uses a customizable collection name, in mongodb_session. Going further, we might want to have a unified mechanism through which submodules could define their collection(s) and have them be customized in a UI in a unified way.
Comments
#1
Suggested patch aligns on
mongodb_watchdog_collectionname.#2
Also note, a larger fix for this whole 'collection name' question can also be found in the 6.x branch at #971232: Collection name variables should be removed. I feel the decision to use hardcoded collection names instead of having them in variables should be reconsidered, though: under some circumstances, it can be very useful to be able to switch collection on the fly without having to patch code for this: I had a use case for this very recently.
#3
Suppose a shorten name is better. Also adds hook_update_N() and missed hook_uninstall()
#4
I was keeping the uninstall for another issue to avoid mixing questions ;-). Good catch for the update_N, though. But we're missing use of the variable in the install hooks.
But since you've added the uninstall hook, it raises another question: should the collection be dropped on uninstall, as modules do with SQL tables ? Rerolled accordingly to drop the watchdog collection as well as the spurious watchdog_even_ collections for those running without the needed patch.
#5
Rerolled, there was a skipped typo in the original patch.
#6
Rerolled:
- watchdog_event collections have a hardcoded watchdog_event_* name, not dependent on the name of the watchdog collection.
- include collection dropping on uninstall
#7
Big +1 to dropping data on uninstall. the only regexp confused me (probably because of late night)
I think this good to go but I need to test install-uninstall to confirm
+++ b/mongodb_watchdog/mongodb_watchdog.installundefined@@ -1,24 +1,55 @@
+ // Then drop all watchdog event collections in all known database aliases.
+ $aliases = variable_get('mongodb_connections', array());
+ $aliases['default'] = TRUE;
+ $regex = '/\.watchdog_event_[[:xdigit:]]{32}$/';
Looks fine, but having test for this is best. Regexp should have comment to function that generates collection name to follow a changes that can happen in future in collection generation/
Also droping of data on uninstall is right choice that been missed.
+++ b/mongodb_watchdog/mongodb_watchdog.installundefined@@ -1,24 +1,55 @@
+ if ($count) {
+ drupal_set_message(format_plural($count, 'Dropped 1 watchdog collection', 'Dropped @count watchdog collections'));
Do we actually need this?
+++ b/mongodb_watchdog/mongodb_watchdog.installundefined@@ -27,32 +58,34 @@ function mongodb_watchdog_enable() {
function mongodb_watchdog_ensure_indexes() {
+ $watchdog = mongodb_collection(variable_get('mongodb_watchdog', 'watchdog'));
+
// Index for adding/updating increments.
$index = array(
'line' => 1,
'timestamp' => -1
);
- mongodb_collection('watchdog')->ensureIndex($index);
+ $watchdog->ensureIndex($index);
Great catch - hardcoded collection name and optimization.
Powered by Dreditor.
#8
looks like most of this patch already commited
#9
I mark this as fixed. Most of the code seems to be in dev.
#10
Automatically closed -- issue fixed for 2 weeks with no activity.