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?

CommentFileSizeAuthor
#4 1503824_1.patch2.47 KBmig5
#2 1503824-hosting_site_count.patch2.15 KBmig5
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Steven Jones’s picture

If 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.

Anonymous’s picture

Status: Active » Needs review
FileSize
2.15 KB

See 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)

Steven Jones’s picture

Status: Needs review » Needs work

Thanks 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:


if (count($statuses)) {
  $query .= ' (status IN (' . db_placeholders($statuses) . '))';
  $args = array_merge($args, $statuses);
}

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

Anonymous’s picture

Status: Needs work » Needs review
FileSize
2.47 KB

Thanks 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

Steven Jones’s picture

Status: Needs review » Needs work

Thanks for the patch, the doxygen looks like a great improvement. A few minor points:

+++ b/modules/hosting/site/hosting_site.module
@@ -283,17 +283,47 @@ function hosting_site_access($op, $node, $account) {
+ * @param $platform
+ *   (optional) A platform nid to only count sites on that specific platform.
+ *
+ * @param $statuses
+ *   (optional) An array of site statuses (defined by their constants) by which to include
+ *   such sites in the site count.

There should not be any blank lines between the @param lines.

+++ b/modules/hosting/site/hosting_site.module
@@ -283,17 +283,47 @@ function hosting_site_access($op, $node, $account) {
+ * @return ¶
+ *   The result of the database query

Could we change this to be something like: 'The number of sites matching the specified criteria.'

+++ b/modules/hosting/site/hosting_site.module
@@ -283,17 +283,47 @@ function hosting_site_access($op, $node, $account) {
+  $query = "SELECT count(nid) FROM {hosting_site} WHERE";

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.

+++ b/modules/hosting/site/hosting_site.module
@@ -283,17 +283,47 @@ function hosting_site_access($op, $node, $account) {
+  if (is_null($statuses)) {
+    $query .= " status = %d";
+  ¶
+    $args[] = HOSTING_SITE_ENABLED;
+  }

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 quick count() to see if it needs to do anything, as per the code at the moment.

+++ b/modules/hosting/site/hosting_site.module
@@ -283,17 +283,47 @@ function hosting_site_access($op, $node, $account) {
+    $query .= " status = %d";
+  ¶
+    $args[] = HOSTING_SITE_ENABLED;

Can we remove this extra blank line please?

+++ b/modules/hosting/site/hosting_site.module
@@ -283,17 +283,47 @@ function hosting_site_access($op, $node, $account) {
+  else {
+    if (count($statuses)) {

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.

Steven Jones’s picture

Version: 6.x-2.x-dev » 6.x-1.x-dev
Status: Needs work » Fixed

Committed 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

Status: Fixed » Closed (fixed)

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

  • Commit 79bf601 on 6.x-2.x, dev-ssl-ip-allocation-refactor, dev-1205458-move_sites_out_of_platforms, 7.x-3.x, dev-588728-views-integration, dev-1403208-new_roles, dev-helmo-3.x authored by mig5, committed by Steven Jones:
    Issue #1503824 by mig5: Fixed hosting_site_count() ignores disabled...
  • Commit 7f179b1 on 6.x-2.x, dev-ssl-ip-allocation-refactor, dev-1205458-move_sites_out_of_platforms, 7.x-3.x, dev-588728-views-integration, dev-1403208-new_roles, dev-helmo-3.x by Steven Jones:
    Issue #1503824 by Steven Jones: Fixed hosting_site_count() ignores...

  • Commit 79bf601 on 6.x-2.x, dev-ssl-ip-allocation-refactor, dev-1205458-move_sites_out_of_platforms, 7.x-3.x, dev-588728-views-integration, dev-1403208-new_roles, dev-helmo-3.x authored by mig5, committed by Steven Jones:
    Issue #1503824 by mig5: Fixed hosting_site_count() ignores disabled...
  • Commit 7f179b1 on 6.x-2.x, dev-ssl-ip-allocation-refactor, dev-1205458-move_sites_out_of_platforms, 7.x-3.x, dev-588728-views-integration, dev-1403208-new_roles, dev-helmo-3.x by Steven Jones:
    Issue #1503824 by Steven Jones: Fixed hosting_site_count() ignores...