Currently the function hosting_site_count() is hardcoded to only return a count of sites that are in an 'Enabled' state. Disabled sites are not counted.
This is an issue because if you view the Platforms list at /hosting/platforms, the count of 'Sites' per platform is not accurate (if you have a platform with only disabled sites, it reports that there are 0 sites. Similarly, it may report, say, '3 sites' on a platform with 3 enabled sites and 1 or more disabled sites).
I think that hosting_site_count should have an optional argument, $disabled = FALSE, and if TRUE, alter the query so that it includes sites with a disabled status.
Only issue is that this is an API change (even if we add a new function to handle this instead, which is the other option but not as nice), and at the same time it's a bug in 6.x-1.x as far as I'm concerned, so I think we can't avoid an API change in 6.x-1.x. Thoughts?
Comment | File | Size | Author |
---|---|---|---|
#4 | 1503824_1.patch | 2.47 KB | mig5 |
#2 | 1503824-hosting_site_count.patch | 2.15 KB | mig5 |
Comments
Comment #1
Steven Jones CreditAttribution: Steven Jones commentedIf we add a optional argument to the function, but default it to the existing functionality, then according to the flexible rules of Drupal's API changes, it is not a change.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedSee attached patch for review.
I realised this should also include queued sites (e.g ones that have been created for installation but have not yet been installed in the task queue), not just disabled sites.
I had to re-arrange the order of the SQL query to make this work - I don't know if there's a more efficient way to structure three different types of query based on the arguments, but 'WHERE platform = N' seems to need to come first to do this, for some reason. It works at least.
This function is only elsewhere called in hosting_cron's implementation of hook_hosting_queues(), and it takes no arguments there, so the default behaviour should still be retained (platform is optional, and only enabled sites by default)
Comment #3
Steven Jones CreditAttribution: Steven Jones commentedThanks for the patch.
Given that we're now basically specifying the statuses to query for, I wonder if the second argument should just be an array of the statuses? If we make the default value of the second argument NULL, and then do a quick check on the argument within the body of the function to see if it's
is_null
then if it is, set the argument to the current functionality.To query a single field for multiple values you can use the 'IN' SQL operator, and a Drupal helper function db_placeholders
Something like:
Additionally the parameters and the return value of the function should be documented in the doxygen of the function, as described here: http://drupal.org/node/1354#functions
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks for the feedback. I've tried to implement it as you said and I seem to have it working.
I have zero confidence in pretty much anything these days though, as you know - not even the doxygen, which is not something I've even done before - so I'd appreciate some rigorous testing.
Thanks
Comment #5
Steven Jones CreditAttribution: Steven Jones commentedThanks for the patch, the doxygen looks like a great improvement. A few minor points:
There should not be any blank lines between the @param lines.
Could we change this to be something like: 'The number of sites matching the specified criteria.'
I guess because of this, we could end up in the situation where we don't actually append any clauses after the 'WHERE' in the query, resulting in invalid SQL. Maybe we should add the 'WHERE' later if we know that we're adding any additional conditions.
As this section is basically setting up the default value for the second argument, I wonder if this should go at the top of the function (with a little comment to say what its doing), then the rest of the function can just assume that the
$status
variable is an array, and can just do a quickcount()
to see if it needs to do anything, as per the code at the moment.Can we remove this extra blank line please?
You don't need to nest the else/if like this, you should do it as per the Drupal coding standards: http://drupal.org/coding-standards#controlstruct
Sorry for being super-picky, I'm just in that sort of mood.
Comment #6
Steven Jones CreditAttribution: Steven Jones commentedCommitted your patch to 6.x-1.x and then cleaned it up as per my comments: 6.x-1.x.
Also committed to 6.x-1.x