Thanks for all the work on this module, I'm trying it out now for watchdog. One thing I noticed is that the collection name variable used for mongodb_watchdog is a very generic "mongodb_collectionname". It works because the other components use a different convention, but if this module expands for other purposes, each component should have its own variable. So mongodb_watchdog should use mongodb_collectionname_watchdog, for example.
The attached patch changes the name to mongodb_collectionname_watchdog in the code and with an update hook.

CommentFileSizeAuthor
#15 971232-removed-collection-names-1.patch6.04 KBthebuckst0p
#13 971232-removed-collection-names.patch3.14 KBthebuckst0p
#6 mongodb-watchdog-collectionname_1.patch3.17 KBthebuckst0p
#5 mongodb-971232-remove-vars.patch1.03 KBthebuckst0p
#1 mongodb-watchdog-collectionname.patch3.17 KBthebuckst0p
mongodb-watchdog-collectionname.patch3.17 KBthebuckst0p

Comments

thebuckst0p’s picture

thebuckst0p commented
StatusFileSize
new mongodb-watchdog-collectionname.patch3.17 KB

Looking at this further, it seems that a similar naming convention was attempted, but not applied consistently: the settings form in mongodb_watchdog_access_logging_settings() sets a variable "mongodb_watchdog_collectionname", but then never uses that variable, using (prior to my patch) the generic "mongodb_collectionname" to connect.
I think the naming convention mongodb_VARIABLE_COMPONENT makes more sense, but to work better with the mongodb_watchdog_VARIABLE convention used in the module's other settings, I've re-rolled the patch to use mongodb_watchdog_collectionname (instead of mongodb_collectionname_watchdog in the first version and mongodb_collectionname in the latest release).

thebuckst0p’s picture

thebuckst0p commented
Title: Collection name variable should be less generic » Collection name variable should be less generic (and consistent)
thebuckst0p’s picture

thebuckst0p commented

In fact, the other settings - mongodb_watchdog_connection and mongodb_watchdog_dbname - are never used, so they should just be removed... the mongodb_collection() doesn't allow for a different connection per collection anyway. Maybe this is a bigger issue (of this module suite's architecture generally) than just the variable names.

chx’s picture

chx commented

These variables should simply be yanked. These were added before there was proper connection handling and there is no point IMO. Just point your collections to separate databases as needed.

thebuckst0p’s picture

thebuckst0p commented
StatusFileSize
new mongodb-971232-remove-vars.patch1.03 KB

This patch removes the unused mongodb_watchdog_connection and mongodb_watchdog_dbname variables (putting here for reference). I had already removed them in my own production copy, so I committed it to 6.x-1.x (36888c0beed).

The inconsistent use of collection variables is separate, though -- I need to look at this again to remember the details.

thebuckst0p’s picture

thebuckst0p commented
Priority: Normal » Major
StatusFileSize
new mongodb-watchdog-collectionname_1.patch3.17 KB

The original issue of inconsistent variable names still exists, and is still a serious bug in the 6.x module. I've re-rolled the patch from #1 with the latest sha's. Can someone confirm that it's ok, and I'll commit it?

chx’s picture

chx commented
Status: Needs review » Needs work 
-    $collection = mongodb_collection(variable_get('mongodb_collectionname', 'watchdog'));
+    $collection = mongodb_collection(variable_get('mongodb_watchdog_collectionname', 'watchdog'));

shoudl be

-    $collection = mongodb_collection(variable_get('mongodb_collectionname', 'watchdog'));
+    $collection = mongodb_collection('mongodb_watchdog');

Just remove them.

thebuckst0p’s picture

thebuckst0p commented

Hmm. I don't understand how to "just point your collections to separate databases as needed" - how is this done, with single connection and dbname variables?

Also the collection is analogous to a MySql table, why would you want 'mongodb_' in the name? That would be like calling every table mysql_TABLE. What am I not understanding?

Thanks

chx’s picture

chx commented
Priority: Major » Normal
Status: Needs work » Needs review

Yeah, call it watchdog, sorry, that was a typo. With a single collection / dbname you can't -- just add another database to the config. They are autocreated anyways. Why would you want to name your collections something else? What is the use case? Multisite with a shared mongodb ...? I can't imagine anyone running a shared mongodb due to the single writer thread issue.

chx’s picture

chx commented
Priority: Normal » Major
Status: Needs review » Needs work

Sorry, crosspost.

chx’s picture

chx commented
Title: Collection name variable should be less generic (and consistent) » Collection name variables should be removed

And now a real title.

thebuckst0p’s picture

thebuckst0p commented

Gotcha, that makes sense.
That would leave nothing in the settings form at admin/reports/settings/mongodb, what do you suggest doing with that? (I'm thinking putting a "No settings yet" message, since it's conceivable it might be needed later. I have code elsewhere that controls the # of days to store logs, for instance (which I should post & commit separately).

thebuckst0p’s picture

thebuckst0p commented
Status: Needs work » Needs review
StatusFileSize
new 971232-removed-collection-names.patch3.14 KB

See attached patch, how does that look?

chx’s picture

chx commented

Care to nuke from the other modules as well? Edit: i will follow suit in 7.x

thebuckst0p’s picture

thebuckst0p commented
StatusFileSize
new 971232-removed-collection-names-1.patch6.04 KB

Now also includes the 'session' collection name which I missed earlier.

thebuckst0p’s picture

thebuckst0p commented

Committed to 6.x.

thebuckst0p’s picture

thebuckst0p commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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