Both Flat Collapsed viewing mode and the Newest First sort order are broken in all 4.7.x versions.

This is because they are defined as 0:
define('COMMENT_MODE_FLAT_COLLAPSED', 0);
define('COMMENT_ORDER_NEWEST_FIRST', 0);

In the function comment_render, there is a test to see if viewing mode and viewing sort order are set:

    if (empty($mode)) {
      $mode = $user->mode ? $user->mode : ($_SESSION['comment_mode'] ? $_SESSION['comment_mode'] : variable_get('comment_default_mode', COMMENT_MODE_THREADED_EXPANDED));
    }

    if (empty($order)) {
      $order = $user->sort ? $user->sort : ($_SESSION['comment_sort'] ? $_SESSION['comment_sort'] : variable_get('comment_default_order', COMMENT_ORDER_NEWEST_FIRST));
    }

Both Flat Collapsed and Newest First will evaluate false in empty() as well as the if statements:
$mode = $user->mode ? $user->mode :
$order = $user->sort ? $user->sort :

Which means as soon as you select those options, it gets reverted to the default.

The simple fix is to change the constants:
define('COMMENT_MODE_FLAT_COLLAPSED', 1);
define('COMMENT_MODE_FLAT_EXPANDED', 2);
define('COMMENT_MODE_THREADED_COLLAPSED', 3);
define('COMMENT_MODE_THREADED_EXPANDED', 4);

and

define('COMMENT_ORDER_NEWEST_FIRST', 1);
define('COMMENT_ORDER_OLDEST_FIRST', 2);

I'm not sure how to go about getting this changed, so could someone please help me out?
Thanks

CommentFileSizeAuthor
#24 comment.controls_0.patch5.41 KBtenrapid
#16 comment.controls.patch6.49 KBtenrapid
#13 user_comment_mode.patch2.99 KBJaza
#9 comment.module_56.patch1.28 KBtenrapid
#3 comment.constants.patch1.13 KBtenrapid

Comments

yourhiro’s picture

yourhiro commented
Title: Comment Module - Flat Collapsed mode and Newest First order broken » Flat Collapsed mode and Newest First order broken
Status: Active » Needs review
killes@www.drop.org’s picture

killes@www.drop.org commented
Priority: Normal » Critical
Status: Needs review » Active

not a patch yet, but I'd sure like to see one.

tenrapid’s picture

tenrapid commented
StatusFileSize
new comment.constants.patch1.13 KB

The question is if CONSTANTS are considered to be zero based or not.

If not: here's a patch.

tenrapid’s picture

tenrapid commented
Status: Active » Needs review

hrrmpf ...

markus_petrux’s picture

markus_petrux commented

Does this need an update step to migrate current values?

tenrapid’s picture

tenrapid commented

No, the values in Drupal 4.6 are the same as in my patch.

tenrapid’s picture

tenrapid commented

Hmm, there is already an update step that decrements these entries by one in the variable table -> system_update_174().

So I'll probably have to come up with another patch that changes the logic instead of the values.

markus_petrux’s picture

markus_petrux commented
Status: Needs review » Needs work

:)

tenrapid’s picture

tenrapid commented
Version: 4.7.0-rc3 » x.y.z
Status: Needs work » Needs review
StatusFileSize
new comment.module_56.patch1.28 KB

New patch keeps the zero based constants and changes the logic instead.

dries’s picture

dries commented
Status: Needs review » Fixed

Works. Committed to CVS HEAD.

Zen’s picture

Zen commented
Status: Fixed » Active

HEAD: All comment views appear to be defaulting to 0 atm. This could be because of the fact that $user->mode always defaults to 0.

-K

tenrapid’s picture

tenrapid commented

After all I'm for making these two constants based on one again as it is in Drupal 4.6.

Keeping them zero based would require more code and logic:
- change default value of the columns 'mode' and 'order' in the {users} table to NULL
- extend the update step 174 to accomadate above columns to these changes

So what is the way to go?

Jaza’s picture

Jaza commented
Title: Flat Collapsed mode and Newest First order broken » Comments always displayed in flat collapsed mode by default
Status: Active » Needs review
StatusFileSize
new user_comment_mode.patch2.99 KB

Attached patch preserves the constants starting at zero, and changes the 'mode' field in the 'users' table to be NULL by default. This fixes the problem of comments always being displayed in 'flat collapsed' mode, unless comment controls are available and the user explicitly selects another mode using these controls.

