This is required for panels/ctools.

Comments

sun’s picture

Issue tags: +API clean-up

Forgot my tag.

dww’s picture

Priority: Critical » Normal
Status: Needs review » Needs work

Sorry, 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

sun’s picture

Status: Needs work » Needs review
StatusFileSize
new4.37 KB

There we go. ;)

dww’s picture

Status: Needs review » Reviewed & tested by the community

Much 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!

dries’s picture

Status: Reviewed & tested by the community » Fixed

Alright, this looks fair. Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -API clean-up

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