Hello!

Currently, it's possible to rearrange the fields that can be altered from a comment, on the "Comment fields" form for the given node.

Unfortunately, the order isn't actually headed! Also, if you create a fieldset with the field_group module, it will be completely ignored.

I have a patch to fix this - I'll attach it in a moment.

Regards,
David.

CommentFileSizeAuthor
#4 comment_alter-comment-fields-1973280-4.patch6.47 KBdsnopek
#1 comment_alter-comment-fields-1973280-1.patch2.96 KBdsnopek

Comments

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Active » Needs review
StatusFileSize
new comment_alter-comment-fields-1973280-1.patch2.96 KB

Patch is attached! Please let me know what you think.

boobaa’s picture

boobaa
he/him
commented
Status: Needs review » Needs work

Okay, now I know what the score is about this issue. In fact, one may have the very same fields on the comment entity's bundle being displayed and the content type itself. In this case your patch fails, since it overwrites comment form widgets of comment fields with node form widgets of comment_alterable node fields. I think this is the most important reason of the current implementation, namely putting comment_altered form widgets into comment_alter's own form part. OTOH, this causes that field_group and form widgets' weight doesn't get into account.

So it seems that basically there are two reasons which are mutually exclusive:

  1. One may have the same field on the node and the comment.
  2. One may want to be rearrange comment_alter's extra fields just like other comment fields (and may even want to be able to use field_group as well).

I'm all in if there's a solution that is acceptable for both, though I wonder if it's possible at all.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

So, my patch makes #2 work and #1 was already broken due to the way we're using hook_field_extra_fields().

Personally, I don't think we should support having fields that are BOTH alterable from a comment and also attached to the comment itself. I think we should just disallow that - it would be very confusing to the user, ie., how could they tell two version of the exact same field apart? Of course, other fields can be attached to the comment entity! Just not the exact same ones.

What do you think?

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Needs work » Needs review
StatusFileSize
new comment_alter-comment-fields-1973280-4.patch6.47 KB

I've written a new patch (against latest git) which does the same as the old patch PLUS:

  1. It won't let you check "Enable altering this field from comments" if the field already exists on the comment and it gives a short message in the description.
  2. It won't let you add a field to a comment if the field is already alterable from comment on the parent node (it removes it from the "Add existing field" select list)
  3. If somehow the site gets into this invalid state, it will put a message on the system "Status report" explaining the problem and asking the user to correct it.

Personally, I think this is the best solution. I can't think of any valid use case where the user wants the exact same field on the comment twice (and it'd be impossible to tell the different between them).

Please let me know what you think!

boobaa’s picture

boobaa
he/him
commented
Status: Needs review » Fixed

Well, it looks like your approach is limiting the user, which is undesirable and turned out to be unnecessary. Git now contains a solution that does allow having (eg.) 'body' field on both the node and the comment, while still supporting comment_altering it. IOW, this has been fixed by now.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

Yeah, I guess you're right - it is unnecessary. :-) Prefixing the fields is what comment_driven does too. I just couldn't think of a single use-case where having the same field on comment twice would be desirable and not totally confusing.

But I'm perfectly happy with this approach! It definitely wins on flexibility.

Status: Fixed » Closed (fixed)

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