This patch also looks for all instances where the 'mode' field in 'users' is set to 0, and changes it to NULL. This results in some loss of data, but unfortunately there's no way to avoid that. For sites where:

(a) the default viewing mode is NOT 'flat collapsed',
(b) users are able to set their own viewing mode using the comment controls, and
(c) the user has set their mode to 'flat collapsed',

users will simply have to set the viewing mode again (a very minor inconvenience for them, IMO). For any scenarios other than the exact one described above, comment viewing remains the same (i.e. users are unaffected).

I think that having the constants start at zero is a good thing (as a principle, and for consistency with other Drupal constants). And in order to get those zero-based constants working, this patch is necessary.

Jaza’s picture

Jaza commented

Oh, and I should add: the above patch was tested with mysql and pgsql.

markus_petrux’s picture

markus_petrux commented
Status: Needs review » Needs work

I have marked this issue as a dup of this one:
http://drupal.org/node/60214

It's not the same problem, but it's pretty related. Please, consider fixing the problem described in that issue here.

If you have comment controls enabled, these options are stored in $_SESSION (for guests) or in the users table (registered users).

When user selected options exist, global settings are never used. So, if you disable comment controls, there is no way for a user to use the globally defined options.

The attached patch takes user selected options only when the comment controls option is enabled.

tenrapid’s picture

tenrapid commented
Title: Comments always displayed in flat collapsed mode by default » Flat Collapsed mode and Newest First order broken
Status: Needs work » Needs review
StatusFileSize
new comment.controls.patch6.49 KB

Here is a new patch.

I moved the whole display settings logic out of comment_render() and move it into a new function _comment_get_display_setting($setting). I also incorporated the changes to fix markus' issue.

Also Jaza forgot the sort column so I added it.

Zen’s picture

Zen commented
Status: Needs review » Reviewed & tested by the community

Looks good. Also tested with and without comment controls - does the trick.

Thanks
-K

Jaza’s picture

Jaza commented

Very nice. I like the approach of moving all this checking to a separate function, and of making the code more generic. +1.

One question: if the user selects their own value for 'comments_per_page', where does that get saved? There's no field for it in the 'users' table, so I assume it gets serialized and put in the 'data' column... correct? And if so, I guess it's essentially a 'default NULL' field (since it won't get put into 'data' until the user sets a value for it).

moshe weitzman’s picture

moshe weitzman commented

groups.drupal.org is now sufferring from this. if possible, please commit this at earliest opportunity.

kayfish’s picture

kayfish commented

I believe that this is my issue also, submitted under:

http://drupal.org/node/60340

I'll mark the other as a duplicate

dries’s picture

dries commented

* The function could do with some additional code comments.

* $value = isset($user->$setting) ? $user->$setting : (isset($_SESSION['comment_'. $setting]) ? $_SESSION['comment_'. $setting] : $default); is kinda long.

gerhard killesreiter’s picture

gerhard killesreiter commented
Status: Reviewed & tested by the community » Needs work

I'd prefer to change the constants, fix update 174, and leave the database alone if possible at all. We only need to provide an upgrade patch from 4.6 to 4.7.

yourhiro’s picture

yourhiro commented

Pardon my inexperience (in web programming/oss/drupal) but, what ever happened to the whole KISS principle? Why add more logic, function calls, etc when simply changing the constants does everything we need?

It seems like a whole lot of extra work for the sake of...I'm not sure, which is why I'm asking :)
And referring to 0-based constants, what's the benefit(s)? I can understand if it's a binary variable...but when it's not, why make 1 of n constants in a set a "special" value?

Thanks

tenrapid’s picture

tenrapid commented
Status: Needs work » Needs review
StatusFileSize
new comment.controls_0.patch5.41 KB

These constants, well, zero or not, t. i. t. q.

The corresponding user table columns 'mode' and 'sort' exist there for 5 years now and IMO they should go into the 'data' column. But because this won't happen and in a sense of tradition this new patch keeps the database untouched and changes the constants instead. Back to their 4.6 values.

The patch also fixes (removes) the update step 174, incorporates Dries' comment and removes some crufty pager comments and code.

killes@www.drop.org’s picture

killes@www.drop.org commented

this looks very promising. Any testers? Especially the update from 4.6 shoudl be tested.

rkerr’s picture

rkerr commented

I haven't tested but will +1 the #24 approach.

Steven’s picture

Steven commented
Status: Needs review » Fixed

After this patch, the integer values used by 4.6 and 4.7 are the same. So the update can indeed be removed (but should be left in there to ensure consistent updating).

Committed to HEAD, thanks.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)