Closed adrienne closed 2 years ago
Sure, the canComment()
is exactly as you've described, but covers a number of cases - all to point whether the user can comment on the element in question. For core purposes, all we really are about is the possibility of whether users can comment.
Happy with your proposal. I suppose for the reason codes, if you'd like them defined as const
variables would be good to maintain, as throwing around string's would be problematic (eg: 'MANUAL_CLOSURE' vs 'this comment is closed'), which you've already alluded to here.
I think that's otherwise pretty reasonable! No pressure to PR, but of course welcome it. If not, I can probably add this in the new year.
@engram-design i need it much sooner than the new year, so i'll probably do it this week 😄
I was going to use an abstract class as a fake enum to define the constants -- Craft first-party has several of these defined, and i was going to use the same pattern. It keeps them all in one place and makes them easy to reference.
Either/or, whatever you prefer - comfy with either approach.
@engram-design - done :) please review and let me know what you think! I made the changes pretty minimal but this framework should be quite extensible if you think of more reasons/checks to perform in future, too.
@engram-design just making sure you see this :D
Added in 1.8.12
What are you trying to do?
So there are several circumstances under which commenting is unavailable for a given user on an entry:
All of this is exposed to the comment-related views only as a single flag via
settings.canComment(element)
, which is great if the question you want answered is "should I show the comment form or not", but not so great when the question is "what should I tell the visitor about why the comment form is not showing." When creating custom templates, which is pretty common, we may want to give the user that information.What's your proposed solution?
Investigating the canComment() function, almost all of these checks are already separated out -- the only one which is not is disambiguating case 1 (manual closure) from case 2 (expiration). Now,
canComment()
is only used four other times in the codebase (see my GitHub search here), but it really does want to be a boolean for obvious reasons.So my suggestion is twofold:
First, refactor all the actual checks from
canComment()
into a separate function, something likecommentAccess()
. That function can return an array like:Make that function public, so it can be called as
settings.commentAccess(element)
from a custom comment template.(Also disambiguate manual closure from expiration, while we're at it.)
Second, for DRYness purposes, refactor
canComment()
so it uses thecommentAccess()
function to do all the heavy lifting. That's obviously as simple asIf this sounds reasonable to you, @engram-design, I'll submit a pull request for this, but i don't want to do the work if you're not willing to incorporate it.