This is required for panels/ctools.
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | drupal.comments-per-thread.3.patch | 4.37 KB | sun |
| drupal.comments-per-thread.patch | 2.08 KB | sun |
This is required for panels/ctools.
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | drupal.comments-per-thread.3.patch | 4.37 KB | sun |
| drupal.comments-per-thread.patch | 2.08 KB | sun |
Comments
Comment #1
sunForgot my tag.
Comment #2
dwwSorry, that's not a critical bug.
But yes, this is an extremely good idea. We don't want functions getting their input implicitly on the side, when we can just pass them directly as arguments. It's sort of like wanting the function to do all its work and return the results explicitly in the return value, not via side-effects.
My first concern is if we should provide defaults for $mode and $comments_per_page, but I don't think that's necessary (or even, a good idea). It'd basically encourage lazy developers to introduce bugs by not honoring settings. So, I think it's a Good Thing(tm) that this patch doesn't provide defaults for those arguments.
However, if this is an public-facing API, and we expect callers to honor the settings, shouldn't we also make _comment_get_display_setting() a public-facing API function and call it comment_get_display_setting() while we're at it?
Thanks,
-Derek
Comment #3
sunThere we go. ;)
Comment #4
dwwMuch better, yes. ;) This is great. Core is now using a public-facing API (variable_get()) to provide the arguments to this function, so people who follow the "Do As Core Does" motto will get it right. And, we remove a bloaty, stupid function from core that shouldn't exist, anyway. Thanks!
Comment #5
dries commentedAlright, this looks fair. Committed to CVS HEAD. Thanks.