Posted by thebuckst0p on November 14, 2010 at 7:39am
3 followers
| Project: | Mongodb |
| Version: | 6.x-1.x-dev |
| Component: | Watchdog |
| Category: | task |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| mongodb-watchdog-collectionname.patch | 3.17 KB | Ignored: Check issue status. | None | None |
Comments
#1
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).
#2
#3
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.#4
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.
#5
This patch removes the unused
mongodb_watchdog_connectionandmongodb_watchdog_dbnamevariables (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.
#6
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?
#7
- $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.
#8
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
#9
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.
#10
Sorry, crosspost.
#11
And now a real title.
#12
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).
#13
See attached patch, how does that look?
#14
Care to nuke from the other modules as well? Edit: i will follow suit in 7.x
#15
Now also includes the 'session' collection name which I missed earlier.
#16
Committed to 6.x.
#17
#18
Automatically closed -- issue fixed for 2 weeks with no activity.