- Patch #191499 by catch: remove display order settings and cleaned up some white space.

6 files changed, 106 insertions, 133 deletions

diff --git a/CHANGELOG.txt b/CHANGELOG.txt
index 0e95ceed8..ec3f541e9 100644
--- a/CHANGELOG.txt
+++ b/CHANGELOG.txt
@@ -13,6 +13,8 @@ Drupal 7.0, xxxx-xx-xx (development version)
     * Implemented drag-and-drop positioning for poll options.
     * Provided descriptions for user permissions.
     * Removed comment controls for users.
+    * Removed display order settings for comment module. Comment display
+      order can now be customised using the Views module.
 - Search:
     * Added support for language-aware searches.
 - Testing:
diff --git a/includes/tests/cache.test b/includes/tests/cache.test
index 09a10214b..a6ff1e0b7 100644
--- a/includes/tests/cache.test
+++ b/includes/tests/cache.test
@@ -3,32 +3,32 @@ class CacheTestCase extends DrupalWebTestCase {
   protected $default_table = 'cache';
   protected $default_cid = 'test_temporary';
   protected $default_value = 'CacheTest';
-  
+
   /**
    * Check whether or not a cache entry exists.
-   * 
+   *
    * @param $cid
    *   The cache id.
    * @param $var
    *   The variable the cache should contain.
    * @param $table
    *   The table the cache item was stored in.
-   * @return 
+   * @return
    *   TRUE on pass, FALSE on fail.
    */
   protected function checkCacheExists($cid, $var, $table = null) {
     if ($table == null) {
       $table = $this->default_table;
     }
-    
+
     $cache = cache_get($cid, $table);
-    
+
     return isset($cache->data) && $cache->data == $var;
   }
 
   /**
    * Assert or a cache entry exists.
-   * 
+   *
    * @param $message
    *   Message to display.
    * @param $var
@@ -54,7 +54,7 @@ class CacheTestCase extends DrupalWebTestCase {
 
   /**
    * Assert or a cache entry has been removed.
-   * 
+   *
    * @param $message
    *   Message to display.
    * @param $cid
@@ -69,9 +69,9 @@ class CacheTestCase extends DrupalWebTestCase {
     if ($cid == NULL) {
       $cid = $this->default_cid;
     }
-    
+
     $cache = cache_get($cid, $table);
-    $this->assertFalse($cache, $message);  
+    $this->assertFalse($cache, $message);
   }
 
   /**
@@ -83,10 +83,10 @@ class CacheTestCase extends DrupalWebTestCase {
     if ($table == NULL) {
       $table = $this->default_table;
     }
-    
-    cache_clear_all(NULL, $table);   
+
+    cache_clear_all(NULL, $table);
   }
-  
+
   /**
    * Setup the lifetime settings for caching.
    *
@@ -95,8 +95,8 @@ class CacheTestCase extends DrupalWebTestCase {
    */
   protected function setupLifetime($time) {
     variable_set('cache_lifetime', $time);
-    variable_set('cache_flush', 0);    
-  } 
+    variable_set('cache_flush', 0);
+  }
 }
 
 class CacheSavingCase extends CacheTestCase {
@@ -147,10 +147,10 @@ class CacheSavingCase extends CacheTestCase {
     $test_object->test1 = $this->randomName('100');
     $test_object->test2 = 100;
     $test_object->test3 = array('drupal1', 'drupal2' => 'drupal3', 'drupal4' => array('drupal5', 'drupal6'));
-    
+
     cache_set('test_object', $test_object, 'cache');
     $cache = cache_get('test_object', 'cache');
-    $this->assertTrue(isset($cache->data) && $cache->data == $test_object, t('Object is saved and restored properly.'));  
+    $this->assertTrue(isset($cache->data) && $cache->data == $test_object, t('Object is saved and restored properly.'));
   }
 
   /*
@@ -159,7 +159,7 @@ class CacheSavingCase extends CacheTestCase {
   function checkVariable($var) {
     cache_set('test_var', $var, 'cache');
     $cache = cache_get('test_var', 'cache');
-    $this->assertTrue(isset($cache->data) && $cache->data === $var, t('@type is saved and restored properly.', array('@type' => ucfirst(gettype($var)))));   
+    $this->assertTrue(isset($cache->data) && $cache->data === $var, t('@type is saved and restored properly.', array('@type' => ucfirst(gettype($var)))));
   }
 }
 
@@ -181,53 +181,53 @@ class CacheClearCase extends CacheTestCase {
   function setUp() {
     $this->default_table = 'cache_page';
     $this->default_value = $this->randomName(10);
-    
+
     parent::setUp();
   }
-  
+
   /**
    * Test clearing using a cid.
    */
   function testClearCid() {
     cache_set('test_cid_clear', $this->default_value, $this->default_table);
-    
+
     $this->assertCacheExists(t('Cache was set for clearing cid.'), $this->default_value, 'test_cid_clear');
     cache_clear_all('test_cid_clear', $this->default_table);
-    
+
     $this->assertCacheRemoved(t('Cache was removed after clearing cid.'), 'test_cid_clear');
-    
+
     cache_set('test_cid_clear1', $this->default_value, $this->default_table);
     cache_set('test_cid_clear2', $this->default_value, $this->default_table);
-    $this->assertTrue($this->checkCacheExists('test_cid_clear1', $this->default_value) 
+    $this->assertTrue($this->checkCacheExists('test_cid_clear1', $this->default_value)
                       && $this->checkCacheExists('test_cid_clear2', $this->default_value),
-                      t('Two caches were created for checking cid "*" with wildcard false.'));                      
+                      t('Two caches were created for checking cid "*" with wildcard false.'));
     cache_clear_all('*', $this->default_table);
-    $this->assertTrue($this->checkCacheExists('test_cid_clear1', $this->default_value) 
+    $this->assertTrue($this->checkCacheExists('test_cid_clear1', $this->default_value)
                       && $this->checkCacheExists('test_cid_clear2', $this->default_value),
-                      t('Two caches still exists after clearing cid "*" with wildcard false.'));    
+                      t('Two caches still exists after clearing cid "*" with wildcard false.'));
   }
-  
+
   /**
    * Test clearing using wildcard.
    */
   function testClearWildcard() {
     cache_set('test_cid_clear1', $this->default_value, $this->default_table);
     cache_set('test_cid_clear2', $this->default_value, $this->default_table);
-    $this->assertTrue($this->checkCacheExists('test_cid_clear1', $this->default_value) 
+    $this->assertTrue($this->checkCacheExists('test_cid_clear1', $this->default_value)
                       && $this->checkCacheExists('test_cid_clear2', $this->default_value),
-                      t('Two caches were created for checking cid "*" with wildcard true.'));                      
+                      t('Two caches were created for checking cid "*" with wildcard true.'));
     cache_clear_all('*', $this->default_table, TRUE);
-    $this->assertFalse($this->checkCacheExists('test_cid_clear1', $this->default_value) 
+    $this->assertFalse($this->checkCacheExists('test_cid_clear1', $this->default_value)
                       || $this->checkCacheExists('test_cid_clear2', $this->default_value),
                       t('Two caches removed after clearing cid "*" with wildcard true.'));
-                      
+
     cache_set('test_cid_clear1', $this->default_value, $this->default_table);
     cache_set('test_cid_clear2', $this->default_value, $this->default_table);
-    $this->assertTrue($this->checkCacheExists('test_cid_clear1', $this->default_value) 
+    $this->assertTrue($this->checkCacheExists('test_cid_clear1', $this->default_value)
                       && $this->checkCacheExists('test_cid_clear2', $this->default_value),
-                      t('Two caches were created for checking cid substring with wildcard true.'));                      
+                      t('Two caches were created for checking cid substring with wildcard true.'));
     cache_clear_all('test_', $this->default_table, TRUE);
-    $this->assertFalse($this->checkCacheExists('test_cid_clear1', $this->default_value) 
+    $this->assertFalse($this->checkCacheExists('test_cid_clear1', $this->default_value)
                       || $this->checkCacheExists('test_cid_clear2', $this->default_value),
                       t('Two caches removed after clearing cid substring with wildcard true.'));
   }
@@ -267,11 +267,11 @@ class CacheExpiryCase extends CacheTestCase {
     cache_set($this->default_cid, $this->default_value, $this->default_table, CACHE_TEMPORARY);
 
     $this->assertCacheExists(t('Temporary cache data without lifetime exists before wipe.'));
-    
+
     $user->cache = isset($user->cache) ? $user->cache +2 : time() + 2;
     $this->assertCacheExists(t('Temporary cache without lifetime valid after user cache expiration.'));
     $user->cache = $user->cache - 2;
-    
+
     $this->generalWipe();
     $this->assertCacheRemoved(t('Temporary cache without lifetime does not exists after wipe.'));
   }
@@ -285,8 +285,8 @@ class CacheExpiryCase extends CacheTestCase {
   function testTemporaryLifetime() {
     $this->setupLifetime(5);
     cache_set($this->default_cid, $this->default_value, $this->default_table, CACHE_TEMPORARY);
-    
-    $this->assertCacheExists(t('Temporary cache with lifetime data exists before wipe.'));   
+
+    $this->assertCacheExists(t('Temporary cache with lifetime data exists before wipe.'));
 
     $user->cache = isset($user->cache) ? $user->cache + 2 : time() + 2;
     $this->assertCacheExists(t('Temporary cache without lifetime valid after user cache expiration.'));
@@ -305,13 +305,13 @@ class CacheExpiryCase extends CacheTestCase {
   function testPermanentNoLifetime() {
     $this->setupLifetime(0);
     cache_set($this->default_cid, $this->default_value, $this->default_table, CACHE_PERMANENT);
-    
+
     $this->assertCacheExists(t('Permanent cache data without lifetime exists before wipe.'));
-    
+
     $user->cache = isset($user->cache) ? $user->cache + 2 : time() + 2;
     $this->assertCacheExists(t('Permanent cache without lifetime valid after user cache expiration.'));
     $user->cache = $user->cache - 2;
-    
+
     $this->generalWipe();
     $this->assertCacheExists(t('Permanent cache without lifetime exists after wipe.'));
   }
@@ -330,7 +330,7 @@ class CacheExpiryCase extends CacheTestCase {
 
     $user->cache = isset($user->cache) ? $user->cache + 2 : time() + 2;
     $this->assertCacheExists(t('Permanent cache with lifetime valid after user cache expiration.'));
-    $user->cache = $user->cache - 2; 
+    $user->cache = $user->cache - 2;
 
     $this->generalWipe();
     $this->assertCacheExists(t('Permanent cache with lifetime exists after wipe.'));
@@ -345,18 +345,18 @@ class CacheExpiryCase extends CacheTestCase {
   function testUnixTimestampNoLifetime() {
     $this->setupLifetime(0);
     cache_set($this->default_cid, $this->default_value, $this->default_table, time() + 1);
-    
+
     $this->assertCacheExists(t('Unit timestamp cache data without lifetime exists before wipe.'));
     $this->generalWipe();
     $this->assertCacheExists(t('Unit timestamp cache without lifetime exists after wipe.'));
     sleep(2); // expire
     $this->assertCacheExists(t('Unit timestamp cache without lifetime exists after expiration.'));
-    
+
     $user->cache = isset($user->cache) ? $user->cache + 2 : time() + 2;
     $this->assertCacheExists(t('Unit timestamp cache without lifetime valid after user cache expiration.'));
     $user->cache = $user->cache - 2;
-    
-    $this->generalWipe();    
+
+    $this->generalWipe();
     $this->assertCacheRemoved(t('Unit timestamp cache without lifetime deleted after expiration and wipe.'));
   }
 
@@ -370,17 +370,17 @@ class CacheExpiryCase extends CacheTestCase {
     $this->setupLifetime(5);
     cache_set($this->default_cid, $this->default_value, $this->default_table, time() + 1);
     global $user;
-    
+
     $this->assertCacheExists(t('Unit timestamp cache data without lifetime exists before wipe.'));
     $this->generalWipe();
     $this->assertCacheExists(t('Unit timestamp cache with lifetime exists after wipe.'));
     sleep(2); // expire
     $this->assertCacheExists(t('Unit timestamp cache with lifetime exists after expiration.'));
-    
+
     $user->cache = $user->cache + 2;
     $this->assertCacheRemoved(t('Unit timestamp cache with lifetime not valid after user cache expiration.'));
     $user->cache = $user->cache - 2;
-    
+
     $this->generalWipe();
     $this->assertCacheRemoved(t('Unit timestamp cache with lifetime deleted after expiration and wipe.'));
   }
diff --git a/install.php b/install.php
index e66ddff86..1ec0eccce 100644
--- a/install.php
+++ b/install.php
@@ -26,8 +26,8 @@ function install_main() {
   if (preg_match("/^simpletest\d+$/", $_SERVER['HTTP_USER_AGENT'])) {
     header('HTTP/1.1 403 Forbidden');
     exit;
-  }  
-    
+  }
+
   require_once './includes/bootstrap.inc';
   drupal_bootstrap(DRUPAL_BOOTSTRAP_CONFIGURATION);
 
diff --git a/modules/comment/comment.install b/modules/comment/comment.install
index 598b476bf..cc1ea7353 100644
--- a/modules/comment/comment.install
+++ b/modules/comment/comment.install
@@ -24,6 +24,11 @@ function comment_update_1() {
   return array();
 }
 
+/**
+ * @defgroup updates-5.x-to-6.x Comment updates from 5.x to 6.x
+ * @{
+ */
+
 function comment_update_6001() {
   $ret[] = update_sql("ALTER TABLE {comments} DROP score");
   $ret[] = update_sql("ALTER TABLE {comments} DROP users");
@@ -70,6 +75,31 @@ function comment_update_6003() {
   return $ret;
 }
 
+/**
+ * @} End of "defgroup updates-5.x-to-6.x"
+ * The next series of updates should start at 7000.
+ */
+
+/**
+ * @defgroup updates-6.x-to-7.x Comment updates from 6.x to 7.x
+ * @{
+ */
+
+/**
+ * Remove comment settings for page ordering.
+ */
+function comment_update_7000() {
+  $types = node_get_types();
+  foreach ($types as $type => $object) {
+    variable_del('comment_default_order' . $type);
+  }
+  return array(array('success' => TRUE, 'query' => 'Comment order settings removed.'));
+}
+
+/**
+ * @} End of "defgroup updates-6.x-to-7.x"
+ * The next series of updates should start at 8000.
+ */
 
 /**
  * Implementation of hook_schema().
diff --git a/modules/comment/comment.module b/modules/comment/comment.module
index 67861b078..fd949a2f6 100644
--- a/modules/comment/comment.module
+++ b/modules/comment/comment.module
@@ -41,16 +41,6 @@ define('COMMENT_MODE_THREADED_COLLAPSED', 3);
 define('COMMENT_MODE_THREADED_EXPANDED', 4);
 
 /**
- * Comments are ordered by date - newest first.
- */
-define('COMMENT_ORDER_NEWEST_FIRST', 1);
-
-/**
- * Comments are ordered by date - oldest first.
- */
-define('COMMENT_ORDER_OLDEST_FIRST', 2);
-
-/**
  * Anonymous posters cannot enter their contact information.
  */
 define('COMMENT_ANONYMOUS_MAYNOT_CONTACT', 0);
@@ -230,7 +220,6 @@ function comment_node_type($op, $info) {
   $settings = array(
     'comment',
     'comment_default_mode',
-    'comment_default_order',
     'comment_default_per_page',
     'comment_anonymous',
     'comment_subject_field',
@@ -350,35 +339,24 @@ function comment_get_recent($number = 10) {
 function comment_new_page_count($num_comments, $new_replies, $node) {
   $comments_per_page = _comment_get_display_setting('comments_per_page', $node);
   $mode = _comment_get_display_setting('mode', $node);
-  $order = _comment_get_display_setting('sort', $node);
   $pagenum = NULL;
   $flat = in_array($mode, array(COMMENT_MODE_FLAT_COLLAPSED, COMMENT_MODE_FLAT_EXPANDED));
-  if ($num_comments <= $comments_per_page || ($flat && $order == COMMENT_ORDER_NEWEST_FIRST)) {
-    // Only one page of comments or flat forum and newest first.
-    // First new comment will always be on first page.
+  if ($num_comments <= $comments_per_page) {
+    // Only one page of comments.
     $pageno = 0;
   }
+  elseif ($flat) {
+    // Flat comments.
+    $count = $num_comments - $new_replies;
+    $pageno =  $count / $comments_per_page;
+  }
   else {
-    if ($flat) {
-      // Flat comments and oldest first.
-      $count = $num_comments - $new_replies;
-    }
-    else {
-      // Threaded comments. See the documentation for comment_render().
-      if ($order == COMMENT_ORDER_NEWEST_FIRST) {
-        // Newest first: find the last thread with a new comment.
-        $result = db_query('(SELECT thread FROM {comments} WHERE nid = %d  AND status = 0 ORDER BY timestamp DESC LIMIT %d) ORDER BY thread DESC LIMIT 1', $node->nid, $new_replies);
-        $thread = db_result($result);
-        $result_count = db_query("SELECT COUNT(*) FROM {comments} WHERE nid = %d AND status = 0 AND thread > '" . $thread . "'", $node->nid);
-      }
-      else {
-        // Oldest first: find the first thread with a new comment.
-        $result = db_query('(SELECT thread FROM {comments} WHERE nid = %d  AND status = 0 ORDER BY timestamp DESC LIMIT %d) ORDER BY SUBSTRING(thread, 1, (LENGTH(thread) - 1)) LIMIT 1', $node->nid, $new_replies);
-        $thread = substr(db_result($result), 0, -1);
-        $result_count = db_query("SELECT COUNT(*) FROM {comments} WHERE nid = %d AND status = 0 AND SUBSTRING(thread, 1, (LENGTH(thread) - 1)) < '" . $thread . "'", $node->nid);
-      }
-      $count = db_result($result_count);
-    }
+    // Threaded comments.
+    // Find the first thread with a new comment.
+    $result = db_query('(SELECT thread FROM {comments} WHERE nid = %d  AND status = 0 ORDER BY timestamp DESC LIMIT %d) ORDER BY SUBSTRING(thread, 1, (LENGTH(thread) - 1)) LIMIT 1', $node->nid, $new_replies);
+    $thread = substr(db_result($result), 0, -1);
+    $result_count = db_query("SELECT COUNT(*) FROM {comments} WHERE nid = %d AND status = 0 AND SUBSTRING(thread, 1, (LENGTH(thread) - 1)) < '" . $thread . "'", $node->nid);
+    $count = db_result($result_count);
     $pageno =  $count / $comments_per_page;
   }
 
@@ -511,13 +489,6 @@ function comment_form_alter(&$form, $form_state, $form_id) {
       '#options' => _comment_get_modes(),
       '#description' => t('Expanded views display the body of the comment. Threaded views keep replies together.'),
     );
-    $form['comment']['comment_default_order'] = array(
-      '#type' => 'radios',
-      '#title' => t('Display order'),
-      '#default_value' => variable_get('comment_default_order_' . $form['#node_type']->type, COMMENT_ORDER_NEWEST_FIRST),
-      '#options' => _comment_get_orders(),
-      '#description' => t('Comments are displayed in ascending or descending order.'),
-    );
     $form['comment']['comment_default_per_page'] = array(
       '#type' => 'select',
       '#title' => t('Comments per page'),
@@ -921,7 +892,6 @@ function comment_render($node, $cid = 0) {
     }
 
     $mode = _comment_get_display_setting('mode', $node);
-    $order = _comment_get_display_setting('sort', $node);
     $comments_per_page = _comment_get_display_setting('comments_per_page', $node);
 
     if ($cid && is_numeric($cid)) {
@@ -955,30 +925,19 @@ function comment_render($node, $cid = 0) {
         $query_count .= ' AND c.status = %d';
         $query_args[] = COMMENT_PUBLISHED;
       }
-
-      if ($order == COMMENT_ORDER_NEWEST_FIRST) {
-        if ($mode == COMMENT_MODE_FLAT_COLLAPSED || $mode == COMMENT_MODE_FLAT_EXPANDED) {
-          $query .= ' ORDER BY c.cid DESC';
-        }
-        else {
-          $query .= ' ORDER BY c.thread DESC';
-        }
+      if ($mode == COMMENT_MODE_FLAT_COLLAPSED || $mode == COMMENT_MODE_FLAT_EXPANDED) {
+        $query .= ' ORDER BY c.cid';
       }
-      elseif ($order == COMMENT_ORDER_OLDEST_FIRST) {
-        if ($mode == COMMENT_MODE_FLAT_COLLAPSED || $mode == COMMENT_MODE_FLAT_EXPANDED) {
-          $query .= ' ORDER BY c.cid';
-        }
-        else {
-          // See comment above. Analysis reveals that this doesn't cost too
-          // much. It scales much much better than having the whole comment
-          // structure.
-          $query .= ' ORDER BY SUBSTRING(c.thread, 1, (LENGTH(c.thread) - 1))';
-        }
+      else {
+        // See comment above. Analysis reveals that this doesn't cost too
+        // much. It scales much much better than having the whole comment
+        // structure.
+        $query .= ' ORDER BY SUBSTRING(c.thread, 1, (LENGTH(c.thread) - 1))';
       }
+
       $query = db_rewrite_sql($query, 'c', 'cid');
       $query_count = db_rewrite_sql($query_count, 'c', 'cid');
 
-      // Start a form, for use with comment control.
       $result = pager_query($query, $comments_per_page, 0, $query_count, $query_args);
 
       $divs = 0;
@@ -1770,7 +1729,6 @@ function theme_comment_post_forbidden($node) {
 function template_preprocess_comment_wrapper(&$variables) {
   // Provide contextual information.
   $variables['display_mode']  = _comment_get_display_setting('mode', $variables['node']);
-  $variables['display_order'] = _comment_get_display_setting('sort', $variables['node']);
   $variables['template_files'][] = 'comment-wrapper-' . $variables['node']->type;
 }
 
@@ -1805,19 +1763,6 @@ function _comment_get_modes() {
 }
 
 /**
- * Return an array of viewing orders for comment listings.
- *
- * We can't use a global variable array because the locale system
- * is not initialized yet when the comment module is loaded.
- */
-function _comment_get_orders() {
-  return array(
-    COMMENT_ORDER_NEWEST_FIRST => t('Date - newest first'),
-    COMMENT_ORDER_OLDEST_FIRST => t('Date - oldest first')
-  );
-}
-
-/**
  * Return an array of "comments per page" settings from which the user
  * can choose.
  */
@@ -1839,10 +1784,6 @@ function _comment_get_display_setting($setting, $node) {
       $value = variable_get('comment_default_mode_' . $node->type, COMMENT_MODE_THREADED_EXPANDED);
       break;
 
-    case 'sort':
-      $value = variable_get('comment_default_order_' . $node->type, COMMENT_ORDER_NEWEST_FIRST);
-      break;
-
     case 'comments_per_page':
       $value = variable_get('comment_default_per_page_' . $node->type, 50);
   }
diff --git a/modules/comment/comment.test b/modules/comment/comment.test
index 96a285040..4f28ea4f5 100644
--- a/modules/comment/comment.test
+++ b/modules/comment/comment.test
@@ -293,7 +293,7 @@ class CommentTestCase extends DrupalWebTestCase {
   }
 
   /**
-   * Checks current pag for specified comment.
+   * Checks current page for specified comment.
    *
    * @param object $comment Comment object.
    * @param boolean $reply The comment is a reply to another comment.
@@ -373,7 +373,7 @@ class CommentTestCase extends DrupalWebTestCase {
    *   Comments per page value.
    */
   function setCommentsPerPage($number) {
-    $this->setCommentSettings('comment_default_per_page', $number, 'Number of comments per page set to ' . $number .'.');
+    $this->setCommentSettings('comment_default_per_page_article', $number, 'Number of comments per page set to ' . $number .'.');
   }
 
   /**