Statistics module is broken
catch - April 18, 2008 - 21:30
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | statistics.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
Description
The patch which removed access rules, removed the {access} table from the database schema and replaced it with {blocked_ips}, however there's a feature in statistics module which lets you ban/unban visitors from the top visitors page, uses the {access} table, you know the rest.
I didn't know this feature existed until it was pointed out by chx here http://drupal.org/node/222109#comment-813032
Mea culpa.
#1
OK so I took a look at this, and IMO the best option is to put this functionality into the User Restrictions module that's going to replace the rest of access rules. It could probably be augmented with IP blocking from comment administration as well (for anonymous spammers). Otherwise we have statistics module blocking IPs, or system.module making assumptions about statistics module - neither of which is all that pretty. So here's a patch to clear this last bit out.
#2
Unless I'm mistaken, we can keep this feature with the blocked_ips table. I think we should. We should probably write a test for it too so we don't reintroduce this problem.
#3
Well, looks at least like those queries need some cleanup.
#4
So there's two possible reasons to use this feature:
1. for convenience for blocking any IP.
2. for emergencies if your site is being hammered by a bot
1. ought to be won't fix - people should really do a whois on an ip address before blocking it, and a two click ban doesn't encourage that.
2. I can see, although I'm still not convinced statistics module should be handling IP address blocking at all, and neither should system.module be making assumptions about statistics module.
However it remains broken, so I think the least intrusive option to fix the breakage would be to modify that query to use new blocked_ips table, and add a #default_value to the IP blocking form which it can take from url arguments for the ban links. I'll get started on that - but if anyone has better ideas please post them!
#5
OK here's a patch. SimpleTests are still todo - I'm using this as an opportunity to review testing documentation since I've not written any tests yet.
However I'm setting it back to review temporarily because I've modified the functionality slightly - the ban link is now not shown unless you have the right permission, nor is it shown next to authenticated users - there's already a link to their account where you can block them via user module so offering IP blocking here seems both unnecessary and prone to error. Easy to take out if I've missed a use case.
#6
Here it is with a test. Note that this introduces the statistics.test file and will need a CVS add.
I also incorporate David Rothstein's suggestion from the original issue to put the form at the top of admin/settings/ip-blocking - when using it via the links with #default_value it's more obvious at the top of the list, and will be easier to use with a long list of IPs.
I get 34 passes, 0 fails, and 4 exceptions. As far as I can tell the exceptions are something to do with clickLink() - but there's few examples in core to go off and I'm at a loss to why, leaving at CNR despite that.
#7
stupid comment duplication spotted by Freso in irc.
#8
OK the four exceptions/warnings in this test are due to a bug in clickLink() - it doesn't like ?foo=bar appended to links apparently. Issue opened here: http://drupal.org/node/255613
#9
Instead of generating an 'Operations' column with empty td's, it is probably better to remove the 'Operations' column entirely when the user has no permission to ban users. It cleans up the UI a bit for those users.
#10
Added a colspan when there's no permission, that's better, thanks!
#11
Committed to CVS HEAD. Bugfix + test + usability improvement = great patch.
#12
Automatically closed -- issue fixed for two weeks with no activity.