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.

CommentFileSizeAuthor
#6 1173058-6.patch9.12 KBfgm
#5 mongo_wdvars-1173058-5.patch8.35 KBfgm
#4 mongo_wdvars-1173058-4.patch8.35 KBfgm
#3 1173058-mongo_wdvars.patch6.47 KBandypost
#1 inconsistent_variable_use-1173058-1.patch4.06 KBfgm

Comments

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Status: Active » Needs review
StatusFileSize
new inconsistent_variable_use-1173058-1.patch4.06 KB

Suggested patch aligns on mongodb_watchdog_collectionname.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented

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.

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 1173058-mongo_wdvars.patch6.47 KB

Suppose a shorten name is better. Also adds hook_update_N() and missed hook_uninstall()

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
StatusFileSize
new mongo_wdvars-1173058-4.patch8.35 KB

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.
fgm’s picture

fgm
Primary language French
Location Paris, France
commented
StatusFileSize
new mongo_wdvars-1173058-5.patch8.35 KB

Rerolled, there was a skipped typo in the original patch.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
StatusFileSize
new 1173058-6.patch9.12 KB

Rerolled:
- watchdog_event collections have a hardcoded watchdog_event_* name, not dependent on the name of the watchdog collection.
- include collection dropping on uninstall

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work

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.

andypost’s picture

andypost
he/him
Primary language Russian
commented

looks like most of this patch already commited

misc’s picture

misc commented
Status: Needs work » Fixed

I mark this as fixed. Most of the code seems to be in dev.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.